fix: propagate executor verdict

Signed-off-by: Shunkichi Sato <[email protected]>

crates/iroha/tests/multisig.rs

@@ -1,8 +1,10 @@
 use std::{
     collections::{BTreeMap, BTreeSet},
     time::Duration,
+    u64,
 };
 
+use derive_more::Constructor;
 use eyre::Result;
 use iroha::{
     client::Client,
@@ -16,14 +18,55 @@ use iroha_test_samples::{
     gen_account_in, ALICE_ID, BOB_ID, BOB_KEYPAIR, CARPENTER_ID, CARPENTER_KEYPAIR,
 };
 
+#[derive(Constructor)]
+struct TestSuite {
+    domain: DomainId,
+    multisig_account_id: AccountId,
+    unauthorized_target_opt: Option<AccountId>,
+    transaction_ttl_ms_opt: Option<u64>,
+}
+
 #[test]
-fn multisig() -> Result<()> {
-    multisig_base(None)
+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)
+}
+
+#[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());
+
+    let suite = TestSuite::new(domain, multisig_account_id, unauthorized_target_opt, None);
+    multisig_base(suite)
 }
 
 #[test]
 fn multisig_expires() -> Result<()> {
-    multisig_base(Some(2))
+    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)
 }
 
 /// # Scenario
