From c6f11bc1da5addff701796796fdd5f83c02cbb5a Mon Sep 17 00:00:00 2001 From: Murisi Tarusenga Date: Tue, 16 Jul 2024 11:42:28 +0200 Subject: [PATCH 01/10] Stop requiring authorization signatures for non-MASP related balance changes. --- crates/namada/src/ledger/native_vp/masp.rs | 66 ++++++++++++---------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/crates/namada/src/ledger/native_vp/masp.rs b/crates/namada/src/ledger/native_vp/masp.rs index c550c0a4db..b2d7655734 100644 --- a/crates/namada/src/ledger/native_vp/masp.rs +++ b/crates/namada/src/ledger/native_vp/masp.rs @@ -687,33 +687,26 @@ where .read_post(&counterpart_balance_key)? .unwrap_or_default(); // Public keys must be the hash of the sources/targets - let address_hash = addr_taddr(counterpart.clone()); + let addr_hash = addr_taddr(counterpart.clone()); // Enable the decoding of these counterpart addresses result .decoder - .insert(address_hash, TAddrData::Addr(counterpart.clone())); + .insert(addr_hash, TAddrData::Addr(counterpart.clone())); + let zero = ValueSum::zero(); // Finally record the actual balance change starting with the initial // state - let pre_entry = result - .pre - .get(&address_hash) - .cloned() - .unwrap_or(ValueSum::zero()); + let pre_entry = result.pre.get(&addr_hash).unwrap_or(&zero).clone(); result.pre.insert( - address_hash, + addr_hash, checked!( pre_entry + &ValueSum::from_pair((*token).clone(), pre_balance) ) .map_err(native_vp::Error::new)?, ); // And then record thee final state - let post_entry = result - .post - .get(&address_hash) - .cloned() - .unwrap_or(ValueSum::zero()); + let post_entry = result.post.get(&addr_hash).cloned().unwrap_or(zero); result.post.insert( - address_hash, + addr_hash, checked!( post_entry + &ValueSum::from_pair((*token).clone(), post_balance) @@ -809,19 +802,21 @@ where } // Check the validity of the keys and get the transfer data - let mut changed_balances = + let changed_balances = self.validate_state_and_get_transfer_data(keys_changed, ibc_msg)?; + // Some constants that will be used repeatedly + let zero = ValueSum::zero(); let masp_address_hash = addr_taddr(MASP); verify_sapling_balancing_value( changed_balances .pre .get(&masp_address_hash) - .unwrap_or(&ValueSum::zero()), + .unwrap_or(&zero), changed_balances .post .get(&masp_address_hash) - .unwrap_or(&ValueSum::zero()), + .unwrap_or(&zero), &shielded_tx.sapling_value_balance(), masp_epoch, &changed_balances.tokens, @@ -845,24 +840,35 @@ where self.valid_note_commitment_update(&shielded_tx)?; // Checks on the transparent bundle, if present + let mut changed_bals_minus_txn = changed_balances.clone(); validate_transparent_bundle( &shielded_tx, - &mut changed_balances, + &mut changed_bals_minus_txn, masp_epoch, conversion_state, &mut signers, )?; - // Ensure that every account for which balance has gone down has - // authorized this transaction - for (addr, pre) in changed_balances.pre { - if changed_balances - .post - .get(&addr) - .unwrap_or(&ValueSum::zero()) - < &pre - && addr != masp_address_hash + // Ensure that every account for which balance has gone down as a result + // of the Transaction has authorized this transaction + for (addr, minus_txn_pre) in changed_bals_minus_txn.pre { + // The pre-balance seen by all VPs including this one + let pre = changed_balances.pre.get(&addr).unwrap_or(&zero); + // The post-balance seen by all VPs including this one + let post = changed_balances.post.get(&addr).unwrap_or(&zero); + // The post-balance if the effects of the Transaction are removed + let minus_txn_post = + changed_bals_minus_txn.post.get(&addr).unwrap_or(&zero); + // Never require a signature from the MASP VP + if addr != masp_address_hash && + // Only require further authorization if without the Transaction, + // this Tx would decrease the balance of this address + minus_txn_post < &minus_txn_pre && + // Only require further authorization from this address if the + // Transaction alters its balance + (minus_txn_pre, minus_txn_post) != (pre.clone(), post) { + // This address will need to provide further authorization signers.insert(addr); } } @@ -870,7 +876,7 @@ where // Ensure that this transaction is authorized by all involved parties for signer in signers { if let Some(TAddrData::Addr(IBC)) = - changed_balances.decoder.get(&signer) + changed_bals_minus_txn.decoder.get(&signer) { // If the IBC address is a signatory, then it means that either // Tx - Transaction(s) causes a decrease in the IBC balance or @@ -887,7 +893,7 @@ where if let Some(transp_bundle) = shielded_tx.transparent_bundle() { for vout in transp_bundle.vout.iter() { if let Some(TAddrData::Ibc(_)) = - changed_balances.decoder.get(&vout.address) + changed_bals_minus_txn.decoder.get(&vout.address) { let error = native_vp::Error::new_const( "Simultaneous credit and debit of IBC account \ @@ -900,7 +906,7 @@ where } } } else if let Some(TAddrData::Addr(signer)) = - changed_balances.decoder.get(&signer) + changed_bals_minus_txn.decoder.get(&signer) { // Otherwise the signer must be decodable so that we can // manually check the signatures From 17b9fcfa9c85f5a115646120f6595e8cf28c38a7 Mon Sep 17 00:00:00 2001 From: Murisi Tarusenga Date: Tue, 16 Jul 2024 12:23:28 +0200 Subject: [PATCH 02/10] Added a changelog entry. --- .../unreleased/improvements/3516-reduce-masp-vp-signatures.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/3516-reduce-masp-vp-signatures.md diff --git a/.changelog/unreleased/improvements/3516-reduce-masp-vp-signatures.md b/.changelog/unreleased/improvements/3516-reduce-masp-vp-signatures.md new file mode 100644 index 0000000000..55e02df5ae --- /dev/null +++ b/.changelog/unreleased/improvements/3516-reduce-masp-vp-signatures.md @@ -0,0 +1,2 @@ +- Eliminates the MASP VPs requirement for all debited accounts to sign a Tx. + ([\#3516](https://github.com/anoma/namada/pull/3516)) \ No newline at end of file From 3369502ffd82d59a982d01f1fe8670db475b081e Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 5 Jun 2024 12:52:04 +0200 Subject: [PATCH 03/10] Moves signatures verification from masp vp to the affected vps --- crates/namada/src/ledger/native_vp/masp.rs | 36 ++++++++-------------- crates/tx/src/action.rs | 12 +++++--- wasm/tx_ibc/src/lib.rs | 4 ++- wasm/tx_transfer/src/lib.rs | 4 ++- wasm/vp_implicit/src/lib.rs | 4 ++- wasm/vp_user/src/lib.rs | 4 ++- 6 files changed, 32 insertions(+), 32 deletions(-) diff --git a/crates/namada/src/ledger/native_vp/masp.rs b/crates/namada/src/ledger/native_vp/masp.rs index b2d7655734..71b63aa757 100644 --- a/crates/namada/src/ledger/native_vp/masp.rs +++ b/crates/namada/src/ledger/native_vp/masp.rs @@ -21,7 +21,6 @@ use namada_core::ibc::apps::transfer::types::msgs::transfer::MsgTransfer as IbcM use namada_core::ibc::apps::transfer::types::packet::PacketData; use namada_core::masp::{addr_taddr, encode_asset_type, ibc_taddr, MaspEpoch}; use namada_core::storage::Key; -use namada_gas::GasMetering; use namada_governance::storage::is_proposal_accepted; use namada_ibc::core::channel::types::commitment::{ compute_packet_commitment, PacketCommitment, @@ -750,6 +749,7 @@ where &self, tx_data: &BatchedTxRef<'_>, keys_changed: &BTreeSet, + verifiers: &BTreeSet
, ) -> Result<()> { let masp_epoch_multiplier = namada_parameters::read_masp_epoch_multiplier_parameter( @@ -908,27 +908,15 @@ where } else if let Some(TAddrData::Addr(signer)) = changed_bals_minus_txn.decoder.get(&signer) { - // Otherwise the signer must be decodable so that we can - // manually check the signatures - let public_keys_index_map = - crate::account::public_keys_index_map( - &self.ctx.pre(), - signer, - )?; - let threshold = - crate::account::threshold(&self.ctx.pre(), signer)? - .unwrap_or(1); - let mut gas_meter = self.ctx.gas_meter.borrow_mut(); - tx_data - .tx - .verify_signatures( - &[tx_data.tx.raw_header_hash()], - public_keys_index_map, - &Some(signer.clone()), - threshold, - || gas_meter.consume(crate::gas::VERIFY_TX_SIG_GAS), - ) - .map_err(native_vp::Error::new)?; + // Otherwise the owner's vp must have been triggered + if !verifiers.contains(signer) { + let error = native_vp::Error::new_alloc(format!( + "The required vp of address {signer} was not triggered" + )) + .into(); + tracing::debug!("{error}"); + return Err(error); + } } else { // We are not able to decode the signer, so just fail let error = native_vp::Error::new_const( @@ -1259,7 +1247,7 @@ where &self, tx_data: &BatchedTxRef<'_>, keys_changed: &BTreeSet, - _verifiers: &BTreeSet
, + verifiers: &BTreeSet
, ) -> Result<()> { let masp_keys_changed: Vec<&Key> = keys_changed.iter().filter(|key| is_masp_key(key)).collect(); @@ -1289,7 +1277,7 @@ where self.is_valid_parameter_change(tx_data) } else if masp_transfer_changes { // The MASP transfer keys can only be changed by a valid Transaction - self.is_valid_masp_transfer(tx_data, keys_changed) + self.is_valid_masp_transfer(tx_data, keys_changed, verifiers) } else { // Changing no MASP keys at all is also fine Ok(()) diff --git a/crates/tx/src/action.rs b/crates/tx/src/action.rs index dbd2e4c20c..43f4dfbe77 100644 --- a/crates/tx/src/action.rs +++ b/crates/tx/src/action.rs @@ -64,11 +64,13 @@ pub enum PgfAction { UpdateStewardCommission(Address), } -/// MASP tx action. +/// MASP tx actions. #[derive(Clone, Debug, BorshDeserialize, BorshSerialize)] -pub struct MaspAction { +pub enum MaspAction { /// The hash of the masp [`crate::types::Section`] - pub masp_section_ref: TxId, + MaspSectionRef(TxId), + /// A required signer for the transaction + MaspSigner(Address), } /// Read actions from temporary storage @@ -127,7 +129,9 @@ pub fn get_masp_section_ref( ) -> Result, ::Err> { Ok(reader.read_actions()?.into_iter().find_map(|action| { // In case of multiple masp actions we get the first one - if let Action::Masp(MaspAction { masp_section_ref }) = action { + if let Action::Masp(MaspAction::MaspSectionRef(masp_section_ref)) = + action + { Some(masp_section_ref) } else { None diff --git a/wasm/tx_ibc/src/lib.rs b/wasm/tx_ibc/src/lib.rs index 78963875f4..a3e1155874 100644 --- a/wasm/tx_ibc/src/lib.rs +++ b/wasm/tx_ibc/src/lib.rs @@ -65,7 +65,9 @@ fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { update_masp_note_commitment_tree(&shielded) .wrap_err("Failed to update the MASP commitment tree")?; if let Some(masp_section_ref) = masp_section_ref { - ctx.push_action(Action::Masp(MaspAction { masp_section_ref }))?; + ctx.push_action(Action::Masp(MaspAction::MaspSectionRef( + masp_section_ref, + )))?; } else { ctx.push_action(Action::IbcShielding)?; } diff --git a/wasm/tx_transfer/src/lib.rs b/wasm/tx_transfer/src/lib.rs index 0cc58998df..c780c7b9f0 100644 --- a/wasm/tx_transfer/src/lib.rs +++ b/wasm/tx_transfer/src/lib.rs @@ -53,7 +53,9 @@ fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { .wrap_err("Encountered error while handling MASP transaction")?; update_masp_note_commitment_tree(&shielded) .wrap_err("Failed to update the MASP commitment tree")?; - ctx.push_action(Action::Masp(MaspAction { masp_section_ref }))?; + ctx.push_action(Action::Masp(MaspAction::MaspSectionRef( + masp_section_ref, + )))?; } Ok(()) diff --git a/wasm/vp_implicit/src/lib.rs b/wasm/vp_implicit/src/lib.rs index 7a0b237d4a..a1ef064d27 100644 --- a/wasm/vp_implicit/src/lib.rs +++ b/wasm/vp_implicit/src/lib.rs @@ -101,7 +101,9 @@ fn validate_tx( &tx, &addr, )?, - Action::Masp(_) => (), + Action::Masp(MaspAction::MaspSigner(source)) => gadget + .verify_signatures_when(|| source == addr, ctx, &tx, &addr)?, + Action::Masp(MaspAction::MaspSectionRef(_)) => (), Action::IbcShielding => (), } } diff --git a/wasm/vp_user/src/lib.rs b/wasm/vp_user/src/lib.rs index 0cfe763c7d..1410b0069d 100644 --- a/wasm/vp_user/src/lib.rs +++ b/wasm/vp_user/src/lib.rs @@ -101,7 +101,9 @@ fn validate_tx( &tx, &addr, )?, - Action::Masp(_) => (), + Action::Masp(MaspAction::MaspSigner(source)) => gadget + .verify_signatures_when(|| source == addr, ctx, &tx, &addr)?, + Action::Masp(MaspAction::MaspSectionRef(_)) => (), Action::IbcShielding => (), } } From 5f70e2ca363216a5665cb5486183146415781305 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 4 Jul 2024 19:33:06 +0200 Subject: [PATCH 04/10] Masp vp checks for signer actions --- crates/namada/src/ledger/native_vp/masp.rs | 15 ++++++++++++++- crates/tx/src/action.rs | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/crates/namada/src/ledger/native_vp/masp.rs b/crates/namada/src/ledger/native_vp/masp.rs index 71b63aa757..74e11cc25a 100644 --- a/crates/namada/src/ledger/native_vp/masp.rs +++ b/crates/namada/src/ledger/native_vp/masp.rs @@ -908,7 +908,8 @@ where } else if let Some(TAddrData::Addr(signer)) = changed_bals_minus_txn.decoder.get(&signer) { - // Otherwise the owner's vp must have been triggered + // Otherwise the owner's vp must have been triggered and the + // relative action must have been written if !verifiers.contains(signer) { let error = native_vp::Error::new_alloc(format!( "The required vp of address {signer} was not triggered" @@ -917,6 +918,18 @@ where tracing::debug!("{error}"); return Err(error); } + + if !namada_tx::action::is_required_masp_signer( + &self.ctx, signer, + )? { + let error = native_vp::Error::new_alloc(format!( + "The required masp signer action for address {signer} \ + is missing" + )) + .into(); + tracing::debug!("{error}"); + return Err(error); + } } else { // We are not able to decode the signer, so just fail let error = native_vp::Error::new_const( diff --git a/crates/tx/src/action.rs b/crates/tx/src/action.rs index 43f4dfbe77..37f92a92e1 100644 --- a/crates/tx/src/action.rs +++ b/crates/tx/src/action.rs @@ -148,3 +148,19 @@ pub fn is_ibc_shielding_transfer( .iter() .any(|action| matches!(action, Action::IbcShielding))) } + +// FIXME: remove if used only in one place +/// Helper function to check if the provided address is mentioned as a masp +/// signer in the [`Actions`]. +pub fn is_required_masp_signer( + reader: &T, + address: &Address, +) -> Result::Err> { + Ok(reader.read_actions()?.iter().any(|action| { + if let Action::Masp(MaspAction::MaspSigner(addr)) = action { + addr == address + } else { + false + } + })) +} From ffd5cfd441307b963a0adeff61688c7531abf17a Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 5 Jul 2024 11:27:51 +0200 Subject: [PATCH 05/10] Refactors masp action checks --- crates/namada/src/ledger/native_vp/masp.rs | 17 +++++++--- crates/namada/src/ledger/protocol/mod.rs | 11 ++++--- crates/tx/src/action.rs | 37 ++++++---------------- 3 files changed, 29 insertions(+), 36 deletions(-) diff --git a/crates/namada/src/ledger/native_vp/masp.rs b/crates/namada/src/ledger/native_vp/masp.rs index 74e11cc25a..a4eee723a2 100644 --- a/crates/namada/src/ledger/native_vp/masp.rs +++ b/crates/namada/src/ledger/native_vp/masp.rs @@ -42,6 +42,7 @@ use namada_sdk::masp::TAddrData; use namada_state::{ConversionState, OptionExt, ResultExt, StateRead}; use namada_token::read_denom; use namada_token::validation::verify_shielded_tx; +use namada_tx::action::Read; use namada_tx::BatchedTxRef; use namada_vp_env::VpEnv; use thiserror::Error; @@ -764,6 +765,7 @@ where })?; let conversion_state = self.ctx.state.in_mem().get_conversion_state(); let ibc_msg = self.ctx.get_ibc_message(tx_data).ok(); + let actions = self.ctx.read_actions()?; let shielded_tx = if let Some(IbcMessage::Envelope(ref envelope)) = ibc_msg { extract_masp_tx_from_envelope(envelope).ok_or_else(|| { @@ -774,7 +776,7 @@ where } else { // Get the Transaction object from the actions let masp_section_ref = - namada_tx::action::get_masp_section_ref(&self.ctx)? + namada_tx::action::get_masp_section_ref(&actions) .ok_or_else(|| { native_vp::Error::new_const( "Missing MASP section reference in action", @@ -919,9 +921,16 @@ where return Err(error); } - if !namada_tx::action::is_required_masp_signer( - &self.ctx, signer, - )? { + // The action is required becuse the target vp might have been + // triggered for other reasons but we need to signal it that it + // is required to validate a discrepancy in its balance change + // because of a masp transaction, which might require a + // different validation than a normal balance change + if !actions.contains(&namada_tx::action::Action::Masp( + namada_tx::action::MaspAction::MaspSigner( + signer.to_owned(), + ), + )) { let error = native_vp::Error::new_alloc(format!( "The required masp signer action for address {signer} \ is missing" diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index 97616f3b34..a3bbb88e5e 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -415,9 +415,10 @@ where if is_accepted { // If the transaction was a masp one append the // transaction refs for the events + let actions = + state.read_actions().map_err(Error::StateError)?; if let Some(masp_section_ref) = - namada_tx::action::get_masp_section_ref(state) - .map_err(Error::StateError)? + namada_tx::action::get_masp_section_ref(&actions) { extended_tx_result .masp_tx_refs @@ -740,8 +741,10 @@ where ); } - let masp_ref = namada_tx::action::get_masp_section_ref(*state) - .map_err(Error::StateError)?; + let actions = + state.read_actions().map_err(Error::StateError)?; + let masp_ref = + namada_tx::action::get_masp_section_ref(&actions); // Ensure that the transaction is actually a masp one, otherwise // reject (is_masp_transfer(&result.changed_keys) && result.is_accepted()) diff --git a/crates/tx/src/action.rs b/crates/tx/src/action.rs index 37f92a92e1..08e629150c 100644 --- a/crates/tx/src/action.rs +++ b/crates/tx/src/action.rs @@ -21,7 +21,7 @@ pub type Actions = Vec; /// An action applied from a tx. #[allow(missing_docs)] -#[derive(Clone, Debug, BorshDeserialize, BorshSerialize)] +#[derive(Clone, Debug, BorshDeserialize, BorshSerialize, PartialEq)] pub enum Action { Pos(PosAction), Gov(GovAction), @@ -32,7 +32,7 @@ pub enum Action { /// PoS tx actions. #[allow(missing_docs)] -#[derive(Clone, Debug, BorshDeserialize, BorshSerialize)] +#[derive(Clone, Debug, BorshDeserialize, BorshSerialize, PartialEq)] pub enum PosAction { BecomeValidator(Address), DeactivateValidator(Address), @@ -50,7 +50,7 @@ pub enum PosAction { /// Gov tx actions. #[allow(missing_docs)] -#[derive(Clone, Debug, BorshDeserialize, BorshSerialize)] +#[derive(Clone, Debug, BorshDeserialize, BorshSerialize, PartialEq)] pub enum GovAction { InitProposal { author: Address }, VoteProposal { id: u64, voter: Address }, @@ -58,14 +58,14 @@ pub enum GovAction { /// PGF tx actions. #[allow(missing_docs)] -#[derive(Clone, Debug, BorshDeserialize, BorshSerialize)] +#[derive(Clone, Debug, BorshDeserialize, BorshSerialize, PartialEq)] pub enum PgfAction { ResignSteward(Address), UpdateStewardCommission(Address), } /// MASP tx actions. -#[derive(Clone, Debug, BorshDeserialize, BorshSerialize)] +#[derive(Clone, Debug, BorshDeserialize, BorshSerialize, PartialEq)] pub enum MaspAction { /// The hash of the masp [`crate::types::Section`] MaspSectionRef(TxId), @@ -124,19 +124,16 @@ fn storage_key() -> storage::Key { /// Helper function to get the optional masp section reference from the /// [`Actions`]. If more than one [`MaspAction`] has been found we return the /// first one -pub fn get_masp_section_ref( - reader: &T, -) -> Result, ::Err> { - Ok(reader.read_actions()?.into_iter().find_map(|action| { - // In case of multiple masp actions we get the first one +pub fn get_masp_section_ref(actions: &Actions) -> Option { + actions.iter().find_map(|action| { if let Action::Masp(MaspAction::MaspSectionRef(masp_section_ref)) = action { - Some(masp_section_ref) + Some(masp_section_ref.to_owned()) } else { None } - })) + }) } /// Helper function to check if the action is IBC shielding transfer @@ -148,19 +145,3 @@ pub fn is_ibc_shielding_transfer( .iter() .any(|action| matches!(action, Action::IbcShielding))) } - -// FIXME: remove if used only in one place -/// Helper function to check if the provided address is mentioned as a masp -/// signer in the [`Actions`]. -pub fn is_required_masp_signer( - reader: &T, - address: &Address, -) -> Result::Err> { - Ok(reader.read_actions()?.iter().any(|action| { - if let Action::Masp(MaspAction::MaspSigner(addr)) = action { - addr == address - } else { - false - } - })) -} From 01484b34ba578b116da477dcec459ff52b41cdc5 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 12 Jul 2024 11:37:40 +0200 Subject: [PATCH 06/10] Renames masp signers to authorizers --- crates/namada/src/ledger/native_vp/masp.rs | 34 ++++++++++------------ crates/tx/src/action.rs | 4 +-- wasm/vp_implicit/src/lib.rs | 2 +- wasm/vp_user/src/lib.rs | 2 +- 4 files changed, 20 insertions(+), 22 deletions(-) diff --git a/crates/namada/src/ledger/native_vp/masp.rs b/crates/namada/src/ledger/native_vp/masp.rs index a4eee723a2..863f184a2b 100644 --- a/crates/namada/src/ledger/native_vp/masp.rs +++ b/crates/namada/src/ledger/native_vp/masp.rs @@ -703,7 +703,7 @@ where ) .map_err(native_vp::Error::new)?, ); - // And then record thee final state + // And then record the final state let post_entry = result.post.get(&addr_hash).cloned().unwrap_or(zero); result.post.insert( addr_hash, @@ -826,7 +826,7 @@ where )?; // The set of addresses that are required to authorize this transaction - let mut signers = BTreeSet::new(); + let mut authorizers = BTreeSet::new(); // Checks on the sapling bundle // 1. The spend descriptions' anchors are valid @@ -848,7 +848,7 @@ where &mut changed_bals_minus_txn, masp_epoch, conversion_state, - &mut signers, + &mut authorizers, )?; // Ensure that every account for which balance has gone down as a result @@ -871,14 +871,14 @@ where (minus_txn_pre, minus_txn_post) != (pre.clone(), post) { // This address will need to provide further authorization - signers.insert(addr); + authorizers.insert(addr); } } // Ensure that this transaction is authorized by all involved parties - for signer in signers { + for authorizer in authorizers { if let Some(TAddrData::Addr(IBC)) = - changed_bals_minus_txn.decoder.get(&signer) + changed_bals_minus_txn.decoder.get(&authorizer) { // If the IBC address is a signatory, then it means that either // Tx - Transaction(s) causes a decrease in the IBC balance or @@ -908,7 +908,7 @@ where } } } else if let Some(TAddrData::Addr(signer)) = - changed_bals_minus_txn.decoder.get(&signer) + changed_bals_minus_txn.decoder.get(&authorizer) { // Otherwise the owner's vp must have been triggered and the // relative action must have been written @@ -927,22 +927,22 @@ where // because of a masp transaction, which might require a // different validation than a normal balance change if !actions.contains(&namada_tx::action::Action::Masp( - namada_tx::action::MaspAction::MaspSigner( + namada_tx::action::MaspAction::MaspAuthorizer( signer.to_owned(), ), )) { let error = native_vp::Error::new_alloc(format!( - "The required masp signer action for address {signer} \ - is missing" + "The required masp authorizer action for address \ + {signer} is missing" )) .into(); tracing::debug!("{error}"); return Err(error); } } else { - // We are not able to decode the signer, so just fail + // We are not able to decode the authorizer, so just fail let error = native_vp::Error::new_const( - "Unable to decode a transaction signer", + "Unable to decode a transaction authorizer", ) .into(); tracing::debug!("{error}"); @@ -973,18 +973,17 @@ fn unepoched_tokens( Ok(()) } -// Handle transparent input fn validate_transparent_input( vin: &TxIn, changed_balances: &mut ChangedBalances, transparent_tx_pool: &mut I128Sum, epoch: MaspEpoch, conversion_state: &ConversionState, - signers: &mut BTreeSet, + authorizers: &mut BTreeSet, ) -> Result<()> { // A decrease in the balance of an account needs to be // authorized by the account of this transparent input - signers.insert(vin.address); + authorizers.insert(vin.address); // Non-masp sources add to the transparent tx pool *transparent_tx_pool = transparent_tx_pool .checked_add( @@ -1060,7 +1059,6 @@ fn validate_transparent_input( Ok(()) } -// Handle transparent output fn validate_transparent_output( out: &TxOut, changed_balances: &mut ChangedBalances, @@ -1132,7 +1130,7 @@ fn validate_transparent_bundle( changed_balances: &mut ChangedBalances, epoch: MaspEpoch, conversion_state: &ConversionState, - signers: &mut BTreeSet, + authorizers: &mut BTreeSet, ) -> Result<()> { // The Sapling value balance adds to the transparent tx pool let mut transparent_tx_pool = shielded_tx.sapling_value_balance(); @@ -1145,7 +1143,7 @@ fn validate_transparent_bundle( &mut transparent_tx_pool, epoch, conversion_state, - signers, + authorizers, )?; } diff --git a/crates/tx/src/action.rs b/crates/tx/src/action.rs index 08e629150c..323de1754c 100644 --- a/crates/tx/src/action.rs +++ b/crates/tx/src/action.rs @@ -69,8 +69,8 @@ pub enum PgfAction { pub enum MaspAction { /// The hash of the masp [`crate::types::Section`] MaspSectionRef(TxId), - /// A required signer for the transaction - MaspSigner(Address), + /// A required authorizer for the transaction + MaspAuthorizer(Address), } /// Read actions from temporary storage diff --git a/wasm/vp_implicit/src/lib.rs b/wasm/vp_implicit/src/lib.rs index a1ef064d27..884c03a228 100644 --- a/wasm/vp_implicit/src/lib.rs +++ b/wasm/vp_implicit/src/lib.rs @@ -101,7 +101,7 @@ fn validate_tx( &tx, &addr, )?, - Action::Masp(MaspAction::MaspSigner(source)) => gadget + Action::Masp(MaspAction::MaspAuthorizer(source)) => gadget .verify_signatures_when(|| source == addr, ctx, &tx, &addr)?, Action::Masp(MaspAction::MaspSectionRef(_)) => (), Action::IbcShielding => (), diff --git a/wasm/vp_user/src/lib.rs b/wasm/vp_user/src/lib.rs index 1410b0069d..a046c19a95 100644 --- a/wasm/vp_user/src/lib.rs +++ b/wasm/vp_user/src/lib.rs @@ -101,7 +101,7 @@ fn validate_tx( &tx, &addr, )?, - Action::Masp(MaspAction::MaspSigner(source)) => gadget + Action::Masp(MaspAction::MaspAuthorizer(source)) => gadget .verify_signatures_when(|| source == addr, ctx, &tx, &addr)?, Action::Masp(MaspAction::MaspSectionRef(_)) => (), Action::IbcShielding => (), From 77b164dd9cbb4136fa6c56ca57922b7bfa02cb74 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 12 Jul 2024 11:39:12 +0200 Subject: [PATCH 07/10] Transfer transaction pushes masp actions --- crates/trans_token/src/storage.rs | 9 ++++++--- crates/tx_prelude/src/token.rs | 10 ++++++---- wasm/tx_transfer/src/lib.rs | 23 +++++++++++++++++++++-- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/crates/trans_token/src/storage.rs b/crates/trans_token/src/storage.rs index 1ba6f238f8..4a21b2d222 100644 --- a/crates/trans_token/src/storage.rs +++ b/crates/trans_token/src/storage.rs @@ -1,6 +1,7 @@ use std::collections::{BTreeMap, BTreeSet}; use namada_core::address::{Address, InternalAddress}; +use namada_core::collections::HashSet; use namada_core::hints; use namada_core::token::{self, Amount, AmountError, DenominatedAmount}; use namada_storage as storage; @@ -263,15 +264,16 @@ where /// Transfer tokens from `sources` to `dests`. Returns an `Err` if any source /// has insufficient balance or if the transfer to any destination would /// overflow (This can only happen if the total supply doesn't fit in -/// `token::Amount`). +/// `token::Amount`). Returns the set of debited accounts. pub fn multi_transfer( storage: &mut S, sources: &BTreeMap<(Address, Address), Amount>, dests: &BTreeMap<(Address, Address), Amount>, -) -> storage::Result<()> +) -> storage::Result> where S: StorageRead + StorageWrite, { + let mut debited_accounts = HashSet::new(); // Collect all the accounts whose balance has changed let mut accounts = BTreeSet::new(); accounts.extend(sources.keys().cloned()); @@ -306,6 +308,7 @@ where ) .ok_or_else(overflow_err)? } else { + debited_accounts.insert(owner.to_owned()); owner_balance .checked_sub( src_amt.checked_sub(dest_amt).ok_or_else(unexpected_err)?, @@ -315,7 +318,7 @@ where // Wite the new balance storage.write(&owner_key, new_owner_balance)?; } - Ok(()) + Ok(debited_accounts) } /// Mint `amount` of `token` as `minter` to `dest`. diff --git a/crates/tx_prelude/src/token.rs b/crates/tx_prelude/src/token.rs index f0cb0f09d6..5e2f630255 100644 --- a/crates/tx_prelude/src/token.rs +++ b/crates/tx_prelude/src/token.rs @@ -3,6 +3,7 @@ use std::collections::BTreeMap; use namada_core::address::Address; +use namada_core::collections::HashSet; use namada_events::extend::UserAccount; use namada_events::{EmitEvents, EventLevel}; use namada_token::event::{TokenEvent, TokenOperation}; @@ -50,13 +51,14 @@ pub fn transfer( Ok(()) } -/// A transparent token transfer that can be used in a transaction. +/// A transparent token transfer that can be used in a transaction. Returns the +/// set of debited accounts. pub fn multi_transfer( ctx: &mut Ctx, sources: &BTreeMap<(Address, Address), Amount>, dests: &BTreeMap<(Address, Address), Amount>, -) -> TxResult { - namada_token::multi_transfer(ctx, sources, dests)?; +) -> crate::EnvResult> { + let debited_accounts = namada_token::multi_transfer(ctx, sources, dests)?; let mut evt_sources = BTreeMap::new(); let mut evt_targets = BTreeMap::new(); @@ -108,5 +110,5 @@ pub fn multi_transfer( }, }); - Ok(()) + Ok(debited_accounts) } diff --git a/wasm/tx_transfer/src/lib.rs b/wasm/tx_transfer/src/lib.rs index c780c7b9f0..bd13095405 100644 --- a/wasm/tx_transfer/src/lib.rs +++ b/wasm/tx_transfer/src/lib.rs @@ -2,9 +2,10 @@ //! This tx uses `token::TransparentTransfer` wrapped inside `SignedTxData` //! as its input as declared in `namada` crate. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use namada_tx_prelude::action::{Action, MaspAction, Write}; +use namada_tx_prelude::masp::addr_taddr; use namada_tx_prelude::*; #[transaction] @@ -33,7 +34,7 @@ fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { .collect::>(); // Effect the multi transfer - token::multi_transfer(ctx, &sources, &targets) + let debited_accounts = token::multi_transfer(ctx, &sources, &targets) .wrap_err("Token transfer failed")?; // Apply the shielded transfer if there is a link to one @@ -53,9 +54,27 @@ fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { .wrap_err("Encountered error while handling MASP transaction")?; update_masp_note_commitment_tree(&shielded) .wrap_err("Failed to update the MASP commitment tree")?; + ctx.push_action(Action::Masp(MaspAction::MaspSectionRef( masp_section_ref, )))?; + let vins_addresses = shielded.transparent_bundle().map_or_else( + Default::default, + |bndl| { + bndl.vin + .iter() + .map(|vin| vin.address) + .collect::>() + }, + ); + let masp_authorizers = debited_accounts.into_iter().filter(|account| { + vins_addresses.contains(&addr_taddr(account.clone())) + }); + for authorizer in masp_authorizers { + ctx.push_action(Action::Masp(MaspAction::MaspAuthorizer( + authorizer, + )))?; + } } Ok(()) From 14d70f383315d1a59875955723b6a3fc7279c81f Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 19 Jul 2024 18:17:09 +0200 Subject: [PATCH 08/10] Masp vp checks that no unneeded actions are pushed --- crates/namada/src/ledger/native_vp/masp.rs | 29 ++++++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/crates/namada/src/ledger/native_vp/masp.rs b/crates/namada/src/ledger/native_vp/masp.rs index 863f184a2b..4b6aa32226 100644 --- a/crates/namada/src/ledger/native_vp/masp.rs +++ b/crates/namada/src/ledger/native_vp/masp.rs @@ -875,6 +875,19 @@ where } } + let mut actions_authorizers: HashSet<&Address> = actions + .iter() + .filter_map(|action| { + if let namada_tx::action::Action::Masp( + namada_tx::action::MaspAction::MaspAuthorizer(addr), + ) = action + { + Some(addr) + } else { + None + } + }) + .collect(); // Ensure that this transaction is authorized by all involved parties for authorizer in authorizers { if let Some(TAddrData::Addr(IBC)) = @@ -926,11 +939,7 @@ where // is required to validate a discrepancy in its balance change // because of a masp transaction, which might require a // different validation than a normal balance change - if !actions.contains(&namada_tx::action::Action::Masp( - namada_tx::action::MaspAction::MaspAuthorizer( - signer.to_owned(), - ), - )) { + if !actions_authorizers.swap_remove(signer) { let error = native_vp::Error::new_alloc(format!( "The required masp authorizer action for address \ {signer} is missing" @@ -949,6 +958,16 @@ where return Err(error); } } + // The transaction shall not push masp authorizer actions that are not + // needed cause this might lead vps to run a wrong validation logic + if !actions_authorizers.is_empty() { + let error = native_vp::Error::new_const( + "Found masp authorizer actions that are not required", + ) + .into(); + tracing::debug!("{error}"); + return Err(error); + } // Verify the proofs verify_shielded_tx(&shielded_tx, |gas| self.ctx.charge_gas(gas)) From dab10a6412be41f439e425dd3184922dc44378b6 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 5 Jun 2024 12:57:32 +0200 Subject: [PATCH 09/10] Changelog #3312 --- .../improvements/3312-outsource-masp-sig-verification.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/3312-outsource-masp-sig-verification.md diff --git a/.changelog/unreleased/improvements/3312-outsource-masp-sig-verification.md b/.changelog/unreleased/improvements/3312-outsource-masp-sig-verification.md new file mode 100644 index 0000000000..bcbba47717 --- /dev/null +++ b/.changelog/unreleased/improvements/3312-outsource-masp-sig-verification.md @@ -0,0 +1,2 @@ +- Moved the signature verifications out of the masp vp and into the affected + addresses' vps. ([\#3312](https://github.com/anoma/namada/issues/3312)) \ No newline at end of file From fc63450e3dfc2882c467d96a2600d761fb40853b Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 19 Jul 2024 21:26:01 +0200 Subject: [PATCH 10/10] Transfer transaction fails if masp transparent inputs are not debited --- wasm/tx_transfer/src/lib.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/wasm/tx_transfer/src/lib.rs b/wasm/tx_transfer/src/lib.rs index bd13095405..b4ee0075f7 100644 --- a/wasm/tx_transfer/src/lib.rs +++ b/wasm/tx_transfer/src/lib.rs @@ -58,7 +58,9 @@ fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { ctx.push_action(Action::Masp(MaspAction::MaspSectionRef( masp_section_ref, )))?; - let vins_addresses = shielded.transparent_bundle().map_or_else( + // Extract the debited accounts for the masp part of the transfer and + // push the relative actions + let vin_addresses = shielded.transparent_bundle().map_or_else( Default::default, |bndl| { bndl.vin @@ -67,9 +69,18 @@ fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { .collect::>() }, ); - let masp_authorizers = debited_accounts.into_iter().filter(|account| { - vins_addresses.contains(&addr_taddr(account.clone())) - }); + let masp_authorizers: Vec<_> = debited_accounts + .into_iter() + .filter(|account| { + vin_addresses.contains(&addr_taddr(account.clone())) + }) + .collect(); + if masp_authorizers.len() != vin_addresses.len() { + return Err(Error::SimpleMessage( + "Transfer transaction does not debit all the expected accounts", + )); + } + for authorizer in masp_authorizers { ctx.push_action(Action::Masp(MaspAction::MaspAuthorizer( authorizer,