Skip to content
Open
Show file tree
Hide file tree
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
51 changes: 35 additions & 16 deletions crates/apollo_batcher/src/batcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,10 @@ use indexmap::{IndexMap, IndexSet};
#[cfg(test)]
use mockall::automock;
use starknet_api::block::{BlockHash, BlockNumber};
use starknet_api::block_hash::block_hash_calculator::PartialBlockHashComponents;
use starknet_api::block_hash::state_diff_hash::calculate_state_diff_hash;
use starknet_api::block_hash::block_hash_calculator::{
PartialBlockHash,
PartialBlockHashComponents,
};
use starknet_api::consensus_transaction::InternalConsensusTransaction;
use starknet_api::core::{ContractAddress, GlobalRoot, Nonce};
use starknet_api::state::{StateNumber, ThinStateDiff};
Expand Down Expand Up @@ -823,7 +825,21 @@ impl Batcher {
);
trace!("Rejected transactions: {:#?}, State diff: {:#?}.", rejected_tx_hashes, state_diff);

let state_diff_commitment = calculate_state_diff_hash(&state_diff);
if let StorageCommitmentBlockHash::Partial(ref components) = &storage_commitment_block_hash
{
self.prev_proposal_commitment = Some((
height,
ProposalCommitment {
partial_block_hash: PartialBlockHash::from_partial_block_hash_components(
components,
)
.map_err(|e| {
error!("Failed to compute partial block hash: {}", e);
BatcherError::InternalError
})?,
},
));
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting cache before commit causes panic on retry

Medium Severity

self.prev_proposal_commitment is now set before commit_proposal writes to storage, whereas the old code set it after. If commit_proposal fails and returns an error, prev_proposal_commitment already holds Some((height, ...)) for the uncommitted height. On a retry of decision_reached for the same height, get_parent_proposal_commitment finds this stale entry with h == height but needs prev_height == height - 1, triggering the assert_eq! and panicking.

Additional Locations (1)

Fix in Cursor Fix in Web


// Commit the proposal to the storage.
self.storage_writer
Expand All @@ -833,8 +849,6 @@ impl Batcher {
BatcherError::InternalError
})?;
info!("Successfully committed proposal for block {} to storage.", height);
self.prev_proposal_commitment =
Some((height, ProposalCommitment { state_diff_commitment }));

// Notify the L1 provider of the new block.
let rejected_l1_handler_tx_hashes = rejected_tx_hashes
Expand Down Expand Up @@ -1131,25 +1145,30 @@ impl Batcher {
Ok(Some(commitment))
}
None => {
// Parent proposal commitment is not cached. Compute it from the stored state diff.
let mut state_diff = self
// Parent proposal commitment is not cached. Read partial block hash
// components from storage and compute the partial block hash.
let (_, components) = self
.storage_reader
.get_state_diff(prev_height)
.get_parent_hash_and_partial_block_hash_components(prev_height)
.map_err(|err| {
error!(
"Failed to read state diff for previous height {prev_height}: {}",
"Failed to read partial block hash components for previous height \
{prev_height}: {}",
err
);
BatcherError::InternalError
})?
.expect("Missing state diff for previous height.");

// Enforcing no deprecated classes (since v0.14.0).
// TODO(dafna): Remove once the state sync bug is fixed.
state_diff.deprecated_declared_classes = Vec::new();
})?;
let components =
components.expect("Missing partial block hash components for previous height.");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panic on cache miss for ParentHash blocks

Low Severity

When prev_proposal_commitment is None (e.g. after node restart), get_parent_proposal_commitment falls back to reading from storage. For blocks stored via StorageCommitmentBlockHash::ParentHash (pre-0.13.2 synced blocks), get_parent_hash_and_partial_block_hash_components returns None for the components, and the .expect(...) panics. The old code gracefully handled all block types by reading the state diff with get_state_diff and computing the hash from it.

Fix in Cursor Fix in Web


Ok(Some(ProposalCommitment {
state_diff_commitment: calculate_state_diff_hash(&state_diff),
partial_block_hash: PartialBlockHash::from_partial_block_hash_components(
&components,
)
.map_err(|e| {
error!("Failed to compute partial block hash: {}", e);
BatcherError::InternalError
})?,
}))
}
}
Expand Down
37 changes: 34 additions & 3 deletions crates/apollo_batcher/src/batcher_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,10 @@ use starknet_api::block::{
BlockNumber,
StarknetVersion,
};
use starknet_api::block_hash::block_hash_calculator::PartialBlockHashComponents;
use starknet_api::block_hash::state_diff_hash::calculate_state_diff_hash;
use starknet_api::block_hash::block_hash_calculator::{
PartialBlockHash,
PartialBlockHashComponents,
};
use starknet_api::consensus_transaction::InternalConsensusTransaction;
use starknet_api::core::{ClassHash, CompiledClassHash, GlobalRoot, Nonce};
use starknet_api::state::ThinStateDiff;
Expand Down Expand Up @@ -164,7 +166,12 @@ async fn proposal_commitment() -> ProposalCommitment {
}

fn parent_proposal_commitment() -> ProposalCommitment {
ProposalCommitment { state_diff_commitment: calculate_state_diff_hash(&test_state_diff()) }
ProposalCommitment {
partial_block_hash: PartialBlockHash::from_partial_block_hash_components(
&PartialBlockHashComponents::default(),
)
.expect("default partial block hash components are valid"),
}
}

fn validate_block_input(proposal_id: ProposalId) -> ValidateBlockInput {
Expand Down Expand Up @@ -1399,6 +1406,14 @@ async fn decision_reached() {
)
.returning(|_, _, _| Ok(()));

mock_dependencies
.storage_reader
.expect_get_parent_hash_and_partial_block_hash_components()
.with(eq(INITIAL_HEIGHT.prev().unwrap()))
.returning(|_| {
Ok((Some(BlockHash::default()), Some(PartialBlockHashComponents::default())))
});

mock_create_builder_for_propose_block(
&mut mock_dependencies.clients.block_builder_factory,
vec![],
Expand Down Expand Up @@ -1471,6 +1486,14 @@ async fn test_execution_info_order_is_kept() {
.collect();
verify_indexed_execution_infos(&execution_infos);

mock_dependencies
.storage_reader
.expect_get_parent_hash_and_partial_block_hash_components()
.with(eq(INITIAL_HEIGHT.prev().unwrap()))
.returning(|_| {
Ok((Some(BlockHash::default()), Some(PartialBlockHashComponents::default())))
});

mock_create_builder_for_propose_block(
&mut mock_dependencies.clients.block_builder_factory,
vec![],
Expand Down Expand Up @@ -1550,6 +1573,14 @@ async fn decision_reached_return_success_when_l1_commit_block_fails(

mock_dependencies.clients.mempool_client.expect_commit_block().returning(|_| Ok(()));

mock_dependencies
.storage_reader
.expect_get_parent_hash_and_partial_block_hash_components()
.with(eq(INITIAL_HEIGHT.prev().unwrap()))
.returning(|_| {
Ok((Some(BlockHash::default()), Some(PartialBlockHashComponents::default())))
});

mock_create_builder_for_propose_block(
&mut mock_dependencies.clients.block_builder_factory,
vec![],
Expand Down
9 changes: 5 additions & 4 deletions crates/apollo_batcher/src/block_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ use mockall::automock;
use starknet_api::block::{BlockHashAndNumber, BlockInfo};
use starknet_api::block_hash::block_hash_calculator::{
calculate_block_commitments,
PartialBlockHash,
PartialBlockHashComponents,
TransactionHashingData,
};
Expand Down Expand Up @@ -187,10 +188,10 @@ impl BlockExecutionArtifacts {

pub fn commitment(&self) -> ProposalCommitment {
ProposalCommitment {
state_diff_commitment: self
.partial_block_hash_components
.header_commitments
.state_diff_commitment,
partial_block_hash: PartialBlockHash::from_partial_block_hash_components(
&self.partial_block_hash_components,
)
.expect("partial_block_hash_components are valid for current version"),
}
}

Expand Down
8 changes: 4 additions & 4 deletions crates/apollo_batcher_types/src/batcher_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ use chrono::prelude::*;
use indexmap::IndexMap;
use serde::{Deserialize, Serialize};
use starknet_api::block::{BlockHashAndNumber, BlockInfo, BlockNumber};
use starknet_api::block_hash::block_hash_calculator::BlockHeaderCommitments;
use starknet_api::block_hash::block_hash_calculator::{BlockHeaderCommitments, PartialBlockHash};
use starknet_api::consensus_transaction::InternalConsensusTransaction;
use starknet_api::core::StateDiffCommitment;
use starknet_api::execution_resources::GasAmount;
use starknet_api::state::ThinStateDiff;
use starknet_api::transaction::TransactionHash;
Expand All @@ -36,9 +35,10 @@ pub struct ProposalId(pub u64);

pub type Round = u32;

#[derive(Clone, Debug, Copy, Default, Eq, PartialEq, Serialize, Deserialize)]
/// Commitment identifying a proposed block (its partial block hash).
#[derive(Clone, Copy, Debug, Default, Eq, Hash, PartialEq, Serialize, Deserialize)]
pub struct ProposalCommitment {
pub state_diff_commitment: StateDiffCommitment,
pub partial_block_hash: PartialBlockHash,
}

#[derive(Clone, Debug, Serialize, Deserialize)]
Expand Down
2 changes: 1 addition & 1 deletion crates/apollo_consensus_orchestrator/src/build_proposal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ async fn get_proposal_content(
})?;
}
GetProposalContent::Finished { id, final_n_executed_txs } => {
let proposal_commitment = ProposalCommitment(id.state_diff_commitment.0.0);
let proposal_commitment = ProposalCommitment(id.partial_block_hash.0);
content = truncate_to_executed_txs(&mut content, final_n_executed_txs);

info!(
Expand Down
13 changes: 5 additions & 8 deletions crates/apollo_consensus_orchestrator/src/build_proposal_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,12 @@ use apollo_consensus::types::ProposalCommitment as ConsensusProposalCommitment;
use apollo_infra::component_client::ClientError;
use apollo_transaction_converter::{MockTransactionConverterTrait, TransactionConverterError};
use assert_matches::assert_matches;
use starknet_api::block_hash::block_hash_calculator::PartialBlockHash;
use starknet_api::core::ClassHash;
use tokio_util::task::AbortOnDropHandle;

use crate::build_proposal::{build_proposal, BuildProposalError};
use crate::test_utils::{
create_proposal_build_arguments,
INTERNAL_TX_BATCH,
STATE_DIFF_COMMITMENT,
};
use crate::test_utils::{create_proposal_build_arguments, INTERNAL_TX_BATCH, PARTIAL_BLOCK_HASH};

#[tokio::test]
async fn build_proposal_succeed() {
Expand All @@ -28,7 +25,7 @@ async fn build_proposal_succeed() {
proposal_args.deps.batcher.expect_get_proposal_content().returning(|_| {
Ok(GetProposalContentResponse {
content: GetProposalContent::Finished {
id: ProposalCommitment { state_diff_commitment: STATE_DIFF_COMMITMENT },
id: ProposalCommitment { partial_block_hash: PartialBlockHash(PARTIAL_BLOCK_HASH) },
final_n_executed_txs: 0,
},
})
Expand All @@ -37,7 +34,7 @@ async fn build_proposal_succeed() {
tokio::time::sleep(Duration::from_millis(100)).await;

let res = build_proposal(proposal_args.into()).await.unwrap();
assert_eq!(res, ConsensusProposalCommitment::default());
assert_eq!(res, ConsensusProposalCommitment(PARTIAL_BLOCK_HASH));
}

#[tokio::test]
Expand Down Expand Up @@ -112,7 +109,7 @@ async fn cende_fail() {
proposal_args.deps.batcher.expect_get_proposal_content().times(1).returning(|_| {
Ok(GetProposalContentResponse {
content: GetProposalContent::Finished {
id: ProposalCommitment { state_diff_commitment: STATE_DIFF_COMMITMENT },
id: ProposalCommitment { partial_block_hash: PartialBlockHash(PARTIAL_BLOCK_HASH) },
final_n_executed_txs: 0,
},
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ impl SequencerConsensusContext {
proposal_commitment: commitment,
parent_proposal_commitment: central_objects
.parent_proposal_commitment
.map(|commitment| ProposalCommitment(commitment.state_diff_commitment.0.0)),
.map(|commitment| ProposalCommitment(commitment.partial_block_hash.0)),
})
.await
{
Expand Down
Loading
Loading