@@ -34,23 +77,28 @@ fn multisig_expires() -> Result<()> {
 /// 4. Other signatories approve the multisig transaction
 /// 5. When the quorum is met, if it is not expired, the multisig transaction executes
 #[expect(clippy::cast_possible_truncation)]
-fn multisig_base(transaction_ttl_ms: Option<u64>) -> Result<()> {
+fn multisig_base(suite: TestSuite) -> Result<()> {
     const N_SIGNATORIES: usize = 5;
 
+    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();
 
-    let kingdom: DomainId = "kingdom".parse().unwrap();
-
     // Assume some domain registered after genesis
     let register_and_transfer_kingdom: [InstructionBox; 2] = [
-        Register::domain(Domain::new(kingdom.clone())).into(),
-        Transfer::domain(ALICE_ID.clone(), kingdom.clone(), BOB_ID.clone()).into(),
+        Register::domain(Domain::new(domain.clone())).into(),
+        Transfer::domain(ALICE_ID.clone(), domain.clone(), BOB_ID.clone()).into(),
     ];
     test_client.submit_all_blocking(register_and_transfer_kingdom)?;
 
     // Populate residents in the domain
-    let mut residents = core::iter::repeat_with(|| gen_account_in(&kingdom))
+    let mut residents = core::iter::repeat_with(|| gen_account_in(&domain))
         .take(1 + N_SIGNATORIES)
         .collect::<BTreeMap<AccountId, KeyPair>>();
     alt_client((BOB_ID.clone(), BOB_KEYPAIR.clone()), &test_client).submit_all_blocking(
@@ -61,13 +109,6 @@ fn multisig_base(transaction_ttl_ms: Option<u64>) -> Result<()> {
             .map(Register::account),
     )?;
 
-    // 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(&kingdom).0;
-
-    // DEBUG: You could target unauthorized one (e.g. Alice) to fail
-    let transaction_target = multisig_account_id.clone();
-
     let not_signatory = residents.pop_first().unwrap();
     let mut signatories = residents;
 
@@ -80,7 +121,7 @@ fn multisig_base(transaction_ttl_ms: Option<u64>) -> Result<()> {
             .collect(),
         // Quorum can be reached without the first signatory
         (1..=N_SIGNATORIES).skip(1).sum::<usize>() as u16,
-        transaction_ttl_ms.unwrap_or(u64::MAX),
+        transaction_ttl_ms_opt.unwrap_or(u64::MAX),
     );
 
     // Any account in another domain cannot register a multisig account without special permission
@@ -104,6 +145,10 @@ fn multisig_base(transaction_ttl_ms: Option<u64>) -> Result<()> {
         .expect("multisig account should be created");
 
     let key: Name = "success_marker".parse().unwrap();
+    let transaction_target = unauthorized_target_opt
+        .as_ref()
+        .unwrap_or(&multisig_account_id)
+        .clone();
     let instructions = vec![SetKeyValue::account(
         transaction_target.clone(),
         key.clone(),
@@ -129,7 +174,7 @@ fn multisig_base(transaction_ttl_ms: Option<u64>) -> Result<()> {
         .expect_err("instructions shouldn't execute without enough approvals");
 
     // Allow time to elapse to test the expiration
-    if let Some(ms) = transaction_ttl_ms {
+    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()))?;
@@ -141,17 +186,28 @@ fn multisig_base(transaction_ttl_ms: Option<u64>) -> Result<()> {
     alt_client(approver, &test_client).submit_blocking(approve.clone())?;
 
     // Subsequent approvals should succeed unless the proposal is expired
-    for approver in approvers {
-        match alt_client(approver, &test_client).submit_blocking(approve.clone()) {
-            Ok(_hash) => assert!(transaction_ttl_ms.is_none()),
-            Err(_err) => assert!(transaction_ttl_ms.is_some()),
+    for _ in 0..(N_SIGNATORIES - 4) {
+        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()),
         }
     }
 
+    // The last approve to proceed to validate and execute the instructions
+    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()),
+    }
+
     // Check if the multisig transaction has executed
-    match test_client.query_single(FindAccountMetadata::new(transaction_target, key.clone())) {
-        Ok(_value) => assert!(transaction_ttl_ms.is_none()),
-        Err(_err) => assert!(transaction_ttl_ms.is_some()),
+    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()),
     }
 
     Ok(())

wasm/libs/default_executor/src/lib.rs

@@ -68,7 +68,7 @@ trait VisitExecute: Instruction {
         unimplemented!("should be overridden unless `Self::visit_execute` is overridden")
     }
 
-    fn execute(self, _executor: &Executor) -> Result<(), ValidationFail> {
+    fn execute(self, _executor: &mut Executor) -> Result<(), ValidationFail> {
         unimplemented!("should be overridden unless `Self::visit_execute` is overridden")
     }
 }

wasm/libs/default_executor/src/multisig/account.rs

@@ -42,7 +42,7 @@ impl VisitExecute for MultisigRegister {
         }
     }
 
-    fn execute(self, executor: &Executor) -> Result<(), ValidationFail> {
+    fn execute(self, executor: &mut Executor) -> Result<(), ValidationFail> {
         let host = executor.host();
         let registrant = executor.context().authority.clone();
         let multisig_account = self.account;

wasm/libs/default_executor/src/multisig/transaction.rs

@@ -14,27 +14,14 @@ impl VisitExecute for MultisigPropose {
         let target_account = self.account.clone();
         let multisig_role = multisig_role_for(&target_account);
         let instructions_hash = HashOf::new(&self.instructions);
-        let is_top_down_proposal = host
-            .query_single(FindAccountMetadata::new(
-                proposer.clone(),
-                SIGNATORIES.parse().unwrap(),
-            ))
-            .map_or(false, |proposer_signatories| {
-                proposer_signatories
-                    .try_into_any::<BTreeMap<AccountId, u8>>()
-                    .dbg_unwrap()
-                    .contains_key(&target_account)
-            });
 
-        if !is_top_down_proposal {
-            let Ok(_role_found) = host
-                .query(FindRolesByAccountId::new(proposer))
-                .filter_with(|role_id| role_id.eq(multisig_role))
-                .execute_single()
-            else {
-                deny!(executor, "not qualified to propose multisig");
-            };
-        }
+        let Ok(_role_found) = host
+            .query(FindRolesByAccountId::new(proposer))
+            .filter_with(|role_id| role_id.eq(multisig_role))
+            .execute_single()
+        else {
+            deny!(executor, "not qualified to propose multisig");
+        };
 
         let Err(_proposal_not_found) = host.query_single(FindAccountMetadata::new(
             target_account.clone(),
@@ -44,7 +31,7 @@ impl VisitExecute for MultisigPropose {
         };
     }
 
-    fn execute(self, executor: &Executor) -> Result<(), ValidationFail> {
+    fn execute(self, executor: &mut Executor) -> Result<(), ValidationFail> {
         let host = executor.host().clone();
         let proposer = executor.context().authority.clone();
         let target_account = self.account;
@@ -74,9 +61,13 @@ impl VisitExecute for MultisigPropose {
 
                     MultisigPropose::new(signatory, [approve_me.into()].to_vec())
                 };
-                let mut executor = executor.clone();
+                let init_authority = executor.context().authority.clone();
+                // Authorize as the multisig account
                 executor.context_mut().authority = target_account.clone();
-                propose_to_approve_me.execute(&executor)?;
+                propose_to_approve_me
+                    .execute(executor)
+                    .dbg_expect("top-down proposals shouldn't fail");
+                executor.context_mut().authority = init_authority;
             }
         }
 
@@ -139,7 +130,7 @@ impl VisitExecute for MultisigApprove {
         };
     }
 
-    fn execute(self, executor: &Executor) -> Result<(), ValidationFail> {
+    fn execute(self, executor: &mut Executor) -> Result<(), ValidationFail> {
         let host = executor.host().clone();
         let approver = executor.context().authority.clone();
         let target_account = self.account;
@@ -242,10 +233,11 @@ impl VisitExecute for MultisigApprove {
             if !is_expired {
                 // Validate and execute the authenticated multisig transaction
                 for instruction in instructions {
-                    // Create an instance per instruction to reset the context mutation
-                    let mut executor = executor.clone();
+                    let init_authority = executor.context().authority.clone();
+                    // Authorize as the multisig account
                     executor.context_mut().authority = target_account.clone();
-                    executor.visit_instruction(&instruction)
+                    executor.visit_instruction(&instruction);
+                    executor.context_mut().authority = init_authority;
                 }
             } else {
                 // TODO Notify that the proposal has expired, while returning Ok for the entry deletion to take effect