fix(sequencer): avoid including unexecuted rollup data #2190
+79
−119
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ethanoroshiba
approved these changes
May 21, 2025
There was a problem hiding this comment.
LGTM. In the PR description a new kind of integration test is discussed, but I think it wouldn't be too much lift to create a new app-level test and simply compare the results of two apps, one with the proposer flow and the other with a non-proposer flow. That being said, in the interest of getting the fix in, I think it can probably be ticketed for a followup. Nice catch!
joroshiba
approved these changes
May 21, 2025
noot
approved these changes
May 21, 2025
ethanoroshiba
pushed a commit
that referenced
this pull request
May 27, 2025
## Summary This is a bugfix for `App::prepare_proposal`. ## Background In `prepare_proposal`, while iterating the txs to be executed, we append the rollup data bytes to the collection of these _before_ checking and executing the tx. This is an issue if the given tx fails the size checks or fails execution. The end result is that the proposer later returns an error in finalize_block as the rest of the validators have a different (correct) view of the sequencer block and its included Merkle roots. ## Changes - Changed the type stored by the `App` under `EXECUTION_RESULTS_KEY` in the ephemeral store from `(rollup_data_bytes, tx_results, tx_ids)` to a `Vec<ExecutedTransaction>` where the `ExecutedTransaction` holds the checked tx and the corresponding execution result. This avoids the possibility of a mismatch between the number (or order) of the three discrete collections currently cached. ## Testing There are no current unit or integration tests which would catch this. This probably suits a new form of integration test more akin to the app upgrade tests, or even a new system-test. In the meantime, I tested locally by sending a few rollup data submission txs which were too large to all be included in a single block to a 5 node network. ## Changelogs Changelogs updated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is a bugfix for
App::prepare_proposal.Background
In
prepare_proposal, while iterating the txs to be executed, we append the rollup data bytes to the collection of these before checking and executing the tx.This is an issue if the given tx fails the size checks or fails execution. The end result is that the proposer later returns an error in finalize_block as the rest of the validators have a different (correct) view of the sequencer block and its included Merkle roots.
Changes
AppunderEXECUTION_RESULTS_KEYin the ephemeral store from(rollup_data_bytes, tx_results, tx_ids)to aVec<ExecutedTransaction>where theExecutedTransactionholds the checked tx and the corresponding execution result. This avoids the possibility of a mismatch between the number (or order) of the three discrete collections currently cached.Testing
There are no current unit or integration tests which would catch this. This probably suits a new form of integration test more akin to the app upgrade tests, or even a new system-test.
In the meantime, I tested locally by sending a few rollup data submission txs which were too large to all be included in a single block to a 5 node network.
Changelogs
Changelogs updated.