Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plug BlockSpaceAllocator logic into ProcessProposal #895

Conversation

sug0
Copy link
Collaborator

@sug0 sug0 commented Dec 14, 2022

Based on #897

Adds some validation of the BlockSpaceAllocator logic into ProcessProposal.

SPECS: https://hackmd.io/unZg8ia4SZCKDQIY-s8d_Q?view=

@sug0 sug0 added the ledger label Dec 14, 2022
@sug0 sug0 mentioned this pull request Dec 14, 2022
8 tasks
@sug0 sug0 marked this pull request as ready for review December 15, 2022 14:35
Comment on lines +128 to +129
AllocationError = 8, /* NOTE: keep these values in sync with
* [`ErrorCodes::is_recoverable`] */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ErrorCodes::is_recoverable should be matching on this enum rather than returning (self as u32) <= 3, to enforce this (related to this NOTE but not the PR itself though, we shouldn't change this as part of this PR)

Copy link
Collaborator Author

@sug0 sug0 Dec 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's how it was defined in ProcessProposal before, this is_recoverable() method came from a ProcessProposal refactor from back when I was working on eth events vote extensions

apps/src/lib/node/ledger/shell/prepare_proposal.rs Outdated Show resolved Hide resolved
@sug0 sug0 changed the title Plug block space allocator logic into ProcessProposal Plug BlockSpaceAllocator logic into ProcessProposal Dec 16, 2022
Copy link
Member

@batconjurer batconjurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can the tx building fns in prepare proposal be defined in the same order they are used in the main method?

@sug0 sug0 marked this pull request as draft December 20, 2022 09:28
@sug0 sug0 marked this pull request as ready for review December 20, 2022 09:37
@sug0 sug0 requested a review from batconjurer December 20, 2022 09:37
@sug0 sug0 merged commit a8f5abf into tiago/ethbridge/optimize-block-proposal Dec 20, 2022
@sug0 sug0 deleted the tiago/ethbridge/process-proposal-allocator branch December 20, 2022 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants