diff --git a/.github/workflows/tests-rs-workspace.yml b/.github/workflows/tests-rs-workspace.yml index 1c159c2c706..6e64434f886 100644 --- a/.github/workflows/tests-rs-workspace.yml +++ b/.github/workflows/tests-rs-workspace.yml @@ -319,7 +319,9 @@ jobs: # wall time) is reduced here to the single comprehensive simulation — # see the doc comment on # run_chain_comprehensive_mixed_operations_with_epoch_change_and_quorum_rotation - # for what it covers. The remaining simulations run in their own phase + # for what it covers — plus the process_proposal collision module, + # which pins a consensus-halt regression and runs in under a second + # combined. The remaining simulations run in their own phase # below on push and nightly only, so a regression in one of them is # caught minutes after merge — the same safety-net pattern as the # shielded phase. Keeping this phase's filter identical across events @@ -352,7 +354,7 @@ jobs: --package keyword-search-contract \ --all-features \ --locked \ - -E 'not test(~shield) and (not binary_id(=drive-abci::strategy_tests) or test(~comprehensive_mixed_operations))' + -E 'not test(~shield) and (not binary_id(=drive-abci::strategy_tests) or test(~comprehensive_mixed_operations) or test(~process_proposal_collision))' env: RUST_MIN_STACK: 4194304 CARGO_PROFILE_DEV_DEBUG: "0" @@ -380,7 +382,7 @@ jobs: --package drive-abci \ --all-features \ --locked \ - -E 'binary_id(=drive-abci::strategy_tests) and not test(~comprehensive_mixed_operations) and not test(~shield)' + -E 'binary_id(=drive-abci::strategy_tests) and not test(~comprehensive_mixed_operations) and not test(~process_proposal_collision) and not test(~shield)' env: RUST_MIN_STACK: 4194304 CARGO_PROFILE_DEV_DEBUG: "0" diff --git a/packages/rs-drive-abci/src/abci/handler/prepare_proposal.rs b/packages/rs-drive-abci/src/abci/handler/prepare_proposal.rs index e85bd7dc285..269abc53a7d 100644 --- a/packages/rs-drive-abci/src/abci/handler/prepare_proposal.rs +++ b/packages/rs-drive-abci/src/abci/handler/prepare_proposal.rs @@ -3,7 +3,7 @@ use crate::abci::AbciError; use crate::error::Error; use crate::execution::engine::consensus_params_update::consensus_params_update; use crate::execution::types::block_execution_context::v0::{ - BlockExecutionContextV0Getters, BlockExecutionContextV0Setters, + BlockExecutionContextV0Getters, BlockExecutionContextV0Setters, ProposerResults, }; use crate::platform_types::block_execution_outcome; use crate::platform_types::block_proposal::v0::BlockProposal; @@ -154,6 +154,12 @@ where .as_ref() .expect("transaction must be started"); + // Taken from the proposal execution is about to consume: process proposal needs them to + // tell this block apart from a competing one at the same height and round, and nothing else + // in the block execution context retains them. + let proposed_app_version = block_proposal.proposed_app_version; + let validator_set_quorum_hash = block_proposal.validator_set_quorum_hash; + // Running the proposal executes all the state transitions for the block let mut run_result = app.platform().run_block_proposal( block_proposal, @@ -280,7 +286,11 @@ where app_version: platform_version.protocol_version as u64, }; - block_execution_context.set_proposer_results(Some(response.clone())); + block_execution_context.set_proposer_results(Some(ProposerResults { + response: response.clone(), + proposed_app_version, + validator_set_quorum_hash, + })); app.block_execution_context() .write() diff --git a/packages/rs-drive-abci/src/abci/handler/process_proposal.rs b/packages/rs-drive-abci/src/abci/handler/process_proposal.rs index 69bae1c5fcd..f1df6687163 100644 --- a/packages/rs-drive-abci/src/abci/handler/process_proposal.rs +++ b/packages/rs-drive-abci/src/abci/handler/process_proposal.rs @@ -3,11 +3,12 @@ use crate::abci::AbciError; use crate::error::Error; use crate::execution::engine::consensus_params_update::consensus_params_update; use crate::execution::types::block_execution_context::v0::{ - BlockExecutionContextV0Getters, BlockExecutionContextV0MutableGetters, + BlockExecutionContextV0Getters, BlockExecutionContextV0MutableGetters, ProposerResults, }; use crate::execution::types::block_state_info::v0::{ BlockStateInfoV0Getters, BlockStateInfoV0Setters, }; +use crate::execution::types::block_state_info::BlockStateInfo; use crate::platform_types::block_execution_outcome; use crate::platform_types::platform_state::PlatformStateV0Methods; use crate::platform_types::state_transitions_processing_result::StateTransitionExecutionResult; @@ -16,6 +17,76 @@ use dpp::dashcore::Network; use dpp::version::TryIntoPlatformVersioned; use tenderdash_abci::proto::abci as proto; use tenderdash_abci::proto::abci::tx_record::TxAction; +use tenderdash_abci::proto::ToMillis; + +/// Compares the proposal our own `PrepareProposal` executed against the one `request` describes, +/// returning the name of the first execution input that differs, or `None` when the two are the +/// same block. +/// +/// Tenderdash computes a block hash only once `PrepareProposal` has returned, so a context it +/// left behind has no hash to match on and identity has to come from the execution inputs +/// instead. Every field `run_block_proposal` reads is compared here, the round excepted — the +/// caller has already established that. The request fields left out are exactly the ones +/// `BlockProposal` discards on conversion — `proposed_last_commit`, `misbehavior`, +/// `next_validators_hash` and the consensus block version — which therefore cannot reach +/// execution or move the app hash. +fn prepared_proposal_difference( + block_state_info: &BlockStateInfo, + proposer_results: &ProposerResults, + request: &proto::RequestProcessProposal, +) -> Option<&'static str> { + if u64::try_from(request.height).ok() != Some(block_state_info.height()) { + return Some("height"); + } + + let request_block_time_ms = request.time.as_ref().and_then(|time| time.to_millis().ok()); + if request_block_time_ms != Some(block_state_info.block_time_ms()) { + return Some("block time"); + } + + if request.proposer_pro_tx_hash != block_state_info.proposer_pro_tx_hash() { + return Some("proposer pro tx hash"); + } + + if request.core_chain_locked_height != block_state_info.core_chain_locked_height() { + return Some("core chain locked height"); + } + + if request.core_chain_lock_update != proposer_results.response.core_chain_lock_update { + return Some("core chain lock update"); + } + + if request.proposed_app_version != proposer_results.proposed_app_version { + return Some("proposed app version"); + } + + if request.quorum_hash != proposer_results.validator_set_quorum_hash { + return Some("validator set quorum hash"); + } + + if request.version.as_ref().map(|version| version.app) + != Some(proposer_results.response.app_version) + { + return Some("consensus app version"); + } + + // The transactions Tenderdash builds the block from are the ones we did not ask it to drop, + // in the order we returned them, so compare the bytes rather than just how many there are. + let prepared_transactions = proposer_results + .response + .tx_records + .iter() + .filter(|record| { + record.action != TxAction::Removed as i32 && record.action != TxAction::Delayed as i32 + }) + .map(|record| &record.tx); + + if !prepared_transactions.eq(request.txs.iter()) { + return Some("transactions"); + } + + None +} pub fn process_proposal<'a, A, C>( app: &A, @@ -37,95 +108,99 @@ where // We were not the proposer, and we should process something new drop_block_execution_context = true; } else if let Some(current_block_hash) = block_state_info.block_hash() { - // There is also the possibility that this block already came in, but tenderdash crashed - // Now tenderdash is sending it again - if let Some(proposal_info) = block_execution_context.proposer_results() { - tracing::debug!( - method = "process_proposal", - "we knew block hash, block execution context already had a proposer result", - ); - // We were the proposer as well, so we have the result in cache - return Ok(proto::ResponseProcessProposal { - status: proto::response_process_proposal::ProposalStatus::Accept.into(), - app_hash: proposal_info.app_hash.clone(), - tx_results: proposal_info.tx_results.clone(), - consensus_param_updates: proposal_info.consensus_param_updates.clone(), - validator_set_update: proposal_info.validator_set_update.clone(), - events: Vec::new(), - }); - } - if current_block_hash.as_slice() == request.hash { - // We were not the proposer, just drop the execution context - tracing::warn!( + // There is also the possibility that this block already came in, but tenderdash crashed + // Now tenderdash is sending it again + if let Some(proposer_results) = block_execution_context.proposer_results() { + tracing::debug!( method = "process_proposal", - "block execution context already existed, but we are running it again for same height {}/round {}", - request.height, - request.round, + "we knew block hash, block execution context already had a proposer result", ); + // We were the proposer as well, so we have the result in cache + return Ok(proto::ResponseProcessProposal { + status: proto::response_process_proposal::ProposalStatus::Accept.into(), + app_hash: proposer_results.response.app_hash.clone(), + tx_results: proposer_results.response.tx_results.clone(), + consensus_param_updates: proposer_results + .response + .consensus_param_updates + .clone(), + validator_set_update: proposer_results + .response + .validator_set_update + .clone(), + events: Vec::new(), + }); + } + + // We were not the proposer, just drop the execution context + tracing::warn!( + method = "process_proposal", + "block execution context already existed, but we are running it again for same height {}/round {}", + request.height, + request.round, + ); drop_block_execution_context = true; } else { - // We are getting a different block hash for a block of the same round - // This is a terrible issue - tracing::error!( + // A different block for the same height and round. With a single honest + // proposer per round this only happens when the block we hold is stale: + // typically our own proposal, built while we were still catching up, and + // now the block the network actually committed at this height and round + // arrives through consensus catch-up. The cached result belongs to the + // other block, so it must neither be served nor block execution; the only + // correct answer is to execute the block we were asked about. + tracing::warn!( method = "process_proposal", block_state_info = ?block_state_info, - "received a process proposal request twice with different hash for height {}/round {}: existing hash {:?}, new hash {:?}", + "received a process proposal request for a different block at the same height {}/round {}: existing hash {}, new hash {}; dropping the existing block execution context and executing the new block", request.height, request.round, - current_block_hash, - request.hash, + hex::encode(current_block_hash), + hex::encode(&request.hash), ); - Err(Error::Abci(AbciError::BadRequest( - "received a process proposal request twice with different hash".to_string(), - )))?; + drop_block_execution_context = true; } } else { - // we were the proposer - let Some(proposal_info) = block_execution_context.proposer_results() else { + // We were the proposer. Tenderdash only computes the block hash after our + // PrepareProposal has returned, so the context we hold carries none and the block + // we prepared has to be recognised by the inputs it executed. + let Some(proposer_results) = block_execution_context.proposer_results() else { Err(Error::Abci(AbciError::BadRequest( "received a process proposal request twice".to_string(), )))? }; - let expected_transactions = proposal_info - .tx_records - .iter() - .filter_map(|record| { - if record.action == TxAction::Removed as i32 - || record.action == TxAction::Delayed as i32 - { - None - } else { - Some(&record.tx) - } - }) - .collect::>(); - - // While it is true that the length could be same, seeing how this is such a rare situation - // It does not seem worth to deal with situations where the length is the same but the transactions have changed - if expected_transactions.len() == request.txs.len() - && proposal_info.core_chain_lock_update == request.core_chain_lock_update + if let Some(difference) = + prepared_proposal_difference(block_state_info, proposer_results, &request) { - let (app_hash, tx_results, consensus_param_updates, validator_set_update) = { - tracing::debug!( - method = "process_proposal", - "we didn't know block hash (we were most likely proposer), block execution context already had a proposer result {:?}", - proposal_info, - ); - - // Cloning all required properties from proposal_info and then dropping it - let app_hash = proposal_info.app_hash.clone(); - let tx_results = proposal_info.tx_results.clone(); - let consensus_param_updates = proposal_info.consensus_param_updates.clone(); - let validator_set_update = proposal_info.validator_set_update.clone(); - ( - app_hash, - tx_results, - consensus_param_updates, - validator_set_update, - ) - }; + // A different block at the same height and round, reaching us before Tenderdash + // ever told us a hash for the one we prepared. Answering it with the prepared + // app hash is the same failure the hash-aware branch above guards against, so + // take the same way out: discard the stale context and execute what we were + // asked about. + tracing::warn!( + method = "process_proposal", + block_state_info = ?block_state_info, + difference, + "received a process proposal request for a block that is not the one we prepared at the same height {}/round {}; dropping the existing block execution context and executing the new block", + request.height, + request.round, + ); + + drop_block_execution_context = true; + } else { + tracing::debug!( + method = "process_proposal", + "we didn't know block hash (we were most likely proposer), block execution context already had a proposer result {:?}", + proposer_results, + ); + + // Cloning all required properties from the prepared response and then dropping it + let app_hash = proposer_results.response.app_hash.clone(); + let tx_results = proposer_results.response.tx_results.clone(); + let consensus_param_updates = + proposer_results.response.consensus_param_updates.clone(); + let validator_set_update = proposer_results.response.validator_set_update.clone(); // We need to set the block hash block_execution_context @@ -143,14 +218,7 @@ where validator_set_update, events: Vec::new(), }); - } else { - tracing::warn!( - method = "process_proposal", - "we didn't know block hash (we were most likely proposer), block execution context already had a proposer result, but we are requesting a different amount of transactions, dropping the cache", - ); - - drop_block_execution_context = true; - }; + } } } diff --git a/packages/rs-drive-abci/src/execution/types/block_execution_context/mod.rs b/packages/rs-drive-abci/src/execution/types/block_execution_context/mod.rs index 3a866d17c75..42ebb8405e6 100644 --- a/packages/rs-drive-abci/src/execution/types/block_execution_context/mod.rs +++ b/packages/rs-drive-abci/src/execution/types/block_execution_context/mod.rs @@ -4,14 +4,13 @@ pub mod v0; use crate::error::Error; use crate::execution::types::block_execution_context::v0::{ BlockExecutionContextV0Getters, BlockExecutionContextV0MutableGetters, - BlockExecutionContextV0OwnedGetters, BlockExecutionContextV0Setters, + BlockExecutionContextV0OwnedGetters, BlockExecutionContextV0Setters, ProposerResults, }; use crate::execution::types::block_state_info::BlockStateInfo; use crate::platform_types::epoch_info::EpochInfo; use crate::platform_types::platform_state::PlatformState; use crate::platform_types::withdrawal::unsigned_withdrawal_txs::v0::UnsignedWithdrawalTxs; use derive_more::From; -use tenderdash_abci::proto::abci::ResponsePrepareProposal; /// The versioned block execution context #[derive(Debug, From, Clone)] @@ -54,7 +53,7 @@ impl BlockExecutionContextV0Getters for BlockExecutionContext { } } - fn proposer_results(&self) -> Option<&ResponsePrepareProposal> { + fn proposer_results(&self) -> Option<&ProposerResults> { match self { BlockExecutionContext::V0(v0) => v0.proposer_results.as_ref(), } @@ -86,7 +85,7 @@ impl BlockExecutionContextV0Setters for BlockExecutionContext { } } - fn set_proposer_results(&mut self, results: Option) { + fn set_proposer_results(&mut self, results: Option) { match self { BlockExecutionContext::V0(v0) => v0.proposer_results = results, } @@ -112,7 +111,7 @@ impl BlockExecutionContextV0MutableGetters for BlockExecutionContext { } } - fn proposer_results_mut(&mut self) -> Option<&mut ResponsePrepareProposal> { + fn proposer_results_mut(&mut self) -> Option<&mut ProposerResults> { match self { BlockExecutionContext::V0(v0) => v0.proposer_results_mut(), } @@ -147,8 +146,8 @@ impl BlockExecutionContextV0OwnedGetters for BlockExecutionContext { } } - /// Consumes the object and returns the owned `ResponsePrepareProposal`. - fn proposer_results_owned(self) -> Option { + /// Consumes the object and returns the owned `ProposerResults`. + fn proposer_results_owned(self) -> Option { match self { BlockExecutionContext::V0(v0) => v0.proposer_results, } diff --git a/packages/rs-drive-abci/src/execution/types/block_execution_context/v0/mod.rs b/packages/rs-drive-abci/src/execution/types/block_execution_context/v0/mod.rs index dee0160394e..d166a0a96df 100644 --- a/packages/rs-drive-abci/src/execution/types/block_execution_context/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/types/block_execution_context/v0/mod.rs @@ -9,6 +9,27 @@ use dpp::address_funds::PlatformAddress; use dpp::fee::Credits; use tenderdash_abci::proto::abci::ResponsePrepareProposal; +/// What our own `PrepareProposal` left behind for a later `ProcessProposal` to answer from. +/// +/// Tenderdash computes the block hash only once `PrepareProposal` has returned, so the context +/// left behind carries no block hash and `ProcessProposal` has to recognise the prepared block +/// by the inputs it executed instead. Most of those are kept elsewhere already — height, round, +/// block time, proposer and core chain locked height in the block state info, the transactions +/// and the core chain lock update in the response — and the two below are the ones nothing else +/// retains. +#[derive(Debug, Clone)] +pub struct ProposerResults { + /// The response `PrepareProposal` returned, replayed verbatim when the block coming back + /// through `ProcessProposal` is the one that was prepared. + pub response: ResponsePrepareProposal, + /// The app version this node voted for in the block it prepared. + /// `update_validator_proposed_app_version` writes it to state, so it moves the app hash. + pub proposed_app_version: u64, + /// The quorum the prepared block's unsigned withdrawal transactions were built against, + /// and which is asked to sign them on extend vote. + pub validator_set_quorum_hash: [u8; 32], +} + /// V0 of the Block execution context #[derive(Debug, Clone)] pub struct BlockExecutionContextV0 { @@ -22,8 +43,8 @@ pub struct BlockExecutionContextV0 { pub block_address_balance_changes: BTreeMap, /// Block state pub block_platform_state: PlatformState, - /// The response prepare proposal if proposed by us - pub proposer_results: Option, + /// The prepare proposal results if proposed by us + pub proposer_results: Option, } /// A trait defining getter methods for interacting with a BlockExecutionContextV0. pub trait BlockExecutionContextV0Getters { @@ -40,7 +61,7 @@ pub trait BlockExecutionContextV0Getters { fn block_platform_state(&self) -> &PlatformState; /// Returns a reference of the proposer_results field. - fn proposer_results(&self) -> Option<&ResponsePrepareProposal>; + fn proposer_results(&self) -> Option<&ProposerResults>; } /// A trait defining setter methods for interacting with a BlockExecutionContextV0. @@ -58,7 +79,7 @@ pub trait BlockExecutionContextV0Setters { fn set_block_platform_state(&mut self, state: PlatformState); /// Sets the proposer_results field. - fn set_proposer_results(&mut self, results: Option); + fn set_proposer_results(&mut self, results: Option); } /// A trait defining methods for interacting with a BlockExecutionContextV0. @@ -73,7 +94,7 @@ pub trait BlockExecutionContextV0MutableGetters { fn block_platform_state_mut(&mut self) -> &mut PlatformState; /// Returns a mutable reference to the proposer_results field. - fn proposer_results_mut(&mut self) -> Option<&mut ResponsePrepareProposal>; + fn proposer_results_mut(&mut self) -> Option<&mut ProposerResults>; /// Returns a mut reference of the withdrawal_transactions field. fn unsigned_withdrawal_transactions_mut(&mut self) -> &mut UnsignedWithdrawalTxs; @@ -91,7 +112,7 @@ pub trait BlockExecutionContextV0OwnedGetters { fn block_platform_state_owned(self) -> PlatformState; /// Consumes the BlockExecutionContextV0 and returns the proposer_results field. - fn proposer_results_owned(self) -> Option; + fn proposer_results_owned(self) -> Option; } impl BlockExecutionContextV0Getters for BlockExecutionContextV0 { @@ -116,7 +137,7 @@ impl BlockExecutionContextV0Getters for BlockExecutionContextV0 { } /// Returns a reference to the proposer_results field. - fn proposer_results(&self) -> Option<&ResponsePrepareProposal> { + fn proposer_results(&self) -> Option<&ProposerResults> { self.proposer_results.as_ref() } } @@ -139,7 +160,7 @@ impl BlockExecutionContextV0Setters for BlockExecutionContextV0 { self.block_platform_state = state; } /// Sets the proposer_results field. - fn set_proposer_results(&mut self, results: Option) { + fn set_proposer_results(&mut self, results: Option) { self.proposer_results = results; } } @@ -161,7 +182,7 @@ impl BlockExecutionContextV0MutableGetters for BlockExecutionContextV0 { } /// Returns a mutable reference to the proposer_results field. - fn proposer_results_mut(&mut self) -> Option<&mut ResponsePrepareProposal> { + fn proposer_results_mut(&mut self) -> Option<&mut ProposerResults> { self.proposer_results.as_mut() } @@ -186,8 +207,8 @@ impl BlockExecutionContextV0OwnedGetters for BlockExecutionContextV0 { self.block_platform_state } - /// Consumes the object and returns the owned `ResponsePrepareProposal`. - fn proposer_results_owned(self) -> Option { + /// Consumes the object and returns the owned `ProposerResults`. + fn proposer_results_owned(self) -> Option { self.proposer_results } } diff --git a/packages/rs-drive-abci/tests/strategy_tests/test_cases/mod.rs b/packages/rs-drive-abci/tests/strategy_tests/test_cases/mod.rs index dcd6ddd7ea4..1e963cb1cbd 100644 --- a/packages/rs-drive-abci/tests/strategy_tests/test_cases/mod.rs +++ b/packages/rs-drive-abci/tests/strategy_tests/test_cases/mod.rs @@ -7,6 +7,7 @@ mod core_update_tests; mod data_contract_history_tests; mod identity_and_document_tests; mod identity_transfer_tests; +mod process_proposal_collision_tests; mod required_since_update_tests; // TODO: re-enable once OperationType shielded variants are implemented // mod shielded_tests; diff --git a/packages/rs-drive-abci/tests/strategy_tests/test_cases/process_proposal_collision_tests.rs b/packages/rs-drive-abci/tests/strategy_tests/test_cases/process_proposal_collision_tests.rs new file mode 100644 index 00000000000..b2fe58a7222 --- /dev/null +++ b/packages/rs-drive-abci/tests/strategy_tests/test_cases/process_proposal_collision_tests.rs @@ -0,0 +1,569 @@ +//! Two proposals for the same height and round. +//! +//! A node that hands over from block sync to consensus while still behind becomes a +//! proposer at a historical height (it is in that era's quorum) and builds a block of its own +//! there. The committed block for that height then reaches it through consensus catch-up: +//! same height, same round, a different block. Drive's `process_proposal` keeps an execution +//! context keyed only by height and round, so it answers the second block from the first +//! block's cache, or refuses it outright. Either answer wedges Tenderdash: a cached app hash +//! fails `ValidateBlockWithRoundState` (panic in `ApplyCommit`), and an ABCI error panics in +//! `mustEnsureProcess`. The WAL replays both proposals on restart, so the node never recovers. +//! +//! These tests assert the behaviour Drive needs: a proposal that is not the one the execution +//! context holds must be executed afresh, never served from the cache and never rejected with +//! an error. That has to hold in both states the context can be in — after Tenderdash has told +//! us a block hash, and in the window right after `PrepareProposal` where it has not computed +//! one yet and the prepared block is identified by its execution inputs alone. +#[cfg(test)] +mod tests { + use crate::execution::run_chain_for_strategy; + use crate::strategy::{ChainExecutionOutcome, NetworkStrategy}; + use dpp::block::block_info::BlockInfo; + use dpp::block::epoch::Epoch; + use dpp::dashcore::hashes::Hash; + use drive_abci::config::{ExecutionConfig, PlatformConfig, PlatformTestConfig}; + use drive_abci::execution::types::block_execution_context::v0::BlockExecutionContextV0Getters; + use drive_abci::execution::types::block_state_info::v0::BlockStateInfoV0Getters; + use drive_abci::mimic::MimicExecuteBlockOptions; + use drive_abci::platform_types::platform_state::PlatformStateV0Methods; + use drive_abci::test::helpers::setup::TestPlatformBuilder; + use platform_version::version::PlatformVersion; + use strategy_tests::{IdentityInsertInfo, StartAddresses, StartIdentities, Strategy}; + use tenderdash_abci::proto::abci::response_process_proposal::ProposalStatus; + use tenderdash_abci::proto::abci::tx_record::TxAction; + use tenderdash_abci::proto::abci::{ + RequestPrepareProposal, RequestProcessProposal, ResponsePrepareProposal, + }; + use tenderdash_abci::proto::google::protobuf::Timestamp; + use tenderdash_abci::proto::version::Consensus; + use tenderdash_abci::Application; + + const BLOCK_SPACING_MS: u64 = 3000; + const MAX_TX_BYTES_PER_BLOCK: i64 = 40000; + + fn strategy() -> NetworkStrategy { + NetworkStrategy { + strategy: Strategy { + start_contracts: vec![], + operations: vec![], + start_identities: StartIdentities::default(), + start_addresses: StartAddresses::default(), + identity_inserts: IdentityInsertInfo::default(), + identity_contract_nonce_gaps: None, + signer: None, + }, + total_hpmns: 50, + extra_normal_mns: 0, + validator_quorum_count: 10, + chain_lock_quorum_count: 10, + upgrading_info: None, + proposer_strategy: Default::default(), + rotate_quorums: false, + failure_testing: None, + query_testing: None, + verify_state_transition_results: false, + ..Default::default() + } + } + + fn config() -> PlatformConfig { + PlatformConfig { + execution: ExecutionConfig { + verify_sum_trees: true, + ..Default::default() + }, + block_spacing_ms: BLOCK_SPACING_MS, + testing_configs: PlatformTestConfig::default_minimal_verifications(), + ..Default::default() + } + } + + /// The time of the next block after `outcome`. `time_offset_ms` shifts it so that two + /// proposals built from different offsets are different blocks. + fn next_block_time_ms(outcome: &ChainExecutionOutcome, time_offset_ms: u64) -> u64 { + outcome.end_time_ms + BLOCK_SPACING_MS + time_offset_ms + } + + fn timestamp(time_ms: u64) -> Timestamp { + Timestamp { + seconds: (time_ms / 1000) as i64, + nanos: ((time_ms % 1000) * 1_000_000) as i32, + } + } + + /// A `RequestProcessProposal` for the next block after `outcome`, attributed to the + /// proposer at `proposer_index`, with `hash` as its block hash. + fn next_block_request( + outcome: &ChainExecutionOutcome, + proposer_index: usize, + hash: [u8; 32], + time_offset_ms: u64, + ) -> RequestProcessProposal { + let platform_state = outcome.abci_app.platform.state.load(); + let height = platform_state.last_committed_block_height() + 1; + let core_height = platform_state.last_committed_core_height(); + let proposer = outcome.proposers[proposer_index].pro_tx_hash(); + + RequestProcessProposal { + txs: vec![], + proposed_last_commit: None, + misbehavior: vec![], + hash: hash.to_vec(), + height: height as i64, + time: Some(timestamp(next_block_time_ms(outcome, time_offset_ms))), + next_validators_hash: [0u8; 32].to_vec(), + round: 0, + core_chain_locked_height: core_height, + core_chain_lock_update: None, + proposer_pro_tx_hash: proposer.to_byte_array().to_vec(), + proposed_app_version: PlatformVersion::latest().protocol_version as u64, + version: Some(Consensus { + block: 0, + app: PlatformVersion::latest().protocol_version as u64, + }), + quorum_hash: outcome + .current_quorum() + .quorum_hash + .to_byte_array() + .to_vec(), + } + } + + /// The `RequestPrepareProposal` the node's own Tenderdash sends when it is the proposer at + /// `proposer_index` for the next block after `outcome`. Deliberately paired with + /// `next_block_request` at the same offset and proposer index: the two then describe the + /// same block, so a test that varies exactly one field isolates that field. + fn next_block_prepare_request( + outcome: &ChainExecutionOutcome, + proposer_index: usize, + time_offset_ms: u64, + ) -> RequestPrepareProposal { + let platform_state = outcome.abci_app.platform.state.load(); + let height = platform_state.last_committed_block_height() + 1; + let core_height = platform_state.last_committed_core_height(); + let proposer = outcome.proposers[proposer_index].pro_tx_hash(); + + RequestPrepareProposal { + max_tx_bytes: MAX_TX_BYTES_PER_BLOCK, + txs: vec![], + local_last_commit: None, + misbehavior: vec![], + height: height as i64, + time: Some(timestamp(next_block_time_ms(outcome, time_offset_ms))), + next_validators_hash: [0u8; 32].to_vec(), + round: 0, + core_chain_locked_height: core_height, + proposer_pro_tx_hash: proposer.to_byte_array().to_vec(), + proposed_app_version: PlatformVersion::latest().protocol_version as u64, + // Tenderdash sends 0 on prepare proposal; the app version it puts in the header + // is the one that comes back in the response. + version: Some(Consensus { block: 0, app: 0 }), + quorum_hash: outcome + .current_quorum() + .quorum_hash + .to_byte_array() + .to_vec(), + } + } + + /// The `RequestProcessProposal` Tenderdash sends back for the block it just had prepared: + /// the transactions the proposer kept, in order, the app version the response asked for in + /// the header, and the block hash Tenderdash has computed by now. + fn process_request_for_prepared_block( + prepare_request: &RequestPrepareProposal, + prepare_response: &ResponsePrepareProposal, + time_ms: u64, + hash: [u8; 32], + ) -> RequestProcessProposal { + let txs = prepare_response + .tx_records + .iter() + .filter(|record| { + record.action != TxAction::Removed as i32 + && record.action != TxAction::Delayed as i32 + }) + .map(|record| record.tx.clone()) + .collect(); + + RequestProcessProposal { + txs, + proposed_last_commit: None, + misbehavior: vec![], + hash: hash.to_vec(), + height: prepare_request.height, + time: Some(timestamp(time_ms)), + next_validators_hash: prepare_request.next_validators_hash.clone(), + round: prepare_request.round, + core_chain_locked_height: prepare_response + .core_chain_lock_update + .as_ref() + .map(|chain_lock| chain_lock.core_block_height) + .unwrap_or(prepare_request.core_chain_locked_height), + core_chain_lock_update: prepare_response.core_chain_lock_update.clone(), + proposer_pro_tx_hash: prepare_request.proposer_pro_tx_hash.clone(), + proposed_app_version: prepare_request.proposed_app_version, + version: Some(Consensus { + block: 0, + app: prepare_response.app_version, + }), + quorum_hash: prepare_request.quorum_hash.clone(), + } + } + + /// The block time the execution context currently holds, which is the block Drive last + /// executed. Reading it tells a re-execution apart from a cache hit without depending on + /// the two blocks reaching different app hashes. + fn context_block_time_ms(outcome: &ChainExecutionOutcome) -> u64 { + outcome + .abci_app + .block_execution_context + .read() + .unwrap() + .as_ref() + .expect("a block execution context must be held after process proposal") + .block_state_info() + .block_time_ms() + } + + fn context_block_hash(outcome: &ChainExecutionOutcome) -> Option<[u8; 32]> { + outcome + .abci_app + .block_execution_context + .read() + .unwrap() + .as_ref() + .expect("a block execution context must be held after process proposal") + .block_state_info() + .block_hash() + } + + /// The node prepared its own block at (H, 0) and then receives the network's block for + /// (H, 0) — a different block. Drive must execute it and return *its* app hash, not the + /// app hash of the block the node itself proposed. + #[tokio::test] + async fn process_proposal_for_a_different_block_must_not_be_served_from_the_proposer_cache() { + let config = config(); + let mut platform = TestPlatformBuilder::new() + .with_config(config.clone()) + .build_with_mock_rpc(); + + let outcome = run_chain_for_strategy( + &mut platform, + 5, + strategy(), + config, + 7, + &mut None, + &mut None, + ) + .await; + + let platform_state = outcome.abci_app.platform.state.load(); + let height = platform_state.last_committed_block_height() + 1; + let block_info = BlockInfo { + time_ms: next_block_time_ms(&outcome, 0), + height, + core_height: platform_state.last_committed_core_height(), + epoch: Epoch::new(outcome.end_epoch_index).expect("epoch"), + }; + drop(platform_state); + + // PrepareProposal + ProcessProposal of our own block at (H, 0), not finalized: this is + // the state a proposer is in while it waits for votes that will never come. + let own_block = outcome + .abci_app + .mimic_execute_block( + outcome.proposers[0].pro_tx_hash().to_byte_array(), + outcome.current_quorum(), + PlatformVersion::latest().protocol_version, + block_info, + 0, + &[], + false, + vec![], + MimicExecuteBlockOptions { + dont_finalize_block: true, + rounds_before_finalization: None, + max_tx_bytes_per_block: MAX_TX_BYTES_PER_BLOCK as u64, + independent_process_proposal_verification: false, + }, + ) + .expect("our own block should prepare and process"); + + // The network's block for the same height and round: different proposer, different + // time, different hash. + let network_block = next_block_request(&outcome, 1, [0x42u8; 32], 1000); + + let response = outcome + .abci_app + .process_proposal(network_block.clone()) + .expect("a different block at the same height/round must be processed, not refused"); + + assert_eq!(response.status, ProposalStatus::Accept as i32); + + // What the network block really executes to: run it against a clean context. + outcome + .abci_app + .block_execution_context + .write() + .unwrap() + .take(); + let reference = outcome + .abci_app + .process_proposal(network_block) + .expect("clean execution of the network block"); + + assert_ne!( + reference.app_hash, + own_block.root_app_hash.to_vec(), + "test premise: the two blocks must execute to different app hashes" + ); + assert_eq!( + response.app_hash, reference.app_hash, + "Drive answered the network's block with the app hash of the node's own proposal" + ); + } + + /// Two different blocks processed for the same height and round, neither of them ours: + /// the second must be executed, not answered with an ABCI error (which Tenderdash turns + /// into a panic that the WAL replay reproduces on every restart). + #[tokio::test] + async fn process_proposal_for_a_second_different_block_in_the_same_round_must_execute() { + let config = config(); + let mut platform = TestPlatformBuilder::new() + .with_config(config.clone()) + .build_with_mock_rpc(); + + let outcome = run_chain_for_strategy( + &mut platform, + 5, + strategy(), + config, + 7, + &mut None, + &mut None, + ) + .await; + + let first = next_block_request(&outcome, 1, [0x42u8; 32], 0); + let second = next_block_request(&outcome, 2, [0x43u8; 32], 1000); + + let first_response = outcome + .abci_app + .process_proposal(first) + .expect("first block processes"); + assert_eq!(first_response.status, ProposalStatus::Accept as i32); + + let second_response = outcome.abci_app.process_proposal(second.clone()).expect( + "a second, different block at the same height/round must be executed, not refused", + ); + assert_eq!(second_response.status, ProposalStatus::Accept as i32); + + outcome + .abci_app + .block_execution_context + .write() + .unwrap() + .take(); + let reference = outcome + .abci_app + .process_proposal(second) + .expect("clean execution of the second block"); + + assert_ne!(first_response.app_hash, reference.app_hash); + assert_eq!( + second_response.app_hash, reference.app_hash, + "Drive must answer the second block with its own app hash" + ); + } + + /// The window right after `PrepareProposal`, before Tenderdash has computed a hash for the + /// block it just had prepared: the execution context holds a proposer result and no block + /// hash, so there is no hash to compare a competing block against. + /// + /// The competing block here matches the prepared one in everything the height-and-round + /// cache used to look at — same transaction count (none either side) and the same absent + /// core chain lock update — and even in block time, differing only in its proposer. That is + /// enough to make it a different block: the proposer is written to state as the block's + /// author and as the caster of its app version vote, so it moves the app hash. Drive must + /// execute it rather than hand back the prepared block's app hash. + #[tokio::test] + async fn process_proposal_for_a_different_block_before_any_block_hash_is_known_must_not_be_served_from_the_proposer_cache( + ) { + let config = config(); + let mut platform = TestPlatformBuilder::new() + .with_config(config.clone()) + .build_with_mock_rpc(); + + let outcome = run_chain_for_strategy( + &mut platform, + 5, + strategy(), + config, + 7, + &mut None, + &mut None, + ) + .await; + + // Our own PrepareProposal, and nothing after it: no ProcessProposal has run, so + // nothing has told the context a block hash. + let prepare_response = outcome + .abci_app + .prepare_proposal(next_block_prepare_request(&outcome, 0, 0)) + .expect("our own block should prepare"); + assert_eq!( + context_block_hash(&outcome), + None, + "test premise: the context left by prepare proposal must carry no block hash" + ); + + // The competing block: same height, same round, same block time, no transactions and + // no core chain lock update on either side. Only the proposer differs. + let competing_block = next_block_request(&outcome, 1, [0x42u8; 32], 0); + + let response = outcome + .abci_app + .process_proposal(competing_block.clone()) + .expect("a different block at the same height/round must be processed, not refused"); + + assert_eq!(response.status, ProposalStatus::Accept as i32); + + // What the competing block really executes to: run it against a clean context. + outcome + .abci_app + .block_execution_context + .write() + .unwrap() + .take(); + let reference = outcome + .abci_app + .process_proposal(competing_block) + .expect("clean execution of the competing block"); + + assert_ne!( + reference.app_hash, prepare_response.app_hash, + "test premise: the two blocks must execute to different app hashes" + ); + assert_eq!( + response.app_hash, reference.app_hash, + "Drive answered a competing block with the app hash of the block it had prepared" + ); + } + + /// The same window, with a competing block that differs from the prepared one only in its + /// block time. Nothing in the pair reaches state through a path that a shifted time alone + /// would redirect, so the two execute to the same app hash and the served hash proves + /// nothing; what the context holds afterwards does. A cache hit would leave the prepared + /// block's time in place and merely stamp the request's hash onto it, so finding the + /// competing block's time there is what says Drive executed the block it was asked about. + #[tokio::test] + async fn process_proposal_for_a_block_differing_only_in_time_before_any_block_hash_is_known_must_execute( + ) { + let config = config(); + let mut platform = TestPlatformBuilder::new() + .with_config(config.clone()) + .build_with_mock_rpc(); + + let outcome = run_chain_for_strategy( + &mut platform, + 5, + strategy(), + config, + 7, + &mut None, + &mut None, + ) + .await; + + outcome + .abci_app + .prepare_proposal(next_block_prepare_request(&outcome, 0, 0)) + .expect("our own block should prepare"); + assert_eq!( + context_block_time_ms(&outcome), + next_block_time_ms(&outcome, 0), + "test premise: the context holds the prepared block's time" + ); + + // Same proposer, same height and round, no transactions and no core chain lock update + // on either side: a shifted block time is the only difference. + let competing_block = next_block_request(&outcome, 0, [0x44u8; 32], 1000); + + let response = outcome + .abci_app + .process_proposal(competing_block) + .expect("a different block at the same height/round must be processed, not refused"); + + assert_eq!(response.status, ProposalStatus::Accept as i32); + assert_eq!( + context_block_time_ms(&outcome), + next_block_time_ms(&outcome, 1000), + "Drive answered a block one second later than the one it prepared out of the prepared context" + ); + assert_eq!( + context_block_hash(&outcome), + Some([0x44u8; 32]), + "the context must be the competing block's own" + ); + } + + /// The legitimate case the proposer cache exists for: Tenderdash comes back with the very + /// block it just had prepared, now carrying a hash. Identity is established from the + /// execution inputs, the prepared result is replayed instead of executing the block twice, + /// and the hash Tenderdash computed is recorded on the context that extend vote and + /// finalize block go on to use. + #[tokio::test] + async fn process_proposal_for_the_prepared_block_must_still_be_served_from_the_proposer_cache() + { + let config = config(); + let mut platform = TestPlatformBuilder::new() + .with_config(config.clone()) + .build_with_mock_rpc(); + + let outcome = run_chain_for_strategy( + &mut platform, + 5, + strategy(), + config, + 7, + &mut None, + &mut None, + ) + .await; + + let prepare_request = next_block_prepare_request(&outcome, 0, 0); + let prepare_response = outcome + .abci_app + .prepare_proposal(prepare_request.clone()) + .expect("our own block should prepare"); + + let prepared_block = process_request_for_prepared_block( + &prepare_request, + &prepare_response, + next_block_time_ms(&outcome, 0), + [0x45u8; 32], + ); + + let response = outcome + .abci_app + .process_proposal(prepared_block) + .expect("the prepared block must be processed"); + + assert_eq!(response.status, ProposalStatus::Accept as i32); + assert_eq!( + response.app_hash, prepare_response.app_hash, + "the block we prepared must still be answered from the proposer cache" + ); + assert_eq!( + context_block_hash(&outcome), + Some([0x45u8; 32]), + "the cached context must record the hash Tenderdash computed for it" + ); + assert_eq!( + context_block_time_ms(&outcome), + next_block_time_ms(&outcome, 0), + "the cached context must be the one prepare proposal built" + ); + } +}