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

app: drop stateful mempool #4627

Merged
merged 9 commits into from
Jun 24, 2024
Merged

app: drop stateful mempool #4627

merged 9 commits into from
Jun 24, 2024

Conversation

erwanor
Copy link
Member

@erwanor erwanor commented Jun 17, 2024

Describe your changes

This is a draft PR that removes the stateful mempool from the application, replacing it with:

  • a stateless mempool that gates inclusion on validity against the latest version of the app state
  • a prepare proposal implementation that filters for valid transactions
  • a process proposal implementation that validates block quality and lays the groundwork to do optimistic execution

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    Consensus breaking

@erwanor erwanor marked this pull request as ready for review June 17, 2024 14:56
@erwanor erwanor added A-node Area: System design and implementation for node software consensus-breaking breaking change to execution of on-chain data labels Jun 17, 2024
@erwanor erwanor force-pushed the erwan/abci_plus_mempool branch from 0320b47 to 723b61b Compare June 17, 2024 18:28
@erwanor erwanor requested review from hdevalence and conorsch June 17, 2024 18:29
@erwanor erwanor marked this pull request as draft June 18, 2024 12:53
@erwanor erwanor marked this pull request as ready for review June 19, 2024 21:29
crates/core/app/src/app/mod.rs Show resolved Hide resolved
crates/core/app/src/app/mod.rs Show resolved Hide resolved
crates/core/app/src/server/consensus.rs Show resolved Hide resolved
@conorsch
Copy link
Contributor

Really solid work here. Especially grateful for the careful commenting throughout, as this work builds on a lot of recent and rather subtle refinements to the constraints on message sizes acceptable to the app.

@conorsch
Copy link
Contributor

Not merging because review from @hdevalence is still outstanding. Once it's got final approval, @erwanor, let's use the new squash-with-commit-comments-preserved workflow we've been discussing to get it in!

Comment on lines +54 to +58
/// The maximum size of a CometBFT block payload (1MB)
const MAX_BLOCK_TXS_PAYLOAD_BYTES: usize = 1024 * 1024;

/// The maximum size of a single individual transaction (30KB).
const MAX_TRANSACTION_SIZE_BYTES: usize = 30 * 1024;
Copy link
Member

Choose a reason for hiding this comment

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

Did we ever choose these in a principled way? Should they be parameters rather than hardcoded constants?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was picked with three things in mind:

  • large enough to allow JIT liquidity for multiple pairs
  • large enough to allow a ranged collection of positions
  • large enough to allow multiple IBC client updates
  • small enough to allow many of those actions to be performed in a single block even in the extreme case where each transaction is one of those things

Making them parameters make sense, I didn't do it because it would require more work with penumbra-governance and hooking them into https://docs.rs/tendermint/latest/tendermint/consensus/params/struct.Params.html

Copy link
Member

@hdevalence hdevalence left a comment

Choose a reason for hiding this comment

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

I think we should consider making the constants be parameters, but I'm good to merge either way.

@erwanor erwanor self-assigned this Jun 24, 2024
@erwanor erwanor merged commit d95258b into main Jun 24, 2024
13 checks passed
@erwanor erwanor deleted the erwan/abci_plus_mempool branch June 24, 2024 21:58
conorsch added a commit that referenced this pull request Jun 28, 2024
During team sync today we decided that this new value is superior.

Follow-up to related work in #4614, #4627, and #4632.
conorsch added a commit that referenced this pull request Jun 28, 2024
During team sync today we decided that this new value is superior.

Follow-up to related work in #4614, #4627, and #4632.
erwanor pushed a commit that referenced this pull request Jun 28, 2024
## Describe your changes
During team sync today we decided that this new value is superior.



## Issue ticket number and link
Follow-up to related work in #4614, #4627, and #4632.

## Checklist before requesting a review

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

> This change does affect consensus and so should be treated carefully.

Co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-node Area: System design and implementation for node software consensus-breaking breaking change to execution of on-chain data
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants