feat(pending): pending block sealed instead of re-added to mempool #468
Conversation
This file contains 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
Trantorian1
added
feature
Trantorian1
force-pushed
the
feat/avoid_pending_re_exec
branch
from
05fd24e to
74f48fc
Compare
Mohiiit
reviewed
Jan 20, 2025
There was a problem hiding this comment.
Great work! The test cases are really clean and well-structured. I have a couple of thoughts:
- Should we consider adding E2E tests for this? For example, we could simulate spawning Madara, shutting it down mid-process, and then restarting it to verify that everything works as expected.
- I found the stages a bit confusing. In most test cases, when we store the pending block in stage 2, it’s mentioned that this simulates stopping and restarting the node. However, isn’t that more related to the block production task? Maybe I’m misunderstanding, but this part could use some clarification.
| #[rstest::rstest] | ||
| #[tokio::test] | ||
| #[allow(clippy::too_many_arguments)] | ||
| async fn block_prod_pending_close_on_startup_pass( |
| .get_block_state_diff(&mp_block::BlockId::Tag(mp_block::BlockTag::Latest)) | ||
| .expect("Failed to retrieve latest state diff from db") | ||
| .expect("Missing latest state diff"); | ||
| assert_eq!(state_diff, state_diff); |
There was a problem hiding this comment.
we should equate with pending_state_diff here right?
| #[rstest::rstest] | ||
| #[tokio::test] | ||
| #[allow(clippy::too_many_arguments)] | ||
| async fn block_prod_pending_close_on_startup_pass_on_top( |
There was a problem hiding this comment.
isn't this test very similar to the one above?
| #[rstest::rstest] | ||
| #[tokio::test] | ||
| #[allow(clippy::too_many_arguments)] | ||
| async fn block_prod_pending_close_on_startup_fail_missing_class( |
There was a problem hiding this comment.
do we have test case which ensures that the state_diff in the pending state is getting updated with the class?
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.
Important
Most of the changes in this pr are just me adding some tests. Actual logic changes are pretty small.
Pull Request type
What is the current behavior?
Pending block transactions are re-added back to the mempool and re-executed upon startup. This happens in case of graceful or ungraceful shutdown in the middle of block production: the pending block will be saved to db to avoid loss of state.
This leads to several issues:
Resolves: #458
What is the new behavior?
Pending block, alongside the pending state diff, visited segments and declared classes are retrieved from db and closed into a new finalized block.
This means that if a sequencer node is shutdown mid-production, the state contained in the pending block will automatically be persisted upon startup as it own finalized block without re-executing transactions. This also guarantees that any changes to the config will not be applied to transactions executed under different parameters (these will only affect the next block).
Does this introduce a breaking change?
No.