From 5054efbf741cdd1f7d1e8e9d153a793c17a0b3e7 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 29 Aug 2024 13:23:20 +0200 Subject: [PATCH 01/12] Fixes MASP tx expiration in the SDK --- crates/sdk/src/tx.rs | 10 +++++--- .../src/masp/shielded_wallet.rs | 25 +++++++++++-------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index ce5758d959..18ba58da91 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -2611,6 +2611,7 @@ pub async fn build_ibc_transfer( masp_transfer_data, masp_fee_data, !(args.tx.dry_run || args.tx.dry_run_wrapper), + args.tx.expiration.to_datetime(), ) .await?; let shielded_tx_epoch = shielded_parts.as_ref().map(|trans| trans.0.epoch); @@ -3071,6 +3072,7 @@ pub async fn build_shielded_transfer( transfer_data, masp_fee_data, !(args.tx.dry_run || args.tx.dry_run_wrapper), + args.tx.expiration.to_datetime(), ) .await? .expect("Shielded transfer must have shielded parts"); @@ -3236,6 +3238,7 @@ pub async fn build_shielding_transfer( transfer_data, None, !(args.tx.dry_run || args.tx.dry_run_wrapper), + args.tx.expiration.to_datetime(), ) .await? .expect("Shielding transfer must have shielded parts"); @@ -3357,6 +3360,7 @@ pub async fn build_unshielding_transfer( transfer_data, masp_fee_data, !(args.tx.dry_run || args.tx.dry_run_wrapper), + args.tx.expiration.to_datetime(), ) .await? .expect("Shielding transfer must have shielded parts"); @@ -3409,13 +3413,13 @@ async fn construct_shielded_parts( data: Vec, fee_data: Option, update_ctx: bool, + expiration: Option, ) -> Result)>> { // Precompute asset types to increase chances of success in decoding let token_map = context.wallet().await.get_addresses(); let tokens = token_map.values().collect(); let stx_result = { - let expiration = context.tx_builder().expiration.to_datetime(); let mut shielded = context.shielded_mut().await; _ = shielded .precompute_asset_types(context.client(), tokens) @@ -3775,7 +3779,6 @@ pub async fn gen_ibc_shielding_transfer( amount: validated_amount, }; let shielded_transfer = { - let expiration = context.tx_builder().expiration.to_datetime(); let mut shielded = context.shielded_mut().await; shielded .gen_shielded_transfer( @@ -3783,7 +3786,8 @@ pub async fn gen_ibc_shielding_transfer( vec![masp_transfer_data], // Fees are paid from the transparent balance of the relayer None, - expiration, + // Expiration can be set directly on the ibc transaction + None, true, ) .await diff --git a/crates/shielded_token/src/masp/shielded_wallet.rs b/crates/shielded_token/src/masp/shielded_wallet.rs index b51fb652de..a16b7644b7 100644 --- a/crates/shielded_token/src/masp/shielded_wallet.rs +++ b/crates/shielded_token/src/masp/shielded_wallet.rs @@ -910,14 +910,6 @@ pub trait ShieldedApi: expiration: Option, update_ctx: bool, ) -> Result, TransferErr> { - let last_block_height = Self::query_block(context.client()) - .await - .map_err(|e| TransferErr::General(e.to_string()))? - .unwrap_or(1); - let max_block_time = - Self::query_max_block_time_estimate(context.client()) - .await - .map_err(|e| TransferErr::General(e.to_string()))?; // Determine epoch in which to submit potential shielded transaction let epoch = Self::query_masp_epoch(context.client()) .await @@ -949,13 +941,24 @@ pub trait ShieldedApi: // TODO: if the user requested the default expiration, there might be a // small discrepancy between the datetime we calculate here and the one - // we set for the transaction. This should be small enough to not cause - // any issue, in case refactor this function to request the precise - // datetime to the caller + // we set for the transaction (since we compute two different + // DateTimeUtc::now()). This should be small enough to not cause + // any issue, in case refactor the build process to compute a single + // expiration at the beginning and use it both here and for the + // transaction let expiration_height: u32 = match expiration { Some(expiration) => { // Try to match a DateTime expiration with a plausible // corresponding block height + let last_block_height = Self::query_block(context.client()) + .await + .map_err(|e| TransferErr::General(e.to_string()))? + .unwrap_or(1); + let max_block_time = + Self::query_max_block_time_estimate(context.client()) + .await + .map_err(|e| TransferErr::General(e.to_string()))?; + #[allow(clippy::disallowed_methods)] let current_time = DateTimeUtc::now(); let delta_time = From e4bb265d4fbf6c88814f64b5725c08dd8118c6f3 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 30 Aug 2024 13:29:58 +0200 Subject: [PATCH 02/12] Adds integration test for masp expiration. Misc adjustments to `MockNode` --- crates/node/src/shell/testing/node.rs | 64 ++++++--- crates/tests/src/integration/masp.rs | 185 +++++++++++++++++++++++++- crates/tests/src/integration/setup.rs | 3 +- crates/tx/src/data/mod.rs | 19 ++- 4 files changed, 245 insertions(+), 26 deletions(-) diff --git a/crates/node/src/shell/testing/node.rs b/crates/node/src/shell/testing/node.rs index 1799e28d93..0cf4fe1ccc 100644 --- a/crates/node/src/shell/testing/node.rs +++ b/crates/node/src/shell/testing/node.rs @@ -38,7 +38,7 @@ use namada_sdk::tendermint::abci::types::VoteInfo; use namada_sdk::tendermint_proto::google::protobuf::Timestamp; use namada_sdk::time::DateTimeUtc; use namada_sdk::tx::data::ResultCode; -use namada_sdk::tx::event::Code as CodeAttr; +use namada_sdk::tx::event::{Batch as BatchAttr, Code as CodeAttr}; use namada_sdk::{ethereum_structs, governance}; use regex::Regex; use tokio::sync::mpsc; @@ -248,11 +248,13 @@ pub enum NodeResults { } // TODO: wrap `MockNode` in a single `Arc` +// FIXME: look at this todo, not sure it's correct though #[derive(Clone)] pub struct MockNode { pub shell: Arc>>, pub test_dir: Arc, - pub results: Arc>>, + pub tx_result_codes: Arc>>, + pub tx_results: Arc>>>, pub blocks: Arc>>, pub services: Arc, pub auto_drive_services: bool, @@ -505,9 +507,9 @@ impl MockNode { }; let resp = locked.finalize_block(req).expect("Test failed"); - let mut error_codes = resp + let mut result_codes = resp .events - .into_iter() + .iter() .map(|e| { let code = e .read_attribute_opt::() @@ -520,7 +522,16 @@ impl MockNode { } }) .collect::>(); - self.results.lock().unwrap().append(&mut error_codes); + let mut tx_results = resp + .events + .into_iter() + .map(|e| e.read_attribute::>().unwrap()) + .collect::>(); + self.tx_result_codes + .lock() + .unwrap() + .append(&mut result_codes); + self.tx_results.lock().unwrap().append(&mut tx_results); locked.commit(); // Cache the block @@ -600,7 +611,7 @@ impl MockNode { }) .collect(); if result != tendermint::abci::response::ProcessProposal::Accept { - self.results.lock().unwrap().append(&mut errors); + self.tx_result_codes.lock().unwrap().append(&mut errors); return; } @@ -644,7 +655,7 @@ impl MockNode { let resp = locked.finalize_block(req).unwrap(); let mut error_codes = resp .events - .into_iter() + .iter() .map(|e| { let code = e .read_attribute_opt::() @@ -657,7 +668,16 @@ impl MockNode { } }) .collect::>(); - self.results.lock().unwrap().append(&mut error_codes); + let mut txs_results = resp + .events + .into_iter() + .filter_map(|e| e.read_attribute_opt::>().unwrap()) + .collect::>(); + self.tx_result_codes + .lock() + .unwrap() + .append(&mut error_codes); + self.tx_results.lock().unwrap().append(&mut txs_results); self.blocks.lock().unwrap().insert( height, block::Response { @@ -704,23 +724,34 @@ impl MockNode { /// Check that applying a tx succeeded. pub fn success(&self) -> bool { - self.results + self.tx_result_codes .lock() .unwrap() .iter() .all(|r| *r == NodeResults::Ok) + && self + .tx_results + .lock() + .unwrap() + .iter() + .all(|inner_results| inner_results.are_results_successfull()) } /// Return a tx result if the tx failed in mempool pub fn is_broadcast_err(&self) -> Option { - self.results.lock().unwrap().iter().find_map(|r| match r { - NodeResults::Ok | NodeResults::Failed(_) => None, - NodeResults::Rejected(tx_result) => Some(tx_result.clone()), - }) + self.tx_result_codes + .lock() + .unwrap() + .iter() + .find_map(|r| match r { + NodeResults::Ok | NodeResults::Failed(_) => None, + NodeResults::Rejected(tx_result) => Some(tx_result.clone()), + }) } pub fn clear_results(&self) { - self.results.lock().unwrap().clear(); + self.tx_result_codes.lock().unwrap().clear(); + self.tx_results.lock().unwrap().clear(); } pub fn assert_success(&self) { @@ -728,11 +759,10 @@ impl MockNode { panic!( "Assert failed: The node did not execute \ successfully:\nErrors:\n {:?}", - self.results.lock().unwrap() + self.tx_result_codes.lock().unwrap() ); - } else { - self.clear_results(); } + self.clear_results(); } } diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 19d092dbb3..7d7175edff 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -11,6 +11,7 @@ use namada_node::shell::testing::node::NodeResults; use namada_node::shell::testing::utils::{Bin, CapturedOutput}; use namada_sdk::masp::fs::FsShieldedUtils; use namada_sdk::state::{StorageRead, StorageWrite}; +use namada_sdk::time::DateTimeUtc; use namada_sdk::token::storage_key::masp_token_map_key; use namada_sdk::token::{self, DenominatedAmount}; use namada_sdk::DEFAULT_GAS_LIMIT; @@ -1517,19 +1518,23 @@ fn multiple_unfetched_txs_same_block() -> Result<()> { node.clear_results(); node.submit_txs(txs); { - let results = node.results.lock().unwrap(); + let results = node.tx_result_codes.lock().unwrap(); // If empty than failed in process proposal assert!(!results.is_empty()); + // FIXME: can use is_success for result in results.iter() { assert!(matches!(result, NodeResults::Ok)); } } - // Finalize the next block to actually execute the decrypted txs + // FIXME: is this correct??? + // FIXME: maybe should assert the length of the result? Or maybe this is not + // needed naymore Finalize the next block to actually execute the + // decrypted txs node.clear_results(); node.finalize_and_commit(None); { - let results = node.results.lock().unwrap(); + let results = node.tx_result_codes.lock().unwrap(); for result in results.iter() { assert!(matches!(result, NodeResults::Ok)); } @@ -1538,6 +1543,180 @@ fn multiple_unfetched_txs_same_block() -> Result<()> { Ok(()) } +/// Tests that an expired masp tx is rejected by the vp +#[test] +fn expired_masp_tx() -> Result<()> { + // This address doesn't matter for tests. But an argument is required. + let validator_one_rpc = "http://127.0.0.1:26567"; + // Download the shielded pool parameters before starting node + let _ = FsShieldedUtils::new(PathBuf::new()); + let (mut node, _services) = setup::setup()?; + _ = node.next_epoch(); + + // Add the relevant viewing keys to the wallet otherwise the shielded + // context won't precache the masp data + run( + &node, + Bin::Wallet, + vec![ + "add", + "--alias", + "alias_a", + "--value", + AA_VIEWING_KEY, + "--unsafe-dont-encrypt", + ], + )?; + node.assert_success(); + + // 1. Shield tokens + _ = node.next_epoch(); + run( + &node, + Bin::Client, + vec![ + "shield", + "--source", + ALBERT_KEY, + "--target", + AA_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "100", + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + // sync shielded context + run( + &node, + Bin::Client, + vec!["shielded-sync", "--node", validator_one_rpc], + )?; + + // 2. Shielded operation to avoid the need of a signature on the inner tx. + // Dump the tx to than reload and submit + let tempdir = tempfile::tempdir().unwrap(); + + _ = node.next_epoch(); + run( + &node, + Bin::Client, + vec![ + "transfer", + "--source", + A_SPENDING_KEY, + "--target", + AC_PAYMENT_ADDRESS, + "--token", + NAM, + "--amount", + "50", + "--gas-payer", + CHRISTEL_KEY, + // This is used to set the correct expiration in the masp object, + // we don't care about the expiration at the tx level + "--expiration", + #[allow(clippy::disallowed_methods)] + &DateTimeUtc::now().to_string(), + "--output-folder-path", + tempdir.path().to_str().unwrap(), + "--dump-tx", + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + + let file_path = tempdir + .path() + .read_dir() + .unwrap() + .next() + .unwrap() + .unwrap() + .path(); + let tx_bytes = std::fs::read(&file_path).unwrap(); + std::fs::remove_file(&file_path).unwrap(); + + let sk = christel_keypair(); + let pk = sk.to_public(); + + let native_token = node + .shell + .lock() + .unwrap() + .state + .in_mem() + .native_token + .clone(); + let mut tx = namada_sdk::tx::Tx::deserialize(&tx_bytes).unwrap(); + // Remove the expiration field to avoid a failure because of it, we only + // want to check the expiration in the masp vp + tx.header.expiration = None; + tx.add_wrapper( + namada_sdk::tx::data::wrapper::Fee { + amount_per_gas_unit: DenominatedAmount::native(1.into()), + token: native_token.clone(), + }, + pk.clone(), + DEFAULT_GAS_LIMIT.into(), + ); + tx.sign_wrapper(sk.clone()); + let wrapper_hash = tx.wrapper_hash(); + let inner_cmt = tx.first_commitments().unwrap(); + + // Skip at least 20 blocks to ensure expiration (this is because of the + // default masp expiration) + for _ in 0..=20 { + node.finalize_and_commit(); + } + node.clear_results(); + node.submit_txs(vec![tx.to_bytes()]); + { + let codes = node.tx_result_codes.lock().unwrap(); + // If empty than failed in process proposal + assert!(!codes.is_empty()); + + for code in codes.iter() { + assert!(matches!(code, NodeResults::Ok)); + } + + let results = node.tx_results.lock().unwrap(); + // We submitted a single batch + assert_eq!(results.len(), 1); + + for result in results.iter() { + // The batch should contain a single inner tx + assert_eq!(result.0.len(), 1); + + let inner_tx_result = result + .get_inner_tx_result( + wrapper_hash.as_ref(), + itertools::Either::Right(inner_cmt), + ) + .expect("Missing expected tx result") + .as_ref() + .expect("Result is supposed to be Ok"); + + assert!( + inner_tx_result + .vps_result + .rejected_vps + .contains(&namada_sdk::address::MASP) + ); + assert!(inner_tx_result.vps_result.errors.contains(&( + namada_sdk::address::MASP, + "Native VP error: MASP transaction is expired".to_string() + ))); + } + } + + Ok(()) +} + // Test that a masp unshield transaction can be succesfully executed even across // an epoch boundary. #[test] diff --git a/crates/tests/src/integration/setup.rs b/crates/tests/src/integration/setup.rs index 69ddd908d1..e006319498 100644 --- a/crates/tests/src/integration/setup.rs +++ b/crates/tests/src/integration/setup.rs @@ -224,7 +224,8 @@ fn create_node( test_dir: ManuallyDrop::new(test_dir), }), services: Arc::new(services), - results: Arc::new(Mutex::new(vec![])), + tx_result_codes: Arc::new(Mutex::new(vec![])), + tx_results: Arc::new(Mutex::new(vec![])), blocks: Arc::new(Mutex::new(HashMap::new())), auto_drive_services, }; diff --git a/crates/tx/src/data/mod.rs b/crates/tx/src/data/mod.rs index c180d30537..84b2bf0b01 100644 --- a/crates/tx/src/data/mod.rs +++ b/crates/tx/src/data/mod.rs @@ -363,31 +363,40 @@ impl TxResult { self.0.iter() } - /// Return the length of the collecction of inner tx results. + /// Return the length of the collection of inner tx results. #[inline] pub fn len(&self) -> usize { self.0.len() } - /// Check if the collecction of inner tx results is empty. + /// Check if the collection of inner tx results is empty. #[inline] pub fn is_empty(&self) -> bool { self.0.is_empty() } - /// Check if the collecction of inner tx results contains no errors. + /// Check if all the inner txs in the collection have been successfully + /// applied. + #[inline] + pub fn are_results_successfull(&self) -> bool { + self.iter().all(|(_, res)| matches!(res, Ok(batched_result) if batched_result.is_accepted()) + ) + } + + // FIXME: check usage of this and maybe remove it + /// Check if the collection of inner tx results contains no errors. #[inline] pub fn are_results_ok(&self) -> bool { self.iter().all(|(_, res)| res.is_ok()) } - /// Check if the collecction of inner tx results contains any ok results. + /// Check if the collection of inner tx results contains any ok results. #[inline] pub fn are_any_ok(&self) -> bool { self.iter().any(|(_, res)| res.is_ok()) } - /// Check if the collecction of inner tx results contains any errors. + /// Check if the collection of inner tx results contains any errors. #[inline] pub fn are_any_err(&self) -> bool { self.iter().any(|(_, res)| res.is_err()) From b4f1a1bec11aa7cdd8fc575610a9c2a8684f61cf Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 30 Aug 2024 14:35:53 +0200 Subject: [PATCH 03/12] Removes double execution checks from integration tests. Improve assertions in integration tests --- crates/node/src/shell/testing/node.rs | 5 ++-- crates/tests/src/integration/ledger_tests.rs | 22 +++------------- crates/tests/src/integration/masp.rs | 27 +++----------------- 3 files changed, 10 insertions(+), 44 deletions(-) diff --git a/crates/node/src/shell/testing/node.rs b/crates/node/src/shell/testing/node.rs index 0cf4fe1ccc..c16444d309 100644 --- a/crates/node/src/shell/testing/node.rs +++ b/crates/node/src/shell/testing/node.rs @@ -758,8 +758,9 @@ impl MockNode { if !self.success() { panic!( "Assert failed: The node did not execute \ - successfully:\nErrors:\n {:?}", - self.tx_result_codes.lock().unwrap() + successfully:\nErrors:\n {:?},\nTxs results:\n {:?}", + self.tx_result_codes.lock().unwrap(), + self.tx_results.lock().unwrap() ); } self.clear_results(); diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index 3d73f4ee73..494e65fe01 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/crates/tests/src/integration/ledger_tests.rs @@ -16,7 +16,6 @@ use namada_core::hash::Hash; use namada_core::storage::{DbColFam, Key}; use namada_core::token::NATIVE_MAX_DECIMAL_PLACES; use namada_node::shell::testing::client::run; -use namada_node::shell::testing::node::NodeResults; use namada_node::shell::testing::utils::{Bin, CapturedOutput}; use namada_node::shell::SnapshotSync; use namada_node::storage::DbSnapshot; @@ -1807,24 +1806,9 @@ fn enforce_fee_payment() -> Result<()> { node.clear_results(); node.submit_txs(txs); - { - let results = node.results.lock().unwrap(); - // If empty than failed in process proposal - assert!(!results.is_empty()); - - for result in results.iter() { - assert!(matches!(result, NodeResults::Ok)); - } - } - // Finalize the next block to execute the txs - node.clear_results(); - node.finalize_and_commit(None); - { - let results = node.results.lock().unwrap(); - for result in results.iter() { - assert!(matches!(result, NodeResults::Ok)); - } - } + // If empty than failed in process proposal + assert!(!node.tx_result_codes.lock().unwrap().is_empty()); + node.assert_success(); // Assert balances let captured = CapturedOutput::of(|| { diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 7d7175edff..98e48404f2 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -1517,28 +1517,9 @@ fn multiple_unfetched_txs_same_block() -> Result<()> { node.clear_results(); node.submit_txs(txs); - { - let results = node.tx_result_codes.lock().unwrap(); - // If empty than failed in process proposal - assert!(!results.is_empty()); - - // FIXME: can use is_success - for result in results.iter() { - assert!(matches!(result, NodeResults::Ok)); - } - } - // FIXME: is this correct??? - // FIXME: maybe should assert the length of the result? Or maybe this is not - // needed naymore Finalize the next block to actually execute the - // decrypted txs - node.clear_results(); - node.finalize_and_commit(None); - { - let results = node.tx_result_codes.lock().unwrap(); - for result in results.iter() { - assert!(matches!(result, NodeResults::Ok)); - } - } + // If empty than failed in process proposal + assert!(!node.tx_result_codes.lock().unwrap().is_empty()); + node.assert_success(); Ok(()) } @@ -1671,7 +1652,7 @@ fn expired_masp_tx() -> Result<()> { // Skip at least 20 blocks to ensure expiration (this is because of the // default masp expiration) for _ in 0..=20 { - node.finalize_and_commit(); + node.finalize_and_commit(None); } node.clear_results(); node.submit_txs(vec![tx.to_bytes()]); From 4fb09ee7385ff1da08d5d198405b115c9733a2d1 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 30 Aug 2024 14:54:01 +0200 Subject: [PATCH 04/12] Removes wrong result method --- crates/node/src/shell/finalize_block.rs | 2 +- crates/tx/src/data/mod.rs | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 68e4ffe17c..676b641851 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -6090,7 +6090,7 @@ mod test_finalize_block { assert_eq!(tx_results.len(), 2); // all txs should have succeeded - assert!(tx_results.are_results_ok()); + assert!(tx_results.are_results_successfull()); } #[test] diff --git a/crates/tx/src/data/mod.rs b/crates/tx/src/data/mod.rs index 84b2bf0b01..3299924cdc 100644 --- a/crates/tx/src/data/mod.rs +++ b/crates/tx/src/data/mod.rs @@ -383,13 +383,6 @@ impl TxResult { ) } - // FIXME: check usage of this and maybe remove it - /// Check if the collection of inner tx results contains no errors. - #[inline] - pub fn are_results_ok(&self) -> bool { - self.iter().all(|(_, res)| res.is_ok()) - } - /// Check if the collection of inner tx results contains any ok results. #[inline] pub fn are_any_ok(&self) -> bool { From 25ed6ded8b5a8426bf73ee9e55f3d08d4a19698c Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 30 Aug 2024 15:14:58 +0200 Subject: [PATCH 05/12] Wraps `MockNode` into an `Arc` --- crates/node/src/shell/testing/node.rs | 28 +++++++++++++++++---------- crates/tests/src/integration/setup.rs | 24 +++++++++++------------ 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/crates/node/src/shell/testing/node.rs b/crates/node/src/shell/testing/node.rs index c16444d309..9622a9d53e 100644 --- a/crates/node/src/shell/testing/node.rs +++ b/crates/node/src/shell/testing/node.rs @@ -247,19 +247,27 @@ pub enum NodeResults { Failed(ResultCode), } -// TODO: wrap `MockNode` in a single `Arc` -// FIXME: look at this todo, not sure it's correct though -#[derive(Clone)] -pub struct MockNode { - pub shell: Arc>>, - pub test_dir: Arc, - pub tx_result_codes: Arc>>, - pub tx_results: Arc>>>, - pub blocks: Arc>>, - pub services: Arc, +pub struct InnerMockNode { + pub shell: Mutex>, + pub test_dir: SalvageableTestDir, + pub tx_result_codes: Mutex>, + pub tx_results: Mutex>>, + pub blocks: Mutex>, + pub services: MockServices, pub auto_drive_services: bool, } +#[derive(Clone)] +pub struct MockNode(pub Arc); + +impl Deref for MockNode { + type Target = Arc; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + pub struct SalvageableTestDir { pub test_dir: ManuallyDrop, pub keep_temp: bool, diff --git a/crates/tests/src/integration/setup.rs b/crates/tests/src/integration/setup.rs index e006319498..807c44892d 100644 --- a/crates/tests/src/integration/setup.rs +++ b/crates/tests/src/integration/setup.rs @@ -19,8 +19,8 @@ use namada_apps_lib::wallet::pre_genesis; use namada_core::chain::ChainIdPrefix; use namada_core::collections::HashMap; use namada_node::shell::testing::node::{ - mock_services, MockNode, MockServicesCfg, MockServicesController, - MockServicesPackage, SalvageableTestDir, + mock_services, InnerMockNode, MockNode, MockServicesCfg, + MockServicesController, MockServicesPackage, SalvageableTestDir, }; use namada_node::shell::testing::utils::TestDir; use namada_node::shell::Shell; @@ -202,8 +202,8 @@ fn create_node( shell_handlers, controller, } = mock_services(services_cfg); - let node = MockNode { - shell: Arc::new(Mutex::new(Shell::new( + let node = MockNode(Arc::new(InnerMockNode { + shell: Mutex::new(Shell::new( config::Ledger::new( global_args.base_dir, chain_id.clone(), @@ -218,17 +218,17 @@ fn create_node( None, 50 * 1024 * 1024, // 50 kiB 50 * 1024 * 1024, // 50 kiB - ))), - test_dir: Arc::new(SalvageableTestDir { + )), + test_dir: SalvageableTestDir { keep_temp, test_dir: ManuallyDrop::new(test_dir), - }), - services: Arc::new(services), - tx_result_codes: Arc::new(Mutex::new(vec![])), - tx_results: Arc::new(Mutex::new(vec![])), - blocks: Arc::new(Mutex::new(HashMap::new())), + }, + services, + tx_result_codes: Mutex::new(vec![]), + tx_results: Mutex::new(vec![]), + blocks: Mutex::new(HashMap::new()), auto_drive_services, - }; + })); let init_req = namada_apps_lib::tendermint::abci::request::InitChain { time: Timestamp { From 58cf3387aafc463a1d790aeafdb5af9df1915b49 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 30 Aug 2024 16:44:34 +0200 Subject: [PATCH 06/12] Fixes integration tests --- crates/node/src/shell/testing/node.rs | 2 +- crates/tests/src/integration/ledger_tests.rs | 29 +++++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/crates/node/src/shell/testing/node.rs b/crates/node/src/shell/testing/node.rs index 9622a9d53e..8122b49574 100644 --- a/crates/node/src/shell/testing/node.rs +++ b/crates/node/src/shell/testing/node.rs @@ -533,7 +533,7 @@ impl MockNode { let mut tx_results = resp .events .into_iter() - .map(|e| e.read_attribute::>().unwrap()) + .filter_map(|e| e.read_attribute_opt::>().unwrap()) .collect::>(); self.tx_result_codes .lock() diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index 494e65fe01..e5c57bb1d1 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/crates/tests/src/integration/ledger_tests.rs @@ -16,6 +16,7 @@ use namada_core::hash::Hash; use namada_core::storage::{DbColFam, Key}; use namada_core::token::NATIVE_MAX_DECIMAL_PLACES; use namada_node::shell::testing::client::run; +use namada_node::shell::testing::node::NodeResults; use namada_node::shell::testing::utils::{Bin, CapturedOutput}; use namada_node::shell::SnapshotSync; use namada_node::storage::DbSnapshot; @@ -1807,8 +1808,28 @@ fn enforce_fee_payment() -> Result<()> { node.clear_results(); node.submit_txs(txs); // If empty than failed in process proposal - assert!(!node.tx_result_codes.lock().unwrap().is_empty()); - node.assert_success(); + let codes = node.tx_result_codes.lock().unwrap(); + assert!(!codes.is_empty()); + + for code in codes.iter() { + assert!(matches!(code, NodeResults::Ok)); + } + + let results = node.tx_results.lock().unwrap(); + // We submitted two batches + assert_eq!(results.len(), 2); + let first_result = &results[0]; + let second_result = &results[1]; + + // The batches should contain a single inner tx each + assert_eq!(first_result.0.len(), 1); + assert_eq!(second_result.0.len(), 1); + + // First transaction pay fees but then fails on the token transfer because + // of a lack of funds + assert!(first_result.are_any_err()); + // Second transaction is correctly applied + assert!(second_result.are_results_successfull()); // Assert balances let captured = CapturedOutput::of(|| { @@ -1827,7 +1848,7 @@ fn enforce_fee_payment() -> Result<()> { ) }); assert!(captured.result.is_ok()); - // This is the result of the two fee payemnts and the successful transfer to + // This is the result of the two fee payments and the successful transfer to // Christel assert!(captured.contains("nam: 1799950")); @@ -1849,7 +1870,7 @@ fn enforce_fee_payment() -> Result<()> { assert!(captured.result.is_ok()); // Bertha must not receive anything because the transaction fails. This is // because we evaluate fee payments before the inner transactions, so by the - // time we execute the transfer, Albert doesn't have enough funds anynmore + // time we execute the transfer, Albert doesn't have enough funds anymore assert!(captured.contains("nam: 2000000")); let captured = CapturedOutput::of(|| { From d8dcfe473dfb81bab45daa7db41e702d36f8e270 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 30 Aug 2024 17:03:48 +0200 Subject: [PATCH 07/12] Adds expiration to `gen_ibc_shielding_transfer` --- crates/apps_lib/src/cli.rs | 35 +++++++++++++++++++++++++++++++++++ crates/sdk/src/args.rs | 2 ++ crates/sdk/src/tx.rs | 3 +-- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 7124945d0f..f3e5470180 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -6805,6 +6805,7 @@ pub mod args { target: chain_ctx.get(&self.target), token: self.token, amount: self.amount, + expiration: self.expiration, port_id: self.port_id, channel_id: self.channel_id, }) @@ -6818,14 +6819,26 @@ pub mod args { let target = TRANSFER_TARGET.parse(matches); let token = TOKEN_STR.parse(matches); let amount = InputAmount::Unvalidated(AMOUNT.parse(matches)); + let expiration = EXPIRATION_OPT.parse(matches); let port_id = PORT_ID.parse(matches); let channel_id = CHANNEL_ID.parse(matches); + let no_expiration = NO_EXPIRATION.parse(matches); + let expiration = if no_expiration { + TxExpiration::NoExpiration + } else { + match expiration { + Some(exp) => TxExpiration::Custom(exp), + None => TxExpiration::Default, + } + }; + Self { query, output_folder, target, token, amount, + expiration, port_id, channel_id, } @@ -6843,6 +6856,28 @@ pub mod args { .def() .help(wrap!("The amount to transfer in decimal.")), ) + .arg( + EXPIRATION_OPT + .def() + .help(wrap!( + "The expiration datetime of the masp transaction, \ + after which the tx won't be accepted anymore. If \ + not provided, a default will be set. All of \ + these examples are \ + equivalent:\n2012-12-12T12:12:12Z\n2012-12-12 \ + 12:12:12Z\n2012- 12-12T12: 12:12Z" + )) + .conflicts_with_all([NO_EXPIRATION.name]), + ) + .arg( + NO_EXPIRATION + .def() + .help(wrap!( + "Force the construction of the transaction \ + without an expiration (highly discouraged)." + )) + .conflicts_with_all([EXPIRATION_OPT.name]), + ) .arg(PORT_ID.def().help(wrap!( "The port ID via which the token is received." ))) diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index ac382e9e95..e431e78f64 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -2892,6 +2892,8 @@ pub struct GenIbcShieldingTransfer { pub token: String, /// Transferred token amount pub amount: InputAmount, + /// The optional expiration of the masp shielding transaction + pub expiration: TxExpiration, /// Port ID via which the token is received pub port_id: PortId, /// Channel ID via which the token is received diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 18ba58da91..753be8db19 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -3786,8 +3786,7 @@ pub async fn gen_ibc_shielding_transfer( vec![masp_transfer_data], // Fees are paid from the transparent balance of the relayer None, - // Expiration can be set directly on the ibc transaction - None, + args.expiration.to_datetime(), true, ) .await From 258bc0144c4d2f22c008d5550f60927c7d7c54a4 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 4 Sep 2024 17:28:02 +0200 Subject: [PATCH 08/12] Updates expiration help message --- crates/apps_lib/src/cli.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index f3e5470180..0d5dd478b6 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -6862,10 +6862,8 @@ pub mod args { .help(wrap!( "The expiration datetime of the masp transaction, \ after which the tx won't be accepted anymore. If \ - not provided, a default will be set. All of \ - these examples are \ - equivalent:\n2012-12-12T12:12:12Z\n2012-12-12 \ - 12:12:12Z\n2012- 12-12T12: 12:12Z" + not provided, a default will be set. Example: \ + 2012-12-12T12:12:12Z" )) .conflicts_with_all([NO_EXPIRATION.name]), ) @@ -7319,10 +7317,8 @@ pub mod args { .help(wrap!( "The expiration datetime of the transaction, after \ which the tx won't be accepted anymore. If not \ - provided, a default will be set. All of these \ - examples are \ - equivalent:\n2012-12-12T12:12:12Z\n2012-12-12 \ - 12:12:12Z\n2012- 12-12T12: 12:12Z" + provided, a default will be set. Example: \ + 2012-12-12T12:12:12Z" )) .conflicts_with_all([NO_EXPIRATION.name]), ) From 48c0fef2bb8c8128a7af0c58c72bc104c40cab8d Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 4 Sep 2024 17:45:36 +0200 Subject: [PATCH 09/12] Fixes typo --- crates/tests/src/integration/masp.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 98e48404f2..e9c45fa60f 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -1380,7 +1380,7 @@ fn multiple_unfetched_txs_same_block() -> Result<()> { vec!["shielded-sync", "--node", validator_one_rpc], )?; - // 2. Shielded operations without fetching. Dump the txs to than reload and + // 2. Shielded operations without fetching. Dump the txs to then reload and // submit in the same block let tempdir = tempfile::tempdir().unwrap(); let mut txs_bytes = vec![]; @@ -1578,7 +1578,7 @@ fn expired_masp_tx() -> Result<()> { )?; // 2. Shielded operation to avoid the need of a signature on the inner tx. - // Dump the tx to than reload and submit + // Dump the tx to then reload and submit let tempdir = tempfile::tempdir().unwrap(); _ = node.next_epoch(); From 80edaf1b5f61b4a0d9efcabba7b05db11299c700 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 4 Sep 2024 17:52:07 +0200 Subject: [PATCH 10/12] Improves test description --- crates/tests/src/integration/masp.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index e9c45fa60f..b3aef4713a 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -1597,8 +1597,13 @@ fn expired_masp_tx() -> Result<()> { "50", "--gas-payer", CHRISTEL_KEY, - // This is used to set the correct expiration in the masp object, - // we don't care about the expiration at the tx level + // We want to create an expired masp tx. Doing so will also set the + // expiration field of the header which can be a problem because + // this would lead to the transaction being rejected by the + // protocol check while we want to test expiration in the masp vp. + // However, this is not a real issue: to avoid the failure in + // protocol we are going to overwrite the header with one having no + // expiration "--expiration", #[allow(clippy::disallowed_methods)] &DateTimeUtc::now().to_string(), From bfe39a85be0a0bb805d5bc827c7512e7f8a4819e Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 4 Sep 2024 18:11:04 +0200 Subject: [PATCH 11/12] Changes target of `MockNode` deref --- crates/node/src/shell/testing/node.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/node/src/shell/testing/node.rs b/crates/node/src/shell/testing/node.rs index 8122b49574..95c22c481b 100644 --- a/crates/node/src/shell/testing/node.rs +++ b/crates/node/src/shell/testing/node.rs @@ -261,7 +261,7 @@ pub struct InnerMockNode { pub struct MockNode(pub Arc); impl Deref for MockNode { - type Target = Arc; + type Target = InnerMockNode; fn deref(&self) -> &Self::Target { &self.0 From f41e88aa502e3516fce439fb479e8aad92a2efba Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 30 Aug 2024 16:48:05 +0200 Subject: [PATCH 12/12] Changelog #3724 --- .changelog/unreleased/bug-fixes/3724-fix-masp-expiration.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/3724-fix-masp-expiration.md diff --git a/.changelog/unreleased/bug-fixes/3724-fix-masp-expiration.md b/.changelog/unreleased/bug-fixes/3724-fix-masp-expiration.md new file mode 100644 index 0000000000..8612012a64 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/3724-fix-masp-expiration.md @@ -0,0 +1,2 @@ +- Fixed the SDK to generate MASP transactions with the correct expiration (if + provided). ([\#3724](https://github.com/anoma/namada/pull/3724)) \ No newline at end of file