Skip to content

Commit

Permalink
refactor: preserve voting block during block sync (#4853)
Browse files Browse the repository at this point in the history 
Signed-off-by: Shanin Roman <[email protected]>
  • Loading branch information
@Erigara
Erigara authored Jul 22, 2024
1 parent cacc805 commit b94fa54
Show file tree
Hide file tree
Showing 3 changed files with 165 additions and 51 deletions.
121 changes: 94 additions & 27 deletions core/src/block.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -426,7 +428,7 @@ mod valid {
state_block: &mut StateBlock<'_>,
) -> WithEvents<Result<ValidBlock, (SignedBlock, BlockValidationError)>> {
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)));
}
Expand All @@ -451,17 +453,22 @@ mod valid {
genesis_account: &AccountId,
state: &'state State,
voting_block: &mut Option<VotingBlock>,
soft_fork: bool,
) -> WithEvents<Result<(ValidBlock, StateBlock<'state>), (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,
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -651,25 +670,73 @@ mod valid {
self,
topology: &Topology,
) -> WithEvents<Result<CommittedBlock, (ValidBlock, BlockValidationError)>> {
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<VotingBlock>,
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`].
Expand Down
11 changes: 11 additions & 0 deletions core/src/state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Arc<SignedBlock>> {
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<HashOf<SignedBlock>> {
self.block_hashes().iter().nth_back(0).copied()
Expand Down
84 changes: 60 additions & 24 deletions core/src/sumeragi/main_loop.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -449,18 +450,14 @@ 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,
state,
genesis_account,
&|e| self.send_event(e),
block_sync_type,
voting_block,
) {
Ok(BlockSyncOk::CommitBlock(block, state_block, topology)) => {
self.topology = topology;
Expand Down Expand Up @@ -1256,6 +1253,7 @@ fn handle_block_sync<'state, F: Fn(PipelineEventBox)>(
genesis_account,
handle_events,
block_sync_type,
&mut None,
)
}

Expand All @@ -1266,33 +1264,38 @@ fn handle_categorized_block_sync<'state, F: Fn(PipelineEventBox)>(
genesis_account: &AccountId,
handle_events: &F,
block_sync_type: Result<BlockSyncType, BlockSyncError>,
voting_block: &mut Option<VotingBlock>,
) -> Result<BlockSyncOk<'state>, (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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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());
}
}

0 comments on commit b94fa54

Please sign in to comment.