diff --git a/core/src/block.rs b/core/src/block.rs index 4a9fc6e1630..92ab0361a74 100644 --- a/core/src/block.rs +++ b/core/src/block.rs @@ -265,8 +265,10 @@ mod chained { } mod valid { + use commit::CommittedBlock; use indexmap::IndexMap; - use iroha_data_model::{account::AccountId, ChainId}; + use iroha_data_model::{account::AccountId, events::pipeline::PipelineEventBox, ChainId}; + use storage::storage::StorageReadOnly; use super::*; use crate::{state::StateBlock, sumeragi::network_topology::Role}; @@ -426,7 +428,7 @@ mod valid { state_block: &mut StateBlock<'_>, ) -> WithEvents> { if let Err(error) = - Self::validate_header(&block, topology, genesis_account, state_block) + Self::validate_header(&block, topology, genesis_account, state_block, false) { return WithEvents::new(Err((block, error))); } @@ -451,17 +453,22 @@ mod valid { genesis_account: &AccountId, state: &'state State, voting_block: &mut Option, + soft_fork: bool, ) -> WithEvents), (SignedBlock, BlockValidationError)>> { if let Err(error) = - Self::validate_header(&block, topology, genesis_account, &state.view()) + Self::validate_header(&block, topology, genesis_account, &state.view(), soft_fork) { return WithEvents::new(Err((block, error))); } // Release block writer before creating new one let _ = voting_block.take(); - let mut state_block = state.block(); + let mut state_block = if soft_fork { + state.block_and_revert() + } else { + state.block() + }; if let Err(error) = Self::validate_transactions( &block, @@ -480,11 +487,16 @@ mod valid { topology: &Topology, genesis_account: &AccountId, state: &impl StateReadOnly, + soft_fork: bool, ) -> Result<(), BlockValidationError> { - let expected_block_height = state - .height() - .checked_add(1) - .expect("INTERNAL BUG: Block height exceeds usize::MAX"); + let expected_block_height = if soft_fork { + state.height() + } else { + state + .height() + .checked_add(1) + .expect("INTERNAL BUG: Block height exceeds usize::MAX") + }; let actual_height = block .header() .height @@ -499,7 +511,11 @@ mod valid { }); } - let expected_prev_block_hash = state.latest_block_hash(); + let expected_prev_block_hash = if soft_fork { + state.prev_block_hash() + } else { + state.latest_block_hash() + }; let actual_prev_block_hash = block.header().prev_block_hash; if expected_prev_block_hash != actual_prev_block_hash { @@ -534,10 +550,13 @@ mod valid { } } - if block - .transactions() - .any(|tx| state.has_transaction(tx.as_ref().hash())) - { + if block.transactions().any(|tx| { + state + .transactions() + .get(&tx.as_ref().hash()) + // In case of soft-fork transaction is check if it was added at the same height as candidate block + .is_some_and(|height| height.get() < expected_block_height) + }) { return Err(BlockValidationError::HasCommittedTransactions); } @@ -651,25 +670,73 @@ mod valid { self, topology: &Topology, ) -> WithEvents> { - if !self.as_ref().header().is_genesis() { - if let Err(err) = Self::verify_proxy_tail_signature(self.as_ref(), topology) { - return WithEvents::new(Err((self, err.into()))); - } + WithEvents::new(match Self::is_commit(self.as_ref(), topology) { + Err(err) => Err((self, err)), + Ok(()) => Ok(CommittedBlock(self)), + }) + } - let votes_count = self.as_ref().signatures().len(); + /// Validate and commit block if possible. + /// + /// This method is different from calling [`ValidBlock::validate_keep_voting_block`] and [`ValidateBlock::commit`] in the following ways: + /// - signatures are checked eagerly so voting block is kept if block doesn't have valid signatures + /// + /// # Errors + /// Combinations of errors from [`ValidBlock::validate_keep_voting_block`] and [`ValidateBlock::commit`]. + #[allow(clippy::too_many_arguments)] + pub fn commit_keep_voting_block<'state, F: Fn(PipelineEventBox)>( + block: SignedBlock, + topology: &Topology, + expected_chain_id: &ChainId, + genesis_account: &AccountId, + state: &'state State, + voting_block: &mut Option, + soft_fork: bool, + send_events: F, + ) -> WithEvents< + Result<(CommittedBlock, StateBlock<'state>), (SignedBlock, BlockValidationError)>, + > { + if let Err(err) = Self::is_commit(&block, topology) { + return WithEvents::new(Err((block, err))); + } + + WithEvents::new( + Self::validate_keep_voting_block( + block, + topology, + expected_chain_id, + genesis_account, + state, + voting_block, + soft_fork, + ) + .unpack(send_events) + .map(|(block, state_block)| (CommittedBlock(block), state_block)), + ) + } + + /// Check if block satisfy requirements to be committed + /// + /// # Errors + /// + /// - Block has duplicate proxy tail signatures + /// - Block is not signed by the proxy tail + /// - Block doesn't have enough signatures + fn is_commit(block: &SignedBlock, topology: &Topology) -> Result<(), BlockValidationError> { + if !block.header().is_genesis() { + Self::verify_proxy_tail_signature(block, topology)?; + + let votes_count = block.signatures().len(); if votes_count < topology.min_votes_for_commit() { - return WithEvents::new(Err(( - self, - SignatureVerificationError::NotEnoughSignatures { - votes_count, - min_votes_for_commit: topology.min_votes_for_commit(), - } - .into(), - ))); + return Err(SignatureVerificationError::NotEnoughSignatures { + votes_count, + min_votes_for_commit: topology.min_votes_for_commit(), + } + .into()); } } - WithEvents::new(Ok(CommittedBlock(self))) + Ok(()) } /// Add additional signatures for [`Self`]. diff --git a/core/src/state.rs b/core/src/state.rs index 4b138674fd8..3aa6b576144 100644 --- a/core/src/state.rs +++ b/core/src/state.rs @@ -1143,6 +1143,17 @@ pub trait StateReadOnly { NonZeroUsize::new(self.height()).and_then(|height| self.kura().get_block_by_height(height)) } + /// Get a reference to the previous to latest block. Returns none if at least 2 blocks are not committed. + /// + /// If you only need hash of the previous block prefer using [`Self::prev_block_hash`] + #[inline] + fn prev_block(&self) -> Option> { + self.height() + .checked_sub(1) + .and_then(NonZeroUsize::new) + .and_then(|height| self.kura().get_block_by_height(height)) + } + /// Return the hash of the latest block fn latest_block_hash(&self) -> Option> { self.block_hashes().iter().nth_back(0).copied() diff --git a/core/src/sumeragi/main_loop.rs b/core/src/sumeragi/main_loop.rs index c368f48afd2..3a579611e45 100644 --- a/core/src/sumeragi/main_loop.rs +++ b/core/src/sumeragi/main_loop.rs @@ -402,6 +402,7 @@ impl Sumeragi { genesis_account, state, existing_voting_block, + false, ) .unpack(|e| self.send_event(e)) .map(|(block, state_block)| VotingBlock::new(block, state_block)) @@ -449,11 +450,6 @@ impl Sumeragi { ); let block_sync_type = categorize_block_sync(&block, &state.view()); - if block_sync_type.is_ok() { - // Release block writer before creating new one - let _ = voting_block.take(); - } - match handle_categorized_block_sync( &self.chain_id, block, @@ -461,6 +457,7 @@ impl Sumeragi { genesis_account, &|e| self.send_event(e), block_sync_type, + voting_block, ) { Ok(BlockSyncOk::CommitBlock(block, state_block, topology)) => { self.topology = topology; @@ -1256,6 +1253,7 @@ fn handle_block_sync<'state, F: Fn(PipelineEventBox)>( genesis_account, handle_events, block_sync_type, + &mut None, ) } @@ -1266,33 +1264,38 @@ fn handle_categorized_block_sync<'state, F: Fn(PipelineEventBox)>( genesis_account: &AccountId, handle_events: &F, block_sync_type: Result, + voting_block: &mut Option, ) -> Result, (SignedBlock, BlockSyncError)> { - let (mut state_block, soft_fork) = match block_sync_type { - Ok(BlockSyncType::CommitBlock) => (state.block(), false), - Ok(BlockSyncType::ReplaceTopBlock) => (state.block_and_revert(), true), + let soft_fork = match block_sync_type { + Ok(BlockSyncType::CommitBlock) => false, + Ok(BlockSyncType::ReplaceTopBlock) => true, Err(e) => return Err((block, e)), }; - let latest_block = state_block - .latest_block() - .expect("INTERNAL BUG: No latest block"); - let mut topology = Topology::new(latest_block.commit_topology().cloned()); - topology.block_committed(&latest_block, state_block.world.peers().cloned()); - topology.nth_rotation(block.header().view_change_index as usize); - - ValidBlock::validate( + + let topology = { + let view = state.view(); + let latest_block = if soft_fork { + view.prev_block().expect("INTERNAL BUG: No prev block") + } else { + view.latest_block().expect("INTERNAL BUG: No latest block") + }; + let mut topology = Topology::new(latest_block.commit_topology().cloned()); + topology.block_committed(&latest_block, view.world.peers().cloned()); + topology.nth_rotation(block.header().view_change_index as usize); + topology + }; + + ValidBlock::commit_keep_voting_block( block, &topology, chain_id, genesis_account, - &mut state_block, + state, + voting_block, + soft_fork, + handle_events, ) .unpack(handle_events) - .and_then(|block| { - block - .commit(&topology) - .unpack(handle_events) - .map_err(|(block, err)| (block.into(), err)) - }) .map_err(|(block, error)| { ( block, @@ -1303,7 +1306,7 @@ fn handle_categorized_block_sync<'state, F: Fn(PipelineEventBox)>( }, ) }) - .map(|block| { + .map(|(block, state_block)| { if soft_fork { BlockSyncOk::ReplaceTopBlock(block, state_block, topology) } else { @@ -1710,4 +1713,37 @@ mod tests { assert_eq!(block_height, nonzero!(1_usize)); } } + + #[test] + #[allow(clippy::redundant_clone)] + async fn block_sync_commit_err_keep_voting_block() { + let chain_id = ChainId::from("00000000-0000-0000-0000-000000000000"); + + let (leader_public_key, leader_private_key) = KeyPair::random().into_parts(); + let peer_id = PeerId::new("127.0.0.1:8080".parse().unwrap(), leader_public_key); + let topology = Topology::new(vec![peer_id]); + let (state, _, mut block, genesis_public_key) = + create_data_for_test(&chain_id, &topology, &leader_private_key); + + // Malform block signatures so that block going to be rejected + block.replace_signatures_unchecked(Vec::new()); + + let mut voting_block = Some(VotingBlock::new( + ValidBlock::new_dummy(&leader_private_key), + state.block(), + )); + + let block_sync_type = categorize_block_sync(&block, &state.view()); + let result = handle_categorized_block_sync( + &chain_id, + block, + &state, + &genesis_public_key, + &|_| {}, + block_sync_type, + &mut voting_block, + ); + assert!(matches!(result, Err((_, BlockSyncError::BlockNotValid(_))))); + assert!(voting_block.is_some()); + } }