refactor(sequencer): generate `SequencerBlock` after transaction execution in proposal phase #1562

noot · 2024-09-25T19:01:17Z

Summary

refactor the sequencer app to generate and store the resulting SequencerBlock after transaction execution even in the proposal phase.

Background

previously, we were only generating the SequencerBlock in finalize_block, however with the upcoming builder APIs (#1519) we require the (proposed) SequencerBlock to be available after execution in the proposal phase.

Changes

create a post_execute_transactions method and move the after-execution logic that generates the SequencerBlock from finalize_block to there.
call this method after transaction execution in process_proposal.
if txs were executed in prepare_proposal, post_execute_transactions is still called in process_proposal, as the block hash is not available in prepare_proposal.

Testing

existing unit tests pass, app logic was not changed, just refactored

Related Issues

related to #1322

…ock can be constructed during proposal

…r-builder-api

joroshiba · 2024-09-25T19:21:15Z

crates/astria-sequencer/src/app/mod.rs

+    // cache of results of executing of transactions in `process_proposal`.
+    // cleared at the end of the block.
+    finalize_block: Option<abci::response::FinalizeBlock>,


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.

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?

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.

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)

SuperFluffy · 2024-09-27T12:11:34Z

crates/astria-sequencer/src/app/mod.rs

@@ -458,6 +482,16 @@ impl App {
        );

        self.executed_proposal_hash = process_proposal.hash;
+        self.post_execute_transactions(


Why not pass the entire process_proposal to this method?

this is also called in finalize_block potentially, so it might not have process_proposal available

SuperFluffy · 2024-09-27T12:16:30Z

crates/astria-sequencer/src/app/tests_app/mempool.rs

This test is so complex, I don't know how to evaluate this change to it.

yeah :/ i didn't write the test, just modified it/added some asserts for sanity when updating it

SuperFluffy · 2024-09-27T12:19:18Z

crates/astria-sequencer/src/app/mod.rs

-    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,


Hard to follow all the side effects in this function.

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)

SuperFluffy · 2024-09-27T12:22:09Z

crates/astria-sequencer/src/app/mod.rs

+        // 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 {


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.

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?

I suppose it's fine. We should probably sit down and refactor of all of this. It looks like we have a bit of code duplication going on.

SuperFluffy

I have a hard time following the changes, which might be an artifact of how app execution is structured.

I am also surprised that making sequencer blocks available during an earlier phase leads to changes in mempool tests.

noot · 2024-09-27T19:31:42Z

I am also surprised that making sequencer blocks available during an earlier phase leads to changes in mempool tests.

the changes are because process_proposal must now also be called if prepare_proposal is called (this is the normal ABCI flow btw). that's really the only change to the tests

…r-builder-api

…l store

Fraser999

There's one mention of self.execution_results left behind in the doc comments for execute_transactions_prepare_proposal which would be good to have updated.

I also think we should look to abstract the usage of the ephemeral store behind our own API provided by the extension traits in app/state_ext.rs (similar to how cached block deposits are handled in the bridge extension traits). We'd also then keep the keys for these in app::state_ext along with the others. But I can handle changing to that as part of the ongoing storage updates.

Neither of these are blocking though, so approving as is.

…r-builder-api

SuperFluffy · 2024-10-04T15:33:52Z

crates/astria-sequencer/src/app/mod.rs

@@ -629,15 +662,16 @@ impl App {
        self.metrics
            .set_transactions_in_mempool_total(self.mempool.len().await);

-        self.execution_results = Some(execution_results);
+        let mut state_tx = Arc::try_begin_transaction(&mut self.state)


This looks complex. Can you add comment above explaining why it's necessary? Seems like a hack around cnidarium not providing us with something we need.

I.e. // HACK: <reason> or // XXX: <reason>

* main: refactor(sequencer): generate `SequencerBlock` after transaction execution in proposal phase (#1562) fix(sequencer)!: rewrite check_tx to be more efficient and fix regression (#1515) fix(charts): hermes monitoring and prometheus rules (#1564)

noot added 3 commits September 24, 2024 17:23

refactor of App to have post_execute_transactions hook so SequencerBl…

9f00b18

…ock can be constructed during proposal

clean up and fix unit tests

62d76f6

cleanup

621bb09

noot requested a review from a team as a code owner September 25, 2024 19:01

noot requested a review from Fraser999 September 25, 2024 19:01

github-actions bot added the sequencer pertaining to the astria-sequencer crate label Sep 25, 2024

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

82c0bf0

…r-builder-api

joroshiba reviewed Sep 25, 2024

View reviewed changes

merge w main

3609462

SuperFluffy reviewed Sep 27, 2024

View reviewed changes

noot added 2 commits September 27, 2024 16:13

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

1a01c09

…r-builder-api

move execution_results and finalize_block from App to object ephemera…

591fd94

…l store

noot force-pushed the noot/sequencer-builder-api branch from 06ea6d2 to 591fd94 Compare September 27, 2024 20:50

noot requested review from joroshiba and SuperFluffy September 27, 2024 20:51

noot mentioned this pull request Sep 30, 2024

feat(sequencer): integrate connect oracle and vote extension logic #1236

Open

merge w main

30e0200

Fraser999 approved these changes Oct 4, 2024

View reviewed changes

noot added 2 commits October 4, 2024 11:24

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

7baa03f

…r-builder-api

update comments

4f9bfe0

SuperFluffy reviewed Oct 4, 2024

View reviewed changes

add comment

09001ad

noot added this pull request to the merge queue Oct 7, 2024

Merged via the queue into main with commit f4dba95 Oct 7, 2024
44 checks passed

noot deleted the noot/sequencer-builder-api branch October 7, 2024 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(sequencer): generate `SequencerBlock` after transaction execution in proposal phase #1562

refactor(sequencer): generate `SequencerBlock` after transaction execution in proposal phase #1562

noot commented Sep 25, 2024 •

edited

Loading

joroshiba Sep 25, 2024

noot Sep 25, 2024 •

edited

Loading

SuperFluffy Sep 27, 2024 •

edited

Loading

noot Sep 27, 2024

SuperFluffy Sep 27, 2024 •

edited

Loading

noot Sep 27, 2024

SuperFluffy Sep 27, 2024

noot Sep 27, 2024

SuperFluffy Sep 27, 2024

noot Sep 27, 2024

SuperFluffy Sep 27, 2024

noot Sep 27, 2024

SuperFluffy Oct 4, 2024

SuperFluffy left a comment

noot commented Sep 27, 2024

Fraser999 left a comment

SuperFluffy Oct 4, 2024 •

edited

Loading

refactor(sequencer): generate SequencerBlock after transaction execution in proposal phase #1562

refactor(sequencer): generate SequencerBlock after transaction execution in proposal phase #1562

Conversation

noot commented Sep 25, 2024 • edited Loading

Summary

Background

Changes

Testing

Related Issues

Choose a reason for hiding this comment

noot Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

SuperFluffy Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy left a comment

Choose a reason for hiding this comment

noot commented Sep 27, 2024

Fraser999 left a comment

Choose a reason for hiding this comment

SuperFluffy Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

refactor(sequencer): generate `SequencerBlock` after transaction execution in proposal phase #1562

refactor(sequencer): generate `SequencerBlock` after transaction execution in proposal phase #1562

noot commented Sep 25, 2024 •

edited

Loading

noot Sep 25, 2024 •

edited

Loading

SuperFluffy Sep 27, 2024 •

edited

Loading

SuperFluffy Sep 27, 2024 •

edited

Loading

SuperFluffy Oct 4, 2024 •

edited

Loading