diff --git a/client/examples/million_accounts_genesis.rs b/client/examples/million_accounts_genesis.rs index affb34c130d..6d9ca52b1a9 100644 --- a/client/examples/million_accounts_genesis.rs +++ b/client/examples/million_accounts_genesis.rs @@ -3,7 +3,7 @@ use std::{thread, time::Duration}; use iroha::{ crypto::KeyPair, - data_model::{::isi::InstructionBox, prelude::*}, + data_model::{isi::InstructionBox, prelude::*}, }; use iroha_genesis::{GenesisTransaction, GenesisTransactionBuilder}; use iroha_primitives::unique_vec; diff --git a/client/src/client.rs b/client/src/client.rs index 0ad711a0301..efd0b914510 100644 --- a/client/src/client.rs +++ b/client/src/client.rs @@ -450,7 +450,9 @@ impl Client { tx_builder.set_nonce(nonce); }; - tx_builder.with_metadata(metadata).sign(&self.key_pair) + tx_builder + .with_metadata(metadata) + .sign(self.key_pair.private_key()) } /// Signs transaction @@ -458,7 +460,7 @@ impl Client { /// # Errors /// Fails if signature generation fails pub fn sign_transaction(&self, transaction: TransactionBuilder) -> SignedTransaction { - transaction.sign(&self.key_pair) + transaction.sign(self.key_pair.private_key()) } /// Signs query @@ -1634,20 +1636,12 @@ mod tests { use http::Response; use super::*; - use crate::data_model::{asset::Asset, query::error::QueryExecutionFail, ValidationFail}; + use crate::data_model::{asset::Asset, ValidationFail}; #[test] fn certain_errors() -> Result<()> { let mut sut = QueryResponseHandler::>::new(QueryRequest::dummy()); - let responses = vec![ - ( - StatusCode::UNAUTHORIZED, - ValidationFail::QueryFailed(QueryExecutionFail::Signature( - "whatever".to_owned(), - )), - ), - (StatusCode::UNPROCESSABLE_ENTITY, ValidationFail::TooComplex), - ]; + let responses = vec![(StatusCode::UNPROCESSABLE_ENTITY, ValidationFail::TooComplex)]; for (status_code, err) in responses { let resp = Response::builder().status(status_code).body(err.encode())?; diff --git a/client/src/query_builder.rs b/client/src/query_builder.rs index 7a59c893907..d77321f9542 100644 --- a/client/src/query_builder.rs +++ b/client/src/query_builder.rs @@ -1,11 +1,10 @@ use std::fmt::Debug; -use iroha_data_model::query::{IterableQuery, QueryOutputBox}; - use crate::{ client::{Client, QueryOutput, QueryResult}, data_model::query::{ - predicate::PredicateBox, sorting::Sorting, FetchSize, Pagination, Query, QueryOutputBox, + predicate::PredicateBox, sorting::Sorting, FetchSize, IterableQuery, Pagination, Query, + QueryOutputBox, }, }; diff --git a/client/tests/integration/asset.rs b/client/tests/integration/asset.rs index 919df14ce97..804083d8550 100644 --- a/client/tests/integration/asset.rs +++ b/client/tests/integration/asset.rs @@ -307,7 +307,7 @@ fn find_rate_and_make_exchange_isi_should_succeed() { asset_id.account_id().clone(), ) .with_instructions([instruction]) - .sign(&owner_key_pair); + .sign(owner_key_pair.private_key()); test_client .submit_transaction_blocking(&transaction) diff --git a/client/tests/integration/domain_owner_permissions.rs b/client/tests/integration/domain_owner_permissions.rs index b2f3bf88444..e53442bec65 100644 --- a/client/tests/integration/domain_owner_permissions.rs +++ b/client/tests/integration/domain_owner_permissions.rs @@ -26,7 +26,7 @@ fn domain_owner_domain_permissions() -> Result<()> { // Asset definitions can't be registered by "bob@kingdom" by default let transaction = TransactionBuilder::new(chain_id.clone(), bob_id.clone()) .with_instructions([Register::asset_definition(coin.clone())]) - .sign(&bob_keypair); + .sign(bob_keypair.private_key()); let err = test_client .submit_transaction_blocking(&transaction) .expect_err("Tx should fail due to permissions"); @@ -52,7 +52,7 @@ fn domain_owner_domain_permissions() -> Result<()> { test_client.submit_blocking(Grant::permission(token.clone(), bob_id.clone()))?; let transaction = TransactionBuilder::new(chain_id, bob_id.clone()) .with_instructions([Register::asset_definition(coin)]) - .sign(&bob_keypair); + .sign(bob_keypair.private_key()); test_client.submit_transaction_blocking(&transaction)?; test_client.submit_blocking(Revoke::permission(token, bob_id.clone()))?; @@ -148,7 +148,7 @@ fn domain_owner_asset_definition_permissions() -> Result<()> { let coin = AssetDefinition::numeric(coin_id.clone()); let transaction = TransactionBuilder::new(chain_id, bob_id.clone()) .with_instructions([Register::asset_definition(coin)]) - .sign(&bob_keypair); + .sign(bob_keypair.private_key()); test_client.submit_transaction_blocking(&transaction)?; // check that "alice@wonderland" as owner of domain can transfer asset definitions in her domain @@ -217,7 +217,7 @@ fn domain_owner_asset_permissions() -> Result<()> { Register::asset_definition(coin), Register::asset_definition(store), ]) - .sign(&bob_keypair); + .sign(bob_keypair.private_key()); test_client.submit_transaction_blocking(&transaction)?; // check that "alice@wonderland" as owner of domain can register and unregister assets in her domain diff --git a/client/tests/integration/permissions.rs b/client/tests/integration/permissions.rs index 59d5de0c439..51c938d31e4 100644 --- a/client/tests/integration/permissions.rs +++ b/client/tests/integration/permissions.rs @@ -102,7 +102,7 @@ fn permissions_disallow_asset_transfer() { ); let transfer_tx = TransactionBuilder::new(chain_id, mouse_id) .with_instructions([transfer_asset]) - .sign(&mouse_keypair); + .sign(mouse_keypair.private_key()); let err = iroha .submit_transaction_blocking(&transfer_tx) .expect_err("Transaction was not rejected."); @@ -151,7 +151,7 @@ fn permissions_disallow_asset_burn() { ); let burn_tx = TransactionBuilder::new(chain_id, mouse_id) .with_instructions([burn_asset]) - .sign(&mouse_keypair); + .sign(mouse_keypair.private_key()); let err = iroha .submit_transaction_blocking(&burn_tx) @@ -239,7 +239,7 @@ fn permissions_differ_not_only_by_names() { let grant_hats_access_tx = TransactionBuilder::new(chain_id.clone(), mouse_id.clone()) .with_instructions([allow_alice_to_set_key_value_in_hats]) - .sign(&mouse_keypair); + .sign(mouse_keypair.private_key()); client .submit_transaction_blocking(&grant_hats_access_tx) .expect("Failed grant permission to modify Mouse's hats"); @@ -275,7 +275,7 @@ fn permissions_differ_not_only_by_names() { let grant_shoes_access_tx = TransactionBuilder::new(chain_id, mouse_id) .with_instructions([allow_alice_to_set_key_value_in_shoes]) - .sign(&mouse_keypair); + .sign(mouse_keypair.private_key()); client .submit_transaction_blocking(&grant_shoes_access_tx) @@ -328,7 +328,7 @@ fn stored_vs_granted_token_payload() -> Result<()> { let transaction = TransactionBuilder::new(chain_id, mouse_id) .with_instructions([allow_alice_to_set_key_value_in_mouse_asset]) - .sign(&mouse_keypair); + .sign(mouse_keypair.private_key()); iroha .submit_transaction_blocking(&transaction) .expect("Failed to grant permission to alice."); diff --git a/client/tests/integration/roles.rs b/client/tests/integration/roles.rs index 599ccb3bc91..9832bfc7ad9 100644 --- a/client/tests/integration/roles.rs +++ b/client/tests/integration/roles.rs @@ -76,7 +76,7 @@ fn register_and_grant_role_for_metadata_access() -> Result<()> { let grant_role = Grant::role(role_id.clone(), alice_id.clone()); let grant_role_tx = TransactionBuilder::new(chain_id, mouse_id.clone()) .with_instructions([grant_role]) - .sign(&mouse_keypair); + .sign(mouse_keypair.private_key()); test_client.submit_transaction_blocking(&grant_role_tx)?; // Alice modifies Mouse's metadata @@ -236,7 +236,7 @@ fn grant_revoke_role_permissions() -> Result<()> { let grant_role = Grant::role(role_id.clone(), alice_id.clone()); let grant_role_tx = TransactionBuilder::new(chain_id.clone(), mouse_id.clone()) .with_instructions([grant_role]) - .sign(&mouse_keypair); + .sign(mouse_keypair.private_key()); test_client.submit_transaction_blocking(&grant_role_tx)?; let set_key_value = SetKeyValue::account( @@ -263,7 +263,7 @@ fn grant_revoke_role_permissions() -> Result<()> { // Alice can modify Mouse's metadata after permission token is granted to role let grant_role_permission_tx = TransactionBuilder::new(chain_id.clone(), mouse_id.clone()) .with_instructions([grant_role_permission]) - .sign(&mouse_keypair); + .sign(mouse_keypair.private_key()); test_client.submit_transaction_blocking(&grant_role_permission_tx)?; let found_permissions = test_client .request(FindPermissionsByAccountId::new(alice_id.clone()))? @@ -274,7 +274,7 @@ fn grant_revoke_role_permissions() -> Result<()> { // Alice can't modify Mouse's metadata after permission token is removed from role let revoke_role_permission_tx = TransactionBuilder::new(chain_id.clone(), mouse_id.clone()) .with_instructions([revoke_role_permission]) - .sign(&mouse_keypair); + .sign(mouse_keypair.private_key()); test_client.submit_transaction_blocking(&revoke_role_permission_tx)?; let found_permissions = test_client .request(FindPermissionsByAccountId::new(alice_id.clone()))? diff --git a/client/tests/integration/tx_chain_id.rs b/client/tests/integration/tx_chain_id.rs index 53dd1a5617a..f0ac7d63f88 100644 --- a/client/tests/integration/tx_chain_id.rs +++ b/client/tests/integration/tx_chain_id.rs @@ -1,6 +1,6 @@ use std::str::FromStr; -use iroha_client::data_model::prelude::*; +use iroha::data_model::prelude::*; use iroha_primitives::numeric::numeric; use test_network::*; use test_samples::gen_account_in; @@ -44,10 +44,10 @@ fn send_tx_with_different_chain_id() { ); let asset_transfer_tx_0 = TransactionBuilder::new(chain_id_0, sender_id.clone()) .with_instructions([transfer_instruction.clone()]) - .sign(&sender_keypair); + .sign(sender_keypair.private_key()); let asset_transfer_tx_1 = TransactionBuilder::new(chain_id_1, sender_id.clone()) .with_instructions([transfer_instruction]) - .sign(&sender_keypair); + .sign(sender_keypair.private_key()); test_client .submit_transaction_blocking(&asset_transfer_tx_0) .unwrap(); diff --git a/client/tests/integration/upgrade.rs b/client/tests/integration/upgrade.rs index 1a4f998d87f..97455d66c11 100644 --- a/client/tests/integration/upgrade.rs +++ b/client/tests/integration/upgrade.rs @@ -4,7 +4,6 @@ use eyre::Result; use futures_util::TryStreamExt as _; use iroha::{ client::{self, Client, QueryResult}, - crypto::KeyPair, data_model::prelude::*, }; use iroha_logger::info; @@ -23,11 +22,9 @@ fn executor_upgrade_should_work() -> Result<()> { let admin_id: AccountId = format!("{ADMIN_PUBLIC_KEY_MULTIHASH}@admin") .parse() .unwrap(); - let admin_keypair = KeyPair::new( - admin_id.signatory().clone(), - ADMIN_PRIVATE_KEY_MULTIHASH.parse().unwrap(), - ) - .unwrap(); + let admin_private_key = ADMIN_PRIVATE_KEY_MULTIHASH + .parse::() + .unwrap(); let (_rt, _peer, client) = ::new().with_port(10_795).start_with_runtime(); wait_for_genesis_committed(&vec![client.clone()], 0); @@ -48,7 +45,7 @@ fn executor_upgrade_should_work() -> Result<()> { let transfer_alice_rose = Transfer::asset_numeric(alice_rose, 1u32, admin_id.clone()); let transfer_rose_tx = TransactionBuilder::new(chain_id.clone(), admin_id.clone()) .with_instructions([transfer_alice_rose.clone()]) - .sign(&admin_keypair); + .sign(&admin_private_key); let _ = client .submit_transaction_blocking(&transfer_rose_tx) .expect_err("Should fail"); @@ -62,7 +59,7 @@ fn executor_upgrade_should_work() -> Result<()> { // Creating new transaction instead of cloning, because we need to update it's creation time let transfer_rose_tx = TransactionBuilder::new(chain_id, admin_id.clone()) .with_instructions([transfer_alice_rose]) - .sign(&admin_keypair); + .sign(&admin_private_key); client .submit_transaction_blocking(&transfer_rose_tx) .expect("Should succeed"); diff --git a/configs/swarm/executor.wasm b/configs/swarm/executor.wasm index e7188d6cbaa..2e81bade893 100644 Binary files a/configs/swarm/executor.wasm and b/configs/swarm/executor.wasm differ diff --git a/core/benches/blocks/apply_blocks.rs b/core/benches/blocks/apply_blocks.rs index 28daef9e720..eef0b25d7dd 100644 --- a/core/benches/blocks/apply_blocks.rs +++ b/core/benches/blocks/apply_blocks.rs @@ -44,7 +44,7 @@ impl StateApplyBlocks { &mut state_block, instructions, alice_id.clone(), - &alice_keypair, + alice_keypair.private_key(), ); let _events = state_block.apply_without_execution(&block); state_block.commit(); diff --git a/core/benches/blocks/common.rs b/core/benches/blocks/common.rs index 13eedfb89ed..879ccbcc931 100644 --- a/core/benches/blocks/common.rs +++ b/core/benches/blocks/common.rs @@ -25,25 +25,25 @@ pub fn create_block( state: &mut StateBlock<'_>, instructions: Vec, account_id: AccountId, - key_pair: &KeyPair, + private_key: &PrivateKey, ) -> CommittedBlock { let chain_id = ChainId::from("00000000-0000-0000-0000-000000000000"); let transaction = TransactionBuilder::new(chain_id.clone(), account_id) .with_instructions(instructions) - .sign(key_pair); + .sign(private_key); let limits = state.transaction_executor().transaction_limits; let (peer_public_key, _) = KeyPair::random().into_parts(); let peer_id = PeerId::new("127.0.0.1:8080".parse().unwrap(), peer_public_key); let topology = Topology::new(vec![peer_id]); let block = BlockBuilder::new( - vec![AcceptedTransaction::accept(transaction, &chain_id, &limits).unwrap()], + vec![AcceptedTransaction::accept(transaction, &chain_id, limits).unwrap()], topology.clone(), Vec::new(), ) .chain(0, state) - .sign(key_pair.private_key()) + .sign(private_key) .unpack(|_| {}) .commit(&topology) .unpack(|_| {}) diff --git a/core/benches/blocks/validate_blocks.rs b/core/benches/blocks/validate_blocks.rs index 0d26e9ed407..c9e7e082ab4 100644 --- a/core/benches/blocks/validate_blocks.rs +++ b/core/benches/blocks/validate_blocks.rs @@ -10,7 +10,7 @@ use common::*; pub struct StateValidateBlocks { state: State, instructions: Vec>, - key_pair: KeyPair, + private_key: PrivateKey, account_id: AccountId, } @@ -41,7 +41,7 @@ impl StateValidateBlocks { Self { state, instructions, - key_pair: alice_keypair, + private_key: alice_keypair.private_key().clone(), account_id: alice_id, } } @@ -58,7 +58,7 @@ impl StateValidateBlocks { Self { state, instructions, - key_pair, + private_key, account_id, }: Self, ) { @@ -68,7 +68,7 @@ impl StateValidateBlocks { &mut state_block, instructions, account_id.clone(), - &key_pair, + &private_key, ); let _events = state_block.apply_without_execution(&block); assert_eq!(state_block.height(), i); diff --git a/core/benches/kura.rs b/core/benches/kura.rs index 7f15ed8d8be..bc3b55a49e6 100644 --- a/core/benches/kura.rs +++ b/core/benches/kura.rs @@ -28,12 +28,12 @@ async fn measure_block_size_for_n_executors(n_executors: u32) { let transfer = Transfer::asset_numeric(alice_xor_id, 10u32, bob_id); let tx = TransactionBuilder::new(chain_id.clone(), alice_id.clone()) .with_instructions([transfer]) - .sign(&alice_keypair); + .sign(alice_keypair.private_key()); let transaction_limits = TransactionLimits { max_instruction_number: 4096, max_wasm_size_bytes: 0, }; - let tx = AcceptedTransaction::accept(tx, &chain_id, &transaction_limits) + let tx = AcceptedTransaction::accept(tx, &chain_id, transaction_limits) .expect("Failed to accept Transaction."); let dir = tempfile::tempdir().expect("Could not create tempfile."); let cfg = Config { diff --git a/core/benches/validation.rs b/core/benches/validation.rs index a4c43460399..009e39fe00a 100644 --- a/core/benches/validation.rs +++ b/core/benches/validation.rs @@ -80,12 +80,12 @@ fn build_test_and_transient_state() -> State { fn accept_transaction(criterion: &mut Criterion) { let chain_id = ChainId::from("00000000-0000-0000-0000-000000000000"); - let transaction = build_test_transaction(chain_id.clone()).sign(&STARTER_KEYPAIR); + let transaction = build_test_transaction(chain_id.clone()).sign(STARTER_KEYPAIR.private_key()); let mut success_count = 0; let mut failures_count = 0; let _ = criterion.bench_function("accept", |b| { b.iter(|| { - match AcceptedTransaction::accept(transaction.clone(), &chain_id, &TRANSACTION_LIMITS) { + match AcceptedTransaction::accept(transaction.clone(), &chain_id, TRANSACTION_LIMITS) { Ok(_) => success_count += 1, Err(_) => failures_count += 1, } @@ -98,13 +98,13 @@ fn sign_transaction(criterion: &mut Criterion) { let chain_id = ChainId::from("00000000-0000-0000-0000-000000000000"); let transaction = build_test_transaction(chain_id); - let key_pair = KeyPair::random(); + let (_, private_key) = KeyPair::random().into_parts(); let mut count = 0; let _ = criterion.bench_function("sign", |b| { b.iter_batched( || transaction.clone(), |transaction| { - let _: SignedTransaction = transaction.sign(&key_pair); + let _: SignedTransaction = transaction.sign(&private_key); count += 1; }, BatchSize::SmallInput, @@ -117,9 +117,9 @@ fn validate_transaction(criterion: &mut Criterion) { let chain_id = ChainId::from("00000000-0000-0000-0000-000000000000"); let transaction = AcceptedTransaction::accept( - build_test_transaction(chain_id.clone()).sign(&STARTER_KEYPAIR), + build_test_transaction(chain_id.clone()).sign(STARTER_KEYPAIR.private_key()), &chain_id, - &TRANSACTION_LIMITS, + TRANSACTION_LIMITS, ) .expect("Failed to accept transaction."); let mut success_count = 0; @@ -142,9 +142,9 @@ fn sign_blocks(criterion: &mut Criterion) { let chain_id = ChainId::from("00000000-0000-0000-0000-000000000000"); let transaction = AcceptedTransaction::accept( - build_test_transaction(chain_id.clone()).sign(&STARTER_KEYPAIR), + build_test_transaction(chain_id.clone()).sign(STARTER_KEYPAIR.private_key()), &chain_id, - &TRANSACTION_LIMITS, + TRANSACTION_LIMITS, ) .expect("Failed to accept transaction."); let kura = iroha_core::kura::Kura::blank_kura_for_testing(); diff --git a/core/src/block.rs b/core/src/block.rs index 064bde62ec4..6e5cacdcaad 100644 --- a/core/src/block.rs +++ b/core/src/block.rs @@ -132,8 +132,6 @@ mod pending { commit_topology: Topology, event_recommendations: Vec, ) -> Self { - assert!(!transactions.is_empty(), "Empty block created"); - Self(Pending { commit_topology, transactions, @@ -154,14 +152,17 @@ mod pending { .iter() .map(|value| value.as_ref().hash()) .collect::>() - .hash(), + .hash() + .expect("INTERNAL BUG: Empty block created"), timestamp_ms: SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) .expect("Failed to get the current system time") .as_millis() .try_into() .expect("Time should fit into u64"), - view_change_index: view_change_index as u32, + view_change_index: view_change_index + .try_into() + .expect("View change index should fit into u32"), consensus_estimation_ms: DEFAULT_CONSENSUS_ESTIMATION .as_millis() .try_into() @@ -239,7 +240,7 @@ mod chained { mod valid { use indexmap::IndexMap; - use iroha_data_model::ChainId; + use iroha_data_model::{account::AccountId, ChainId}; use super::*; use crate::{state::StateBlock, sumeragi::network_topology::Role}; @@ -289,24 +290,14 @@ mod valid { block: &SignedBlock, topology: &Topology, ) -> Result<(), SignatureVerificationError> { - // TODO: ? - //let roles: &[Role] = if topology.view_change_index() >= 1 { - // &[Role::ValidatingPeer, Role::ObservingPeer] - //} else { - // if topology - // .filter_signatures_by_roles(&[Role::ObservingPeer], block.signatures()) - // .next() - // .is_some() - // { - // return Err(SignatureVerificationError::UnknownSignatory); - // } - - // &[Role::ValidatingPeer] - //}; - let roles = &[Role::ValidatingPeer, Role::ObservingPeer]; + let valid_roles: &[Role] = if topology.view_change_index() >= 1 { + &[Role::ValidatingPeer, Role::ObservingPeer] + } else { + &[Role::ValidatingPeer] + }; topology - .filter_signatures_by_roles(roles, block.signatures()) + .filter_signatures_by_roles(valid_roles, block.signatures()) .try_fold(IndexMap::::default(), |mut acc, signature| { let signatory_idx = usize::try_from(signature.0) .map_err(|_err| SignatureVerificationError::UnknownSignatory)?; @@ -333,6 +324,13 @@ mod valid { Ok(()) })?; + Ok(()) + } + + fn verify_no_undefined_signatures( + block: &SignedBlock, + topology: &Topology, + ) -> Result<(), SignatureVerificationError> { if topology .filter_signatures_by_roles(&[Role::Undefined], block.signatures()) .next() @@ -398,7 +396,7 @@ mod valid { block: SignedBlock, topology: &Topology, expected_chain_id: &ChainId, - genesis_public_key: &PublicKey, + genesis_account: &AccountId, state_block: &mut StateBlock<'_>, ) -> WithEvents> { let expected_block_height = state_block.height() + 1; @@ -430,6 +428,7 @@ mod valid { if !block.header().is_genesis() { if let Err(err) = Self::verify_leader_signature(&block, topology) .and_then(|()| Self::verify_validator_signatures(&block, topology)) + .and_then(|()| Self::verify_no_undefined_signatures(&block, topology)) { return WithEvents::new(Err((block, err.into()))); } @@ -460,12 +459,9 @@ mod valid { ))); } - if let Err(error) = Self::validate_transactions( - &block, - expected_chain_id, - genesis_public_key, - state_block, - ) { + if let Err(error) = + Self::validate_transactions(&block, expected_chain_id, genesis_account, state_block) + { return WithEvents::new(Err((block, error.into()))); } @@ -475,7 +471,7 @@ mod valid { fn validate_transactions( block: &SignedBlock, expected_chain_id: &ChainId, - genesis_public_key: &PublicKey, + genesis_account: &AccountId, state_block: &mut StateBlock<'_>, ) -> Result<(), TransactionValidationError> { let is_genesis = block.header().is_genesis(); @@ -486,16 +482,19 @@ mod valid { .cloned() .try_for_each(|CommittedTransaction { value, error }| { let transaction_executor = state_block.transaction_executor(); - let limits = &transaction_executor.transaction_limits; let tx = if is_genesis { AcceptedTransaction::accept_genesis( GenesisTransaction(value), expected_chain_id, - genesis_public_key, + genesis_account, ) } else { - AcceptedTransaction::accept(value, expected_chain_id, limits) + AcceptedTransaction::accept( + value, + expected_chain_id, + transaction_executor.transaction_limits, + ) }?; if error.is_some() { @@ -522,9 +521,21 @@ mod valid { &mut self, signature: BlockSignature, topology: &Topology, - ) -> Result<(), iroha_crypto::Error> { - let signatory = &topology.as_ref()[signature.0 as usize]; - self.0.add_signature(signature, signatory.public_key()) + ) -> Result<(), SignatureVerificationError> { + let signatory_idx = usize::try_from(signature.0) + .expect("INTERNAL BUG: Number of peers exceeds usize::MAX"); + let signatory = &topology.as_ref()[signatory_idx]; + + assert_ne!(Role::Leader, topology.role(signatory)); + if topology.view_change_index() == 0 { + assert_ne!(Role::ObservingPeer, topology.role(signatory),); + } + assert_ne!(Role::Undefined, topology.role(signatory)); + assert_ne!(Role::ProxyTail, topology.role(signatory)); + + self.0 + .add_signature(signature, signatory.public_key()) + .map_err(|_err| SignatureVerificationError::UnknownSignature) } /// Replace block's signatures. Returns previous block signatures @@ -544,6 +555,7 @@ mod valid { if let Err(err) = Self::verify_leader_signature(self.as_ref(), topology) .and_then(|()| Self::verify_validator_signatures(self.as_ref(), topology)) + .and_then(|()| Self::verify_no_undefined_signatures(self.as_ref(), topology)) { self.0.replace_signatures_unchecked(prev_signatures); WithEvents::new(Err(err)) @@ -604,7 +616,9 @@ mod valid { header: BlockHeader { height: 2, previous_block_hash: None, - transactions_hash: None, + transactions_hash: HashOf::from_untyped_unchecked(Hash::prehashed( + [1; Hash::LENGTH], + )), timestamp_ms: 0, view_change_index: 0, consensus_estimation_ms: 0, @@ -635,13 +649,9 @@ mod valid { #[cfg(test)] mod tests { use iroha_crypto::SignatureOf; - use iroha_primitives::unique_vec::UniqueVec; use super::*; - use crate::{ - kura::Kura, query::store::LiveQueryStore, state::State, - sumeragi::network_topology::test_peers, - }; + use crate::sumeragi::network_topology::test_peers; #[test] fn signature_verification_ok() { @@ -892,7 +902,7 @@ mod tests { use iroha_data_model::prelude::*; use iroha_genesis::GENESIS_DOMAIN_ID; use iroha_primitives::unique_vec::UniqueVec; - use test_samples::gen_account_in; + use test_samples::{gen_account_in, SAMPLE_GENESIS_ACCOUNT_ID}; use super::*; use crate::{ @@ -937,10 +947,10 @@ mod tests { Register::asset_definition(AssetDefinition::numeric(asset_definition_id)); // Making two transactions that have the same instruction - let transaction_limits = &state_block.transaction_executor().transaction_limits; + let transaction_limits = state_block.transaction_executor().transaction_limits; let tx = TransactionBuilder::new(chain_id.clone(), alice_id) .with_instructions([create_asset_definition]) - .sign(&alice_keypair); + .sign(alice_keypair.private_key()); let tx = AcceptedTransaction::accept(tx, &chain_id, transaction_limits).expect("Valid"); // Creating a block of two identical transactions and validating it @@ -994,10 +1004,10 @@ mod tests { Register::asset_definition(AssetDefinition::numeric(asset_definition_id.clone())); // Making two transactions that have the same instruction - let transaction_limits = &state_block.transaction_executor().transaction_limits; + let transaction_limits = state_block.transaction_executor().transaction_limits; let tx = TransactionBuilder::new(chain_id.clone(), alice_id.clone()) .with_instructions([create_asset_definition]) - .sign(&alice_keypair); + .sign(alice_keypair.private_key()); let tx = AcceptedTransaction::accept(tx, &chain_id, transaction_limits).expect("Valid"); let fail_mint = Mint::asset_numeric( @@ -1010,12 +1020,12 @@ mod tests { let tx0 = TransactionBuilder::new(chain_id.clone(), alice_id.clone()) .with_instructions([fail_mint]) - .sign(&alice_keypair); + .sign(alice_keypair.private_key()); let tx0 = AcceptedTransaction::accept(tx0, &chain_id, transaction_limits).expect("Valid"); let tx2 = TransactionBuilder::new(chain_id.clone(), alice_id) .with_instructions([succeed_mint]) - .sign(&alice_keypair); + .sign(alice_keypair.private_key()); let tx2 = AcceptedTransaction::accept(tx2, &chain_id, transaction_limits).expect("Valid"); // Creating a block of two identical transactions and validating it @@ -1065,7 +1075,7 @@ mod tests { let query_handle = LiveQueryStore::test().start(); let state = State::new(world, kura, query_handle); let mut state_block = state.block(); - let transaction_limits = &state_block.transaction_executor().transaction_limits; + let transaction_limits = state_block.transaction_executor().transaction_limits; let domain_id = DomainId::from_str("domain").expect("Valid"); let create_domain = Register::domain(Domain::new(domain_id)); @@ -1079,12 +1089,12 @@ mod tests { let instructions_accept: [InstructionBox; 2] = [create_domain.into(), create_asset.into()]; let tx_fail = TransactionBuilder::new(chain_id.clone(), alice_id.clone()) .with_instructions(instructions_fail) - .sign(&alice_keypair); + .sign(alice_keypair.private_key()); let tx_fail = AcceptedTransaction::accept(tx_fail, &chain_id, transaction_limits).expect("Valid"); let tx_accept = TransactionBuilder::new(chain_id.clone(), alice_id) .with_instructions(instructions_accept) - .sign(&alice_keypair); + .sign(alice_keypair.private_key()); let tx_accept = AcceptedTransaction::accept(tx_accept, &chain_id, transaction_limits).expect("Valid"); @@ -1159,11 +1169,11 @@ mod tests { // Sign with `genesis_wrong_key` as peer which has incorrect genesis key pair let tx = TransactionBuilder::new(chain_id.clone(), genesis_wrong_account_id.clone()) .with_instructions([isi]) - .sign(&genesis_wrong_key); + .sign(genesis_wrong_key.private_key()); let tx = AcceptedTransaction::accept_genesis( iroha_genesis::GenesisTransaction(tx), &chain_id, - genesis_wrong_key.public_key(), + &SAMPLE_GENESIS_ACCOUNT_ID, ) .expect("Valid"); @@ -1184,7 +1194,7 @@ mod tests { block, &topology, &chain_id, - genesis_correct_key.public_key(), + &SAMPLE_GENESIS_ACCOUNT_ID, &mut state_block, ) .unpack(|_| {}) diff --git a/core/src/gossiper.rs b/core/src/gossiper.rs index c964bbb151f..4f5018aa9f6 100644 --- a/core/src/gossiper.rs +++ b/core/src/gossiper.rs @@ -110,7 +110,7 @@ impl TransactionGossiper { let state_view = self.state.view(); for tx in txs { - let transaction_limits = &state_view.config.transaction_limits; + let transaction_limits = state_view.config.transaction_limits; match AcceptedTransaction::accept(tx, &self.chain_id, transaction_limits) { Ok(tx) => match self.queue.push(tx, &state_view) { diff --git a/core/src/queue.rs b/core/src/queue.rs index 1fb321da655..1e343eb4033 100644 --- a/core/src/queue.rs +++ b/core/src/queue.rs @@ -22,12 +22,6 @@ use crate::{prelude::*, EventsSender}; impl AcceptedTransaction { // TODO: We should have another type of transaction like `CheckedTransaction` in the type system? - fn is_signatory_consistent(&self) -> bool { - let authority = self.as_ref().authority(); - let signatory = &self.as_ref().signature().0; - authority.signatory_matches(signatory) - } - /// Check if [`self`] is committed or rejected. fn is_in_blockchain(&self, state_view: &StateView<'_>) -> bool { state_view.has_transaction(self.as_ref().hash()) @@ -77,8 +71,6 @@ pub enum Error { MaximumTransactionsPerUser, /// The transaction is already in the queue IsInQueue, - /// Signatories in signature and payload mismatch - SignatoryInconsistent, } /// Failure that can pop up when pushing transaction into the queue @@ -175,8 +167,6 @@ impl Queue { Err(Error::Expired) } else if tx.is_in_blockchain(state_view) { Err(Error::InBlockchain) - } else if !tx.is_signatory_consistent() { - Err(Error::SignatoryInconsistent) } else { Ok(()) } @@ -436,12 +426,12 @@ pub mod tests { let tx = TransactionBuilder::new_with_time_source(chain_id.clone(), account_id, time_source) .with_instructions(instructions) - .sign(key_pair); + .sign(key_pair.private_key()); let limits = TransactionLimits { max_instruction_number: 4096, max_wasm_size_bytes: 0, }; - AcceptedTransaction::accept(tx, &chain_id, &limits).expect("Failed to accept Transaction.") + AcceptedTransaction::accept(tx, &chain_id, limits).expect("Failed to accept Transaction.") } pub fn world_with_test_domains() -> World { @@ -687,13 +677,13 @@ pub mod tests { TransactionBuilder::new_with_time_source(chain_id.clone(), alice_id, &time_source) .with_instructions(instructions); tx.set_ttl(Duration::from_millis(TTL_MS)); - let tx = tx.sign(&alice_keypair); + let tx = tx.sign(alice_keypair.private_key()); let limits = TransactionLimits { max_instruction_number: 4096, max_wasm_size_bytes: 0, }; let tx_hash = tx.hash(); - let tx = AcceptedTransaction::accept(tx, &chain_id, &limits) + let tx = AcceptedTransaction::accept(tx, &chain_id, limits) .expect("Failed to accept Transaction."); queue .push(tx.clone(), &state_view) diff --git a/core/src/smartcontracts/isi/mod.rs b/core/src/smartcontracts/isi/mod.rs index 659f778ecea..052a3ac5ebd 100644 --- a/core/src/smartcontracts/isi/mod.rs +++ b/core/src/smartcontracts/isi/mod.rs @@ -493,8 +493,8 @@ mod tests { let instructions: [InstructionBox; 0] = []; let tx = TransactionBuilder::new(chain_id.clone(), SAMPLE_GENESIS_ACCOUNT_ID.clone()) .with_instructions(instructions) - .sign(&SAMPLE_GENESIS_ACCOUNT_KEYPAIR); - let tx_limits = &state_block.transaction_executor().transaction_limits; + .sign(SAMPLE_GENESIS_ACCOUNT_KEYPAIR.private_key()); + let tx_limits = state_block.transaction_executor().transaction_limits; assert!(matches!( AcceptedTransaction::accept(tx, &chain_id, tx_limits), Err(AcceptTransactionFail::UnexpectedGenesisAccountSignature) diff --git a/core/src/smartcontracts/isi/query.rs b/core/src/smartcontracts/isi/query.rs index 39d7038fac6..e027929ab83 100644 --- a/core/src/smartcontracts/isi/query.rs +++ b/core/src/smartcontracts/isi/query.rs @@ -182,12 +182,6 @@ impl ValidQueryRequest { query: SignedQuery, state_ro: &impl StateReadOnly, ) -> Result { - if !query.authority().signatory_matches(&query.signature().0) { - return Err(Error::Signature(String::from( - "Signature public key doesn't correspond to the account.", - )) - .into()); - } state_ro.world().executor().validate_query( state_ro, query.authority(), @@ -395,15 +389,15 @@ mod tests { let instructions: [InstructionBox; 0] = []; let tx = TransactionBuilder::new(chain_id.clone(), ALICE_ID.clone()) .with_instructions(instructions) - .sign(&ALICE_KEYPAIR); - AcceptedTransaction::accept(tx, &chain_id, &limits)? + .sign(ALICE_KEYPAIR.private_key()); + AcceptedTransaction::accept(tx, &chain_id, limits)? }; let invalid_tx = { let isi = Fail::new("fail".to_owned()); let tx = TransactionBuilder::new(chain_id.clone(), ALICE_ID.clone()) .with_instructions([isi.clone(), isi]) - .sign(&ALICE_KEYPAIR); - AcceptedTransaction::accept(tx, &chain_id, &huge_limits)? + .sign(ALICE_KEYPAIR.private_key()); + AcceptedTransaction::accept(tx, &chain_id, huge_limits)? }; let mut transactions = vec![valid_tx; valid_tx_per_block]; @@ -560,9 +554,9 @@ mod tests { let instructions: [InstructionBox; 0] = []; let tx = TransactionBuilder::new(chain_id.clone(), ALICE_ID.clone()) .with_instructions(instructions) - .sign(&ALICE_KEYPAIR); + .sign(ALICE_KEYPAIR.private_key()); - let tx_limits = &state_block.transaction_executor().transaction_limits; + let tx_limits = state_block.transaction_executor().transaction_limits; let va_tx = AcceptedTransaction::accept(tx, &chain_id, tx_limits)?; let (peer_public_key, _) = KeyPair::random().into_parts(); @@ -584,7 +578,7 @@ mod tests { let unapplied_tx = TransactionBuilder::new(chain_id, ALICE_ID.clone()) .with_instructions([Unregister::account(gen_account_in("domain").0)]) - .sign(&ALICE_KEYPAIR); + .sign(ALICE_KEYPAIR.private_key()); let wrong_hash = unapplied_tx.hash(); let not_found = FindTransactionByHash::new(wrong_hash).execute(&state_view); assert!(matches!( diff --git a/core/src/snapshot.rs b/core/src/snapshot.rs index 08c908e0b73..a2d100e86bb 100644 --- a/core/src/snapshot.rs +++ b/core/src/snapshot.rs @@ -161,7 +161,10 @@ pub fn try_read_snapshot( }; let state = seed.deserialize(&mut deserializer)?; let state_view = state.view(); - let snapshot_height = state_view.height() as usize; + let snapshot_height = state_view + .height() + .try_into() + .expect("Blockchain height should fit into usize"); if snapshot_height > block_count { return Err(TryReadError::MismatchedHeight { snapshot_height, diff --git a/core/src/sumeragi/main_loop.rs b/core/src/sumeragi/main_loop.rs index 7bb5e735e74..53fc50d764b 100644 --- a/core/src/sumeragi/main_loop.rs +++ b/core/src/sumeragi/main_loop.rs @@ -1,5 +1,5 @@ //! The main event loop that powers sumeragi. -use std::sync::mpsc; +use std::{collections::BTreeSet, sync::mpsc}; use iroha_crypto::{HashOf, KeyPair}; use iroha_data_model::{block::*, events::pipeline::PipelineEventBox, peer::PeerId}; @@ -209,7 +209,7 @@ impl Sumeragi { fn init_listen_for_genesis( &mut self, - genesis_public_key: &PublicKey, + genesis_account: &AccountId, state: &State, shutdown_receiver: &mut tokio::sync::oneshot::Receiver<()>, ) -> Result<(), EarlyReturn> { @@ -242,7 +242,7 @@ impl Sumeragi { block, &self.topology, &self.chain_id, - genesis_public_key, + genesis_account, &mut state_block, ) .unpack(|e| self.send_event(e)) @@ -286,10 +286,10 @@ impl Sumeragi { } } - fn sumeragi_init_commit_genesis( + fn init_commit_genesis( &mut self, genesis_network: GenesisNetwork, - genesis_public_key: &PublicKey, + genesis_account: &AccountId, state: &State, ) { std::thread::sleep(Duration::from_millis(250)); // TODO: Why this sleep? @@ -303,7 +303,7 @@ impl Sumeragi { let transactions: Vec<_> = genesis_network .into_transactions() .into_iter() - .map(|tx| AcceptedTransaction::accept_genesis(tx, &self.chain_id, genesis_public_key)) + .map(|tx| AcceptedTransaction::accept_genesis(tx, &self.chain_id, genesis_account)) .collect::>() .expect("Genesis invalid"); @@ -353,14 +353,18 @@ impl Sumeragi { .block_committed(block.as_ref(), state_block.world.peers().cloned()); self.connect_peers(&self.topology); + let block_hash = block.as_ref().hash(); + let block_height = block.as_ref().header().height(); + Strategy::kura_store_block(&self.kura, block); + // Commit new block making it's effect visible for the rest of application state_block.commit(); info!( peer_id=%self.peer_id, %prev_role, next_role=%self.role(), - block_hash=%block.as_ref().hash(), - new_height=%block.as_ref().header().height, + block_hash=%block_hash, + new_height=%block_height, "{}", Strategy::LOG_MESSAGE, ); #[cfg(debug_assertions)] @@ -371,8 +375,6 @@ impl Sumeragi { "Topology after commit" ); - Strategy::kura_store_block(&self.kura, block); - // NOTE: This sends `BlockStatus::Applied` event, // so it should be done AFTER public facing state update state_events.into_iter().for_each(|e| self.send_event(e)); @@ -408,7 +410,7 @@ impl Sumeragi { &self, state: &'state State, topology: &Topology, - genesis_public_key: &PublicKey, + genesis_account: &AccountId, BlockCreated { block }: BlockCreated, ) -> Option> { let mut state_block = state.block(); @@ -417,7 +419,7 @@ impl Sumeragi { block, topology, &self.chain_id, - genesis_public_key, + genesis_account, &mut state_block, ) .unpack(|e| self.send_event(e)) @@ -444,14 +446,15 @@ impl Sumeragi { } #[allow(clippy::too_many_lines)] + #[allow(clippy::too_many_arguments)] fn handle_message<'state>( &mut self, message: BlockMessage, state: &'state State, voting_block: &mut Option>, view_change_index: usize, - genesis_public_key: &PublicKey, - voting_signatures: &mut Vec, + genesis_account: &AccountId, + voting_signatures: &mut BTreeSet, #[cfg_attr(not(debug_assertions), allow(unused_variables))] is_genesis_peer: bool, ) { #[allow(clippy::suspicious_operation_groupings)] @@ -467,9 +470,10 @@ impl Sumeragi { // Release block writer before creating new one // FIX: Restore `voting_block` if `handle_block_sync` returns Err // Currently it's not possible because block writer needs to be released + // Look at https://github.com/hyperledger/iroha/issues/4643 let _ = voting_block.take(); - match handle_block_sync(&self.chain_id, block, state, genesis_public_key, &|e| { + match handle_block_sync(&self.chain_id, block, state, genesis_account, &|e| { self.send_event(e) }) { Ok(BlockSyncOk::CommitBlock(block, state_block, topology)) => { @@ -556,7 +560,7 @@ impl Sumeragi { let _ = voting_block.take(); if let Some(mut v_block) = - self.validate_block(state, topology, genesis_public_key, block_created) + self.validate_block(state, topology, genesis_account, block_created) { v_block.block.sign(&self.key_pair, topology); @@ -567,6 +571,7 @@ impl Sumeragi { } else { // FIX: Restore `voting_block` // Currently it's not possible because block writer needs to be released + // Look at https://github.com/hyperledger/iroha/issues/4643 } } (BlockMessage::BlockCreated(block_created), Role::ObservingPeer) => { @@ -579,7 +584,7 @@ impl Sumeragi { let _ = voting_block.take(); if let Some(mut v_block) = - self.validate_block(state, topology, genesis_public_key, block_created) + self.validate_block(state, topology, genesis_account, block_created) { if view_change_index >= 1 { v_block.block.sign(&self.key_pair, topology); @@ -599,6 +604,7 @@ impl Sumeragi { } else { // FIX: Restore `voting_block` // Currently it's not possible because block writer needs to be released + // Look at https://github.com/hyperledger/iroha/issues/4643 } } (BlockMessage::BlockCreated(block_created), Role::ProxyTail) => { @@ -613,98 +619,165 @@ impl Sumeragi { let _ = voting_block.take(); if let Some(mut valid_block) = - self.validate_block(state, &self.topology, genesis_public_key, block_created) + self.validate_block(state, &self.topology, genesis_account, block_created) { // NOTE: Up until this point it was unknown which block is expected to be received, // therefore all the signatures (of any hash) were collected and will now be pruned - add_signatures::( - &mut valid_block, - voting_signatures.drain(..), - &self.topology, - ); + + for signature in core::mem::take(voting_signatures) { + if let Err(error) = + valid_block.block.add_signature(signature, &self.topology) + { + debug!(?error, "Signature not valid"); + } + } *voting_block = self.try_commit_block(valid_block, is_genesis_peer); } else { // FIX: Restore `voting_block` // Currently it's not possible because block writer needs to be released + // Look at https://github.com/hyperledger/iroha/issues/4643 } } - (BlockMessage::BlockSigned(BlockSigned { signatures }), Role::ProxyTail) => { + (BlockMessage::BlockSigned(BlockSigned { hash, signature }), Role::ProxyTail) => { info!( peer_id=%self.peer_id, role=%self.role(), "Received block signatures" ); - let roles: &[Role] = if view_change_index >= 1 { - &[Role::ValidatingPeer, Role::ObservingPeer] - } else { - &[Role::ValidatingPeer] - }; - let valid_signatures = self - .topology - .filter_signatures_by_roles(roles, &signatures) - .cloned(); + if let Ok(signatory_idx) = usize::try_from(signature.0) { + let signatory = &self.topology.as_ref()[signatory_idx]; - if let Some(mut voted_block) = voting_block.take() { - add_signatures::(&mut voted_block, valid_signatures, &self.topology); - *voting_block = self.try_commit_block(voted_block, is_genesis_peer); + match self.topology.role(signatory) { + Role::Leader => error!( + peer_id=%self.peer_id, + role=%self.role(), + "Signatory is leader" + ), + Role::Undefined => error!( + peer_id=%self.peer_id, + role=%self.role(), + "Unknown signatory" + ), + Role::ObservingPeer if view_change_index == 0 => error!( + peer_id=%self.peer_id, + role=%self.role(), + "Signatory is observing peer" + ), + Role::ProxyTail => error!( + peer_id=%self.peer_id, + role=%self.role(), + "Signatory is proxy tail" + ), + _ => { + if let Some(mut voted_block) = voting_block.take() { + let actual_hash = voted_block.block.as_ref().hash_of_payload(); + + if hash != actual_hash { + error!( + peer_id=%self.peer_id, + role=%self.role(), + expected_hash=?hash, + ?actual_hash, + "Block hash mismatch" + ); + } else if let Err(err) = + voted_block.block.add_signature(signature, &self.topology) + { + error!( + peer_id=%self.peer_id, + role=%self.role(), + ?err, + "Signature not valid" + ); + } else { + *voting_block = + self.try_commit_block(voted_block, is_genesis_peer); + } + } else { + // NOTE: Due to the nature of distributed systems, signatures can sometimes be received before + // the block (sent by the leader). Collect the signatures and wait for the block to be received + if !voting_signatures.insert(signature) { + error!( + peer_id=%self.peer_id, + role=%self.role(), + "Duplicate signature" + ); + } + } + } + } } else { - // NOTE: Due to the nature of distributed systems, signatures can sometimes be received before - // the block (sent by the leader). Collect the signatures and wait for the block to be received - voting_signatures.extend(valid_signatures); + error!( + peer_id=%self.peer_id, + role=%self.role(), + "Signatory index exceeds usize::MAX" + ); } } (BlockMessage::BlockCommitted(BlockCommitted { .. }), Role::Leader) if self.topology.is_consensus_required().is_none() => {} ( - BlockMessage::BlockCommitted(BlockCommitted { signatures }), + BlockMessage::BlockCommitted(BlockCommitted { hash, signatures }), Role::Leader | Role::ValidatingPeer | Role::ObservingPeer, ) => { if let Some(mut voted_block) = voting_block.take() { - match voted_block - .block - // NOTE: The manipulation of the topology relies upon all peers seeing the same signature set. - // Therefore we must clear the signatures and accept what the proxy tail giveth. - .replace_signatures(signatures, &self.topology) - .unpack(|e| self.send_event(e)) - { - Ok(prev_signatures) => { - match voted_block - .block - .commit(&self.topology) - .unpack(|e| self.send_event(e)) - { - Ok(committed_block) => { - self.commit_block(committed_block, voted_block.block_state) - } - Err((mut block, error)) => { - error!( - peer_id=%self.peer_id, - role=%self.role(), - ?error, - "Block failed to be committed" - ); - - block - .replace_signatures(prev_signatures, &self.topology) - .unpack(|e| self.send_event(e)) - .expect("INTERNAL BUG: Failed to replace signatures"); - voted_block.block = block; - *voting_block = Some(voted_block); + let actual_hash = voted_block.block.as_ref().hash(); + + if actual_hash == hash { + match voted_block + .block + // NOTE: The manipulation of the topology relies upon all peers seeing the same signature set. + // Therefore we must clear the signatures and accept what the proxy tail has giveth. + .replace_signatures(signatures, &self.topology) + .unpack(|e| self.send_event(e)) + { + Ok(prev_signatures) => { + match voted_block + .block + .commit(&self.topology) + .unpack(|e| self.send_event(e)) + { + Ok(committed_block) => { + self.commit_block(committed_block, voted_block.state_block) + } + Err((mut block, error)) => { + error!( + peer_id=%self.peer_id, + role=%self.role(), + ?error, + "Block failed to be committed" + ); + + block + .replace_signatures(prev_signatures, &self.topology) + .unpack(|e| self.send_event(e)) + .expect("INTERNAL BUG: Failed to replace signatures"); + voted_block.block = block; + *voting_block = Some(voted_block); + } } } + Err(error) => { + error!( + peer_id=%self.peer_id, + role=%self.role(), + ?error, + "Received incorrect signatures" + ); + + *voting_block = Some(voted_block); + } } - Err(error) => { - error!( - peer_id=%self.peer_id, - role=%self.role(), - ?error, - "Received incorrect signatures" - ); - - *voting_block = Some(voted_block); - } + } else { + error!( + peer_id=%self.peer_id, + role=%self.role(), + expected_hash=?hash, + ?actual_hash, + "Block hash mismatch" + ); } } else { error!( @@ -757,7 +830,7 @@ impl Sumeragi { self.broadcast_packet(msg); } - self.commit_block(committed_block, voting_block.block_state); + self.commit_block(committed_block, voting_block.state_block); return None; } @@ -836,7 +909,7 @@ fn reset_state( was_commit: &mut bool, topology: &mut Topology, voting_block: &mut Option, - voting_signatures: &mut Vec, + voting_signatures: &mut BTreeSet, last_view_change_time: &mut Instant, view_change_time: &mut Duration, ) { @@ -899,26 +972,30 @@ pub(crate) fn run( // Connect peers with initial topology sumeragi.connect_peers(&sumeragi.topology); + let genesis_account = AccountId::new( + iroha_genesis::GENESIS_DOMAIN_ID.clone(), + genesis_network.public_key.clone(), + ); + let span = span!(tracing::Level::TRACE, "genesis").entered(); - let is_genesis_peer = - if state.view().height() == 0 || state.view().latest_block_hash().is_none() { - if let Some(genesis) = genesis_network.genesis { - sumeragi.sumeragi_init_commit_genesis(genesis, &genesis_network.public_key, &state); - true - } else { - if let Err(err) = sumeragi.init_listen_for_genesis( - &genesis_network.public_key, - &state, - &mut shutdown_receiver, - ) { - info!(?err, "Sumeragi Thread is being shut down."); - return; - } - false - } + let is_genesis_peer = if state.view().height() == 0 + || state.view().latest_block_hash().is_none() + { + if let Some(genesis) = genesis_network.genesis { + sumeragi.init_commit_genesis(genesis, &genesis_account, &state); + true } else { + if let Err(err) = + sumeragi.init_listen_for_genesis(&genesis_account, &state, &mut shutdown_receiver) + { + info!(?err, "Sumeragi Thread is being shut down."); + return; + } false - }; + } + } else { + false + }; span.exit(); info!( @@ -929,7 +1006,7 @@ pub(crate) fn run( let mut voting_block = None; // Proxy tail collection of voting block signatures - let mut voting_signatures = Vec::new(); + let mut voting_signatures = BTreeSet::new(); let mut should_sleep = false; let mut view_change_proof_chain = ProofChain::default(); // Duration after which a view change is suggested @@ -1004,7 +1081,7 @@ pub(crate) fn run( &state, &mut voting_block, view_change_index, - &genesis_network.public_key, + &genesis_account, &mut voting_signatures, is_genesis_peer, ); @@ -1085,24 +1162,6 @@ pub(crate) fn run( } } -fn add_signatures( - block: &mut VotingBlock, - signatures: impl IntoIterator, - topology: &Topology, -) { - for signature in signatures { - if let Err(error) = block.block.add_signature(signature, topology) { - let err_msg = "Signature not valid"; - - if EXPECT_VALID { - error!(?error, err_msg); - } else { - debug!(?error, err_msg); - } - } - } -} - /// Type enumerating early return types to reduce cyclomatic /// complexity of the main loop items and allow direct short /// circuiting with the `?` operator. Candidate for `impl @@ -1184,7 +1243,7 @@ fn handle_block_sync<'state, F: Fn(PipelineEventBox)>( chain_id: &ChainId, block: SignedBlock, state: &'state State, - genesis_public_key: &PublicKey, + genesis_account: &AccountId, handle_events: &F, ) -> Result, (SignedBlock, BlockSyncError)> { let block_height = block.header().height; @@ -1234,7 +1293,7 @@ fn handle_block_sync<'state, F: Fn(PipelineEventBox)>( block, &topology, chain_id, - genesis_public_key, + genesis_account, &mut state_block, ) .unpack(handle_events) @@ -1265,6 +1324,7 @@ fn handle_block_sync<'state, F: Fn(PipelineEventBox)>( #[cfg(test)] mod tests { + use iroha_genesis::GENESIS_DOMAIN_ID; use test_samples::gen_account_in; use tokio::test; @@ -1287,10 +1347,13 @@ mod tests { chain_id: &ChainId, topology: &Topology, leader_private_key: &PrivateKey, - ) -> (State, Arc, SignedBlock, PublicKey) { + ) -> (State, Arc, SignedBlock, AccountId) { // Predefined world state let (alice_id, alice_keypair) = gen_account_in("wonderland"); - let genesis_public_key = alice_keypair.public_key().clone(); + let genesis_account = AccountId::new( + GENESIS_DOMAIN_ID.clone(), + alice_keypair.public_key().clone(), + ); let account = Account::new(alice_id.clone()).build(&alice_id); let domain_id = "wonderland".parse().expect("Valid"); let mut domain = Domain::new(domain_id).build(&alice_id); @@ -1308,11 +1371,11 @@ mod tests { // Making two transactions that have the same instruction let tx = TransactionBuilder::new(chain_id.clone(), alice_id.clone()) .with_instructions([fail_box]) - .sign(&alice_keypair); + .sign(alice_keypair.private_key()); let tx = AcceptedTransaction::accept( tx, chain_id, - &state_block.transaction_executor().transaction_limits, + state_block.transaction_executor().transaction_limits, ) .expect("Valid"); @@ -1342,21 +1405,21 @@ mod tests { let tx1 = TransactionBuilder::new(chain_id.clone(), alice_id.clone()) .with_instructions([create_asset_definition1]) - .sign(&alice_keypair); + .sign(alice_keypair.private_key()); let tx1 = AcceptedTransaction::accept( tx1, chain_id, - &state_block.transaction_executor().transaction_limits, + state_block.transaction_executor().transaction_limits, ) .map(Into::into) .expect("Valid"); let tx2 = TransactionBuilder::new(chain_id.clone(), alice_id) .with_instructions([create_asset_definition2]) - .sign(&alice_keypair); + .sign(alice_keypair.private_key()); let tx2 = AcceptedTransaction::accept( tx2, chain_id, - &state_block.transaction_executor().transaction_limits, + state_block.transaction_executor().transaction_limits, ) .map(Into::into) .expect("Valid"); @@ -1368,7 +1431,7 @@ mod tests { .unpack(|_| {}) }; - (state, kura, block.into(), genesis_public_key) + (state, kura, block.into(), genesis_account) } #[test] diff --git a/core/src/sumeragi/message.rs b/core/src/sumeragi/message.rs index 390763094f7..30bb1ae3ea3 100644 --- a/core/src/sumeragi/message.rs +++ b/core/src/sumeragi/message.rs @@ -1,5 +1,6 @@ //! Contains message structures for p2p communication during consensus. -use iroha_data_model::block::{BlockSignature, SignedBlock}; +use iroha_crypto::HashOf; +use iroha_data_model::block::{BlockPayload, BlockSignature, SignedBlock}; use iroha_macro::*; use parity_scale_codec::{Decode, Encode}; @@ -37,7 +38,6 @@ impl ControlFlowMessage { /// `BlockCreated` message structure. #[derive(Debug, Clone, Decode, Encode)] -#[non_exhaustive] pub struct BlockCreated { /// The corresponding block. pub block: SignedBlock, @@ -54,24 +54,32 @@ impl From<&ValidBlock> for BlockCreated { /// `BlockSigned` message structure. #[derive(Debug, Clone, Decode, Encode)] -#[non_exhaustive] pub struct BlockSigned { - /// Set of signatures. - pub signatures: Vec, + /// Hash of the block being signed. + pub hash: HashOf, + /// Signature of the block + pub signature: BlockSignature, } impl From<&ValidBlock> for BlockSigned { fn from(block: &ValidBlock) -> Self { Self { - signatures: block.as_ref().signatures().cloned().collect(), + hash: block.as_ref().hash_of_payload(), + signature: block + .as_ref() + .signatures() + .last() + .cloned() + .expect("INTERNAL BUG: Block must have at least one signature"), } } } /// `BlockCommitted` message structure. -#[derive(Debug, Clone, Decode, Encode)] -#[non_exhaustive] +#[derive(Debug, Clone, Encode)] pub struct BlockCommitted { + /// Hash of the block being signed. + pub hash: HashOf, /// Set of signatures. pub signatures: Vec, } @@ -79,6 +87,7 @@ pub struct BlockCommitted { impl From<&CommittedBlock> for BlockCommitted { fn from(block: &CommittedBlock) -> Self { Self { + hash: block.as_ref().hash(), signatures: block.as_ref().signatures().cloned().collect(), } } @@ -86,7 +95,6 @@ impl From<&CommittedBlock> for BlockCommitted { /// `BlockSyncUpdate` message structure #[derive(Debug, Clone, Decode, Encode)] -#[non_exhaustive] pub struct BlockSyncUpdate { /// The corresponding block. pub block: SignedBlock, @@ -100,3 +108,56 @@ impl From<&SignedBlock> for BlockSyncUpdate { } } } + +mod candidate { + use indexmap::IndexSet; + use parity_scale_codec::Input; + + use super::*; + + #[derive(Decode)] + struct BlockCommittedCandidate { + /// Hash of the block being signed. + pub hash: HashOf, + /// Set of signatures. + pub signatures: Vec, + } + + impl BlockCommittedCandidate { + fn validate(self) -> Result { + self.validate_signatures()?; + + Ok(BlockCommitted { + hash: self.hash, + signatures: self.signatures, + }) + } + + fn validate_signatures(&self) -> Result<(), &'static str> { + if self.signatures.is_empty() { + return Err("No signatures in block"); + } + + self.signatures + .iter() + .map(|signature| &signature.0) + .try_fold(IndexSet::new(), |mut acc, elem| { + if !acc.insert(elem) { + return Err("Duplicate signature"); + } + + Ok(acc) + })?; + + Ok(()) + } + } + + impl Decode for BlockCommitted { + fn decode(input: &mut I) -> Result { + BlockCommittedCandidate::decode(input)? + .validate() + .map_err(Into::into) + } + } +} diff --git a/core/src/sumeragi/mod.rs b/core/src/sumeragi/mod.rs index 6c0fe4d93ae..a0df438d211 100644 --- a/core/src/sumeragi/mod.rs +++ b/core/src/sumeragi/mod.rs @@ -9,7 +9,7 @@ use std::{ use eyre::Result; use iroha_config::parameters::actual::{Common as CommonConfig, Sumeragi as SumeragiConfig}; -use iroha_data_model::{block::SignedBlock, prelude::*}; +use iroha_data_model::{account::AccountId, block::SignedBlock, prelude::*}; use iroha_genesis::GenesisNetwork; use iroha_logger::prelude::*; use network_topology::{Role, Topology}; @@ -72,7 +72,7 @@ impl SumeragiHandle { fn replay_block( chain_id: &ChainId, - genesis_public_key: &PublicKey, + genesis_account: &AccountId, block: &SignedBlock, state_block: &mut StateBlock<'_>, events_sender: &EventsSender, @@ -85,7 +85,7 @@ impl SumeragiHandle { block.clone(), topology, chain_id, - genesis_public_key, + genesis_account, state_block, ) .unpack(|e| { @@ -178,11 +178,16 @@ impl SumeragiHandle { }; } + let genesis_account = AccountId::new( + iroha_genesis::GENESIS_DOMAIN_ID.clone(), + genesis_network.public_key.clone(), + ); + for block in blocks_iter { let mut state_block = state.block(); Self::replay_block( &common_config.chain_id, - &genesis_network.public_key, + &genesis_account, &block, &mut state_block, &events_sender, @@ -268,7 +273,7 @@ pub struct VotingBlock<'state> { /// At what time has this peer voted for this block pub voted_at: Instant, /// [`WorldState`] after applying transactions to it but before it was committed - pub block_state: StateBlock<'state>, + pub state_block: StateBlock<'state>, } impl AsRef for VotingBlock<'_> { @@ -283,7 +288,7 @@ impl VotingBlock<'_> { VotingBlock { block, voted_at: Instant::now(), - block_state: state_block, + state_block, } } } diff --git a/core/src/sumeragi/network_topology.rs b/core/src/sumeragi/network_topology.rs index 0b7ec997c4b..4b58d52a5b3 100644 --- a/core/src/sumeragi/network_topology.rs +++ b/core/src/sumeragi/network_topology.rs @@ -20,7 +20,12 @@ use iroha_data_model::{ /// /// Above is an illustration of how the various operations work for a f = 2 topology. #[derive(Debug, Clone, PartialEq, Eq)] -pub struct Topology(Vec, usize); +pub struct Topology( + /// Ordered set of peers + Vec, + /// Current view change index. Reset to 0 after every block commit + usize, +); /// Topology with at least one peer #[derive(Debug, Clone, PartialEq, Eq, derive_more::Deref)] @@ -147,9 +152,11 @@ impl Topology { }; } - signatures - .into_iter() - .filter(move |signature| filtered.contains(&(signature.0 as usize))) + signatures.into_iter().filter(move |signature| { + filtered.contains( + &(usize::try_from(signature.0).expect("Peer index should fit into usize")), + ) + }) } /// What role does this peer have in the topology. @@ -209,13 +216,17 @@ impl Topology { self.0 = topology.into_iter().map(|(_, peer)| peer).collect(); } - /// Recreate topology for given block + /// Rotate topology after a block has been committed pub fn block_committed( &mut self, block: &SignedBlock, new_peers: impl IntoIterator, ) { - self.lift_up_peers(block.signatures().map(|s| s.0 as usize)); + self.lift_up_peers( + block + .signatures() + .map(|s| s.0.try_into().expect("Peer index should fit into usize")), + ); self.rotate_set_a(); self.update_peer_list(new_peers); self.1 = 0; diff --git a/core/src/sumeragi/view_change.rs b/core/src/sumeragi/view_change.rs index c2a7b80bb46..88a68337780 100644 --- a/core/src/sumeragi/view_change.rs +++ b/core/src/sumeragi/view_change.rs @@ -2,6 +2,7 @@ //! Where view change is a process of changing topology due to some faulty network behavior. use eyre::Result; +use indexmap::IndexSet; use iroha_crypto::{HashOf, PublicKey, SignatureOf}; use iroha_data_model::block::SignedBlock; use parity_scale_codec::{Decode, Encode}; @@ -26,7 +27,7 @@ struct ViewChangeProofPayload { /// Hash of the latest committed block. latest_block: HashOf, /// Within a round, what is the index of the view change this proof is trying to prove. - view_change_index: u64, + view_change_index: u32, } /// The proof of a view change. It needs to be signed by f+1 peers for proof to be valid and view change to happen. @@ -43,7 +44,9 @@ pub struct ProofBuilder(SignedViewChangeProof); impl ProofBuilder { /// Constructor from index. pub fn new(latest_block: HashOf, view_change_index: usize) -> Self { - let view_change_index = view_change_index as u64; + let view_change_index = view_change_index + .try_into() + .expect("INTERNAL BUG: view change index is too large"); let proof = SignedViewChangeProof { payload: ViewChangeProofPayload { @@ -67,13 +70,21 @@ impl ProofBuilder { impl SignedViewChangeProof { /// Verify the signatures of `other` and add them to this proof. fn merge_signatures(&mut self, other: Vec, topology: &Topology) { - for (public_key, signature) in other { - if topology.position(&public_key).is_none() { - continue; - } - - self.signatures.push((public_key, signature)); - } + let signatures = core::mem::take(&mut self.signatures) + .into_iter() + .collect::>(); + + self.signatures = other + .into_iter() + .fold(signatures, |mut acc, (public_key, signature)| { + if topology.position(&public_key).is_some() { + acc.insert((public_key, signature)); + } + + acc + }) + .into_iter() + .collect(); } /// Verify if the proof is valid, given the peers in `topology`. @@ -104,8 +115,10 @@ impl ProofChain { .iter() .enumerate() .take_while(|(i, proof)| { + let view_change_index = proof.payload.view_change_index as usize; + proof.payload.latest_block == latest_block - && proof.payload.view_change_index == (*i as u64) + && view_change_index == *i && proof.verify(topology) }) .count() @@ -118,8 +131,8 @@ impl ProofChain { .iter() .enumerate() .take_while(|(i, proof)| { - proof.payload.latest_block == latest_block - && proof.payload.view_change_index == (*i as u64) + let view_change_index = proof.payload.view_change_index as usize; + proof.payload.latest_block == latest_block && view_change_index == *i }) .count(); self.0.truncate(valid_count); @@ -140,7 +153,7 @@ impl ProofChain { return Err(Error::BlockHashMismatch); } let next_unfinished_view_change = self.verify_with_state(topology, latest_block); - if new_proof.payload.view_change_index != (next_unfinished_view_change as u64) { + if new_proof.payload.view_change_index as usize != next_unfinished_view_change { return Err(Error::ViewChangeNotFound); // We only care about the current view change that may or may not happen. } @@ -216,17 +229,28 @@ mod candidate { fn validate(self) -> Result { self.validate_signatures()?; - // TODO: If it is possible, we should instead reject decoding proofs that - // have duplicated signatures. This would require change in the code as well - let unique_signatures = self.signatures.into_iter().collect::>(); - Ok(SignedViewChangeProof { + signatures: self.signatures, payload: self.payload, - signatures: unique_signatures.into_iter().collect(), }) } fn validate_signatures(&self) -> Result<(), &'static str> { + if self.signatures.is_empty() { + return Err("Proof missing signatures"); + } + + self.signatures + .iter() + .map(|signature| &signature.0) + .try_fold(IndexSet::new(), |mut acc, elem| { + if !acc.insert(elem) { + return Err("Duplicate signature"); + } + + Ok(acc) + })?; + self.signatures .iter() .try_for_each(|(public_key, payload)| { diff --git a/core/src/tx.rs b/core/src/tx.rs index efbd0170f82..624a77f9918 100644 --- a/core/src/tx.rs +++ b/core/src/tx.rs @@ -14,9 +14,7 @@ pub use iroha_data_model::prelude::*; use iroha_data_model::{ isi::error::Mismatch, query::error::FindError, - transaction::{ - error::TransactionLimitError, TransactionLimits, TransactionPayload, TransactionSignature, - }, + transaction::{error::TransactionLimitError, TransactionLimits, TransactionPayload}, }; use iroha_genesis::GenesisTransaction; use iroha_logger::{debug, error}; @@ -71,7 +69,7 @@ impl AcceptedTransaction { pub fn accept_genesis( tx: GenesisTransaction, expected_chain_id: &ChainId, - genesis_public_key: &PublicKey, + genesis_account: &AccountId, ) -> Result { let actual_chain_id = tx.0.chain_id(); @@ -82,13 +80,8 @@ impl AcceptedTransaction { })); } - let TransactionSignature(public_key, signature) = tx.0.signature(); - if public_key != genesis_public_key { - return Err(SignatureVerificationFail { - signature: signature.clone(), - reason: "Signature doesn't correspond to genesis public key".to_string(), - } - .into()); + if genesis_account != tx.0.authority() { + return Err(AcceptTransactionFail::UnexpectedGenesisAccountSignature); } Ok(Self(tx.0)) @@ -102,7 +95,7 @@ impl AcceptedTransaction { pub fn accept( tx: SignedTransaction, expected_chain_id: &ChainId, - limits: &TransactionLimits, + limits: TransactionLimits, ) -> Result { let actual_chain_id = tx.chain_id(); @@ -113,10 +106,6 @@ impl AcceptedTransaction { })); } - if *iroha_genesis::GENESIS_DOMAIN_ID == *tx.authority().domain_id() { - return Err(AcceptTransactionFail::UnexpectedGenesisAccountSignature); - } - match &tx.instructions() { Executable::Instructions(instructions) => { let instruction_count = instructions.len(); diff --git a/core/test_network/src/lib.rs b/core/test_network/src/lib.rs index b63ddd92a50..16e3a850e22 100644 --- a/core/test_network/src/lib.rs +++ b/core/test_network/src/lib.rs @@ -1,7 +1,5 @@ //! Module for starting peers and networks. Used only for tests use core::{fmt::Debug, str::FromStr as _, time::Duration}; -#[cfg(debug_assertions)] -use std::sync::atomic::AtomicBool; use std::{collections::BTreeMap, ops::Deref, path::Path, sync::Arc, thread}; use eyre::Result; @@ -138,7 +136,7 @@ impl TestGenesis for GenesisNetwork { impl Network { /// Collect the freeze handles from all the peers in the network. #[cfg(debug_assertions)] - pub fn get_freeze_status_handles(&self) -> Vec { + pub fn get_freeze_status_handles(&self) -> Vec { self.peers() .filter_map(|peer| peer.irohad.as_ref()) .map(|iroha| iroha.freeze_status()) diff --git a/crypto/src/lib.rs b/crypto/src/lib.rs index aa7270b6e00..dfe5fa32471 100755 --- a/crypto/src/lib.rs +++ b/crypto/src/lib.rs @@ -839,12 +839,6 @@ pub mod error { /// Returned when an error occurs during key generation #[display(fmt = "Key generation failed. {_0}")] KeyGen(String), - /// Returned when an error occurs during digest generation - #[display(fmt = "Digest generation failed. {_0}")] - DigestGen(String), - /// Returned when an error occurs during creation of [`SignaturesOf`] - #[display(fmt = "`SignaturesOf` must contain at least one signature")] - EmptySignatureIter, /// A General purpose error message that doesn't fit in any category #[display(fmt = "General error. {_0}")] // This is going to cause a headache Other(String), diff --git a/data_model/src/block.rs b/data_model/src/block.rs index 6c24e10ba5e..ba0c09c8ab7 100644 --- a/data_model/src/block.rs +++ b/data_model/src/block.rs @@ -54,12 +54,11 @@ mod model { #[getset(get_copy = "pub")] pub height: u64, /// Hash of the previous block in the chain. - #[getset(get = "pub")] + #[getset(get_copy = "pub")] pub previous_block_hash: Option>, /// Hash of merkle tree root of transactions' hashes. - #[getset(get = "pub")] - // TODO: How can it be `None`??? - pub transactions_hash: Option>>, + #[getset(get_copy = "pub")] + pub transactions_hash: HashOf>, /// Creation timestamp (unix time in milliseconds). #[getset(skip)] pub timestamp_ms: u64, @@ -231,6 +230,12 @@ impl SignedBlock { signature: BlockSignature, public_key: &iroha_crypto::PublicKey, ) -> Result<(), iroha_crypto::Error> { + if self.signatures().any(|s| signature.0 == s.0) { + return Err(iroha_crypto::Error::Signing( + "Duplicate signature".to_owned(), + )); + } + signature.1.verify(public_key, self.payload())?; let SignedBlock::V1(block) = self; @@ -292,10 +297,6 @@ mod candidate { self.validate_signatures()?; self.validate_header()?; - if self.payload.transactions.is_empty() { - return Err("Block is empty"); - } - Ok(SignedBlockV1 { signatures: self.signatures, payload: self.payload, @@ -303,8 +304,13 @@ mod candidate { } fn validate_signatures(&self) -> Result<(), &'static str> { + if self.signatures.is_empty() { + return Err("Block missing signatures"); + } + self.signatures .iter() + .map(|signature| signature.0) .try_fold(BTreeSet::new(), |mut acc, elem| { if !acc.insert(elem) { return Err("Duplicate signature in block"); @@ -315,6 +321,7 @@ mod candidate { Ok(()) } + fn validate_header(&self) -> Result<(), &'static str> { let actual_txs_hash = self.payload.header.transactions_hash; @@ -324,7 +331,8 @@ mod candidate { .iter() .map(|value| value.as_ref().hash()) .collect::>() - .hash(); + .hash() + .ok_or("Block is empty")?; if expected_txs_hash != actual_txs_hash { return Err("Transactions' hash incorrect. Expected: {expected_txs_hash:?}, actual: {actual_txs_hash:?}"); diff --git a/data_model/src/events/pipeline.rs b/data_model/src/events/pipeline.rs index 2b7bba7f745..f4708ed9c93 100644 --- a/data_model/src/events/pipeline.rs +++ b/data_model/src/events/pipeline.rs @@ -354,7 +354,9 @@ mod tests { Self { height, previous_block_hash: None, - transactions_hash: None, + transactions_hash: HashOf::from_untyped_unchecked(Hash::prehashed( + [1_u8; Hash::LENGTH], + )), timestamp_ms: 0, view_change_index: 0, consensus_estimation_ms: 0, diff --git a/data_model/src/query/mod.rs b/data_model/src/query/mod.rs index 4bcc2381c76..ea32111d21a 100644 --- a/data_model/src/query/mod.rs +++ b/data_model/src/query/mod.rs @@ -1268,7 +1268,7 @@ pub mod http { Serialize, IntoSchema, )] - pub struct QuerySignature(pub PublicKey, pub SignatureOf); + pub struct QuerySignature(pub SignatureOf); /// I/O ready structure to send queries. #[derive(Debug, Clone, Encode, Serialize, IntoSchema)] @@ -1311,11 +1311,11 @@ pub mod http { impl SignedQueryCandidate { fn validate(self) -> Result { - let QuerySignature(public_key, signature) = &self.signature; + let QuerySignature(signature) = &self.signature; - if signature.verify(public_key, &self.payload).is_err() { - return Err("Query signature not valid"); - } + signature + .verify(&self.payload.authority.signatory, &self.payload) + .map_err(|_| "Query signature is not valid")?; Ok(SignedQueryV1 { payload: self.payload, @@ -1438,7 +1438,7 @@ pub mod http { let signature = SignatureOf::new(key_pair.private_key(), &self.payload); SignedQueryV1 { - signature: QuerySignature(key_pair.public_key().clone(), signature), + signature: QuerySignature(signature), payload: self.payload, } .into() @@ -1488,12 +1488,6 @@ pub mod error { )] #[cfg_attr(feature = "std", derive(thiserror::Error))] pub enum QueryExecutionFail { - /// Query has the wrong signature: {0} - Signature( - #[skip_from] - #[skip_try_from] - String, - ), /// {0} #[cfg_attr(feature = "std", error(transparent))] Find(FindError), diff --git a/data_model/src/transaction.rs b/data_model/src/transaction.rs index 178e06fe0ea..a86e069ced1 100644 --- a/data_model/src/transaction.rs +++ b/data_model/src/transaction.rs @@ -158,10 +158,7 @@ mod model { Serialize, IntoSchema, )] - pub struct TransactionSignature( - pub iroha_crypto::PublicKey, - pub SignatureOf, - ); + pub struct TransactionSignature(pub SignatureOf); /// Transaction that contains a signature /// @@ -263,6 +260,13 @@ declare_versioned!(SignedTransaction 1..2, Debug, Display, Clone, PartialEq, Eq, declare_versioned!(SignedTransaction 1..2, Debug, Display, Clone, PartialEq, Eq, PartialOrd, Ord, FromVariant, IntoSchema); impl SignedTransaction { + /// Transaction payload. Used for tests + #[cfg(feature = "transparent_api")] + pub fn payload(&self) -> &TransactionPayload { + let SignedTransaction::V1(tx) = self; + &tx.payload + } + /// Return transaction instructions #[inline] pub fn instructions(&self) -> &Executable { @@ -383,10 +387,11 @@ mod candidate { } fn validate_signature(&self) -> Result<(), &'static str> { - let TransactionSignature(public_key, signature) = &self.signature; + let TransactionSignature(signature) = &self.signature; + signature - .verify(public_key, &self.payload) - .map_err(|_| "Transaction contains invalid signatures")?; + .verify(&self.payload.authority.signatory, &self.payload) + .map_err(|_| "Transaction signature is invalid")?; Ok(()) } @@ -756,11 +761,8 @@ mod http { /// Sign transaction with provided key pair. #[must_use] - pub fn sign(self, key_pair: &iroha_crypto::KeyPair) -> SignedTransaction { - let signature = TransactionSignature( - key_pair.public_key().clone(), - SignatureOf::new(key_pair.private_key(), &self.payload), - ); + pub fn sign(self, private_key: &iroha_crypto::PrivateKey) -> SignedTransaction { + let signature = TransactionSignature(SignatureOf::new(private_key, &self.payload)); SignedTransactionV1 { signature, diff --git a/docs/source/references/schema.json b/docs/source/references/schema.json index 490b88ef5e2..44ced7ddc36 100644 --- a/docs/source/references/schema.json +++ b/docs/source/references/schema.json @@ -612,7 +612,7 @@ }, { "name": "transactions_hash", - "type": "Option>>" + "type": "HashOf>" }, { "name": "timestamp_ms", @@ -2469,9 +2469,6 @@ "Option": { "Option": "Duration" }, - "Option>>": { - "Option": "HashOf>" - }, "Option>": { "Option": "HashOf" }, @@ -2904,32 +2901,27 @@ }, "QueryExecutionFail": { "Enum": [ - { - "tag": "Signature", - "discriminant": 0, - "type": "String" - }, { "tag": "Find", - "discriminant": 1, + "discriminant": 0, "type": "FindError" }, { "tag": "Conversion", - "discriminant": 2, + "discriminant": 1, "type": "String" }, { "tag": "UnknownCursor", - "discriminant": 3 + "discriminant": 2 }, { "tag": "FetchSizeTooBig", - "discriminant": 4 + "discriminant": 3 }, { "tag": "InvalidSingularParameters", - "discriminant": 5 + "discriminant": 4 } ] }, @@ -3020,12 +3012,7 @@ } ] }, - "QuerySignature": { - "Tuple": [ - "PublicKey", - "SignatureOf" - ] - }, + "QuerySignature": "SignatureOf", "Register": { "Struct": [ { @@ -3903,12 +3890,7 @@ } ] }, - "TransactionSignature": { - "Tuple": [ - "PublicKey", - "SignatureOf" - ] - }, + "TransactionSignature": "SignatureOf", "TransactionStatus": { "Enum": [ { diff --git a/genesis/src/lib.rs b/genesis/src/lib.rs index 8784f7dc074..79c5d046c6d 100644 --- a/genesis/src/lib.rs +++ b/genesis/src/lib.rs @@ -121,7 +121,7 @@ fn build_and_sign_genesis( ); let transaction = TransactionBuilder::new(chain_id, genesis_account_id) .with_instructions(instructions) - .sign(genesis_key_pair); + .sign(genesis_key_pair.private_key()); GenesisTransaction(transaction) } diff --git a/torii/src/lib.rs b/torii/src/lib.rs index a987608ee3f..4db82d4c7f1 100644 --- a/torii/src/lib.rs +++ b/torii/src/lib.rs @@ -338,7 +338,6 @@ impl Error { Config(_) | StatusSegmentNotFound(_) => StatusCode::NOT_FOUND, PushIntoQueue(err) => match **err { queue::Error::Full => StatusCode::INTERNAL_SERVER_ERROR, - queue::Error::SignatoryInconsistent => StatusCode::UNAUTHORIZED, _ => StatusCode::BAD_REQUEST, }, #[cfg(feature = "telemetry")] @@ -363,7 +362,6 @@ impl Error { Conversion(_) | UnknownCursor | FetchSizeTooBig | InvalidSingularParameters => { StatusCode::BAD_REQUEST } - Signature(_) => StatusCode::UNAUTHORIZED, Find(_) => StatusCode::NOT_FOUND, }, TooComplex => StatusCode::UNPROCESSABLE_ENTITY, diff --git a/torii/src/routing.rs b/torii/src/routing.rs index b75eacf6b05..1d52394630e 100644 --- a/torii/src/routing.rs +++ b/torii/src/routing.rs @@ -53,7 +53,7 @@ pub async fn handle_transaction( ) -> Result { let state_view = state.view(); let transaction_limits = state_view.config.transaction_limits; - let transaction = AcceptedTransaction::accept(transaction, &chain_id, &transaction_limits) + let transaction = AcceptedTransaction::accept(transaction, &chain_id, transaction_limits) .map_err(Error::AcceptTransaction)?; queue .push(transaction, &state_view)