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

Initialize BlockSpaceAllocator with max_proposal_bytes #887

Conversation

sug0
Copy link
Collaborator

@sug0 sug0 commented Dec 13, 2022

Based on #771

Initialize BlockSpaceAllocator with the max_proposal_bytes genesis parameter, instead of max_tx_bytes (retrieved from RequestPrepareProposal).

Copy link
Contributor

@james-chf james-chf left a comment

Choose a reason for hiding this comment

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

lgtm!

@@ -165,6 +168,9 @@ pub trait QueriesExt {
&'db self,
epoch: Option<Epoch>,
) -> Box<dyn Iterator<Item = (EthAddrBook, Address, token::Amount)> + 'db>;

/// Retrieve the `max_proposal_bytes` consensus parameter from storage.
fn get_max_proposal_bytes(&self) -> ProposalBytes;
Copy link
Contributor

Choose a reason for hiding this comment

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

for sure QueriesExt can be broken up/refactored/done away with soon! The API of the Storage struct grows further...

Copy link
Collaborator Author

@sug0 sug0 Dec 13, 2022

Choose a reason for hiding this comment

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

lol yeah, it's just a catch-all basically

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually deleting it. It will become PosQueries. Any funcitonality that isn't that will have to live elsewhere.

Comment on lines +443 to +451
fn get_max_proposal_bytes(&self) -> ProposalBytes {
let key = get_max_proposal_bytes_key();
let (maybe_value, _gas) = self
.read(&key)
.expect("Must be able to read ProposalBytes from storage");
let value =
maybe_value.expect("ProposalBytes must be present in storage");
decode(value).expect("Must be able to decode ProposalBytes in storage")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think ideally we should be returning errors rather than panicking here. The issue is we do not have a nice way to gracefully shut down within Shell ABCI methods upon encountering an unrecoverable error, so we will have to panic somewhere up the stack right now anyway.

Copy link
Member

Choose a reason for hiding this comment

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

A panic in the shell triggers the abortable spawner which shuts down all the other processes. I think we should panic here.

@sug0 sug0 merged commit 8ef22e5 into tiago/ethbridge/optimize-block-proposal Dec 15, 2022
@sug0 sug0 deleted the tiago/ethbridge/max-proposal-bytes branch December 15, 2022 10:16
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