refactor(sequencer): generate SequencerBlock after transaction execution in proposal phase
#1562
Conversation
…ock can be constructed during proposal
| // cache of results of executing of transactions in `process_proposal`. | ||
| // cleared at the end of the block. | ||
| finalize_block: Option<abci::response::FinalizeBlock>, |
There was a problem hiding this comment.
Should we utilize the ephemeral storage within cnidarium vs adding to state on the app? I know @SuperFluffy has been trying to decrease our global state bloat.
There was a problem hiding this comment.
this is functionally the same - it's an object stored in memory and cleared at the start of each block. the ephemeral store is also only in memory and is cleared at the end of each block. is the suggestion to have less fields within the App struct?
There was a problem hiding this comment.
I agree that we should employ the ephemeral store more rather than adding fields to app.
There is a tendency to add more and more fields to App that need to be actively set/unset, which is not clean (and I believe with which we had issues in the past).
For a reader it's also better to have a single source of truth of effects that happen during block building, rather than finding some in the ephemeral store and some in the "global" app state.
I should note that this applies to all the other fields as well.
There was a problem hiding this comment.
moved the fields to the ephemeral object store here: 591fd94
this is nice because the fields are automatically cleared when update_state_for_new_round is called, however having to get a StateDelta for writing to it is bit more cumbersome in parts (putting the execution_results in prepare_proposal)
| @@ -458,6 +482,16 @@ impl App { | |||
| ); | |||
|
|
|||
| self.executed_proposal_hash = process_proposal.hash; | |||
| self.post_execute_transactions( | |||
There was a problem hiding this comment.
Why not pass the entire process_proposal to this method?
There was a problem hiding this comment.
this is also called in finalize_block potentially, so it might not have process_proposal available
There was a problem hiding this comment.
This test is so complex, I don't know how to evaluate this change to it.
There was a problem hiding this comment.
yeah :/ i didn't write the test, just modified it/added some asserts for sanity when updating it
| pub(crate) async fn finalize_block( | ||
| /// this must be called after a block's transactions are executed. | ||
| #[instrument(name = "App::post_execute_transactions", skip_all)] | ||
| async fn post_execute_transactions( | ||
| &mut self, |
There was a problem hiding this comment.
Hard to follow all the side effects in this function.
There was a problem hiding this comment.
this function is basically everything that was in finalize_block after txs were executed (minus mempool cleaning, as that needs to be done only when the block is actually finalized)
| // use a dummy app hash here - the actual app hash will be filled out in finalize_block. | ||
| let dummy_app_hash = AppHash::default(); | ||
|
|
||
| let finalize_block = abci::response::FinalizeBlock { |
There was a problem hiding this comment.
It's surprising that this method changes the internal state of self.finalize_block, but the finalize_block takes a finalize_block argument and then mutates the finalize_block field that some other part of the stack changed.
There was a problem hiding this comment.
i could change this to be some intermediate type that doesn't have the app hash yet, which is turned into finalize_block response only in finalize_block. would that be easier to follow?
the changes are because |
noot
force-pushed
the
noot/sequencer-builder-api
branch
from
06ea6d2
to
591fd94
Compare
Summary
refactor the sequencer app to generate and store the resulting
SequencerBlockafter transaction execution even in the proposal phase.Background
previously, we were only generating the
SequencerBlockinfinalize_block, however with the upcoming builder APIs (#1519) we require the (proposed)SequencerBlockto be available after execution in the proposal phase.Changes
post_execute_transactionsmethod and move the after-execution logic that generates theSequencerBlockfromfinalize_blockto there.process_proposal.prepare_proposal,post_execute_transactionsis still called inprocess_proposal, as the block hash is not available inprepare_proposal.Testing
existing unit tests pass, app logic was not changed, just refactored
Related Issues
related to #1322