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

chore(sequencer): avoid hashing proposal data in execution state #2044

Merged
merged 1 commit into from
Mar 19, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
avoid hashing proposal data
Fraser999 committed Mar 18, 2025

Verified

This commit was signed with the committer’s verified signature.
Fraser999 Fraser Hutchison
commit 13551569e11461c0090c9ece988901d035959308
654 changes: 407 additions & 247 deletions crates/astria-sequencer/src/app/execution_state.rs

Large diffs are not rendered by default.

30 changes: 18 additions & 12 deletions crates/astria-sequencer/src/app/mod.rs
Original file line number Diff line number Diff line change
@@ -106,8 +106,8 @@ use crate::{
EventBusSubscription,
},
execution_state::{
ExecutionFingerprintData,
ExecutionState,
ExecutionStateMachine,
},
},
assets::StateWriteExt as _,
@@ -179,7 +179,7 @@ pub(crate) struct App {
//
// Used to avoid double execution of transactions across ABCI calls, as well as
// to indicate when we must clear and re-execute (ie if round has changed).
execution_state: ExecutionState,
execution_state: ExecutionStateMachine,

// This is set when a `FeeChange` or `FeeAssetChange` action is seen in a block to flag
// to the mempool to recost all transactions.
@@ -233,7 +233,7 @@ impl App {
Ok(Self {
state,
mempool,
execution_state: ExecutionState::new(),
execution_state: ExecutionStateMachine::new(),
recost_mempool: false,
write_batch: None,
app_hash,
@@ -321,7 +321,7 @@ impl App {
// this also clears the ephemeral storage.
self.state = Arc::new(StateDelta::new(storage.latest_snapshot()));

self.execution_state = ExecutionState::new();
self.execution_state = ExecutionStateMachine::new();
}

/// Generates a commitment to the `sequence::Actions` in the block's transactions.
@@ -398,23 +398,23 @@ impl App {
// Check the proposal against the prepared proposal fingerprint.
let skip_execution = self
.execution_state
.check_if_prepared_proposal(&process_proposal);
.check_if_prepared_proposal(process_proposal.clone());

// Based on the status after the check, a couple of logs and metrics may
// be updated or emitted.
match self.execution_state.data() {
// The proposal was prepared by this node, so we skip execution.
ExecutionFingerprintData::PreparedValid(_) => {
ExecutionState::PreparedValid(_) => {
trace!("skipping process_proposal as we are the proposer for this block");
}
ExecutionFingerprintData::Prepared(_) => {
ExecutionState::Prepared(_) => {
bail!("prepared proposal fingerprint was not validated, this should not happen")
}
// We have a cached proposal from prepare proposal, but it does not match
// the current proposal. We should clear the cache and execute proposal.
//
// This can happen in HA nodes, but if happening in single nodes likely a bug.
ExecutionFingerprintData::CheckedPreparedMismatch(_) => {
ExecutionState::CheckedPreparedMismatch(_) => {
self.metrics.increment_process_proposal_skipped_proposal();
trace!(
"there was a previously prepared proposal cached, but did not match current \
@@ -424,9 +424,15 @@ impl App {
}
// There was a previously executed full block cached, likely indicates
// a new round of voting has started. Previous proposal may have failed.
ExecutionFingerprintData::ExecutedBlock(_, proposal_hash)
| ExecutionFingerprintData::CheckedExecutedBlockMismatch(_, proposal_hash) => {
if proposal_hash.is_none() {
ExecutionState::ExecutedBlock {
cached_block_hash: _,
cached_proposal,
}
| ExecutionState::CheckedExecutedBlockMismatch {
cached_block_hash: _,
cached_proposal,
} => {
if cached_proposal.is_none() {
trace!(
"there was a previously executed block cached, but no proposal hash, will \
clear and execute"
@@ -439,7 +445,7 @@ impl App {
}
}
// No cached proposal, nothing to do for logging. Common case for validator voting.
ExecutionFingerprintData::Unset => {}
ExecutionState::Unset => {}
}

// If we can skip execution just fetch the cache, otherwise need to run the execution.
Original file line number Diff line number Diff line change
@@ -3,38 +3,15 @@ source: crates/astria-sequencer/src/app/execution_state.rs
expression: state.data()
---
CheckedPreparedMismatch(
[
101,
190,
43,
136,
179,
22,
222,
141,
168,
27,
166,
224,
219,
18,
171,
174,
17,
45,
128,
121,
227,
158,
53,
225,
197,
72,
161,
248,
229,
43,
5,
106,
],
CachedProposal {
time: Time(
2025-03-12 0:44:59.000000032,
),
proposer_address: account::Id(0909090909090909090909090909090909090909),
txs: [],
proposed_last_commit: None,
misbehavior: [],
height: block::Height(1),
next_validators_hash: Hash::None,
},
)
Original file line number Diff line number Diff line change
@@ -3,38 +3,15 @@ source: crates/astria-sequencer/src/app/execution_state.rs
expression: state.data()
---
CheckedPreparedMismatch(
[
101,
190,
43,
136,
179,
22,
222,
141,
168,
27,
166,
224,
219,
18,
171,
174,
17,
45,
128,
121,
227,
158,
53,
225,
197,
72,
161,
248,
229,
43,
5,
106,
],
CachedProposal {
time: Time(
2025-03-12 0:44:59.000000032,
),
proposer_address: account::Id(0909090909090909090909090909090909090909),
txs: [],
proposed_last_commit: None,
misbehavior: [],
height: block::Height(1),
next_validators_hash: Hash::None,
},
)
Original file line number Diff line number Diff line change
@@ -1,40 +1,18 @@
---
source: crates/astria-sequencer/src/app/execution_state.rs
assertion_line: 417
expression: state.data()
---
PreparedValid(
[
101,
190,
43,
136,
179,
22,
222,
141,
168,
27,
166,
224,
219,
18,
171,
174,
17,
45,
128,
121,
227,
158,
53,
225,
197,
72,
161,
248,
229,
43,
5,
106,
],
CachedProposal {
time: Time(
2025-03-12 0:44:59.000000032,
),
proposer_address: account::Id(0909090909090909090909090909090909090909),
txs: [],
proposed_last_commit: None,
misbehavior: [],
height: block::Height(1),
next_validators_hash: Hash::None,
},
)
Original file line number Diff line number Diff line change
@@ -3,8 +3,8 @@ source: crates/astria-sequencer/src/app/execution_state.rs
expression: errors
---
[
"executed block fingerprint attempted to be set before prepared proposal fingerprint validated.",
"executed block fingerprint shouldn't be set after invalid check.",
"executed block fingerprint attempted to be set again.",
"executed block fingerprint shouldn't be set after invalid check.",
"executed block state attempted to be set before prepared proposal state validated",
"executed block state shouldn't be set after invalid check",
"executed block state attempted to be set again",
"executed block state shouldn't be set after invalid check",
]
Original file line number Diff line number Diff line change
@@ -1,40 +1,21 @@
---
source: crates/astria-sequencer/src/app/execution_state.rs
assertion_line: 370
expression: state.data()
---
Prepared(
[
101,
190,
43,
136,
179,
22,
222,
141,
168,
27,
166,
224,
219,
18,
171,
174,
17,
45,
128,
121,
227,
158,
53,
225,
197,
72,
161,
248,
229,
43,
5,
106,
],
CachedProposal {
time: Time(
2025-03-12 0:44:59.000000032,
),
proposer_address: account::Id(0909090909090909090909090909090909090909),
txs: [
"010203",
"fdfeff",
],
proposed_last_commit: None,
misbehavior: [],
height: block::Height(1),
next_validators_hash: Hash::None,
},
)
36 changes: 17 additions & 19 deletions crates/astria-sequencer/src/app/tests_app/mod.rs
Original file line number Diff line number Diff line change
@@ -934,7 +934,7 @@ async fn app_proposal_fingerprint_triggers_update() {
app.commit(storage.clone()).await;

// Commit should clear the fingerprint
assert_eq!(app.execution_state.data(), ExecutionFingerprintData::Unset);
assert_eq!(*app.execution_state.data(), ExecutionState::Unset);

let amount = 100;
let lock_action = BridgeLock {
@@ -1041,20 +1041,21 @@ async fn app_proposal_fingerprint_triggers_update() {
app.prepare_proposal(prepare_proposal.clone(), storage.clone())
.await
.unwrap();
let ExecutionFingerprintData::Prepared(prepare_fingerprint_hash) = app.execution_state.data()
else {
panic!("should be a prepared fingerprint")
let ExecutionState::Prepared(cached_proposal) = app.execution_state.data().clone() else {
panic!("should be in prepared state")
};
app.process_proposal(match_process_proposal.clone(), storage.clone())
.await
.unwrap();
let expected_full_fingerprint =
ExecutionFingerprintData::ExecutedBlock(raw_hash, Some(prepare_fingerprint_hash));
assert_eq!(app.execution_state.data(), expected_full_fingerprint);
let expected_state = ExecutionState::ExecutedBlock {
cached_block_hash: raw_hash,
cached_proposal: Some(cached_proposal),
};
assert_eq!(*app.execution_state.data(), expected_state);
app.finalize_block(finalize_block.clone(), storage.clone())
.await
.unwrap();
assert_eq!(app.execution_state.data(), expected_full_fingerprint);
assert_eq!(*app.execution_state.data(), expected_state);

// Call PrepareProposal, then ProcessProposal where the proposals do not match
// - validate the prepared proposal fingerprint
@@ -1067,15 +1068,15 @@ async fn app_proposal_fingerprint_triggers_update() {
app.process_proposal(non_match_process_proposal.clone(), storage.clone())
.await
.unwrap();
let expected_full_fingerprint = ExecutionFingerprintData::ExecutedBlock(raw_hash, None);
assert_eq!(
ExecutionFingerprintData::ExecutedBlock(raw_hash, None),
app.execution_state.data()
);
let expected_state = ExecutionState::ExecutedBlock {
cached_block_hash: raw_hash,
cached_proposal: None,
};
assert_eq!(*app.execution_state.data(), expected_state);
app.finalize_block(finalize_block.clone(), storage.clone())
.await
.unwrap();
assert_eq!(expected_full_fingerprint, app.execution_state.data());
assert_eq!(*app.execution_state.data(), expected_state);

// Cannot use fingerprint to jump from prepare proposal straight to finalize block
// - the fingerprint at the end of finalize block should not have the prepare proposal data
@@ -1085,12 +1086,9 @@ async fn app_proposal_fingerprint_triggers_update() {
app.finalize_block(finalize_block.clone(), storage.clone())
.await
.unwrap();
assert_eq!(
app.execution_state.data(),
ExecutionFingerprintData::ExecutedBlock(raw_hash, None)
);
assert_eq!(*app.execution_state.data(), expected_state);

// Calling update state for new round should reset key
app.update_state_for_new_round(&storage);
assert_eq!(app.execution_state.data(), ExecutionFingerprintData::Unset);
assert_eq!(*app.execution_state.data(), ExecutionState::Unset);
}
Loading