From 1f766a37c1d12cc5a606a5518d9c05c81715fb88 Mon Sep 17 00:00:00 2001 From: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> Date: Thu, 7 Nov 2024 18:48:18 +0900 Subject: [PATCH 01/11] feat(multisig)!: override TTL per transaction BREAKING CHANGES: - (api-changes) optional `MultisigPropose.transaction_ttl_ms` to override the account default Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> --- crates/iroha/tests/multisig.rs | 4 +- crates/iroha_cli/src/main.rs | 16 +++- .../src/default/isi/multisig/mod.rs | 6 ++ .../src/default/isi/multisig/transaction.rs | 85 ++++++++++++++----- crates/iroha_executor_data_model/src/isi.rs | 2 + docs/source/references/schema.json | 4 + 6 files changed, 93 insertions(+), 24 deletions(-) diff --git a/crates/iroha/tests/multisig.rs b/crates/iroha/tests/multisig.rs index 1ac73daca89..3304046c427 100644 --- a/crates/iroha/tests/multisig.rs +++ b/crates/iroha/tests/multisig.rs @@ -190,7 +190,7 @@ fn multisig_base(suite: TestSuite) -> Result<()> { let proposer = signatories.pop_last().unwrap(); let mut approvers = signatories.into_iter(); - let propose = MultisigPropose::new(multisig_account_id.clone(), instructions); + let propose = MultisigPropose::new(multisig_account_id.clone(), instructions, None); alt_client(proposer, &test_client).submit_blocking(propose)?; // Allow time to elapse to test the expiration @@ -345,7 +345,7 @@ fn multisig_recursion() -> Result<()> { let instructions_hash = HashOf::new(&instructions); let proposer = sigs_0.pop_last().unwrap(); - let propose = MultisigPropose::new(msa_012345.clone(), instructions); + let propose = MultisigPropose::new(msa_012345.clone(), instructions, None); alt_client(proposer, &test_client).submit_blocking(propose)?; diff --git a/crates/iroha_cli/src/main.rs b/crates/iroha_cli/src/main.rs index c0fb1e9761e..9e3faa5287b 100644 --- a/crates/iroha_cli/src/main.rs +++ b/crates/iroha_cli/src/main.rs @@ -1257,6 +1257,9 @@ mod multisig { /// Multisig authority of the multisig transaction #[arg(short, long)] pub account: AccountId, + /// Time-to-live of multisig transactions that overrides to shorten the account default + #[arg(short, long)] + pub transaction_ttl: Option, } impl RunArgs for Propose { @@ -1268,9 +1271,20 @@ mod multisig { let string_content = String::from_utf8(raw_content)?; json5::from_str(&string_content)? }; + let transaction_ttl_ms = self.transaction_ttl.map(|duration| { + duration + .as_millis() + .try_into() + .ok() + .and_then(NonZeroU64::new) + .expect("ttl should be between 1 ms and 584942417 years") + }); + let instructions_hash = HashOf::new(&instructions); println!("{instructions_hash}"); - let propose_multisig_transaction = MultisigPropose::new(self.account, instructions); + + let propose_multisig_transaction = + MultisigPropose::new(self.account, instructions, transaction_ttl_ms); submit([propose_multisig_transaction], Metadata::default(), context) .wrap_err("Failed to propose transaction") diff --git a/crates/iroha_executor/src/default/isi/multisig/mod.rs b/crates/iroha_executor/src/default/isi/multisig/mod.rs index 6ed09717677..957c4fa1ac8 100644 --- a/crates/iroha_executor/src/default/isi/multisig/mod.rs +++ b/crates/iroha_executor/src/default/isi/multisig/mod.rs @@ -35,6 +35,12 @@ fn proposed_at_ms_key(hash: &HashOf>) -> Name { .unwrap() } +fn expires_at_ms_key(hash: &HashOf>) -> Name { + format!("{PROPOSALS}{DELIMITER}{hash}{DELIMITER}expires_at_ms") + .parse() + .unwrap() +} + fn approvals_key(hash: &HashOf>) -> Name { format!("{PROPOSALS}{DELIMITER}{hash}{DELIMITER}approvals") .parse() diff --git a/crates/iroha_executor/src/default/isi/multisig/transaction.rs b/crates/iroha_executor/src/default/isi/multisig/transaction.rs index 542fcc1636b..a700a93d106 100644 --- a/crates/iroha_executor/src/default/isi/multisig/transaction.rs +++ b/crates/iroha_executor/src/default/isi/multisig/transaction.rs @@ -27,6 +27,27 @@ impl VisitExecute for MultisigPropose { .filter_with(|role_id| role_id.eq(multisig_role)) .execute_single() .is_ok(); + let has_not_longer_ttl = { + let Some(account_default_ttl_ms) = host + .query_single(FindAccountMetadata::new( + multisig_account.clone(), + TRANSACTION_TTL_MS.parse().unwrap(), + )) + .ok() + .and_then(|json| json.try_into_any::().ok()) + else { + deny!(executor, "multisig account not found"); + }; + self.transaction_ttl_ms + .map(u64::from) + .map_or(true, |override_ttl_ms| { + override_ttl_ms <= account_default_ttl_ms + }) + }; + + if !(is_downward_proposal || has_not_longer_ttl) { + deny!(executor, "ttl violates the restriction"); + }; if !(is_downward_proposal || has_multisig_role) { deny!(executor, "not qualified to propose multisig"); @@ -51,6 +72,28 @@ impl VisitExecute for MultisigPropose { executor.context_mut().authority = multisig_account.clone(); let instructions_hash = HashOf::new(&self.instructions); + let now_ms: u64 = executor + .context() + .curr_block + .creation_time() + .as_millis() + .try_into() + .dbg_expect("shouldn't overflow within 584942417 years"); + let expires_at_ms: u64 = { + let ttl_ms = self.transaction_ttl_ms.map(u64::from).unwrap_or_else(|| { + executor + .host() + .query_single(FindAccountMetadata::new( + multisig_account.clone(), + TRANSACTION_TTL_MS.parse().unwrap(), + )) + .dbg_unwrap() + .try_into_any() + .dbg_unwrap() + }); + now_ms.saturating_add(ttl_ms) + }; + let approvals = BTreeSet::from([proposer]); let signatories: BTreeMap = executor .host() .query_single(FindAccountMetadata::new( @@ -60,14 +103,6 @@ impl VisitExecute for MultisigPropose { .dbg_unwrap() .try_into_any() .dbg_unwrap(); - let now_ms: u64 = executor - .context() - .curr_block - .creation_time() - .as_millis() - .try_into() - .dbg_expect("shouldn't overflow within 584942417 years"); - let approvals = BTreeSet::from([proposer]); // Recursively deploy multisig authentication down to the personal leaf signatories for signatory in signatories.keys().cloned() { @@ -84,9 +119,12 @@ impl VisitExecute for MultisigPropose { let approve_me = MultisigApprove::new(multisig_account.clone(), instructions_hash); - MultisigPropose::new(signatory, [approve_me.into()].to_vec()) + MultisigPropose::new( + signatory, + [approve_me.into()].to_vec(), + self.transaction_ttl_ms, + ) }; - propose_to_approve_me.visit_execute(executor); } } @@ -103,6 +141,12 @@ impl VisitExecute for MultisigPropose { Json::new(now_ms), ))); + visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( + multisig_account.clone(), + expires_at_ms_key(&instructions_hash).clone(), + Json::new(expires_at_ms), + ))); + visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( multisig_account, approvals_key(&instructions_hash).clone(), @@ -155,14 +199,6 @@ impl VisitExecute for MultisigApprove { .dbg_unwrap() .try_into_any() .dbg_unwrap(); - let transaction_ttl_ms: u64 = host - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - TRANSACTION_TTL_MS.parse().unwrap(), - )) - .dbg_unwrap() - .try_into_any() - .dbg_unwrap(); let instructions: Vec = host .query_single(FindAccountMetadata::new( multisig_account.clone(), @@ -170,10 +206,10 @@ impl VisitExecute for MultisigApprove { ))? .try_into_any() .dbg_unwrap(); - let proposed_at_ms: u64 = host + let expires_at_ms: u64 = host .query_single(FindAccountMetadata::new( multisig_account.clone(), - proposed_at_ms_key(&instructions_hash), + expires_at_ms_key(&instructions_hash), )) .dbg_unwrap() .try_into_any() @@ -209,7 +245,7 @@ impl VisitExecute for MultisigApprove { .map(|(_, weight)| u16::from(weight)) .sum(); - let is_expired = proposed_at_ms.saturating_add(transaction_ttl_ms) < now_ms; + let is_expired = expires_at_ms < now_ms; if is_authenticated || is_expired { // Cleanup the transaction entry @@ -227,6 +263,13 @@ impl VisitExecute for MultisigApprove { )) ); + visit_seq!( + executor.visit_remove_account_key_value(&RemoveKeyValue::account( + multisig_account.clone(), + expires_at_ms_key(&instructions_hash), + )) + ); + visit_seq!( executor.visit_remove_account_key_value(&RemoveKeyValue::account( multisig_account.clone(), diff --git a/crates/iroha_executor_data_model/src/isi.rs b/crates/iroha_executor_data_model/src/isi.rs index 255df1df20e..8ccd3b22713 100644 --- a/crates/iroha_executor_data_model/src/isi.rs +++ b/crates/iroha_executor_data_model/src/isi.rs @@ -100,6 +100,8 @@ pub mod multisig { pub account: AccountId, /// Proposal contents pub instructions: Vec, + /// Optional TTL to override the account default. Cannot be longer than the account default + pub transaction_ttl_ms: Option, } /// Approve a certain multisig transaction diff --git a/docs/source/references/schema.json b/docs/source/references/schema.json index dc3c8ea26b3..0eab35911a2 100644 --- a/docs/source/references/schema.json +++ b/docs/source/references/schema.json @@ -2639,6 +2639,10 @@ { "name": "instructions", "type": "Vec" + }, + { + "name": "transaction_ttl_ms", + "type": "Option>" } ] }, From 8d579d51e11da5fa4bc589fd48688fd208cea242 Mon Sep 17 00:00:00 2001 From: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> Date: Sat, 9 Nov 2024 02:35:07 +0900 Subject: [PATCH 02/11] test(multisig): recursion test suites Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> --- crates/iroha/tests/multisig.rs | 313 ++++++++++++++++++++------------- 1 file changed, 188 insertions(+), 125 deletions(-) diff --git a/crates/iroha/tests/multisig.rs b/crates/iroha/tests/multisig.rs index 3304046c427..92f5a41405d 100644 --- a/crates/iroha/tests/multisig.rs +++ b/crates/iroha/tests/multisig.rs @@ -18,6 +18,36 @@ use iroha_test_samples::{ gen_account_in, ALICE_ID, BOB_ID, BOB_KEYPAIR, CARPENTER_ID, CARPENTER_KEYPAIR, }; +#[test] +fn multisig_normal() -> Result<()> { + multisig_base(TestSuite::normal()) +} + +#[test] +fn multisig_unauthorized() -> Result<()> { + multisig_base(TestSuite::unauthorized()) +} + +#[test] +fn multisig_expires() -> Result<()> { + multisig_base(TestSuite::expires()) +} + +#[test] +fn multisig_recursion_normal() -> Result<()> { + multisig_recursion_base(TestSuite::normal()) +} + +#[test] +fn multisig_recursion_unauthorized() -> Result<()> { + multisig_recursion_base(TestSuite::unauthorized()) +} + +#[test] +fn multisig_recursion_expires() -> Result<()> { + multisig_recursion_base(TestSuite::expires()) +} + #[derive(Constructor)] struct TestSuite { domain: DomainId, @@ -26,47 +56,43 @@ struct TestSuite { transaction_ttl_ms_opt: Option, } -#[test] -fn multisig_normal() -> Result<()> { - // New domain for this test - let domain = "kingdom".parse().unwrap(); - // Create a multisig account ID and discard the corresponding private key - // FIXME #5022 refuse user input to prevent multisig monopoly and pre-registration hijacking - let multisig_account_id = gen_account_in(&domain).0; - // Make some changes to the multisig account itself - let unauthorized_target_opt = None; - // Semi-permanently valid - let transaction_ttl_ms_opt = None; - - let suite = TestSuite::new( - domain, - multisig_account_id, - unauthorized_target_opt, - transaction_ttl_ms_opt, - ); - multisig_base(suite) -} +impl TestSuite { + fn normal() -> Self { + // New domain for this test + let domain = "kingdom".parse().unwrap(); + // Create a multisig account ID and discard the corresponding private key + // FIXME #5022 refuse user input to prevent multisig monopoly and pre-registration hijacking + let multisig_account_id = gen_account_in(&domain).0; + // Make some changes to the multisig account itself + let unauthorized_target_opt = None; + // Semi-permanently valid + let transaction_ttl_ms_opt = None; + + Self::new( + domain, + multisig_account_id, + unauthorized_target_opt, + transaction_ttl_ms_opt, + ) + } -#[test] -fn multisig_unauthorized() -> Result<()> { - let domain = "kingdom".parse().unwrap(); - let multisig_account_id = gen_account_in(&domain).0; - // Someone that the multisig account has no permission to access - let unauthorized_target_opt = Some(ALICE_ID.clone()); + fn unauthorized() -> Self { + let domain = "kingdom".parse().unwrap(); + let multisig_account_id = gen_account_in(&domain).0; + // Someone that the multisig account has no permission to access + let unauthorized_target_opt = Some(ALICE_ID.clone()); - let suite = TestSuite::new(domain, multisig_account_id, unauthorized_target_opt, None); - multisig_base(suite) -} + Self::new(domain, multisig_account_id, unauthorized_target_opt, None) + } -#[test] -fn multisig_expires() -> Result<()> { - let domain = "kingdom".parse().unwrap(); - let multisig_account_id = gen_account_in(&domain).0; - // Expires after 1 sec - let transaction_ttl_ms_opt = Some(1_000); + fn expires() -> Self { + let domain = "kingdom".parse().unwrap(); + let multisig_account_id = gen_account_in(&domain).0; + // Expires after 1 sec + let transaction_ttl_ms_opt = Some(1_000); - let suite = TestSuite::new(domain, multisig_account_id, None, transaction_ttl_ms_opt); - multisig_base(suite) + Self::new(domain, multisig_account_id, None, transaction_ttl_ms_opt) + } } /// # Scenario @@ -265,9 +291,15 @@ fn multisig_base(suite: TestSuite) -> Result<()> { /// / / \ / | \ /// 0 1 2 3 4 5 <--- personal signatories /// ``` -#[test] #[expect(clippy::similar_names, clippy::too_many_lines)] -fn multisig_recursion() -> Result<()> { +fn multisig_recursion_base(suite: TestSuite) -> Result<()> { + let TestSuite { + domain: _, + multisig_account_id: _, + unauthorized_target_opt, + transaction_ttl_ms_opt, + } = suite; + let (network, _rt) = NetworkBuilder::new().start_blocking()?; let test_client = network.client(); @@ -289,80 +321,76 @@ fn multisig_recursion() -> Result<()> { let mut sigs = signatories.clone(); let sigs_345 = sigs.split_off(signatories.keys().nth(3).unwrap()); let sigs_12 = sigs.split_off(signatories.keys().nth(1).unwrap()); - let mut sigs_0 = sigs; - - let register_ms_accounts = |sigs_list: Vec>| { - sigs_list - .into_iter() - .map(|sigs| { - let ms_account_id = gen_account_in(wonderland).0; - let register_ms_account = MultisigRegister::new( - ms_account_id.clone(), - sigs.iter().copied().map(|id| (id.clone(), 1)).collect(), - sigs.len() - .try_into() - .ok() - .and_then(NonZeroU16::new) - .unwrap(), - NonZeroU64::new(u64::MAX).unwrap(), - ); - - test_client - .submit_blocking(register_ms_account) - .expect("multisig account should be registered by account of the same domain"); - - ms_account_id - }) - .collect::>() + let sig_0 = sigs.pop_last().unwrap(); + + let register_ms_account = |sigs: Vec<&AccountId>| { + let ms_account_id = gen_account_in(wonderland).0; + let register = MultisigRegister::new( + ms_account_id.clone(), + // Equal votes + sigs.iter().copied().map(|id| (id.clone(), 1)).collect(), + // Unanimous + sigs.len() + .try_into() + .ok() + .and_then(NonZeroU16::new) + .unwrap(), + transaction_ttl_ms_opt + .and_then(NonZeroU64::new) + .unwrap_or(NonZeroU64::MAX), + ); + + test_client + .submit_blocking(register) + .expect("the domain owner should succeed to register a multisig account"); + + ms_account_id }; - let sigs_list: Vec> = [&sigs_12, &sigs_345] - .into_iter() - .map(|sigs| sigs.keys().collect()) - .collect(); - let msas = register_ms_accounts(sigs_list); - let msa_12 = msas[0].clone(); - let msa_345 = msas[1].clone(); - - let sigs_list = vec![vec![&msa_12, &msa_345]]; - let msas = register_ms_accounts(sigs_list); - let msa_12345 = msas[0].clone(); - - let sig_0 = sigs_0.keys().next().unwrap().clone(); - let sigs_list = vec![vec![&sig_0, &msa_12345]]; - let msas = register_ms_accounts(sigs_list); + let msa_12 = register_ms_account(sigs_12.keys().collect()); + let msa_345 = register_ms_account(sigs_345.keys().collect()); + let msa_12345 = register_ms_account(vec![&msa_12, &msa_345]); // The root multisig account with 6 personal signatories under its umbrella - let msa_012345 = msas[0].clone(); + let msa_012345 = register_ms_account(vec![&sig_0.0, &msa_12345]); // One of personal signatories proposes a multisig transaction let key: Name = "success_marker".parse().unwrap(); + let transaction_target = unauthorized_target_opt + .as_ref() + .unwrap_or(&msa_012345) + .clone(); let instructions = vec![SetKeyValue::account( - msa_012345.clone(), + transaction_target.clone(), key.clone(), "congratulations".parse::().unwrap(), ) .into()]; let instructions_hash = HashOf::new(&instructions); - let proposer = sigs_0.pop_last().unwrap(); + let proposer = sig_0; let propose = MultisigPropose::new(msa_012345.clone(), instructions, None); alt_client(proposer, &test_client).submit_blocking(propose)?; + // Allow time to elapse to test the expiration + if let Some(ms) = transaction_ttl_ms_opt { + std::thread::sleep(Duration::from_millis(ms)) + }; + test_client.submit_blocking(Log::new(Level::DEBUG, "Just ticking time".to_string()))?; + // Check that the entire authentication policy has been deployed down to one of the leaf signatories - let approval_hash_to_12345 = { - let approval_hash_to_012345 = { - let approve: InstructionBox = - MultisigApprove::new(msa_012345.clone(), instructions_hash).into(); + let approval_hash_to_012345 = { + let approve: InstructionBox = + MultisigApprove::new(msa_012345.clone(), instructions_hash).into(); - HashOf::new(&vec![approve]) - }; + HashOf::new(&vec![approve]) + }; + let approval_hash_to_12345 = { let approve: InstructionBox = MultisigApprove::new(msa_12345.clone(), approval_hash_to_012345).into(); HashOf::new(&vec![approve]) }; - let approvals_at_12: BTreeSet = test_client .query_single(FindAccountMetadata::new( msa_12.clone(), @@ -376,57 +404,92 @@ fn multisig_recursion() -> Result<()> { assert!(1 == approvals_at_12.len() && approvals_at_12.contains(&msa_12345)); + // All the rest signatories try to approve the multisig transaction + let mut approvals_iter = sigs_12 + .into_iter() + .map(|sig| (sig, msa_12.clone())) + .chain(sigs_345.into_iter().map(|sig| (sig, msa_345.clone()))) + .map(|(sig, msa)| (sig, MultisigApprove::new(msa, approval_hash_to_12345))); + + // Approve once to see if the proposal expires + let (approver, approve) = approvals_iter.next().unwrap(); + alt_client(approver, &test_client).submit_blocking(approve)?; + + // Subsequent approvals should succeed unless the proposal is expired + for _ in 2..=4 { + let (approver, approve) = approvals_iter.next().unwrap(); + let res = alt_client(approver, &test_client).submit_blocking(approve.clone()); + match &transaction_ttl_ms_opt { + None => assert!(res.is_ok()), + _ => assert!(res.is_err()), + } + } + // Check that the multisig transaction has not yet executed let _err = test_client - .query_single(FindAccountMetadata::new(msa_012345.clone(), key.clone())) + .query_single(FindAccountMetadata::new( + transaction_target.clone(), + key.clone(), + )) .expect_err("instructions shouldn't execute without enough approvals"); - // All the rest signatories approve the multisig transaction - let approve_for_each = |approvers: BTreeMap, - instructions_hash: HashOf>, - ms_account: &AccountId| { - for approver in approvers { - let approve = MultisigApprove::new(ms_account.clone(), instructions_hash); - - alt_client(approver, &test_client) - .submit_blocking(approve) - .expect("should successfully approve the proposal"); - } - }; + // The last approve to proceed to validate and execute the instructions + let (approver, approve) = approvals_iter.next().unwrap(); + let res = alt_client(approver, &test_client).submit_blocking(approve.clone()); + match (&transaction_ttl_ms_opt, &unauthorized_target_opt) { + (None, None) => assert!(res.is_ok()), + _ => assert!(res.is_err()), + } - approve_for_each(sigs_12, approval_hash_to_12345, &msa_12); - approve_for_each(sigs_345, approval_hash_to_12345, &msa_345); + // Check if the multisig transaction has executed + let res = test_client.query_single(FindAccountMetadata::new(transaction_target, key.clone())); + match (&transaction_ttl_ms_opt, &unauthorized_target_opt) { + (None, None) => assert!(res.is_ok()), + _ => assert!(res.is_err()), + } - // Check that the multisig transaction has executed - test_client - .query_single(FindAccountMetadata::new(msa_012345.clone(), key.clone())) - .expect("instructions should execute with enough approvals"); + // Check if the transaction entries on the last approval path are deleted + for (msa, mst_hash) in [ + (msa_345, approval_hash_to_12345), + (msa_12345, approval_hash_to_012345), + (msa_012345, instructions_hash), + ] { + let res = test_client.query_single(FindAccountMetadata::new( + msa, + format!("proposals/{mst_hash}/instructions") + .parse() + .unwrap(), + )); + match (&transaction_ttl_ms_opt, &unauthorized_target_opt) { + // In case the root proposal is failing validation, the entries on the last approval path can exit only by expiring + (None, Some(_)) => assert!(res.is_ok()), + _ => assert!(res.is_err()), + } + } Ok(()) } #[test] -fn reserved_names() { +fn reserved_roles() { let (network, _rt) = NetworkBuilder::new().start_blocking().unwrap(); let test_client = network.client(); let account_in_another_domain = gen_account_in("garden_of_live_flowers").0; + let register = { + let role = format!( + "MULTISIG_SIGNATORY/{}/{}", + account_in_another_domain.domain(), + account_in_another_domain.signatory() + ) + .parse() + .unwrap(); + Register::role(Role::new(role, ALICE_ID.clone())) + }; - { - let register = { - let role = format!( - "MULTISIG_SIGNATORY/{}/{}", - account_in_another_domain.domain(), - account_in_another_domain.signatory() - ) - .parse() - .unwrap(); - Register::role(Role::new(role, ALICE_ID.clone())) - }; - let _err = test_client.submit_blocking(register).expect_err( - "role with this name shouldn't be registered by anyone other than the domain owner", - ); - } + let _err = test_client.submit_blocking(register).expect_err( + "role with this name shouldn't be registered by anyone other than the domain owner", + ); } fn alt_client(signatory: (AccountId, KeyPair), base_client: &Client) -> Client { @@ -445,5 +508,5 @@ fn debug_account(account_id: &AccountId, client: &Client) { .execute_single() .unwrap(); - iroha_logger::error!(?account); + eprintln!("{account:#?}"); } From d45e7079917dfc67eb3050f20a3bf1e8c0abc73d Mon Sep 17 00:00:00 2001 From: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> Date: Sat, 9 Nov 2024 04:18:14 +0900 Subject: [PATCH 03/11] fix(multisig): prune expired path on approve Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> --- .../src/default/isi/multisig/transaction.rs | 177 +++++++++++++----- 1 file changed, 126 insertions(+), 51 deletions(-) diff --git a/crates/iroha_executor/src/default/isi/multisig/transaction.rs b/crates/iroha_executor/src/default/isi/multisig/transaction.rs index a700a93d106..6f53c16a1a7 100644 --- a/crates/iroha_executor/src/default/isi/multisig/transaction.rs +++ b/crates/iroha_executor/src/default/isi/multisig/transaction.rs @@ -162,6 +162,7 @@ impl VisitExecute for MultisigApprove { let approver = executor.context().authority.clone(); let multisig_account = self.account.clone(); let host = executor.host(); + let instructions_hash = self.instructions_hash; let multisig_role = multisig_role_for(&multisig_account); if host @@ -172,72 +173,78 @@ impl VisitExecute for MultisigApprove { { deny!(executor, "not qualified to approve multisig"); }; + + if host + .query_single(FindAccountMetadata::new( + multisig_account.clone(), + approvals_key(&instructions_hash), + )) + .is_err() + { + deny!(executor, "no proposals to approve") + }; } fn execute(self, executor: &mut V) -> Result<(), ValidationFail> { let approver = executor.context().authority.clone(); let multisig_account = self.account; + let instructions_hash = self.instructions_hash; + + // Check if the proposal is expired + prune_expired(multisig_account.clone(), instructions_hash, executor)?; // Authorize as the multisig account executor.context_mut().authority = multisig_account.clone(); - let host = executor.host(); - let instructions_hash = self.instructions_hash; - let signatories: BTreeMap = host + let Some(instructions) = executor + .host() .query_single(FindAccountMetadata::new( multisig_account.clone(), - SIGNATORIES.parse().unwrap(), + instructions_key(&instructions_hash), )) - .dbg_unwrap() - .try_into_any() - .dbg_unwrap(); - let quorum: u16 = host + .ok() + .and_then(|json| json.try_into_any::>().ok()) + else { + // TODO Notify that the proposal has expired, while returning Ok for the entry deletion to take effect + return Ok(()); + }; + let mut approvals: BTreeSet = executor + .host() .query_single(FindAccountMetadata::new( multisig_account.clone(), - QUORUM.parse().unwrap(), + approvals_key(&instructions_hash), )) .dbg_unwrap() .try_into_any() .dbg_unwrap(); - let instructions: Vec = host - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - instructions_key(&instructions_hash), - ))? - .try_into_any() - .dbg_unwrap(); - let expires_at_ms: u64 = host + + approvals.insert(approver); + + visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( + multisig_account.clone(), + approvals_key(&instructions_hash), + Json::new(&approvals), + ))); + + let signatories: BTreeMap = executor + .host() .query_single(FindAccountMetadata::new( multisig_account.clone(), - expires_at_ms_key(&instructions_hash), + SIGNATORIES.parse().unwrap(), )) .dbg_unwrap() .try_into_any() .dbg_unwrap(); - let now_ms: u64 = executor - .context() - .curr_block - .creation_time() - .as_millis() - .try_into() - .dbg_expect("shouldn't overflow within 584942417 years"); - let mut approvals: BTreeSet = host + let quorum: u16 = executor + .host() .query_single(FindAccountMetadata::new( multisig_account.clone(), - approvals_key(&instructions_hash), + QUORUM.parse().unwrap(), )) .dbg_unwrap() .try_into_any() .dbg_unwrap(); - approvals.insert(approver); - - visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( - multisig_account.clone(), - approvals_key(&instructions_hash), - Json::new(&approvals), - ))); - let is_authenticated = quorum <= signatories .into_iter() @@ -245,9 +252,11 @@ impl VisitExecute for MultisigApprove { .map(|(_, weight)| u16::from(weight)) .sum(); - let is_expired = expires_at_ms < now_ms; + if is_authenticated { + for instruction in instructions { + visit_seq!(executor.visit_instruction(&instruction)); + } - if is_authenticated || is_expired { // Cleanup the transaction entry visit_seq!( executor.visit_remove_account_key_value(&RemoveKeyValue::account( @@ -255,38 +264,104 @@ impl VisitExecute for MultisigApprove { approvals_key(&instructions_hash), )) ); - visit_seq!( executor.visit_remove_account_key_value(&RemoveKeyValue::account( multisig_account.clone(), - proposed_at_ms_key(&instructions_hash), + expires_at_ms_key(&instructions_hash), )) ); - visit_seq!( executor.visit_remove_account_key_value(&RemoveKeyValue::account( multisig_account.clone(), - expires_at_ms_key(&instructions_hash), + proposed_at_ms_key(&instructions_hash), )) ); - visit_seq!( executor.visit_remove_account_key_value(&RemoveKeyValue::account( multisig_account.clone(), instructions_key(&instructions_hash), )) ); - - if is_expired { - // TODO Notify that the proposal has expired, while returning Ok for the entry deletion to take effect - } else { - // Validate and execute the authenticated multisig transaction - for instruction in instructions { - visit_seq!(executor.visit_instruction(&instruction)); - } - } } Ok(()) } } + +/// Remove intermediate approvals and the root proposal if expired +fn prune_expired( + multisig_account: AccountId, + instructions_hash: HashOf>, + executor: &mut V, +) -> Result<(), ValidationFail> { + // Confirm entry existence + let Some(expires_at_ms) = executor + .host() + .query_single(FindAccountMetadata::new( + multisig_account.clone(), + expires_at_ms_key(&instructions_hash), + )) + .ok() + .and_then(|json| json.try_into_any::().ok()) + else { + // Removed by another path + return Ok(()); + }; + // Confirm expiration + let now_ms: u64 = executor + .context() + .curr_block + .creation_time() + .as_millis() + .try_into() + .dbg_expect("shouldn't overflow within 584942417 years"); + if now_ms < expires_at_ms { + return Ok(()); + } + // Recurse through approvals + let instructions: Vec = executor + .host() + .query_single(FindAccountMetadata::new( + multisig_account.clone(), + instructions_key(&instructions_hash), + )) + .dbg_unwrap() + .try_into_any() + .dbg_unwrap(); + for instruction in instructions { + if let InstructionBox::Custom(instruction) = instruction { + if let Ok(MultisigInstructionBox::Approve(approve)) = instruction.payload().try_into() { + prune_expired(approve.account, approve.instructions_hash, executor)?; + } + } + } + // Authorize as the multisig account + executor.context_mut().authority = multisig_account.clone(); + // Cleanup the transaction entry + visit_seq!( + executor.visit_remove_account_key_value(&RemoveKeyValue::account( + multisig_account.clone(), + approvals_key(&instructions_hash), + )) + ); + visit_seq!( + executor.visit_remove_account_key_value(&RemoveKeyValue::account( + multisig_account.clone(), + expires_at_ms_key(&instructions_hash), + )) + ); + visit_seq!( + executor.visit_remove_account_key_value(&RemoveKeyValue::account( + multisig_account.clone(), + proposed_at_ms_key(&instructions_hash), + )) + ); + visit_seq!( + executor.visit_remove_account_key_value(&RemoveKeyValue::account( + multisig_account, + instructions_key(&instructions_hash), + )) + ); + + Ok(()) +} From 394eafedc1ae82ecdc9bb98393d5805cefab8ed2 Mon Sep 17 00:00:00 2001 From: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> Date: Tue, 12 Nov 2024 01:37:43 +0900 Subject: [PATCH 04/11] refactor(multisig)!: consolidate metadata read/write BREAKING CHANGES: - (api-changes) `MultisigSpec` `MultisigProposalValue` metadata values Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> --- crates/iroha/tests/multisig.rs | 106 +++++---- crates/iroha_cli/src/main.rs | 24 +- .../src/default/isi/multisig/account.rs | 32 +-- .../src/default/isi/multisig/mod.rs | 27 +-- .../src/default/isi/multisig/transaction.rs | 209 +++++------------- crates/iroha_executor_data_model/src/isi.rs | 67 +++++- crates/iroha_schema_gen/src/lib.rs | 8 + docs/source/references/schema.json | 31 +++ 8 files changed, 246 insertions(+), 258 deletions(-) diff --git a/crates/iroha/tests/multisig.rs b/crates/iroha/tests/multisig.rs index 92f5a41405d..bd6386608e9 100644 --- a/crates/iroha/tests/multisig.rs +++ b/crates/iroha/tests/multisig.rs @@ -1,5 +1,5 @@ use std::{ - collections::{BTreeMap, BTreeSet}, + collections::BTreeMap, num::{NonZeroU16, NonZeroU64}, time::Duration, }; @@ -144,22 +144,24 @@ fn multisig_base(suite: TestSuite) -> Result<()> { let register_multisig_account = MultisigRegister::new( multisig_account_id.clone(), - signatories - .keys() - .enumerate() - .map(|(weight, id)| (id.clone(), 1 + weight as u8)) - .collect(), - // Quorum can be reached without the first signatory - (1..=N_SIGNATORIES) - .skip(1) - .sum::() - .try_into() - .ok() - .and_then(NonZeroU16::new) - .unwrap(), - transaction_ttl_ms_opt - .and_then(NonZeroU64::new) - .unwrap_or(NonZeroU64::MAX), + MultisigSpec::new( + signatories + .keys() + .enumerate() + .map(|(weight, id)| (id.clone(), 1 + weight as u8)) + .collect(), + // Quorum can be reached without the first signatory + (1..=N_SIGNATORIES) + .skip(1) + .sum::() + .try_into() + .ok() + .and_then(NonZeroU16::new) + .unwrap(), + transaction_ttl_ms_opt + .and_then(NonZeroU64::new) + .unwrap_or(NonZeroU64::MAX), + ), ); // Any account in another domain cannot register a multisig account without special permission @@ -267,7 +269,7 @@ fn multisig_base(suite: TestSuite) -> Result<()> { // Check if the transaction entry is deleted let res = test_client.query_single(FindAccountMetadata::new( multisig_account_id, - format!("proposals/{instructions_hash}/instructions") + format!("multisig/proposals/{instructions_hash}") .parse() .unwrap(), )); @@ -325,8 +327,7 @@ fn multisig_recursion_base(suite: TestSuite) -> Result<()> { let register_ms_account = |sigs: Vec<&AccountId>| { let ms_account_id = gen_account_in(wonderland).0; - let register = MultisigRegister::new( - ms_account_id.clone(), + let spec = MultisigSpec::new( // Equal votes sigs.iter().copied().map(|id| (id.clone(), 1)).collect(), // Unanimous @@ -339,19 +340,20 @@ fn multisig_recursion_base(suite: TestSuite) -> Result<()> { .and_then(NonZeroU64::new) .unwrap_or(NonZeroU64::MAX), ); + let register = MultisigRegister::new(ms_account_id.clone(), spec.clone()); test_client .submit_blocking(register) .expect("the domain owner should succeed to register a multisig account"); - ms_account_id + (ms_account_id, spec) }; - let msa_12 = register_ms_account(sigs_12.keys().collect()); - let msa_345 = register_ms_account(sigs_345.keys().collect()); - let msa_12345 = register_ms_account(vec![&msa_12, &msa_345]); + let (msa_12, _spec_12) = register_ms_account(sigs_12.keys().collect()); + let (msa_345, _spec_345) = register_ms_account(sigs_345.keys().collect()); + let (msa_12345, _spec_12345) = register_ms_account(vec![&msa_12, &msa_345]); // The root multisig account with 6 personal signatories under its umbrella - let msa_012345 = register_ms_account(vec![&sig_0.0, &msa_12345]); + let (msa_012345, _spec_012345) = register_ms_account(vec![&sig_0.0, &msa_12345]); // One of personal signatories proposes a multisig transaction let key: Name = "success_marker".parse().unwrap(); @@ -379,30 +381,40 @@ fn multisig_recursion_base(suite: TestSuite) -> Result<()> { test_client.submit_blocking(Log::new(Level::DEBUG, "Just ticking time".to_string()))?; // Check that the entire authentication policy has been deployed down to one of the leaf signatories - let approval_hash_to_012345 = { - let approve: InstructionBox = - MultisigApprove::new(msa_012345.clone(), instructions_hash).into(); + let approve_to_012345: InstructionBox = + MultisigApprove::new(msa_012345.clone(), instructions_hash).into(); + let approval_hash_to_012345 = HashOf::new(&vec![approve_to_012345]); - HashOf::new(&vec![approve]) - }; - let approval_hash_to_12345 = { - let approve: InstructionBox = - MultisigApprove::new(msa_12345.clone(), approval_hash_to_012345).into(); + let approve_to_12345: InstructionBox = + MultisigApprove::new(msa_12345.clone(), approval_hash_to_012345).into(); + let approval_hash_to_12345 = HashOf::new(&vec![approve_to_12345.clone()]); - HashOf::new(&vec![approve]) + let proposal_value_at = |msa: AccountId, mst_hash: HashOf>| { + test_client + .query_single(FindAccountMetadata::new( + msa.clone(), + format!("multisig/proposals/{mst_hash}").parse().unwrap(), + )) + .expect("should be initialized by the root proposal") + .try_into_any::() + .unwrap() }; - let approvals_at_12: BTreeSet = test_client - .query_single(FindAccountMetadata::new( - msa_12.clone(), - format!("proposals/{approval_hash_to_12345}/approvals") - .parse() - .unwrap(), - )) - .expect("leaf approvals should be initialized by the root proposal") - .try_into_any() - .unwrap(); + let proposal_value_at_012345 = proposal_value_at(msa_012345.clone(), instructions_hash); + let proposal_value_at_12 = proposal_value_at(msa_12.clone(), approval_hash_to_12345); - assert!(1 == approvals_at_12.len() && approvals_at_12.contains(&msa_12345)); + assert_eq!(proposal_value_at_12.instructions, vec![approve_to_12345]); + assert_eq!( + proposal_value_at_12.proposed_at_ms, + proposal_value_at_012345.proposed_at_ms + ); + assert_eq!( + proposal_value_at_12.expires_at_ms, + proposal_value_at_012345.expires_at_ms + ); + assert!( + 1 == proposal_value_at_12.approvals.len() + && proposal_value_at_12.approvals.contains(&msa_12345) + ); // All the rest signatories try to approve the multisig transaction let mut approvals_iter = sigs_12 @@ -456,9 +468,7 @@ fn multisig_recursion_base(suite: TestSuite) -> Result<()> { ] { let res = test_client.query_single(FindAccountMetadata::new( msa, - format!("proposals/{mst_hash}/instructions") - .parse() - .unwrap(), + format!("multisig/proposals/{mst_hash}").parse().unwrap(), )); match (&transaction_ttl_ms_opt, &unauthorized_target_opt) { // In case the root proposal is failing validation, the entries on the last approval path can exit only by expiring diff --git a/crates/iroha_cli/src/main.rs b/crates/iroha_cli/src/main.rs index 9e3faa5287b..3c5ea1af9d8 100644 --- a/crates/iroha_cli/src/main.rs +++ b/crates/iroha_cli/src/main.rs @@ -1236,14 +1236,16 @@ mod multisig { } let register_multisig_account = MultisigRegister::new( self.account, - self.signatories.into_iter().zip(self.weights).collect(), - NonZeroU16::new(self.quorum).expect("quorum should not be 0"), - self.transaction_ttl - .as_millis() - .try_into() - .ok() - .and_then(NonZeroU64::new) - .expect("ttl should be between 1 ms and 584942417 years"), + MultisigSpec::new( + self.signatories.into_iter().zip(self.weights).collect(), + NonZeroU16::new(self.quorum).expect("quorum should not be 0"), + self.transaction_ttl + .as_millis() + .try_into() + .ok() + .and_then(NonZeroU64::new) + .expect("ttl should be between 1 ms and 584942417 years"), + ), ); submit([register_multisig_account], Metadata::default(), context) @@ -1329,7 +1331,6 @@ mod multisig { } const DELIMITER: char = '/'; - const PROPOSALS: &str = "proposals"; const MULTISIG_SIGNATORY: &str = "MULTISIG_SIGNATORY"; fn multisig_account_from(role: &RoleId) -> Option { @@ -1372,16 +1373,15 @@ mod multisig { let proposal_kvs = super_account .metadata() .iter() - .filter(|kv| kv.0.as_ref().starts_with(PROPOSALS)); + .filter(|kv| kv.0.as_ref().starts_with("multisig/proposals")); proposal_kvs.fold("", |acc, (k, v)| { let mut path = k.as_ref().split('/'); - let hash = path.nth(1).unwrap(); + let hash = path.nth(2).unwrap(); if acc != hash { context.print_data(&hash).unwrap(); } - path.for_each(|seg| context.print_data(&seg).unwrap()); context.print_data(&v).unwrap(); hash diff --git a/crates/iroha_executor/src/default/isi/multisig/account.rs b/crates/iroha_executor/src/default/isi/multisig/account.rs index 1b27223b93f..c8f785c26c4 100644 --- a/crates/iroha_executor/src/default/isi/multisig/account.rs +++ b/crates/iroha_executor/src/default/isi/multisig/account.rs @@ -6,13 +6,19 @@ impl VisitExecute for MultisigRegister { fn visit(&self, _executor: &mut V) {} fn execute(self, executor: &mut V) -> Result<(), ValidationFail> { - let multisig_account = self.account; + let (multisig_account, spec) = self.into(); let multisig_role = multisig_role_for(&multisig_account); + let metadata = { + let mut res = Metadata::default(); + res.insert(spec_key(), Json::new(&spec)); + res + }; // The multisig registrant needs to have sufficient permission to register personal accounts // TODO Loosen to just being one of the signatories? But impose the procedure of propose and approve? - visit_seq!(executor - .visit_register_account(&Register::account(Account::new(multisig_account.clone())))); + visit_seq!(executor.visit_register_account(&Register::account( + Account::new(multisig_account.clone()).with_metadata(metadata) + ))); let domain_owner = executor .host() @@ -27,30 +33,12 @@ impl VisitExecute for MultisigRegister { // Just having permission to register accounts is insufficient to register multisig roles executor.context_mut().authority = domain_owner.clone(); - visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( - multisig_account.clone(), - SIGNATORIES.parse().unwrap(), - Json::new(&self.signatories), - ))); - - visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( - multisig_account.clone(), - QUORUM.parse().unwrap(), - Json::new(self.quorum), - ))); - - visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( - multisig_account.clone(), - TRANSACTION_TTL_MS.parse().unwrap(), - Json::new(self.transaction_ttl_ms), - ))); - visit_seq!(executor.visit_register_role(&Register::role( // Temporarily grant a multisig role to the domain owner to delegate the role to the signatories Role::new(multisig_role.clone(), domain_owner.clone()), ))); - for signatory in self.signatories.keys().cloned() { + for signatory in spec.signatories.keys().cloned() { visit_seq!(executor .visit_grant_account_role(&Grant::account_role(multisig_role.clone(), signatory))); } diff --git a/crates/iroha_executor/src/default/isi/multisig/mod.rs b/crates/iroha_executor/src/default/isi/multisig/mod.rs index 957c4fa1ac8..58a7c84b37a 100644 --- a/crates/iroha_executor/src/default/isi/multisig/mod.rs +++ b/crates/iroha_executor/src/default/isi/multisig/mod.rs @@ -17,32 +17,15 @@ impl VisitExecute for MultisigInstructionBox { } const DELIMITER: char = '/'; -const SIGNATORIES: &str = "signatories"; -const QUORUM: &str = "quorum"; -const TRANSACTION_TTL_MS: &str = "transaction_ttl_ms"; -const PROPOSALS: &str = "proposals"; +const MULTISIG: &str = "multisig"; const MULTISIG_SIGNATORY: &str = "MULTISIG_SIGNATORY"; -fn instructions_key(hash: &HashOf>) -> Name { - format!("{PROPOSALS}{DELIMITER}{hash}{DELIMITER}instructions") - .parse() - .unwrap() -} - -fn proposed_at_ms_key(hash: &HashOf>) -> Name { - format!("{PROPOSALS}{DELIMITER}{hash}{DELIMITER}proposed_at_ms") - .parse() - .unwrap() -} - -fn expires_at_ms_key(hash: &HashOf>) -> Name { - format!("{PROPOSALS}{DELIMITER}{hash}{DELIMITER}expires_at_ms") - .parse() - .unwrap() +fn spec_key() -> Name { + format!("{MULTISIG}{DELIMITER}spec").parse().unwrap() } -fn approvals_key(hash: &HashOf>) -> Name { - format!("{PROPOSALS}{DELIMITER}{hash}{DELIMITER}approvals") +fn proposal_key(hash: &HashOf>) -> Name { + format!("{MULTISIG}{DELIMITER}proposals{DELIMITER}{hash}") .parse() .unwrap() } diff --git a/crates/iroha_executor/src/default/isi/multisig/transaction.rs b/crates/iroha_executor/src/default/isi/multisig/transaction.rs index 6f53c16a1a7..bb8c25e9ff8 100644 --- a/crates/iroha_executor/src/default/isi/multisig/transaction.rs +++ b/crates/iroha_executor/src/default/isi/multisig/transaction.rs @@ -1,6 +1,7 @@ //! Validation and execution logic of instructions for multisig transactions -use alloc::collections::{btree_map::BTreeMap, btree_set::BTreeSet}; +use alloc::collections::btree_set::BTreeSet; +use core::num::NonZeroU64; use super::*; @@ -11,15 +12,23 @@ impl VisitExecute for MultisigPropose { let host = executor.host(); let instructions_hash = HashOf::new(&self.instructions); let multisig_role = multisig_role_for(&multisig_account); - let is_downward_proposal = host + let Some(multisig_spec) = host .query_single(FindAccountMetadata::new( - proposer.clone(), - SIGNATORIES.parse().unwrap(), + multisig_account.clone(), + spec_key(), )) - .map_or(false, |proposer_signatories| { - proposer_signatories - .try_into_any::>() + .ok() + .and_then(|json| json.try_into_any::().ok()) + else { + deny!(executor, "multisig spec not found or malformed"); + }; + + let is_downward_proposal = host + .query_single(FindAccountMetadata::new(proposer.clone(), spec_key())) + .map_or(false, |json| { + json.try_into_any::() .dbg_unwrap() + .signatories .contains_key(&multisig_account) }); let has_multisig_role = host @@ -27,23 +36,9 @@ impl VisitExecute for MultisigPropose { .filter_with(|role_id| role_id.eq(multisig_role)) .execute_single() .is_ok(); - let has_not_longer_ttl = { - let Some(account_default_ttl_ms) = host - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - TRANSACTION_TTL_MS.parse().unwrap(), - )) - .ok() - .and_then(|json| json.try_into_any::().ok()) - else { - deny!(executor, "multisig account not found"); - }; - self.transaction_ttl_ms - .map(u64::from) - .map_or(true, |override_ttl_ms| { - override_ttl_ms <= account_default_ttl_ms - }) - }; + let has_not_longer_ttl = self.transaction_ttl_ms.map_or(true, |override_ttl_ms| { + override_ttl_ms <= multisig_spec.transaction_ttl_ms + }); if !(is_downward_proposal || has_not_longer_ttl) { deny!(executor, "ttl violates the restriction"); @@ -56,7 +51,7 @@ impl VisitExecute for MultisigPropose { if host .query_single(FindAccountMetadata::new( multisig_account.clone(), - approvals_key(&instructions_hash), + proposal_key(&instructions_hash), )) .is_ok() { @@ -72,37 +67,32 @@ impl VisitExecute for MultisigPropose { executor.context_mut().authority = multisig_account.clone(); let instructions_hash = HashOf::new(&self.instructions); - let now_ms: u64 = executor + let now_ms = executor .context() .curr_block .creation_time() .as_millis() .try_into() + .ok() + .and_then(NonZeroU64::new) .dbg_expect("shouldn't overflow within 584942417 years"); - let expires_at_ms: u64 = { - let ttl_ms = self.transaction_ttl_ms.map(u64::from).unwrap_or_else(|| { - executor - .host() - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - TRANSACTION_TTL_MS.parse().unwrap(), - )) - .dbg_unwrap() - .try_into_any() - .dbg_unwrap() - }); - now_ms.saturating_add(ttl_ms) - }; - let approvals = BTreeSet::from([proposer]); - let signatories: BTreeMap = executor + let spec: MultisigSpec = executor .host() .query_single(FindAccountMetadata::new( multisig_account.clone(), - SIGNATORIES.parse().unwrap(), + spec_key(), )) .dbg_unwrap() .try_into_any() .dbg_unwrap(); + let expires_at_ms = { + let ttl_ms = self.transaction_ttl_ms.unwrap_or(spec.transaction_ttl_ms); + now_ms.saturating_add(ttl_ms.into()) + }; + let approvals = BTreeSet::from([proposer]); + let proposal_value = + MultisigProposalValue::new(self.instructions, now_ms, expires_at_ms, approvals); + let signatories = spec.signatories; // Recursively deploy multisig authentication down to the personal leaf signatories for signatory in signatories.keys().cloned() { @@ -122,7 +112,8 @@ impl VisitExecute for MultisigPropose { MultisigPropose::new( signatory, [approve_me.into()].to_vec(), - self.transaction_ttl_ms, + // Force override by the root proposal TTL + Some(self.transaction_ttl_ms.unwrap_or(spec.transaction_ttl_ms)), ) }; propose_to_approve_me.visit_execute(executor); @@ -131,26 +122,8 @@ impl VisitExecute for MultisigPropose { visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( multisig_account.clone(), - instructions_key(&instructions_hash).clone(), - Json::new(&self.instructions), - ))); - - visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( - multisig_account.clone(), - proposed_at_ms_key(&instructions_hash).clone(), - Json::new(now_ms), - ))); - - visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( - multisig_account.clone(), - expires_at_ms_key(&instructions_hash).clone(), - Json::new(expires_at_ms), - ))); - - visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( - multisig_account, - approvals_key(&instructions_hash).clone(), - Json::new(&approvals), + proposal_key(&instructions_hash).clone(), + Json::new(&proposal_value), ))); Ok(()) @@ -177,7 +150,7 @@ impl VisitExecute for MultisigApprove { if host .query_single(FindAccountMetadata::new( multisig_account.clone(), - approvals_key(&instructions_hash), + proposal_key(&instructions_hash), )) .is_err() { @@ -196,64 +169,47 @@ impl VisitExecute for MultisigApprove { // Authorize as the multisig account executor.context_mut().authority = multisig_account.clone(); - let Some(instructions) = executor + let Some(mut proposal_value) = executor .host() .query_single(FindAccountMetadata::new( multisig_account.clone(), - instructions_key(&instructions_hash), + proposal_key(&instructions_hash), )) .ok() - .and_then(|json| json.try_into_any::>().ok()) + .and_then(|json| json.try_into_any::().ok()) else { // TODO Notify that the proposal has expired, while returning Ok for the entry deletion to take effect return Ok(()); }; - let mut approvals: BTreeSet = executor - .host() - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - approvals_key(&instructions_hash), - )) - .dbg_unwrap() - .try_into_any() - .dbg_unwrap(); - approvals.insert(approver); + proposal_value.approvals.insert(approver); visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( multisig_account.clone(), - approvals_key(&instructions_hash), - Json::new(&approvals), + proposal_key(&instructions_hash), + Json::new(&proposal_value), ))); - let signatories: BTreeMap = executor + let spec: MultisigSpec = executor .host() .query_single(FindAccountMetadata::new( multisig_account.clone(), - SIGNATORIES.parse().unwrap(), - )) - .dbg_unwrap() - .try_into_any() - .dbg_unwrap(); - let quorum: u16 = executor - .host() - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - QUORUM.parse().unwrap(), + spec_key(), )) .dbg_unwrap() .try_into_any() .dbg_unwrap(); - let is_authenticated = quorum - <= signatories + let is_authenticated = u16::from(spec.quorum) + <= spec + .signatories .into_iter() - .filter(|(id, _)| approvals.contains(id)) + .filter(|(id, _)| proposal_value.approvals.contains(id)) .map(|(_, weight)| u16::from(weight)) .sum(); if is_authenticated { - for instruction in instructions { + for instruction in proposal_value.instructions { visit_seq!(executor.visit_instruction(&instruction)); } @@ -261,25 +217,7 @@ impl VisitExecute for MultisigApprove { visit_seq!( executor.visit_remove_account_key_value(&RemoveKeyValue::account( multisig_account.clone(), - approvals_key(&instructions_hash), - )) - ); - visit_seq!( - executor.visit_remove_account_key_value(&RemoveKeyValue::account( - multisig_account.clone(), - expires_at_ms_key(&instructions_hash), - )) - ); - visit_seq!( - executor.visit_remove_account_key_value(&RemoveKeyValue::account( - multisig_account.clone(), - proposed_at_ms_key(&instructions_hash), - )) - ); - visit_seq!( - executor.visit_remove_account_key_value(&RemoveKeyValue::account( - multisig_account.clone(), - instructions_key(&instructions_hash), + proposal_key(&instructions_hash), )) ); } @@ -295,40 +233,33 @@ fn prune_expired( executor: &mut V, ) -> Result<(), ValidationFail> { // Confirm entry existence - let Some(expires_at_ms) = executor + let Some(proposal_value) = executor .host() .query_single(FindAccountMetadata::new( multisig_account.clone(), - expires_at_ms_key(&instructions_hash), + proposal_key(&instructions_hash), )) .ok() - .and_then(|json| json.try_into_any::().ok()) + .and_then(|json| json.try_into_any::().ok()) else { // Removed by another path return Ok(()); }; // Confirm expiration - let now_ms: u64 = executor + let now_ms = executor .context() .curr_block .creation_time() .as_millis() .try_into() + .ok() + .and_then(NonZeroU64::new) .dbg_expect("shouldn't overflow within 584942417 years"); - if now_ms < expires_at_ms { + if now_ms < proposal_value.expires_at_ms { return Ok(()); } // Recurse through approvals - let instructions: Vec = executor - .host() - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - instructions_key(&instructions_hash), - )) - .dbg_unwrap() - .try_into_any() - .dbg_unwrap(); - for instruction in instructions { + for instruction in proposal_value.instructions { if let InstructionBox::Custom(instruction) = instruction { if let Ok(MultisigInstructionBox::Approve(approve)) = instruction.payload().try_into() { prune_expired(approve.account, approve.instructions_hash, executor)?; @@ -338,28 +269,10 @@ fn prune_expired( // Authorize as the multisig account executor.context_mut().authority = multisig_account.clone(); // Cleanup the transaction entry - visit_seq!( - executor.visit_remove_account_key_value(&RemoveKeyValue::account( - multisig_account.clone(), - approvals_key(&instructions_hash), - )) - ); - visit_seq!( - executor.visit_remove_account_key_value(&RemoveKeyValue::account( - multisig_account.clone(), - expires_at_ms_key(&instructions_hash), - )) - ); - visit_seq!( - executor.visit_remove_account_key_value(&RemoveKeyValue::account( - multisig_account.clone(), - proposed_at_ms_key(&instructions_hash), - )) - ); visit_seq!( executor.visit_remove_account_key_value(&RemoveKeyValue::account( multisig_account, - instructions_key(&instructions_hash), + proposal_key(&instructions_hash), )) ); diff --git a/crates/iroha_executor_data_model/src/isi.rs b/crates/iroha_executor_data_model/src/isi.rs index 8ccd3b22713..a6b837ec2b5 100644 --- a/crates/iroha_executor_data_model/src/isi.rs +++ b/crates/iroha_executor_data_model/src/isi.rs @@ -51,6 +51,7 @@ macro_rules! impl_custom_instruction { /// Types for multisig instructions pub mod multisig { + use alloc::collections::btree_set::BTreeSet; use core::num::{NonZeroU16, NonZeroU64}; use super::*; @@ -78,12 +79,8 @@ pub mod multisig { // FIXME #5022 prevent multisig monopoly // FIXME #5022 stop accepting user input: otherwise, after #4426 pre-registration account will be hijacked as a multisig account pub account: AccountId, - /// List of signatories and their relative weights of responsibility for the multisig account - pub signatories: BTreeMap, - /// Threshold of total weight at which the multisig account is considered authenticated - pub quorum: NonZeroU16, - /// Multisig transaction time-to-live in milliseconds based on block timestamps. Defaults to [`DEFAULT_MULTISIG_TTL_MS`] - pub transaction_ttl_ms: NonZeroU64, + /// Specification of the multisig account + pub spec: MultisigSpec, } /// Relative weight of responsibility for the multisig account. @@ -117,4 +114,62 @@ pub mod multisig { MultisigInstructionBox, MultisigRegister | MultisigPropose | MultisigApprove ); + + /// Metadata value for a multisig account specification + #[derive(Debug, Clone, Serialize, Deserialize, IntoSchema, Constructor)] + pub struct MultisigSpec { + /// List of signatories and their relative weights of responsibility for the multisig account + pub signatories: BTreeMap, + /// Threshold of total weight at which the multisig account is considered authenticated + pub quorum: NonZeroU16, + /// Multisig transaction time-to-live in milliseconds based on block timestamps. Defaults to [`DEFAULT_MULTISIG_TTL_MS`] + pub transaction_ttl_ms: NonZeroU64, + } + + /// Metadata value for a multisig transaction proposal + #[derive(Debug, Clone, Serialize, Deserialize, IntoSchema, Constructor)] + pub struct MultisigProposalValue { + /// Proposal contents + pub instructions: Vec, + /// Time in milliseconds at which the proposal was made + pub proposed_at_ms: NonZeroU64, + /// Time in milliseconds at which the proposal will expire + pub expires_at_ms: NonZeroU64, + /// List of approvers of the proposal so far + pub approvals: BTreeSet, + } + + impl From for Json { + fn from(details: MultisigSpec) -> Self { + Json::new(details) + } + } + + impl TryFrom<&Json> for MultisigSpec { + type Error = serde_json::Error; + + fn try_from(payload: &Json) -> serde_json::Result { + serde_json::from_str::(payload.as_ref()) + } + } + + impl From for Json { + fn from(details: MultisigProposalValue) -> Self { + Json::new(details) + } + } + + impl TryFrom<&Json> for MultisigProposalValue { + type Error = serde_json::Error; + + fn try_from(payload: &Json) -> serde_json::Result { + serde_json::from_str::(payload.as_ref()) + } + } + + impl From for (AccountId, MultisigSpec) { + fn from(value: MultisigRegister) -> Self { + (value.account, value.spec) + } + } } diff --git a/crates/iroha_schema_gen/src/lib.rs b/crates/iroha_schema_gen/src/lib.rs index 0c13c559cc4..06f49f5e21b 100644 --- a/crates/iroha_schema_gen/src/lib.rs +++ b/crates/iroha_schema_gen/src/lib.rs @@ -93,6 +93,9 @@ pub fn build_schemas() -> MetaMap { // Multi-signature operations multisig::MultisigInstructionBox, + // Multi-signature account metadata + multisig::MultisigSpec, + multisig::MultisigProposalValue, // Genesis file - used by SDKs to generate the genesis block // TODO: IMO it could/should be removed from the schema @@ -137,6 +140,7 @@ types!( BTreeMap, BTreeMap, BTreeMap, + BTreeSet, BTreeSet, BTreeSet, BlockEvent, @@ -313,6 +317,7 @@ types!( Option, Option, Option, + Option, Option, Option, Pagination, @@ -490,6 +495,7 @@ types!( WasmExecutionFail, WasmSmartContract, + bool, [u8; 32], u16, u32, @@ -628,6 +634,8 @@ mod tests { insert_into_test_map!(iroha_executor_data_model::isi::multisig::MultisigRegister); insert_into_test_map!(iroha_executor_data_model::isi::multisig::MultisigPropose); insert_into_test_map!(iroha_executor_data_model::isi::multisig::MultisigApprove); + insert_into_test_map!(iroha_executor_data_model::isi::multisig::MultisigSpec); + insert_into_test_map!(iroha_executor_data_model::isi::multisig::MultisigProposalValue); map } diff --git a/docs/source/references/schema.json b/docs/source/references/schema.json index 0eab35911a2..e5f22436433 100644 --- a/docs/source/references/schema.json +++ b/docs/source/references/schema.json @@ -2630,6 +2630,26 @@ } ] }, + "MultisigProposalValue": { + "Struct": [ + { + "name": "instructions", + "type": "Vec" + }, + { + "name": "proposed_at_ms", + "type": "NonZero" + }, + { + "name": "expires_at_ms", + "type": "NonZero" + }, + { + "name": "approvals", + "type": "SortedVec" + } + ] + }, "MultisigPropose": { "Struct": [ { @@ -2652,6 +2672,14 @@ "name": "account", "type": "AccountId" }, + { + "name": "spec", + "type": "MultisigSpec" + } + ] + }, + "MultisigSpec": { + "Struct": [ { "name": "signatories", "type": "SortedMap" @@ -4249,6 +4277,9 @@ "value": "TransactionRejectionReason" } }, + "SortedVec": { + "Vec": "AccountId" + }, "SortedVec": { "Vec": "Permission" }, From a6e64932f3ec88bdf02dcd6ad2fedfa7d46521fb Mon Sep 17 00:00:00 2001 From: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> Date: Wed, 13 Nov 2024 02:39:34 +0900 Subject: [PATCH 05/11] feat(multisig): pretty list of pending proposals Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> --- Cargo.lock | 2 + crates/iroha_cli/Cargo.toml | 2 + crates/iroha_cli/src/main.rs | 191 ++++++++++++++++---- crates/iroha_executor_data_model/Cargo.toml | 2 +- scripts/tests/multisig.recursion.sh | 34 +++- scripts/tests/multisig.sh | 33 +++- 6 files changed, 208 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2f5fc18928a..feca6d21788 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2949,6 +2949,7 @@ version = "2.0.0-rc.1.0" dependencies = [ "clap", "color-eyre", + "derive_more", "erased-serde", "error-stack", "eyre", @@ -2958,6 +2959,7 @@ dependencies = [ "json5", "serde", "serde_json", + "serde_with", "supports-color 2.1.0", "thiserror", "vergen", diff --git a/crates/iroha_cli/Cargo.toml b/crates/iroha_cli/Cargo.toml index b843a2dccc2..67620256a48 100644 --- a/crates/iroha_cli/Cargo.toml +++ b/crates/iroha_cli/Cargo.toml @@ -38,8 +38,10 @@ humantime = { workspace = true } json5 = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } +serde_with = { workspace = true } erased-serde = "0.4.5" supports-color = { workspace = true } +derive_more = { workspace = true } [build-dependencies] vergen = { version = "8.3.1", default-features = false } diff --git a/crates/iroha_cli/src/main.rs b/crates/iroha_cli/src/main.rs index 3c5ea1af9d8..188526cfccd 100644 --- a/crates/iroha_cli/src/main.rs +++ b/crates/iroha_cli/src/main.rs @@ -1178,11 +1178,16 @@ mod json { mod multisig { use std::{ + collections::BTreeMap, io::{BufReader, Read as _}, num::{NonZeroU16, NonZeroU64}, + time::{Duration, SystemTime}, }; + use derive_more::{Constructor, Display}; use iroha::executor_data_model::isi::multisig::*; + use serde::Serialize; + use serde_with::{serde_as, DisplayFromStr, SerializeDisplay}; use super::*; @@ -1301,7 +1306,7 @@ mod multisig { pub account: AccountId, /// Instructions to approve #[arg(short, long)] - pub instructions_hash: HashOf>, + pub instructions_hash: ProposalKey, } impl RunArgs for Approve { @@ -1325,14 +1330,39 @@ mod multisig { fn run(self, context: &mut dyn RunContext) -> Result<()> { let client = context.client_from_config(); let me = client.account.clone(); + let Ok(my_multisig_roles) = client + .query(FindRolesByAccountId::new(me.clone())) + .filter_with(|role_id| role_id.name.starts_with(MULTISIG_SIGNATORY)) + .execute_all() + else { + return Ok(()); + }; + let mut stack = my_multisig_roles + .iter() + .filter_map(multisig_account_from) + .map(|account_id| Context::new(me.clone(), account_id, None)) + .collect(); + let mut proposals = BTreeMap::new(); - trace_back_from(me, &client, context) + fold_proposals(&mut proposals, &mut stack, &client)?; + context.print_data(&proposals)?; + + Ok(()) } } const DELIMITER: char = '/'; + const MULTISIG: &str = "multisig"; const MULTISIG_SIGNATORY: &str = "MULTISIG_SIGNATORY"; + fn spec_key() -> Name { + format!("{MULTISIG}{DELIMITER}spec").parse().unwrap() + } + + fn proposal_key_prefix() -> String { + format!("{MULTISIG}{DELIMITER}proposals{DELIMITER}") + } + fn multisig_account_from(role: &RoleId) -> Option { role.name() .as_ref() @@ -1345,52 +1375,135 @@ mod multisig { }) } - /// Recursively trace back to the root multisig account - fn trace_back_from( - account: AccountId, - client: &Client, - context: &mut dyn RunContext, - ) -> Result<()> { - let Ok(multisig_roles) = client - .query(FindRolesByAccountId::new(account)) - .filter_with(|role_id| role_id.name.starts_with(MULTISIG_SIGNATORY)) - .execute_all() - else { - return Ok(()); - }; - - for role_id in multisig_roles { - let super_account_id: AccountId = multisig_account_from(&role_id).unwrap(); + type PendingProposals = BTreeMap; - trace_back_from(super_account_id.clone(), client, context)?; + type ProposalKey = HashOf>; - context.print_data(&super_account_id)?; + #[serde_as] + #[derive(Debug, Serialize, Constructor)] + struct ProposalStatus { + instructions: Vec, + #[serde_as(as = "DisplayFromStr")] + proposed_at: humantime::Timestamp, + #[serde_as(as = "DisplayFromStr")] + expires_in: humantime::Duration, + approval_path: Vec, + } - let super_account = client - .query(FindAccounts) - .filter_with(|account| account.id.eq(super_account_id)) - .execute_single()?; - let proposal_kvs = super_account - .metadata() - .iter() - .filter(|kv| kv.0.as_ref().starts_with("multisig/proposals")); + impl Default for ProposalStatus { + fn default() -> Self { + Self::new( + Vec::new(), + SystemTime::UNIX_EPOCH.into(), + Duration::ZERO.into(), + Vec::new(), + ) + } + } - proposal_kvs.fold("", |acc, (k, v)| { - let mut path = k.as_ref().split('/'); - let hash = path.nth(2).unwrap(); + #[derive(Debug, SerializeDisplay, Display, Constructor)] + #[display(fmt = "{weight} -> [{got}/{quorum}] {target}")] + struct ApprovalEdge { + weight: u8, + got: u16, + quorum: u16, + target: AccountId, + } - if acc != hash { - context.print_data(&hash).unwrap(); - } - context.print_data(&v).unwrap(); + #[derive(Debug, Constructor)] + struct Context { + child: AccountId, + this: AccountId, + key_span: Option<(ProposalKey, ProposalKey)>, + } - hash - }); + fn fold_proposals( + proposals: &mut PendingProposals, + stack: &mut Vec, + client: &Client, + ) -> Result<()> { + let Some(context) = stack.pop() else { + return Ok(()); + }; + let account = client + .query(FindAccounts) + .filter_with(|account| account.id.eq(context.this.clone())) + .execute_single()?; + let spec: MultisigSpec = account + .metadata() + .get(&spec_key()) + .unwrap() + .try_into_any()?; + for (proposal_key, proposal_value) in account + .metadata() + .iter() + .filter_map(|(k, v)| { + k.as_ref().strip_prefix(&proposal_key_prefix()).map(|k| { + ( + k.parse::().unwrap(), + v.try_into_any::().unwrap(), + ) + }) + }) + .filter(|(k, _v)| context.key_span.map_or(true, |(_, top)| *k == top)) + { + let mut is_root_proposal = true; + for instruction in &proposal_value.instructions { + let InstructionBox::Custom(instruction) = instruction else { + continue; + }; + let Ok(MultisigInstructionBox::Approve(approve)) = instruction.payload().try_into() + else { + continue; + }; + is_root_proposal = false; + let leaf = context.key_span.map_or(proposal_key, |(leaf, _)| leaf); + let top = approve.instructions_hash; + stack.push(Context::new( + context.this.clone(), + approve.account, + Some((leaf, top)), + )); + } + let proposal_status = match context.key_span { + None => proposals.entry(proposal_key).or_default(), + Some((leaf, _)) => proposals.get_mut(&leaf).unwrap(), + }; + let edge = ApprovalEdge::new( + *spec.signatories.get(&context.child).unwrap(), + spec.signatories + .iter() + .filter(|(id, _)| proposal_value.approvals.contains(id)) + .map(|(_, weight)| u16::from(*weight)) + .sum(), + spec.quorum.into(), + context.this.clone(), + ); + proposal_status.approval_path.push(edge); + if is_root_proposal { + proposal_status.instructions = proposal_value.instructions; + proposal_status.proposed_at = { + let proposed_at = Duration::from_secs( + Duration::from_millis(proposal_value.proposed_at_ms.into()).as_secs(), + ); + SystemTime::UNIX_EPOCH + .checked_add(proposed_at) + .unwrap() + .into() + }; + proposal_status.expires_in = { + let now = SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .unwrap(); + let expires_at = Duration::from_millis(proposal_value.expires_at_ms.into()); + Duration::from_secs(expires_at.saturating_sub(now).as_secs()).into() + }; + } } - - Ok(()) + fold_proposals(proposals, stack, client) } } + #[cfg(test)] mod tests { use super::*; diff --git a/crates/iroha_executor_data_model/Cargo.toml b/crates/iroha_executor_data_model/Cargo.toml index df1d60ab4da..c1656fa8ffe 100644 --- a/crates/iroha_executor_data_model/Cargo.toml +++ b/crates/iroha_executor_data_model/Cargo.toml @@ -16,6 +16,6 @@ iroha_executor_data_model_derive = { path = "../iroha_executor_data_model_derive iroha_data_model.workspace = true iroha_schema.workspace = true -derive_more = { workspace = true, features = ["constructor", "from"] } +derive_more = { workspace = true } serde.workspace = true serde_json.workspace = true diff --git a/scripts/tests/multisig.recursion.sh b/scripts/tests/multisig.recursion.sh index 1561ca2cf3e..3fd2bcbfa75 100644 --- a/scripts/tests/multisig.recursion.sh +++ b/scripts/tests/multisig.recursion.sh @@ -65,20 +65,36 @@ SIGS_012345=(${SIGNATORIES[0]} $MSA_12345) # propose a multisig transaction INSTRUCTIONS="../scripts/tests/instructions.json" -propose_stdout=($(cat $INSTRUCTIONS | ./iroha --config "client.0.toml" multisig propose --account $MSA_012345)) -INSTRUCTIONS_HASH=${propose_stdout[0]} +cat $INSTRUCTIONS | ./iroha --config "client.0.toml" multisig propose --account $MSA_012345 + +get_list_as_signatory() { + ./iroha --config "client.$1.toml" multisig list all +} + +get_target_account() { + ./iroha account list filter '{"Atom": {"Id": {"Equals": "'$MSA_012345'"}}}' +} # check that one of the leaf signatories is involved -LIST=$(./iroha --config "client.5.toml" multisig list all) -echo "$LIST" | grep $INSTRUCTIONS_HASH +LIST_BEFORE=$(get_list_as_signatory 5) +echo "$LIST_BEFORE" | jq '.[].instructions' | diff - <(cat $INSTRUCTIONS) + +# check that the multisig transaction has not yet executed +ACCOUNT_BEFORE=$(get_target_account) +# NOTE: without ` || false` this line passes even if `success_marker` exists +! echo "$ACCOUNT_BEFORE" | jq -e '.[0].metadata.success_marker' || false # approve the multisig transaction -HASH_TO_12345=$(echo "$LIST" | grep -A1 $MSA_345 | tail -n 1 | tr -d '"') -./iroha --config "client.5.toml" multisig approve --account $MSA_345 --instructions-hash $HASH_TO_12345 +LEAF_INSTRUCTIONS_HASH=$(echo "$LIST_BEFORE" | jq -r 'keys[0]') +./iroha --config "client.5.toml" multisig approve --account $MSA_345 --instructions-hash $LEAF_INSTRUCTIONS_HASH + +# check that the transaction entry is deleted +LIST_AFTER=$(get_list_as_signatory 5) +! echo "$LIST_AFTER" | jq -e '.[].instructions' || false -# check that the multisig transaction is executed -./iroha account list all | grep "congratulations" -! ./iroha --config "client.5.toml" multisig list all | grep $INSTRUCTIONS_HASH +# check that the multisig transaction has executed +ACCOUNT_AFTER=$(get_target_account) +echo "$ACCOUNT_AFTER" | jq -e '.[0].metadata.success_marker' cd - scripts/test_env.py cleanup diff --git a/scripts/tests/multisig.sh b/scripts/tests/multisig.sh index c3bfa298d86..df4e86fff32 100644 --- a/scripts/tests/multisig.sh +++ b/scripts/tests/multisig.sh @@ -47,20 +47,39 @@ TRANSACTION_TTL="1y 6M 2w 3d 12h 30m 30s 500ms" # propose a multisig transaction INSTRUCTIONS="../scripts/tests/instructions.json" -propose_stdout=($(cat $INSTRUCTIONS | ./iroha --config "client.1.toml" multisig propose --account $MULTISIG_ACCOUNT)) -INSTRUCTIONS_HASH=${propose_stdout[0]} +cat $INSTRUCTIONS | ./iroha --config "client.1.toml" multisig propose --account $MULTISIG_ACCOUNT + +get_list_as_signatory() { + ./iroha --config "client.$1.toml" multisig list all +} + +get_target_account() { + ./iroha account list filter '{"Atom": {"Id": {"Equals": "'$MULTISIG_ACCOUNT'"}}}' +} + +# check that the 2nd signatory is involved +LIST_BEFORE=$(get_list_as_signatory 2) +echo "$LIST_BEFORE" | jq '.[].instructions' | diff - <(cat $INSTRUCTIONS) + +# check that the multisig transaction has not yet executed +ACCOUNT_BEFORE=$(get_target_account) +# NOTE: without ` || false` this line passes even if `success_marker` exists +! echo "$ACCOUNT_BEFORE" | jq -e '.[0].metadata.success_marker' || false -# check that 2nd signatory is involved -./iroha --config "client.2.toml" multisig list all | grep $INSTRUCTIONS_HASH # approve the multisig transaction +INSTRUCTIONS_HASH=$(echo "$LIST_BEFORE" | jq -r 'keys[0]') for i in $(seq 2 $N_SIGNATORIES); do ./iroha --config "client.$i.toml" multisig approve --account $MULTISIG_ACCOUNT --instructions-hash $INSTRUCTIONS_HASH done -# check that the multisig transaction is executed -./iroha account list all | grep "congratulations" -! ./iroha --config "client.2.toml" multisig list all | grep $INSTRUCTIONS_HASH +# check that the transaction entry is deleted +LIST_AFTER=$(get_list_as_signatory 2) +! echo "$LIST_AFTER" | jq -e '.[].instructions' || false + +# check that the multisig transaction has executed +ACCOUNT_AFTER=$(get_target_account) +echo "$ACCOUNT_AFTER" | jq -e '.[0].metadata.success_marker' cd - scripts/test_env.py cleanup From 81282a6dd04a0a00b6e3bce57dbeefbc385b75b4 Mon Sep 17 00:00:00 2001 From: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> Date: Fri, 15 Nov 2024 17:37:48 +0900 Subject: [PATCH 06/11] fix(multisig): keep proposals in list while root is pending Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> --- crates/iroha/tests/multisig.rs | 11 +- crates/iroha_cli/src/main.rs | 14 +- .../src/default/isi/multisig/transaction.rs | 299 ++++++++++-------- crates/iroha_executor_data_model/src/isi.rs | 2 + docs/source/references/schema.json | 8 + scripts/tests/multisig.recursion.sh | 46 ++- 6 files changed, 233 insertions(+), 147 deletions(-) diff --git a/crates/iroha/tests/multisig.rs b/crates/iroha/tests/multisig.rs index bd6386608e9..97a600ab1ad 100644 --- a/crates/iroha/tests/multisig.rs +++ b/crates/iroha/tests/multisig.rs @@ -411,10 +411,8 @@ fn multisig_recursion_base(suite: TestSuite) -> Result<()> { proposal_value_at_12.expires_at_ms, proposal_value_at_012345.expires_at_ms ); - assert!( - 1 == proposal_value_at_12.approvals.len() - && proposal_value_at_12.approvals.contains(&msa_12345) - ); + assert!(proposal_value_at_12.approvals.is_empty()); + assert_eq!(proposal_value_at_12.is_relayed, Some(false)); // All the rest signatories try to approve the multisig transaction let mut approvals_iter = sigs_12 @@ -460,8 +458,9 @@ fn multisig_recursion_base(suite: TestSuite) -> Result<()> { _ => assert!(res.is_err()), } - // Check if the transaction entries on the last approval path are deleted + // Check if the transaction entries are deleted for (msa, mst_hash) in [ + (msa_12, approval_hash_to_12345), (msa_345, approval_hash_to_12345), (msa_12345, approval_hash_to_012345), (msa_012345, instructions_hash), @@ -471,7 +470,7 @@ fn multisig_recursion_base(suite: TestSuite) -> Result<()> { format!("multisig/proposals/{mst_hash}").parse().unwrap(), )); match (&transaction_ttl_ms_opt, &unauthorized_target_opt) { - // In case the root proposal is failing validation, the entries on the last approval path can exit only by expiring + // In case the root proposal is failing validation, the relevant entries can exit only by expiring (None, Some(_)) => assert!(res.is_ok()), _ => assert!(res.is_err()), } diff --git a/crates/iroha_cli/src/main.rs b/crates/iroha_cli/src/main.rs index 188526cfccd..457d9f28c67 100644 --- a/crates/iroha_cli/src/main.rs +++ b/crates/iroha_cli/src/main.rs @@ -1402,14 +1402,25 @@ mod multisig { } #[derive(Debug, SerializeDisplay, Display, Constructor)] - #[display(fmt = "{weight} -> [{got}/{quorum}] {target}")] + #[display(fmt = "{weight} {} [{got}/{quorum}] {target}", "self.relation()")] struct ApprovalEdge { weight: u8, + has_approved: bool, got: u16, quorum: u16, target: AccountId, } + impl ApprovalEdge { + fn relation(&self) -> &str { + if self.has_approved { + "joined" + } else { + "->" + } + } + } + #[derive(Debug, Constructor)] struct Context { child: AccountId, @@ -1471,6 +1482,7 @@ mod multisig { }; let edge = ApprovalEdge::new( *spec.signatories.get(&context.child).unwrap(), + proposal_value.approvals.contains(&context.child), spec.signatories .iter() .filter(|(id, _)| proposal_value.approvals.contains(id)) diff --git a/crates/iroha_executor/src/default/isi/multisig/transaction.rs b/crates/iroha_executor/src/default/isi/multisig/transaction.rs index bb8c25e9ff8..7e8154d7d2f 100644 --- a/crates/iroha_executor/src/default/isi/multisig/transaction.rs +++ b/crates/iroha_executor/src/default/isi/multisig/transaction.rs @@ -1,25 +1,18 @@ //! Validation and execution logic of instructions for multisig transactions -use alloc::collections::btree_set::BTreeSet; +use alloc::{collections::btree_set::BTreeSet, vec}; use core::num::NonZeroU64; use super::*; impl VisitExecute for MultisigPropose { fn visit(&self, executor: &mut V) { + let host = executor.host(); let proposer = executor.context().authority.clone(); let multisig_account = self.account.clone(); - let host = executor.host(); let instructions_hash = HashOf::new(&self.instructions); let multisig_role = multisig_role_for(&multisig_account); - let Some(multisig_spec) = host - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - spec_key(), - )) - .ok() - .and_then(|json| json.try_into_any::().ok()) - else { + let Some(multisig_spec) = multisig_spec(multisig_account.clone(), executor) else { deny!(executor, "multisig spec not found or malformed"); }; @@ -62,67 +55,45 @@ impl VisitExecute for MultisigPropose { fn execute(self, executor: &mut V) -> Result<(), ValidationFail> { let proposer = executor.context().authority.clone(); let multisig_account = self.account; - - // Authorize as the multisig account - executor.context_mut().authority = multisig_account.clone(); - let instructions_hash = HashOf::new(&self.instructions); - let now_ms = executor - .context() - .curr_block - .creation_time() - .as_millis() - .try_into() - .ok() - .and_then(NonZeroU64::new) - .dbg_expect("shouldn't overflow within 584942417 years"); - let spec: MultisigSpec = executor - .host() - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - spec_key(), - )) - .dbg_unwrap() - .try_into_any() - .dbg_unwrap(); + let spec = multisig_spec(multisig_account.clone(), executor).unwrap(); + + let now_ms = now_ms(executor); let expires_at_ms = { let ttl_ms = self.transaction_ttl_ms.unwrap_or(spec.transaction_ttl_ms); now_ms.saturating_add(ttl_ms.into()) }; - let approvals = BTreeSet::from([proposer]); - let proposal_value = - MultisigProposalValue::new(self.instructions, now_ms, expires_at_ms, approvals); - let signatories = spec.signatories; + let proposal_value = MultisigProposalValue::new( + self.instructions, + now_ms, + expires_at_ms, + BTreeSet::from([proposer]), + None, + ); + let relay_value = |relay: MultisigApprove| { + MultisigProposalValue::new( + vec![relay.into()], + now_ms, + expires_at_ms, + BTreeSet::new(), + Some(false), + ) + }; + let approve_me = MultisigApprove::new(multisig_account.clone(), instructions_hash); // Recursively deploy multisig authentication down to the personal leaf signatories - for signatory in signatories.keys().cloned() { - let is_multisig_again = executor - .host() - .query(FindRoleIds) - .filter_with(|role_id| role_id.eq(multisig_role_for(&signatory))) - .execute_single_opt() - .dbg_unwrap() - .is_some(); - - if is_multisig_again { - let propose_to_approve_me = { - let approve_me = - MultisigApprove::new(multisig_account.clone(), instructions_hash); - - MultisigPropose::new( - signatory, - [approve_me.into()].to_vec(), - // Force override by the root proposal TTL - Some(self.transaction_ttl_ms.unwrap_or(spec.transaction_ttl_ms)), - ) - }; - propose_to_approve_me.visit_execute(executor); + for signatory in spec.signatories.keys().cloned() { + if is_multisig(&signatory, executor) { + deploy_relayer(signatory, approve_me.clone(), relay_value, executor)?; } } + // Authorize as the multisig account + executor.context_mut().authority = multisig_account.clone(); + visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( - multisig_account.clone(), - proposal_key(&instructions_hash).clone(), + multisig_account, + proposal_key(&instructions_hash), Json::new(&proposal_value), ))); @@ -130,6 +101,83 @@ impl VisitExecute for MultisigPropose { } } +fn deploy_relayer( + relayer: AccountId, + relay: MultisigApprove, + relay_value: impl Fn(MultisigApprove) -> MultisigProposalValue + Clone, + executor: &mut V, +) -> Result<(), ValidationFail> { + let spec = multisig_spec(relayer.clone(), executor).unwrap(); + + let relay_hash = HashOf::new(&vec![relay.clone().into()]); + let sub_relay = MultisigApprove::new(relayer.clone(), relay_hash); + + for signatory in spec.signatories.keys().cloned() { + if is_multisig(&signatory, executor) { + deploy_relayer(signatory, sub_relay.clone(), relay_value.clone(), executor)?; + } + } + + // Authorize as the relayer account + executor.context_mut().authority = relayer.clone(); + + visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( + relayer, + proposal_key(&relay_hash), + Json::new(relay_value(relay)), + ))); + + Ok(()) +} + +fn is_multisig(account: &AccountId, executor: &V) -> bool { + executor + .host() + .query(FindRoleIds) + .filter_with(|role_id| role_id.eq(multisig_role_for(account))) + .execute_single_opt() + .dbg_unwrap() + .is_some() +} + +fn multisig_spec( + multisig_account: AccountId, + executor: &V, +) -> Option { + executor + .host() + .query_single(FindAccountMetadata::new(multisig_account, spec_key())) + .ok() + .and_then(|json| json.try_into_any().ok()) +} + +fn proposal_value( + multisig_account: AccountId, + instructions_hash: HashOf>, + executor: &V, +) -> Option { + executor + .host() + .query_single(FindAccountMetadata::new( + multisig_account, + proposal_key(&instructions_hash), + )) + .ok() + .and_then(|json| json.try_into_any().ok()) +} + +fn now_ms(executor: &V) -> NonZeroU64 { + executor + .context() + .curr_block + .creation_time() + .as_millis() + .try_into() + .ok() + .and_then(NonZeroU64::new) + .dbg_expect("shouldn't overflow within 584942417 years") +} + impl VisitExecute for MultisigApprove { fn visit(&self, executor: &mut V) { let approver = executor.context().authority.clone(); @@ -147,13 +195,7 @@ impl VisitExecute for MultisigApprove { deny!(executor, "not qualified to approve multisig"); }; - if host - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - proposal_key(&instructions_hash), - )) - .is_err() - { + if proposal_value(multisig_account, instructions_hash, executor).is_none() { deny!(executor, "no proposals to approve") }; } @@ -164,42 +206,29 @@ impl VisitExecute for MultisigApprove { let instructions_hash = self.instructions_hash; // Check if the proposal is expired - prune_expired(multisig_account.clone(), instructions_hash, executor)?; - // Authorize as the multisig account - executor.context_mut().authority = multisig_account.clone(); + prune_expired(multisig_account.clone(), instructions_hash, executor)?; - let Some(mut proposal_value) = executor - .host() - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - proposal_key(&instructions_hash), - )) - .ok() - .and_then(|json| json.try_into_any::().ok()) + let Some(mut proposal_value) = + proposal_value(multisig_account.clone(), instructions_hash, executor) else { + // The proposal is pruned // TODO Notify that the proposal has expired, while returning Ok for the entry deletion to take effect return Ok(()); }; + if let Some(true) = proposal_value.is_relayed { + // The relaying approval already has executed + return Ok(()); + } proposal_value.approvals.insert(approver); - visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( multisig_account.clone(), proposal_key(&instructions_hash), Json::new(&proposal_value), ))); - let spec: MultisigSpec = executor - .host() - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - spec_key(), - )) - .dbg_unwrap() - .try_into_any() - .dbg_unwrap(); - + let spec = multisig_spec(multisig_account.clone(), executor).unwrap(); let is_authenticated = u16::from(spec.quorum) <= spec .signatories @@ -209,72 +238,90 @@ impl VisitExecute for MultisigApprove { .sum(); if is_authenticated { + match proposal_value.is_relayed { + None => { + // Cleanup the transaction entry + prune_down(multisig_account, instructions_hash, executor)?; + } + Some(false) => { + // Mark the relaying approval as executed + proposal_value.is_relayed = Some(true); + visit_seq!(executor.visit_set_account_key_value(&SetKeyValue::account( + multisig_account, + proposal_key(&instructions_hash), + proposal_value.clone(), + ))); + } + _ => unreachable!(), + } + for instruction in proposal_value.instructions { visit_seq!(executor.visit_instruction(&instruction)); } - - // Cleanup the transaction entry - visit_seq!( - executor.visit_remove_account_key_value(&RemoveKeyValue::account( - multisig_account.clone(), - proposal_key(&instructions_hash), - )) - ); } Ok(()) } } -/// Remove intermediate approvals and the root proposal if expired +/// Remove an expired proposal and relevant entries, switching the executor authority to this multisig account fn prune_expired( multisig_account: AccountId, instructions_hash: HashOf>, executor: &mut V, ) -> Result<(), ValidationFail> { - // Confirm entry existence - let Some(proposal_value) = executor - .host() - .query_single(FindAccountMetadata::new( - multisig_account.clone(), - proposal_key(&instructions_hash), - )) - .ok() - .and_then(|json| json.try_into_any::().ok()) - else { - // Removed by another path - return Ok(()); - }; - // Confirm expiration - let now_ms = executor - .context() - .curr_block - .creation_time() - .as_millis() - .try_into() - .ok() - .and_then(NonZeroU64::new) - .dbg_expect("shouldn't overflow within 584942417 years"); - if now_ms < proposal_value.expires_at_ms { + let proposal_value = proposal_value(multisig_account.clone(), instructions_hash, executor) + .expect("entry existence should be confirmed in advance"); + + if now_ms(executor) < proposal_value.expires_at_ms { + // Authorize as the multisig account + executor.context_mut().authority = multisig_account.clone(); return Ok(()); } - // Recurse through approvals + + // Go upstream to the root through approvals for instruction in proposal_value.instructions { if let InstructionBox::Custom(instruction) = instruction { if let Ok(MultisigInstructionBox::Approve(approve)) = instruction.payload().try_into() { - prune_expired(approve.account, approve.instructions_hash, executor)?; + return prune_expired(approve.account, approve.instructions_hash, executor); } } } + + // Go downstream, cleaning up relayers + prune_down(multisig_account, instructions_hash, executor) +} + +/// Remove an proposal and relevant entries, switching the executor authority to this multisig account +fn prune_down( + multisig_account: AccountId, + instructions_hash: HashOf>, + executor: &mut V, +) -> Result<(), ValidationFail> { + let spec = multisig_spec(multisig_account.clone(), executor).unwrap(); + // Authorize as the multisig account executor.context_mut().authority = multisig_account.clone(); - // Cleanup the transaction entry + visit_seq!( executor.visit_remove_account_key_value(&RemoveKeyValue::account( - multisig_account, + multisig_account.clone(), proposal_key(&instructions_hash), )) ); + for signatory in spec.signatories.keys().cloned() { + let relay_hash = { + let relay = MultisigApprove::new(multisig_account.clone(), instructions_hash); + HashOf::new(&vec![relay.into()]) + }; + if is_multisig(&signatory, executor) { + prune_down(signatory, relay_hash, executor)? + } + } + + // Restore the authority + executor.context_mut().authority = multisig_account; + Ok(()) } diff --git a/crates/iroha_executor_data_model/src/isi.rs b/crates/iroha_executor_data_model/src/isi.rs index a6b837ec2b5..1b48b2b500f 100644 --- a/crates/iroha_executor_data_model/src/isi.rs +++ b/crates/iroha_executor_data_model/src/isi.rs @@ -137,6 +137,8 @@ pub mod multisig { pub expires_at_ms: NonZeroU64, /// List of approvers of the proposal so far pub approvals: BTreeSet, + /// In case this proposal is some relaying approval, indicates if it has executed or not + pub is_relayed: Option, } impl From for Json { diff --git a/docs/source/references/schema.json b/docs/source/references/schema.json index e5f22436433..e60ca6ade2c 100644 --- a/docs/source/references/schema.json +++ b/docs/source/references/schema.json @@ -2647,6 +2647,10 @@ { "name": "approvals", "type": "SortedVec" + }, + { + "name": "is_relayed", + "type": "Option" } ] }, @@ -2842,6 +2846,9 @@ "Option": { "Option": "TriggerId" }, + "Option": { + "Option": "bool" + }, "Option": { "Option": "u32" }, @@ -5057,6 +5064,7 @@ ] }, "WasmSmartContract": "Vec", + "bool": "bool", "u16": { "Int": "FixedWidth" }, diff --git a/scripts/tests/multisig.recursion.sh b/scripts/tests/multisig.recursion.sh index 3fd2bcbfa75..041759adac8 100644 --- a/scripts/tests/multisig.recursion.sh +++ b/scripts/tests/multisig.recursion.sh @@ -46,7 +46,7 @@ WEIGHTS=($(yes 1 | head -n $N_SIGNATORIES)) # register a multisig account, namely msa12 MSA_12=$(gen_account_id "msa12") SIGS_12=(${SIGNATORIES[@]:1:2}) -./iroha multisig register --account $MSA_12 --signatories ${SIGS_12[*]} --weights 1 1 --quorum 2 +./iroha multisig register --account $MSA_12 --signatories ${SIGS_12[*]} --weights 1 1 --quorum 1 # register a multisig account, namely msa345 MSA_345=$(gen_account_id "msa345") @@ -56,7 +56,7 @@ SIGS_345=(${SIGNATORIES[@]:3:3}) # register a multisig account, namely msa12345 MSA_12345=$(gen_account_id "msa12345") SIGS_12345=($MSA_12 $MSA_345) -./iroha multisig register --account $MSA_12345 --signatories ${SIGS_12345[*]} --weights 1 1 --quorum 1 +./iroha multisig register --account $MSA_12345 --signatories ${SIGS_12345[*]} --weights 1 1 --quorum 2 # register a multisig account, namely msa012345 MSA_012345=$(gen_account_id "msa") @@ -75,26 +75,44 @@ get_target_account() { ./iroha account list filter '{"Atom": {"Id": {"Equals": "'$MSA_012345'"}}}' } +# check that the root proposal is entered +LIST_0_INIT=$(get_list_as_signatory 0) +echo "$LIST_0_INIT" | jq '.[].instructions' | diff - <(cat $INSTRUCTIONS) + # check that one of the leaf signatories is involved -LIST_BEFORE=$(get_list_as_signatory 5) -echo "$LIST_BEFORE" | jq '.[].instructions' | diff - <(cat $INSTRUCTIONS) +LIST_2_INIT=$(get_list_as_signatory 2) +echo "$LIST_2_INIT" | jq '.[].instructions' | diff - <(cat $INSTRUCTIONS) + +LIST_5_INIT=$(get_list_as_signatory 5) +echo "$LIST_5_INIT" | jq '.[].instructions' | diff - <(cat $INSTRUCTIONS) # check that the multisig transaction has not yet executed -ACCOUNT_BEFORE=$(get_target_account) +ACCOUNT_0_INIT=$(get_target_account) # NOTE: without ` || false` this line passes even if `success_marker` exists -! echo "$ACCOUNT_BEFORE" | jq -e '.[0].metadata.success_marker' || false +! echo "$ACCOUNT_0_INIT" | jq -e '.[0].metadata.success_marker' || false + +# approve a relaying approval +HASH_TO_12345=$(echo "$LIST_2_INIT" | jq -r 'keys[0]') +./iroha --config "client.2.toml" multisig approve --account $MSA_12 --instructions-hash $HASH_TO_12345 + +# check that the relaying approval has passed but the whole entry stays in the list +LIST_2_RELAYED=$(get_list_as_signatory 2) +echo "$LIST_2_RELAYED" | jq '.[].instructions' | diff - <(cat $INSTRUCTIONS) + +# give the last approval to execute +./iroha --config "client.5.toml" multisig approve --account $MSA_345 --instructions-hash $HASH_TO_12345 -# approve the multisig transaction -LEAF_INSTRUCTIONS_HASH=$(echo "$LIST_BEFORE" | jq -r 'keys[0]') -./iroha --config "client.5.toml" multisig approve --account $MSA_345 --instructions-hash $LEAF_INSTRUCTIONS_HASH +# check that the transaction entry is deleted when seen from the last approver +LIST_5_EXECUTED=$(get_list_as_signatory 5) +! echo "$LIST_5_EXECUTED" | jq -e '.[].instructions' || false -# check that the transaction entry is deleted -LIST_AFTER=$(get_list_as_signatory 5) -! echo "$LIST_AFTER" | jq -e '.[].instructions' || false +# check that the transaction entry is deleted when seen from another signatory +LIST_2_EXECUTED=$(get_list_as_signatory 2) +! echo "$LIST_2_EXECUTED" | jq -e '.[].instructions' || false # check that the multisig transaction has executed -ACCOUNT_AFTER=$(get_target_account) -echo "$ACCOUNT_AFTER" | jq -e '.[0].metadata.success_marker' +ACCOUNT_0_EXECUTED=$(get_target_account) +echo "$ACCOUNT_0_EXECUTED" | jq -e '.[0].metadata.success_marker' cd - scripts/test_env.py cleanup From a63c87deceeac83c3670d2e5d15890e296759637 Mon Sep 17 00:00:00 2001 From: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> Date: Sat, 16 Nov 2024 22:22:54 +0900 Subject: [PATCH 07/11] docs(multisig): minor fixes Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> --- crates/iroha_executor/src/default/isi/multisig/account.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/iroha_executor/src/default/isi/multisig/account.rs b/crates/iroha_executor/src/default/isi/multisig/account.rs index c8f785c26c4..13f9b2b6a68 100644 --- a/crates/iroha_executor/src/default/isi/multisig/account.rs +++ b/crates/iroha_executor/src/default/isi/multisig/account.rs @@ -15,7 +15,7 @@ impl VisitExecute for MultisigRegister { }; // The multisig registrant needs to have sufficient permission to register personal accounts - // TODO Loosen to just being one of the signatories? But impose the procedure of propose and approve? + // TODO Relax the requirement to just being one of the signatories? But impose a proposal and approval procedure? visit_seq!(executor.visit_register_account(&Register::account( Account::new(multisig_account.clone()).with_metadata(metadata) ))); From bf77524420ba590fdb997e9e930e8074b33b306f Mon Sep 17 00:00:00 2001 From: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> Date: Sat, 16 Nov 2024 23:49:30 +0900 Subject: [PATCH 08/11] fix(multisig): question existence and format of metadata Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> --- .../src/default/isi/multisig/transaction.rs | 67 ++++++++++--------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/crates/iroha_executor/src/default/isi/multisig/transaction.rs b/crates/iroha_executor/src/default/isi/multisig/transaction.rs index 7e8154d7d2f..670c83f201b 100644 --- a/crates/iroha_executor/src/default/isi/multisig/transaction.rs +++ b/crates/iroha_executor/src/default/isi/multisig/transaction.rs @@ -3,6 +3,8 @@ use alloc::{collections::btree_set::BTreeSet, vec}; use core::num::NonZeroU64; +use iroha_smart_contract::data_model::query::error::QueryExecutionFail; + use super::*; impl VisitExecute for MultisigPropose { @@ -11,22 +13,18 @@ impl VisitExecute for MultisigPropose { let proposer = executor.context().authority.clone(); let multisig_account = self.account.clone(); let instructions_hash = HashOf::new(&self.instructions); - let multisig_role = multisig_role_for(&multisig_account); - let Some(multisig_spec) = multisig_spec(multisig_account.clone(), executor) else { - deny!(executor, "multisig spec not found or malformed"); + let multisig_spec = match multisig_spec(multisig_account.clone(), executor) { + Ok(spec) => spec, + Err(err) => deny!(executor, err), }; - let is_downward_proposal = host - .query_single(FindAccountMetadata::new(proposer.clone(), spec_key())) - .map_or(false, |json| { - json.try_into_any::() - .dbg_unwrap() - .signatories - .contains_key(&multisig_account) - }); + .query(FindRolesByAccountId::new(multisig_account.clone())) + .filter_with(|role_id| role_id.eq(multisig_role_for(&proposer))) + .execute_single() + .is_ok(); let has_multisig_role = host .query(FindRolesByAccountId::new(proposer)) - .filter_with(|role_id| role_id.eq(multisig_role)) + .filter_with(|role_id| role_id.eq(multisig_role_for(&multisig_account))) .execute_single() .is_ok(); let has_not_longer_ttl = self.transaction_ttl_ms.map_or(true, |override_ttl_ms| { @@ -43,7 +41,7 @@ impl VisitExecute for MultisigPropose { if host .query_single(FindAccountMetadata::new( - multisig_account.clone(), + multisig_account, proposal_key(&instructions_hash), )) .is_ok() @@ -56,7 +54,7 @@ impl VisitExecute for MultisigPropose { let proposer = executor.context().authority.clone(); let multisig_account = self.account; let instructions_hash = HashOf::new(&self.instructions); - let spec = multisig_spec(multisig_account.clone(), executor).unwrap(); + let spec = multisig_spec(multisig_account.clone(), executor)?; let now_ms = now_ms(executor); let expires_at_ms = { @@ -107,7 +105,7 @@ fn deploy_relayer( relay_value: impl Fn(MultisigApprove) -> MultisigProposalValue + Clone, executor: &mut V, ) -> Result<(), ValidationFail> { - let spec = multisig_spec(relayer.clone(), executor).unwrap(); + let spec = multisig_spec(relayer.clone(), executor)?; let relay_hash = HashOf::new(&vec![relay.clone().into()]); let sub_relay = MultisigApprove::new(relayer.clone(), relay_hash); @@ -143,27 +141,27 @@ fn is_multisig(account: &AccountId, executor: &V) - fn multisig_spec( multisig_account: AccountId, executor: &V, -) -> Option { +) -> Result { executor .host() - .query_single(FindAccountMetadata::new(multisig_account, spec_key())) - .ok() - .and_then(|json| json.try_into_any().ok()) + .query_single(FindAccountMetadata::new(multisig_account, spec_key()))? + .try_into_any() + .map_err(metadata_conversion_error) } fn proposal_value( multisig_account: AccountId, instructions_hash: HashOf>, executor: &V, -) -> Option { +) -> Result { executor .host() .query_single(FindAccountMetadata::new( multisig_account, proposal_key(&instructions_hash), - )) - .ok() - .and_then(|json| json.try_into_any().ok()) + ))? + .try_into_any() + .map_err(metadata_conversion_error) } fn now_ms(executor: &V) -> NonZeroU64 { @@ -184,19 +182,18 @@ impl VisitExecute for MultisigApprove { let multisig_account = self.account.clone(); let host = executor.host(); let instructions_hash = self.instructions_hash; - let multisig_role = multisig_role_for(&multisig_account); if host .query(FindRolesByAccountId::new(approver)) - .filter_with(|role_id| role_id.eq(multisig_role)) + .filter_with(|role_id| role_id.eq(multisig_role_for(&multisig_account))) .execute_single() .is_err() { deny!(executor, "not qualified to approve multisig"); }; - if proposal_value(multisig_account, instructions_hash, executor).is_none() { - deny!(executor, "no proposals to approve") + if let Err(err) = proposal_value(multisig_account, instructions_hash, executor) { + deny!(executor, err) }; } @@ -209,7 +206,7 @@ impl VisitExecute for MultisigApprove { // Authorize as the multisig account prune_expired(multisig_account.clone(), instructions_hash, executor)?; - let Some(mut proposal_value) = + let Ok(mut proposal_value) = proposal_value(multisig_account.clone(), instructions_hash, executor) else { // The proposal is pruned @@ -228,7 +225,7 @@ impl VisitExecute for MultisigApprove { Json::new(&proposal_value), ))); - let spec = multisig_spec(multisig_account.clone(), executor).unwrap(); + let spec = multisig_spec(multisig_account.clone(), executor)?; let is_authenticated = u16::from(spec.quorum) <= spec .signatories @@ -270,8 +267,7 @@ fn prune_expired( instructions_hash: HashOf>, executor: &mut V, ) -> Result<(), ValidationFail> { - let proposal_value = proposal_value(multisig_account.clone(), instructions_hash, executor) - .expect("entry existence should be confirmed in advance"); + let proposal_value = proposal_value(multisig_account.clone(), instructions_hash, executor)?; if now_ms(executor) < proposal_value.expires_at_ms { // Authorize as the multisig account @@ -298,7 +294,7 @@ fn prune_down( instructions_hash: HashOf>, executor: &mut V, ) -> Result<(), ValidationFail> { - let spec = multisig_spec(multisig_account.clone(), executor).unwrap(); + let spec = multisig_spec(multisig_account.clone(), executor)?; // Authorize as the multisig account executor.context_mut().authority = multisig_account.clone(); @@ -325,3 +321,10 @@ fn prune_down( Ok(()) } + +#[expect(clippy::needless_pass_by_value)] +fn metadata_conversion_error(err: serde_json::Error) -> ValidationFail { + ValidationFail::QueryFailed(QueryExecutionFail::Conversion(format!( + "multisig account metadata malformed:\n{err}" + ))) +} From 34a892aa8d3790b877d1e446c276746b4e74d40d Mon Sep 17 00:00:00 2001 From: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> Date: Sun, 17 Nov 2024 00:03:39 +0900 Subject: [PATCH 09/11] review: issue `Log` instruction when proposal is expired Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> --- .../iroha_executor/src/default/isi/multisig/transaction.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/iroha_executor/src/default/isi/multisig/transaction.rs b/crates/iroha_executor/src/default/isi/multisig/transaction.rs index 670c83f201b..bd93abc356e 100644 --- a/crates/iroha_executor/src/default/isi/multisig/transaction.rs +++ b/crates/iroha_executor/src/default/isi/multisig/transaction.rs @@ -6,6 +6,7 @@ use core::num::NonZeroU64; use iroha_smart_contract::data_model::query::error::QueryExecutionFail; use super::*; +use crate::data_model::Level; impl VisitExecute for MultisigPropose { fn visit(&self, executor: &mut V) { @@ -210,7 +211,9 @@ impl VisitExecute for MultisigApprove { proposal_value(multisig_account.clone(), instructions_hash, executor) else { // The proposal is pruned - // TODO Notify that the proposal has expired, while returning Ok for the entry deletion to take effect + // Notify that the proposal has expired, while returning Ok for the entry deletion to take effect + let log = Log::new(Level::INFO, format!("multisig proposal expired:\naccount: {multisig_account}\ninstructions hash: {instructions_hash}")); + visit_seq!(executor.visit_log(&log)); return Ok(()); }; if let Some(true) = proposal_value.is_relayed { From 43efa3ead7311d24089e64dbaef31425f36eb424 Mon Sep 17 00:00:00 2001 From: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> Date: Sun, 17 Nov 2024 00:19:34 +0900 Subject: [PATCH 10/11] review: better diagnostics than `assert!` Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> --- crates/iroha/tests/multisig.rs | 68 +++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 18 deletions(-) diff --git a/crates/iroha/tests/multisig.rs b/crates/iroha/tests/multisig.rs index 97a600ab1ad..0e8f36e17d2 100644 --- a/crates/iroha/tests/multisig.rs +++ b/crates/iroha/tests/multisig.rs @@ -238,8 +238,12 @@ fn multisig_base(suite: TestSuite) -> Result<()> { let approver = approvers.next().unwrap(); let res = alt_client(approver, &test_client).submit_blocking(approve.clone()); match &transaction_ttl_ms_opt { - None => assert!(res.is_ok()), - _ => assert!(res.is_err()), + None => { + res.unwrap(); + } + _ => { + let _err = res.unwrap_err(); + } } } @@ -255,15 +259,23 @@ fn multisig_base(suite: TestSuite) -> Result<()> { let approver = approvers.next().unwrap(); let res = alt_client(approver, &test_client).submit_blocking(approve.clone()); match (&transaction_ttl_ms_opt, &unauthorized_target_opt) { - (None, None) => assert!(res.is_ok()), - _ => assert!(res.is_err()), + (None, None) => { + res.unwrap(); + } + _ => { + let _err = res.unwrap_err(); + } } // Check if the multisig transaction has executed let res = test_client.query_single(FindAccountMetadata::new(transaction_target, key.clone())); match (&transaction_ttl_ms_opt, &unauthorized_target_opt) { - (None, None) => assert!(res.is_ok()), - _ => assert!(res.is_err()), + (None, None) => { + res.unwrap(); + } + _ => { + let _err = res.unwrap_err(); + } } // Check if the transaction entry is deleted @@ -274,9 +286,13 @@ fn multisig_base(suite: TestSuite) -> Result<()> { .unwrap(), )); match (&transaction_ttl_ms_opt, &unauthorized_target_opt) { - // In case failing validation, the entry can exit only by expiring - (None, Some(_)) => assert!(res.is_ok()), - _ => assert!(res.is_err()), + (None, Some(_)) => { + // In case failing validation, the entry can exit only by expiring + res.unwrap(); + } + _ => { + let _err = res.unwrap_err(); + } } Ok(()) @@ -430,8 +446,12 @@ fn multisig_recursion_base(suite: TestSuite) -> Result<()> { let (approver, approve) = approvals_iter.next().unwrap(); let res = alt_client(approver, &test_client).submit_blocking(approve.clone()); match &transaction_ttl_ms_opt { - None => assert!(res.is_ok()), - _ => assert!(res.is_err()), + None => { + res.unwrap(); + } + _ => { + let _err = res.unwrap_err(); + } } } @@ -447,15 +467,23 @@ fn multisig_recursion_base(suite: TestSuite) -> Result<()> { let (approver, approve) = approvals_iter.next().unwrap(); let res = alt_client(approver, &test_client).submit_blocking(approve.clone()); match (&transaction_ttl_ms_opt, &unauthorized_target_opt) { - (None, None) => assert!(res.is_ok()), - _ => assert!(res.is_err()), + (None, None) => { + res.unwrap(); + } + _ => { + let _err = res.unwrap_err(); + } } // Check if the multisig transaction has executed let res = test_client.query_single(FindAccountMetadata::new(transaction_target, key.clone())); match (&transaction_ttl_ms_opt, &unauthorized_target_opt) { - (None, None) => assert!(res.is_ok()), - _ => assert!(res.is_err()), + (None, None) => { + res.unwrap(); + } + _ => { + let _err = res.unwrap_err(); + } } // Check if the transaction entries are deleted @@ -470,9 +498,13 @@ fn multisig_recursion_base(suite: TestSuite) -> Result<()> { format!("multisig/proposals/{mst_hash}").parse().unwrap(), )); match (&transaction_ttl_ms_opt, &unauthorized_target_opt) { - // In case the root proposal is failing validation, the relevant entries can exit only by expiring - (None, Some(_)) => assert!(res.is_ok()), - _ => assert!(res.is_err()), + (None, Some(_)) => { + // In case the root proposal is failing validation, the relevant entries can exit only by expiring + res.unwrap(); + } + _ => { + let _err = res.unwrap_err(); + } } } From ebe8c207e9e852e6756b24b7282441ae18c0908a Mon Sep 17 00:00:00 2001 From: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> Date: Sun, 17 Nov 2024 00:32:55 +0900 Subject: [PATCH 11/11] review: early `deny!` Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com> --- crates/iroha_executor/src/default/mod.rs | 62 ++++++++++++------------ 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/crates/iroha_executor/src/default/mod.rs b/crates/iroha_executor/src/default/mod.rs index 7a8479a8ee3..09f700390df 100644 --- a/crates/iroha_executor/src/default/mod.rs +++ b/crates/iroha_executor/src/default/mod.rs @@ -1254,51 +1254,49 @@ pub mod role { } if role.id().name().as_ref().starts_with(MULTISIG_SIGNATORY) { - if let Some(multisig_account) = multisig_account_from(role.id()) { - if is_domain_owner( - multisig_account.domain(), - &executor.context().authority, - executor.host(), - ) - .unwrap_or_default() - { - let isi = &Register::role(new_role); - if let Err(err) = executor.host().submit(isi) { - deny!(executor, err); - } - execute!(executor, grant_role); + let Some(multisig_account) = multisig_account_from(role.id()) else { + deny!(executor, "violates multisig role name format") + }; + if is_domain_owner( + multisig_account.domain(), + &executor.context().authority, + executor.host(), + ) + .unwrap_or_default() + { + let isi = &Register::role(new_role); + if let Err(err) = executor.host().submit(isi) { + deny!(executor, err); } - deny!( - executor, - "only the domain owner can register multisig roles" - ) + execute!(executor, grant_role); } - deny!(executor, "violates multisig role name format") + deny!( + executor, + "only the domain owner can register multisig roles" + ) } } for permission in role.inner().permissions() { iroha_smart_contract::log::debug!(&format!("Checking `{permission:?}`")); - if let Ok(any_permission) = AnyPermission::try_from(permission) { - if !executor.context().curr_block.is_genesis() { - if let Err(error) = crate::permission::ValidateGrantRevoke::validate_grant( - &any_permission, - role.grant_to(), - executor.context(), - executor.host(), - ) { - deny!(executor, error); - } - } - - new_role = new_role.add_permission(any_permission); - } else { + let Ok(any_permission) = AnyPermission::try_from(permission) else { deny!( executor, ValidationFail::NotPermitted(format!("{permission:?}: Unknown permission")) ); + }; + if !executor.context().curr_block.is_genesis() { + if let Err(error) = crate::permission::ValidateGrantRevoke::validate_grant( + &any_permission, + role.grant_to(), + executor.context(), + executor.host(), + ) { + deny!(executor, error); + } } + new_role = new_role.add_permission(any_permission); } if executor.context().curr_block.is_genesis()