From d8b51149f65fe460eaff1d7377e25e9591c67d5c Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 30 Nov 2023 11:24:59 +0100 Subject: [PATCH 01/24] Updates masp tx to reveal nullifiers --- core/src/types/token.rs | 5 ++++- shared/src/ledger/native_vp/ibc/context.rs | 15 +++++++++++++++ shared/src/vm/host_env.rs | 14 ++++++++++++++ tx_prelude/src/token.rs | 13 +++++++++++++ 4 files changed, 46 insertions(+), 1 deletion(-) diff --git a/core/src/types/token.rs b/core/src/types/token.rs index 5c319b7da6..b8ff751f93 100644 --- a/core/src/types/token.rs +++ b/core/src/types/token.rs @@ -900,6 +900,8 @@ pub const HEAD_TX_KEY: &str = "head-tx"; pub const TX_KEY_PREFIX: &str = "tx-"; /// Key segment prefix for pinned shielded transactions pub const PIN_KEY_PREFIX: &str = "pin-"; +/// Key segment prefix for the nullifiers +pub const MASP_NULLIFIERS_KEY_PREFIX: &str = "nullifiers"; /// Last calculated inflation value handed out pub const MASP_LAST_INFLATION_KEY: &str = "last_inflation"; /// The last locked ratio @@ -1124,7 +1126,8 @@ pub fn is_masp_key(key: &Key) -> bool { if *addr == MASP && (key == HEAD_TX_KEY || key.starts_with(TX_KEY_PREFIX) - || key.starts_with(PIN_KEY_PREFIX))) + || key.starts_with(PIN_KEY_PREFIX) + || key.starts_with(MASP_NULLIFIERS_KEY_PREFIX))) } /// Obtain the storage key for the last locked ratio of a token diff --git a/shared/src/ledger/native_vp/ibc/context.rs b/shared/src/ledger/native_vp/ibc/context.rs index ffe2faa2f5..ad613b15eb 100644 --- a/shared/src/ledger/native_vp/ibc/context.rs +++ b/shared/src/ledger/native_vp/ibc/context.rs @@ -5,6 +5,8 @@ use std::collections::{BTreeSet, HashMap, HashSet}; use borsh_ext::BorshSerializeExt; use masp_primitives::transaction::Transaction; use namada_core::ledger::ibc::{IbcCommonContext, IbcStorageContext}; +use namada_core::types::hash::Hash; +use namada_core::types::token::MASP_NULLIFIERS_KEY_PREFIX; use crate::ledger::ibc::storage::is_ibc_key; use crate::ledger::native_vp::CtxPreStorageRead; @@ -237,6 +239,19 @@ where ); self.write(¤t_tx_key, record.serialize_to_vec())?; self.write(&head_tx_key, (current_tx_idx + 1).serialize_to_vec())?; + for description in shielded + .masp_tx + .sapling_bundle() + .map_or(&vec![], |description| &description.shielded_spends) + { + // Reveal the nullifier to prevent double spending + let nullifier_key = Key::from(masp_addr.to_db_key()) + .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&Hash(description.nullifier.0)) + .expect("Cannot obtain a storage key"); + self.write(&nullifier_key, ())?; + } // If storage key has been supplied, then pin this transaction to it if let Some(key) = &shielded.transfer.key { let pin_key = Key::from(masp_addr.to_db_key()) diff --git a/shared/src/vm/host_env.rs b/shared/src/vm/host_env.rs index 2a527d2ff5..19d15b3d3e 100644 --- a/shared/src/vm/host_env.rs +++ b/shared/src/vm/host_env.rs @@ -13,6 +13,7 @@ use namada_core::ledger::gas::{ use namada_core::types::address::{ESTABLISHED_ADDRESS_BYTES_LEN, MASP}; use namada_core::types::internal::KeyVal; use namada_core::types::storage::TX_INDEX_LENGTH; +use namada_core::types::token::MASP_NULLIFIERS_KEY_PREFIX; use namada_core::types::transaction::TxSentinel; use namada_core::types::validity_predicate::VpSentinel; use thiserror::Error; @@ -2541,6 +2542,19 @@ where ); self.write(¤t_tx_key, record)?; self.write(&head_tx_key, current_tx_idx + 1)?; + for description in shielded + .masp_tx + .sapling_bundle() + .map_or(&vec![], |description| &description.shielded_spends) + { + // Reveal the nullifier to prevent double spending + let nullifier_key = Key::from(masp_addr.to_db_key()) + .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&Hash(description.nullifier.0)) + .expect("Cannot obtain a storage key"); + self.write(&nullifier_key, ())?; + } // If storage key has been supplied, then pin this transaction to it if let Some(key) = &shielded.transfer.key { let pin_key = Key::from(masp_addr.to_db_key()) diff --git a/tx_prelude/src/token.rs b/tx_prelude/src/token.rs index 009cccb36d..5ee657b6df 100644 --- a/tx_prelude/src/token.rs +++ b/tx_prelude/src/token.rs @@ -1,5 +1,6 @@ use masp_primitives::transaction::Transaction; use namada_core::types::address::{Address, MASP}; +use namada_core::types::hash::Hash; use namada_core::types::storage::KeySeg; use namada_core::types::token; pub use namada_core::types::token::*; @@ -60,6 +61,18 @@ pub fn handle_masp_tx( ); ctx.write(¤t_tx_key, record)?; ctx.write(&head_tx_key, current_tx_idx + 1)?; + for description in shielded + .sapling_bundle() + .map_or(&vec![], |description| &description.shielded_spends) + { + // Reveal the nullifier to prevent double spending + let nullifier_key = storage::Key::from(masp_addr.to_db_key()) + .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&Hash(description.nullifier.0)) + .expect("Cannot obtain a storage key"); + ctx.write(&nullifier_key, ())?; + } // If storage key has been supplied, then pin this transaction to it if let Some(key) = &transfer.key { let pin_key = storage::Key::from(masp_addr.to_db_key()) From 9f855dcdfd0062b47f609d2ba1c8ba9e09892460 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 30 Nov 2023 16:30:24 +0100 Subject: [PATCH 02/24] Masp VP checks uniqueness of nullifiers --- core/src/types/token.rs | 9 +++++ shared/src/ledger/native_vp/masp.rs | 52 ++++++++++++++++++++++++++--- 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/core/src/types/token.rs b/core/src/types/token.rs index b8ff751f93..3cb820f4a3 100644 --- a/core/src/types/token.rs +++ b/core/src/types/token.rs @@ -1130,6 +1130,15 @@ pub fn is_masp_key(key: &Key) -> bool { || key.starts_with(MASP_NULLIFIERS_KEY_PREFIX))) } +/// Check if the given storage key is a masp nullifier key +pub fn is_masp_nullifier_key(key: &Key) -> bool { + matches!(&key.segments[..], + [DbKeySeg::AddressSeg(addr), + DbKeySeg::StringSeg(prefix), + .. + ] if *addr == MASP && prefix == MASP_NULLIFIERS_KEY_PREFIX) +} + /// Obtain the storage key for the last locked ratio of a token pub fn masp_last_locked_ratio_key(token_address: &Address) -> Key { key_of_token( diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index 54048f6759..a660e1fcdc 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -1,7 +1,7 @@ //! MASP native VP use std::cmp::Ordering; -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashSet}; use borsh_ext::BorshSerializeExt; use masp_primitives::asset_type::AssetType; @@ -11,10 +11,12 @@ use namada_core::ledger::storage; use namada_core::ledger::storage_api::OptionExt; use namada_core::ledger::vp_env::VpEnv; use namada_core::proto::Tx; -use namada_core::types::address::Address; use namada_core::types::address::InternalAddress::Masp; -use namada_core::types::storage::{Epoch, Key}; -use namada_core::types::token; +use namada_core::types::address::{Address, MASP}; +use namada_core::types::storage::{Epoch, Key, KeySeg}; +use namada_core::types::token::{ + self, is_masp_nullifier_key, MASP_NULLIFIERS_KEY_PREFIX, +}; use namada_sdk::masp::verify_shielded_tx; use ripemd::Digest as RipemdDigest; use sha2::Digest as Sha2Digest; @@ -119,7 +121,7 @@ where fn validate_tx( &self, tx_data: &Tx, - _keys_changed: &BTreeSet, + keys_changed: &BTreeSet, _verifiers: &BTreeSet
, ) -> Result { let epoch = self.ctx.get_block_epoch()?; @@ -151,6 +153,9 @@ where // 2. the transparent transaction value pool's amount must equal // the containing wrapper transaction's fee // amount Satisfies 1. + // 3. The nullifiers provided by the transaction have not been + // revealed previously (even in the same tx) and no unneeded + // nullifier is being revealed by the tx if let Some(transp_bundle) = shielded_tx.transparent_bundle() { if !transp_bundle.vin.is_empty() { tracing::debug!( @@ -161,6 +166,43 @@ where return Ok(false); } } + + let mut revealed_nullifiers = HashSet::new(); + for description in shielded_tx + .sapling_bundle() + .map_or(&vec![], |description| &description.shielded_spends) + { + let nullifier_key = Key::from(MASP.to_db_key()) + .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&namada_core::types::hash::Hash( + description.nullifier.0, + )) + .expect("Cannot obtain a storage key"); + if self.ctx.has_key_pre(&nullifier_key)? + || revealed_nullifiers.contains(&nullifier_key) + { + tracing::debug!( + "MASP double spending attempt, the nullifier {:#?} \ + has already been revealed previously", + description.nullifier.0 + ); + return Ok(false); + } + revealed_nullifiers.insert(nullifier_key); + } + + for nullifier_key in + keys_changed.iter().filter(|key| is_masp_nullifier_key(key)) + { + if !revealed_nullifiers.contains(nullifier_key) { + tracing::debug!( + "An unexpected MASP nullifier key {nullifier_key} has \ + been revealed by the transaction" + ); + return Ok(false); + } + } } if transfer.target != Address::Internal(Masp) { From 5cc290c39a3715e71c5b86a3abd28c1cc28ff02a Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 30 Nov 2023 16:31:30 +0100 Subject: [PATCH 03/24] Removes wrong comment --- core/src/ledger/storage/write_log.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/ledger/storage/write_log.rs b/core/src/ledger/storage/write_log.rs index 0cb7b91a2e..a53ac7db36 100644 --- a/core/src/ledger/storage/write_log.rs +++ b/core/src/ledger/storage/write_log.rs @@ -182,7 +182,6 @@ impl WriteLog { &self, key: &storage::Key, ) -> (Option<&StorageModification>, u64) { - // try to read from tx write log first match self.block_write_log.get(key) { Some(v) => { let gas = match v { From 9ed6d5250ba38b268a8c4e95a820d2cf159c1552 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 30 Nov 2023 19:25:19 +0100 Subject: [PATCH 04/24] Changelog #2240 --- .changelog/unreleased/bug-fixes/2240-nullifier-uniqueness.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/2240-nullifier-uniqueness.md diff --git a/.changelog/unreleased/bug-fixes/2240-nullifier-uniqueness.md b/.changelog/unreleased/bug-fixes/2240-nullifier-uniqueness.md new file mode 100644 index 0000000000..dc80af96ac --- /dev/null +++ b/.changelog/unreleased/bug-fixes/2240-nullifier-uniqueness.md @@ -0,0 +1,2 @@ +- Prevents double-spending in masp by adding a nullifier set. + ([\#2240](https://github.com/anoma/namada/pull/2240)) \ No newline at end of file From f1156c1d8966098499276fd61db323c9f943c692 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 1 Dec 2023 12:05:20 +0100 Subject: [PATCH 05/24] Fixes masp vp nullifier validation --- shared/src/ledger/native_vp/masp.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index a660e1fcdc..d2b9a9b56b 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -189,6 +189,16 @@ where ); return Ok(false); } + + // Check that the nullifier is indeed committed (no temp write + // and no delete) and carries no associated data (the latter not + // strictly necessary for validation, but we don't expect any + // value for this key anyway) + match self.ctx.read_bytes_post(&nullifier_key)? { + Some(value) if value.is_empty() => (), + _ => return Ok(false), + } + revealed_nullifiers.insert(nullifier_key); } From 144536fea507e19dfea612428f2fd96ff4917f91 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 1 Dec 2023 12:36:58 +0100 Subject: [PATCH 06/24] Reduce code duplication to reveal masp nullifiers --- core/src/ledger/masp_utils.rs | 30 ++++++++++++++++++++++ core/src/ledger/mod.rs | 1 + shared/src/ledger/native_vp/ibc/context.rs | 17 ++---------- shared/src/vm/host_env.rs | 16 ++---------- tx_prelude/src/token.rs | 15 ++--------- 5 files changed, 37 insertions(+), 42 deletions(-) create mode 100644 core/src/ledger/masp_utils.rs diff --git a/core/src/ledger/masp_utils.rs b/core/src/ledger/masp_utils.rs new file mode 100644 index 0000000000..43100a315a --- /dev/null +++ b/core/src/ledger/masp_utils.rs @@ -0,0 +1,30 @@ +//! MASP utilities + +use masp_primitives::transaction::Transaction; + +use super::storage_api::StorageWrite; +use crate::ledger::storage_api::Result; +use crate::types::address::MASP; +use crate::types::hash::Hash; +use crate::types::storage::{Key, KeySeg}; +use crate::types::token::MASP_NULLIFIERS_KEY_PREFIX; + +/// Writes the nullifiers of the provided masp transaction to storage +pub fn reveal_nullifiers( + ctx: &mut impl StorageWrite, + transaction: &Transaction, +) -> Result<()> { + for description in transaction + .sapling_bundle() + .map_or(&vec![], |description| &description.shielded_spends) + { + let nullifier_key = Key::from(MASP.to_db_key()) + .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&Hash(description.nullifier.0)) + .expect("Cannot obtain a storage key"); + ctx.write(&nullifier_key, ())?; + } + + Ok(()) +} diff --git a/core/src/ledger/mod.rs b/core/src/ledger/mod.rs index debeaba7db..96cf4d2331 100644 --- a/core/src/ledger/mod.rs +++ b/core/src/ledger/mod.rs @@ -6,6 +6,7 @@ pub mod governance; pub mod ibc; pub mod inflation; pub mod masp_conversions; +pub mod masp_utils; pub mod parameters; pub mod pgf; pub mod replay_protection; diff --git a/shared/src/ledger/native_vp/ibc/context.rs b/shared/src/ledger/native_vp/ibc/context.rs index ad613b15eb..6bb6f90e1c 100644 --- a/shared/src/ledger/native_vp/ibc/context.rs +++ b/shared/src/ledger/native_vp/ibc/context.rs @@ -5,8 +5,7 @@ use std::collections::{BTreeSet, HashMap, HashSet}; use borsh_ext::BorshSerializeExt; use masp_primitives::transaction::Transaction; use namada_core::ledger::ibc::{IbcCommonContext, IbcStorageContext}; -use namada_core::types::hash::Hash; -use namada_core::types::token::MASP_NULLIFIERS_KEY_PREFIX; +use namada_core::ledger::masp_utils; use crate::ledger::ibc::storage::is_ibc_key; use crate::ledger::native_vp::CtxPreStorageRead; @@ -239,19 +238,7 @@ where ); self.write(¤t_tx_key, record.serialize_to_vec())?; self.write(&head_tx_key, (current_tx_idx + 1).serialize_to_vec())?; - for description in shielded - .masp_tx - .sapling_bundle() - .map_or(&vec![], |description| &description.shielded_spends) - { - // Reveal the nullifier to prevent double spending - let nullifier_key = Key::from(masp_addr.to_db_key()) - .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) - .expect("Cannot obtain a storage key") - .push(&Hash(description.nullifier.0)) - .expect("Cannot obtain a storage key"); - self.write(&nullifier_key, ())?; - } + masp_utils::reveal_nullifiers(self, &shielded.masp_tx)?; // If storage key has been supplied, then pin this transaction to it if let Some(key) = &shielded.transfer.key { let pin_key = Key::from(masp_addr.to_db_key()) diff --git a/shared/src/vm/host_env.rs b/shared/src/vm/host_env.rs index 19d15b3d3e..1bef64fdb2 100644 --- a/shared/src/vm/host_env.rs +++ b/shared/src/vm/host_env.rs @@ -10,10 +10,10 @@ use masp_primitives::transaction::Transaction; use namada_core::ledger::gas::{ GasMetering, TxGasMeter, MEMORY_ACCESS_GAS_PER_BYTE, }; +use namada_core::ledger::masp_utils; use namada_core::types::address::{ESTABLISHED_ADDRESS_BYTES_LEN, MASP}; use namada_core::types::internal::KeyVal; use namada_core::types::storage::TX_INDEX_LENGTH; -use namada_core::types::token::MASP_NULLIFIERS_KEY_PREFIX; use namada_core::types::transaction::TxSentinel; use namada_core::types::validity_predicate::VpSentinel; use thiserror::Error; @@ -2542,19 +2542,7 @@ where ); self.write(¤t_tx_key, record)?; self.write(&head_tx_key, current_tx_idx + 1)?; - for description in shielded - .masp_tx - .sapling_bundle() - .map_or(&vec![], |description| &description.shielded_spends) - { - // Reveal the nullifier to prevent double spending - let nullifier_key = Key::from(masp_addr.to_db_key()) - .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) - .expect("Cannot obtain a storage key") - .push(&Hash(description.nullifier.0)) - .expect("Cannot obtain a storage key"); - self.write(&nullifier_key, ())?; - } + masp_utils::reveal_nullifiers(self, &shielded.masp_tx)?; // If storage key has been supplied, then pin this transaction to it if let Some(key) = &shielded.transfer.key { let pin_key = Key::from(masp_addr.to_db_key()) diff --git a/tx_prelude/src/token.rs b/tx_prelude/src/token.rs index 5ee657b6df..ac4a4ba1da 100644 --- a/tx_prelude/src/token.rs +++ b/tx_prelude/src/token.rs @@ -1,6 +1,6 @@ use masp_primitives::transaction::Transaction; +use namada_core::ledger::masp_utils; use namada_core::types::address::{Address, MASP}; -use namada_core::types::hash::Hash; use namada_core::types::storage::KeySeg; use namada_core::types::token; pub use namada_core::types::token::*; @@ -61,18 +61,7 @@ pub fn handle_masp_tx( ); ctx.write(¤t_tx_key, record)?; ctx.write(&head_tx_key, current_tx_idx + 1)?; - for description in shielded - .sapling_bundle() - .map_or(&vec![], |description| &description.shielded_spends) - { - // Reveal the nullifier to prevent double spending - let nullifier_key = storage::Key::from(masp_addr.to_db_key()) - .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) - .expect("Cannot obtain a storage key") - .push(&Hash(description.nullifier.0)) - .expect("Cannot obtain a storage key"); - ctx.write(&nullifier_key, ())?; - } + masp_utils::reveal_nullifiers(ctx, shielded)?; // If storage key has been supplied, then pin this transaction to it if let Some(key) = &transfer.key { let pin_key = storage::Key::from(masp_addr.to_db_key()) From 39e00c5c16fdb68f88626c6ff175ff6948cf2dda Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 4 Dec 2023 11:51:35 +0100 Subject: [PATCH 07/24] Refactors `handle_masp_tx` --- core/src/ledger/masp_utils.rs | 53 ++++++++++++++++++++-- shared/src/ledger/native_vp/ibc/context.rs | 41 ++--------------- shared/src/vm/host_env.rs | 37 ++------------- tx_prelude/src/ibc.rs | 5 +- tx_prelude/src/token.rs | 45 +----------------- wasm/wasm_source/src/tx_transfer.rs | 2 +- 6 files changed, 61 insertions(+), 122 deletions(-) diff --git a/core/src/ledger/masp_utils.rs b/core/src/ledger/masp_utils.rs index 43100a315a..4ddc2bc49a 100644 --- a/core/src/ledger/masp_utils.rs +++ b/core/src/ledger/masp_utils.rs @@ -2,15 +2,18 @@ use masp_primitives::transaction::Transaction; -use super::storage_api::StorageWrite; +use super::storage_api::{StorageRead, StorageWrite}; use crate::ledger::storage_api::Result; use crate::types::address::MASP; use crate::types::hash::Hash; -use crate::types::storage::{Key, KeySeg}; -use crate::types::token::MASP_NULLIFIERS_KEY_PREFIX; +use crate::types::storage::{BlockHeight, Epoch, Key, KeySeg, TxIndex}; +use crate::types::token::{ + Transfer, HEAD_TX_KEY, MASP_NULLIFIERS_KEY_PREFIX, PIN_KEY_PREFIX, + TX_KEY_PREFIX, +}; -/// Writes the nullifiers of the provided masp transaction to storage -pub fn reveal_nullifiers( +// Writes the nullifiers of the provided masp transaction to storage +fn reveal_nullifiers( ctx: &mut impl StorageWrite, transaction: &Transaction, ) -> Result<()> { @@ -28,3 +31,43 @@ pub fn reveal_nullifiers( Ok(()) } + +/// Handle a MASP transaction. +pub fn handle_masp_tx( + ctx: &mut (impl StorageRead + StorageWrite), + transfer: &Transfer, + shielded: &Transaction, +) -> Result<()> { + let masp_addr = MASP; + let head_tx_key = Key::from(masp_addr.to_db_key()) + .push(&HEAD_TX_KEY.to_owned()) + .expect("Cannot obtain a storage key"); + let current_tx_idx: u64 = + ctx.read(&head_tx_key).unwrap_or(None).unwrap_or(0); + let current_tx_key = Key::from(masp_addr.to_db_key()) + .push(&(TX_KEY_PREFIX.to_owned() + ¤t_tx_idx.to_string())) + .expect("Cannot obtain a storage key"); + // Save the Transfer object and its location within the blockchain + // so that clients do not have to separately look these + // up + let record: (Epoch, BlockHeight, TxIndex, Transfer, Transaction) = ( + ctx.get_block_epoch()?, + ctx.get_block_height()?, + ctx.get_tx_index()?, + transfer.clone(), + shielded.clone(), + ); + ctx.write(¤t_tx_key, record)?; + ctx.write(&head_tx_key, current_tx_idx + 1)?; + reveal_nullifiers(ctx, shielded)?; + + // If storage key has been supplied, then pin this transaction to it + if let Some(key) = &transfer.key { + let pin_key = Key::from(masp_addr.to_db_key()) + .push(&(PIN_KEY_PREFIX.to_owned() + key)) + .expect("Cannot obtain a storage key"); + ctx.write(&pin_key, current_tx_idx)?; + } + + Ok(()) +} diff --git a/shared/src/ledger/native_vp/ibc/context.rs b/shared/src/ledger/native_vp/ibc/context.rs index 6bb6f90e1c..1c806afe35 100644 --- a/shared/src/ledger/native_vp/ibc/context.rs +++ b/shared/src/ledger/native_vp/ibc/context.rs @@ -3,7 +3,6 @@ use std::collections::{BTreeSet, HashMap, HashSet}; use borsh_ext::BorshSerializeExt; -use masp_primitives::transaction::Transaction; use namada_core::ledger::ibc::{IbcCommonContext, IbcStorageContext}; use namada_core::ledger::masp_utils; @@ -12,15 +11,12 @@ use crate::ledger::native_vp::CtxPreStorageRead; use crate::ledger::storage::write_log::StorageModification; use crate::ledger::storage::{self as ledger_storage, StorageHasher}; use crate::ledger::storage_api::{self, StorageRead, StorageWrite}; -use crate::types::address::{Address, InternalAddress, MASP}; +use crate::types::address::{Address, InternalAddress}; use crate::types::ibc::{IbcEvent, IbcShieldedTransfer}; use crate::types::storage::{ - BlockHash, BlockHeight, Epoch, Header, Key, KeySeg, TxIndex, -}; -use crate::types::token::{ - self, Amount, DenominatedAmount, Transfer, HEAD_TX_KEY, PIN_KEY_PREFIX, - TX_KEY_PREFIX, + BlockHash, BlockHeight, Epoch, Header, Key, TxIndex, }; +use crate::types::token::{self, Amount, DenominatedAmount}; use crate::vm::WasmCacheAccess; /// Result of a storage API call. @@ -217,36 +213,7 @@ where } fn handle_masp_tx(&mut self, shielded: &IbcShieldedTransfer) -> Result<()> { - let masp_addr = MASP; - let head_tx_key = Key::from(masp_addr.to_db_key()) - .push(&HEAD_TX_KEY.to_owned()) - .expect("Cannot obtain a storage key"); - let current_tx_idx: u64 = - self.ctx.read(&head_tx_key).unwrap_or(None).unwrap_or(0); - let current_tx_key = Key::from(masp_addr.to_db_key()) - .push(&(TX_KEY_PREFIX.to_owned() + ¤t_tx_idx.to_string())) - .expect("Cannot obtain a storage key"); - // Save the Transfer object and its location within the blockchain - // so that clients do not have to separately look these - // up - let record: (Epoch, BlockHeight, TxIndex, Transfer, Transaction) = ( - self.ctx.get_block_epoch()?, - self.ctx.get_block_height()?, - self.ctx.get_tx_index()?, - shielded.transfer.clone(), - shielded.masp_tx.clone(), - ); - self.write(¤t_tx_key, record.serialize_to_vec())?; - self.write(&head_tx_key, (current_tx_idx + 1).serialize_to_vec())?; - masp_utils::reveal_nullifiers(self, &shielded.masp_tx)?; - // If storage key has been supplied, then pin this transaction to it - if let Some(key) = &shielded.transfer.key { - let pin_key = Key::from(masp_addr.to_db_key()) - .push(&(PIN_KEY_PREFIX.to_owned() + key)) - .expect("Cannot obtain a storage key"); - self.write(&pin_key, current_tx_idx.serialize_to_vec())?; - } - Ok(()) + masp_utils::handle_masp_tx(self, &shielded.transfer, &shielded.masp_tx) } fn mint_token( diff --git a/shared/src/vm/host_env.rs b/shared/src/vm/host_env.rs index 1bef64fdb2..0e3fc21929 100644 --- a/shared/src/vm/host_env.rs +++ b/shared/src/vm/host_env.rs @@ -6,12 +6,11 @@ use std::num::TryFromIntError; use borsh::BorshDeserialize; use borsh_ext::BorshSerializeExt; -use masp_primitives::transaction::Transaction; use namada_core::ledger::gas::{ GasMetering, TxGasMeter, MEMORY_ACCESS_GAS_PER_BYTE, }; use namada_core::ledger::masp_utils; -use namada_core::types::address::{ESTABLISHED_ADDRESS_BYTES_LEN, MASP}; +use namada_core::types::address::ESTABLISHED_ADDRESS_BYTES_LEN; use namada_core::types::internal::KeyVal; use namada_core::types::storage::TX_INDEX_LENGTH; use namada_core::types::transaction::TxSentinel; @@ -33,10 +32,9 @@ use crate::types::address::{self, Address}; use crate::types::hash::Hash; use crate::types::ibc::{IbcEvent, IbcShieldedTransfer}; use crate::types::internal::HostEnvResult; -use crate::types::storage::{BlockHeight, Epoch, Key, KeySeg, TxIndex}; +use crate::types::storage::{BlockHeight, Epoch, Key, TxIndex}; use crate::types::token::{ is_any_minted_balance_key, is_any_minter_key, is_any_token_balance_key, - Transfer, HEAD_TX_KEY, PIN_KEY_PREFIX, TX_KEY_PREFIX, }; use crate::vm::memory::VmMemory; use crate::vm::prefix_iter::{PrefixIteratorId, PrefixIterators}; @@ -2521,36 +2519,7 @@ where &mut self, shielded: &IbcShieldedTransfer, ) -> Result<(), storage_api::Error> { - let masp_addr = MASP; - let head_tx_key = Key::from(masp_addr.to_db_key()) - .push(&HEAD_TX_KEY.to_owned()) - .expect("Cannot obtain a storage key"); - let current_tx_idx = - self.read::(&head_tx_key).unwrap_or(None).unwrap_or(0); - let current_tx_key = Key::from(masp_addr.to_db_key()) - .push(&(TX_KEY_PREFIX.to_owned() + ¤t_tx_idx.to_string())) - .expect("Cannot obtain a storage key"); - // Save the Transfer object and its location within the blockchain - // so that clients do not have to separately look these - // up - let record: (Epoch, BlockHeight, TxIndex, Transfer, Transaction) = ( - self.get_block_epoch()?, - self.get_block_height()?, - self.get_tx_index()?, - shielded.transfer.clone(), - shielded.masp_tx.clone(), - ); - self.write(¤t_tx_key, record)?; - self.write(&head_tx_key, current_tx_idx + 1)?; - masp_utils::reveal_nullifiers(self, &shielded.masp_tx)?; - // If storage key has been supplied, then pin this transaction to it - if let Some(key) = &shielded.transfer.key { - let pin_key = Key::from(masp_addr.to_db_key()) - .push(&(PIN_KEY_PREFIX.to_owned() + key)) - .expect("Cannot obtain a storage key"); - self.write(&pin_key, current_tx_idx)?; - } - Ok(()) + masp_utils::handle_masp_tx(self, &shielded.transfer, &shielded.masp_tx) } fn mint_token( diff --git a/tx_prelude/src/ibc.rs b/tx_prelude/src/ibc.rs index dfd0430e5b..3e9382b59a 100644 --- a/tx_prelude/src/ibc.rs +++ b/tx_prelude/src/ibc.rs @@ -6,12 +6,13 @@ use std::rc::Rc; pub use namada_core::ledger::ibc::{ IbcActions, IbcCommonContext, IbcStorageContext, ProofSpec, TransferModule, }; +use namada_core::ledger::masp_utils; use namada_core::ledger::tx_env::TxEnv; use namada_core::types::address::{Address, InternalAddress}; pub use namada_core::types::ibc::{IbcEvent, IbcShieldedTransfer}; use namada_core::types::token::DenominatedAmount; -use crate::token::{burn, handle_masp_tx, mint, transfer}; +use crate::token::{burn, mint, transfer}; use crate::{Ctx, Error}; /// IBC actions to handle an IBC message @@ -52,7 +53,7 @@ impl IbcStorageContext for Ctx { &mut self, shielded: &IbcShieldedTransfer, ) -> Result<(), Error> { - handle_masp_tx(self, &shielded.transfer, &shielded.masp_tx) + masp_utils::handle_masp_tx(self, &shielded.transfer, &shielded.masp_tx) } fn mint_token( diff --git a/tx_prelude/src/token.rs b/tx_prelude/src/token.rs index ac4a4ba1da..70a874bdd8 100644 --- a/tx_prelude/src/token.rs +++ b/tx_prelude/src/token.rs @@ -1,7 +1,5 @@ -use masp_primitives::transaction::Transaction; -use namada_core::ledger::masp_utils; -use namada_core::types::address::{Address, MASP}; -use namada_core::types::storage::KeySeg; +pub use namada_core::ledger::masp_utils; +use namada_core::types::address::Address; use namada_core::types::token; pub use namada_core::types::token::*; @@ -33,45 +31,6 @@ pub fn transfer( Ok(()) } -/// Handle a MASP transaction. -pub fn handle_masp_tx( - ctx: &mut Ctx, - transfer: &Transfer, - shielded: &Transaction, -) -> TxResult { - let masp_addr = MASP; - ctx.insert_verifier(&masp_addr)?; - let head_tx_key = storage::Key::from(masp_addr.to_db_key()) - .push(&HEAD_TX_KEY.to_owned()) - .expect("Cannot obtain a storage key"); - let current_tx_idx: u64 = - ctx.read(&head_tx_key).unwrap_or(None).unwrap_or(0); - let current_tx_key = storage::Key::from(masp_addr.to_db_key()) - .push(&(TX_KEY_PREFIX.to_owned() + ¤t_tx_idx.to_string())) - .expect("Cannot obtain a storage key"); - // Save the Transfer object and its location within the blockchain - // so that clients do not have to separately look these - // up - let record: (Epoch, BlockHeight, TxIndex, Transfer, Transaction) = ( - ctx.get_block_epoch()?, - ctx.get_block_height()?, - ctx.get_tx_index()?, - transfer.clone(), - shielded.clone(), - ); - ctx.write(¤t_tx_key, record)?; - ctx.write(&head_tx_key, current_tx_idx + 1)?; - masp_utils::reveal_nullifiers(ctx, shielded)?; - // If storage key has been supplied, then pin this transaction to it - if let Some(key) = &transfer.key { - let pin_key = storage::Key::from(masp_addr.to_db_key()) - .push(&(PIN_KEY_PREFIX.to_owned() + key)) - .expect("Cannot obtain a storage key"); - ctx.write(&pin_key, current_tx_idx)?; - } - Ok(()) -} - /// Mint that can be used in a transaction. pub fn mint( ctx: &mut Ctx, diff --git a/wasm/wasm_source/src/tx_transfer.rs b/wasm/wasm_source/src/tx_transfer.rs index e92750e016..e473ef1d9e 100644 --- a/wasm/wasm_source/src/tx_transfer.rs +++ b/wasm/wasm_source/src/tx_transfer.rs @@ -38,7 +38,7 @@ fn apply_tx(ctx: &mut Ctx, tx_data: Tx) -> TxResult { }) .transpose()?; if let Some(shielded) = shielded { - token::handle_masp_tx(ctx, &transfer, &shielded)?; + token::masp_utils::handle_masp_tx(ctx, &transfer, &shielded)?; } Ok(()) } From 4fb5142c7326717c7662c7c4e99e5d994081ba4b Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 1 Dec 2023 19:07:29 +0100 Subject: [PATCH 08/24] Updates masp tx with note commitment tree and anchor --- Cargo.lock | 1 + apps/src/lib/node/ledger/shell/init_chain.rs | 28 +++++++++++ core/Cargo.toml | 1 + core/src/ledger/masp_utils.rs | 52 +++++++++++++++++++- core/src/types/token.rs | 17 ++++++- wasm/Cargo.lock | 1 + wasm_for_tests/wasm_source/Cargo.lock | 1 + 7 files changed, 99 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a760f39701..c87d9e7c49 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3930,6 +3930,7 @@ dependencies = [ "itertools 0.10.5", "k256", "masp_primitives", + "masp_proofs", "namada_macros", "num-integer", "num-rational 0.4.1", diff --git a/apps/src/lib/node/ledger/shell/init_chain.rs b/apps/src/lib/node/ledger/shell/init_chain.rs index aad1fcc362..12a18469b2 100644 --- a/apps/src/lib/node/ledger/shell/init_chain.rs +++ b/apps/src/lib/node/ledger/shell/init_chain.rs @@ -2,6 +2,9 @@ use std::collections::HashMap; use std::hash::Hash; +use masp_primitives::merkle_tree::CommitmentTree; +use masp_primitives::sapling::Node; +use masp_proofs::bls12_381; use namada::ledger::parameters::Parameters; use namada::ledger::storage::traits::StorageHasher; use namada::ledger::storage::{DBIter, DB}; @@ -9,10 +12,14 @@ use namada::ledger::storage_api::token::{credit_tokens, write_denom}; use namada::ledger::storage_api::StorageWrite; use namada::ledger::{ibc, pos}; use namada::proof_of_stake::BecomeValidator; +use namada::types::address::MASP; use namada::types::hash::Hash as CodeHash; use namada::types::key::*; use namada::types::storage::KeySeg; use namada::types::time::{DateTimeUtc, TimeZone, Utc}; +use namada::types::token::{ + MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE, +}; use namada::vm::validate_untrusted_wasm; use namada_sdk::eth_bridge::EthBridgeStatus; use namada_sdk::proof_of_stake::types::ValidatorMetaData; @@ -172,6 +179,27 @@ where ibc::init_genesis_storage(&mut self.wl_storage); + // Init masp commitment tree and anchor + let empty_commitment_tree: CommitmentTree = + CommitmentTree::empty(); + let anchor = empty_commitment_tree.root(); + let note_commitment_tree_key = Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_TREE.to_owned()) + .expect("Cannot obtain a storage key"); + self.wl_storage + .write(¬e_commitment_tree_key, empty_commitment_tree) + .unwrap(); + let commitment_tree_anchor_key = Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_ANCHOR_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&namada::core::types::hash::Hash( + bls12_381::Scalar::from(anchor).to_bytes(), + )) + .expect("Cannot obtain a storage key"); + self.wl_storage + .write(&commitment_tree_anchor_key, ()) + .unwrap(); + // Set the initial validator set response.validators = self .get_abci_validator_updates(true, |pk, power| { diff --git a/core/Cargo.toml b/core/Cargo.toml index 53ee824077..e2e2351087 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -51,6 +51,7 @@ index-set.workspace = true itertools.workspace = true k256.workspace = true masp_primitives.workspace = true +masp_proofs.workspace = true num256.workspace = true num_enum = "0.7.0" num-integer = "0.1.45" diff --git a/core/src/ledger/masp_utils.rs b/core/src/ledger/masp_utils.rs index 4ddc2bc49a..9eaeb2faa7 100644 --- a/core/src/ledger/masp_utils.rs +++ b/core/src/ledger/masp_utils.rs @@ -1,9 +1,12 @@ //! MASP utilities +use masp_primitives::merkle_tree::CommitmentTree; +use masp_primitives::sapling::Node; use masp_primitives::transaction::Transaction; +use masp_proofs::bls12_381; use super::storage_api::{StorageRead, StorageWrite}; -use crate::ledger::storage_api::Result; +use crate::ledger::storage_api::{Error, Result}; use crate::types::address::MASP; use crate::types::hash::Hash; use crate::types::storage::{BlockHeight, Epoch, Key, KeySeg, TxIndex}; @@ -11,6 +14,10 @@ use crate::types::token::{ Transfer, HEAD_TX_KEY, MASP_NULLIFIERS_KEY_PREFIX, PIN_KEY_PREFIX, TX_KEY_PREFIX, }; +use crate::types::token::{ + MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE, + MASP_NULLIFIERS_KEY_PREFIX, +}; // Writes the nullifiers of the provided masp transaction to storage fn reveal_nullifiers( @@ -32,6 +39,48 @@ fn reveal_nullifiers( Ok(()) } +// Appends the note commitments of the provided transaction to the merkle tree +// and updates the anchor +fn update_note_commitment_tree( + ctx: &mut (impl StorageRead + StorageWrite), + transaction: &Transaction, +) -> Result<()> { + if let Some(bundle) = transaction.sapling_bundle() { + if !bundle.shielded_outputs.is_empty() { + let tree_key = Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_TREE.to_owned()) + .expect("Cannot obtain a storage key"); + let mut spend_tree = ctx + .read::>(&tree_key)? + .ok_or(Error::SimpleMessage( + "Missing note commitment tree in storage", + ))?; + + for description in &bundle.shielded_outputs { + // Add cmu to the merkle tree + spend_tree + .append(Node::from_scalar(description.cmu)) + .map_err(|_| { + Error::SimpleMessage("Note commitment tree is full") + })?; + } + + // Write the tree back to storage and update the anchor + let updated_anchor = spend_tree.root(); + ctx.write(&tree_key, spend_tree)?; + let anchor_key = Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_ANCHOR_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&Hash(bls12_381::Scalar::from(updated_anchor).to_bytes())) + .expect("Cannot obtain a storage key"); + + ctx.write(&anchor_key, ())?; + } + } + + Ok(()) +} + /// Handle a MASP transaction. pub fn handle_masp_tx( ctx: &mut (impl StorageRead + StorageWrite), @@ -59,6 +108,7 @@ pub fn handle_masp_tx( ); ctx.write(¤t_tx_key, record)?; ctx.write(&head_tx_key, current_tx_idx + 1)?; + update_note_commitment_tree(ctx, shielded)?; reveal_nullifiers(ctx, shielded)?; // If storage key has been supplied, then pin this transaction to it diff --git a/core/src/types/token.rs b/core/src/types/token.rs index 3cb820f4a3..1df1ea37da 100644 --- a/core/src/types/token.rs +++ b/core/src/types/token.rs @@ -902,6 +902,10 @@ pub const TX_KEY_PREFIX: &str = "tx-"; pub const PIN_KEY_PREFIX: &str = "pin-"; /// Key segment prefix for the nullifiers pub const MASP_NULLIFIERS_KEY_PREFIX: &str = "nullifiers"; +/// Key segment prefix for the note commitment merkle tree +pub const MASP_NOTE_COMMITMENT_TREE: &str = "spend_tree"; +/// Key segment prefix for the note commitment anchor +pub const MASP_NOTE_COMMITMENT_ANCHOR_PREFIX: &str = "spend_anchor"; /// Last calculated inflation value handed out pub const MASP_LAST_INFLATION_KEY: &str = "last_inflation"; /// The last locked ratio @@ -1127,7 +1131,9 @@ pub fn is_masp_key(key: &Key) -> bool { && (key == HEAD_TX_KEY || key.starts_with(TX_KEY_PREFIX) || key.starts_with(PIN_KEY_PREFIX) - || key.starts_with(MASP_NULLIFIERS_KEY_PREFIX))) + || key.starts_with(MASP_NULLIFIERS_KEY_PREFIX) + || key == MASP_NOTE_COMMITMENT_TREE + || key.starts_with(MASP_NOTE_COMMITMENT_ANCHOR_PREFIX))) } /// Check if the given storage key is a masp nullifier key @@ -1139,6 +1145,15 @@ pub fn is_masp_nullifier_key(key: &Key) -> bool { ] if *addr == MASP && prefix == MASP_NULLIFIERS_KEY_PREFIX) } +/// Check if the given storage key is a masp anchor key +pub fn is_masp_anchor_key(key: &Key) -> bool { + matches!(&key.segments[..], + [DbKeySeg::AddressSeg(addr), + DbKeySeg::StringSeg(prefix), + .. + ] if *addr == MASP && prefix == MASP_NOTE_COMMITMENT_ANCHOR_PREFIX) +} + /// Obtain the storage key for the last locked ratio of a token pub fn masp_last_locked_ratio_key(token_address: &Address) -> Key { key_of_token( diff --git a/wasm/Cargo.lock b/wasm/Cargo.lock index 66a3bb5817..7c6464415f 100644 --- a/wasm/Cargo.lock +++ b/wasm/Cargo.lock @@ -3020,6 +3020,7 @@ dependencies = [ "itertools 0.10.5", "k256", "masp_primitives", + "masp_proofs", "namada_macros", "num-integer", "num-rational 0.4.1", diff --git a/wasm_for_tests/wasm_source/Cargo.lock b/wasm_for_tests/wasm_source/Cargo.lock index 133935e7f0..5f66a044d6 100644 --- a/wasm_for_tests/wasm_source/Cargo.lock +++ b/wasm_for_tests/wasm_source/Cargo.lock @@ -3020,6 +3020,7 @@ dependencies = [ "itertools 0.10.5", "k256", "masp_primitives", + "masp_proofs", "namada_macros", "num-integer", "num-rational 0.4.1", From cf1ad179c7374a1d91c1a0d96d08bde15c3dbf6e Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Sat, 2 Dec 2023 22:27:08 +0100 Subject: [PATCH 09/24] Updates masp vp to validate note commitment tree and anchor --- shared/src/ledger/native_vp/masp.rs | 139 +++++++++++++++++++++++++++- 1 file changed, 137 insertions(+), 2 deletions(-) diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index d2b9a9b56b..607843adf8 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -5,7 +5,11 @@ use std::collections::{BTreeSet, HashSet}; use borsh_ext::BorshSerializeExt; use masp_primitives::asset_type::AssetType; +use masp_primitives::merkle_tree::CommitmentTree; +use masp_primitives::sapling::Node; use masp_primitives::transaction::components::I128Sum; +use masp_primitives::transaction::Transaction; +use masp_proofs::bls12_381; use namada_core::ledger::gas::MASP_VERIFY_SHIELDED_TX_GAS; use namada_core::ledger::storage; use namada_core::ledger::storage_api::OptionExt; @@ -15,7 +19,9 @@ use namada_core::types::address::InternalAddress::Masp; use namada_core::types::address::{Address, MASP}; use namada_core::types::storage::{Epoch, Key, KeySeg}; use namada_core::types::token::{ - self, is_masp_nullifier_key, MASP_NULLIFIERS_KEY_PREFIX, + self, is_masp_anchor_key, is_masp_nullifier_key, + MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE, + MASP_NULLIFIERS_KEY_PREFIX, }; use namada_sdk::masp::verify_shielded_tx; use ripemd::Digest as RipemdDigest; @@ -110,6 +116,121 @@ fn convert_amount( (asset_type, amount) } +impl<'a, DB, H, CA> MaspVp<'a, DB, H, CA> +where + DB: 'static + storage::DB + for<'iter> storage::DBIter<'iter>, + H: 'static + storage::StorageHasher, + CA: 'static + WasmCacheAccess, +{ + // Check that a transaction carrying output descriptions correctly updates + // the tree and anchor in storage + fn validate_note_commitment_update( + &self, + keys_changed: &BTreeSet, + transaction: &Transaction, + ) -> Result { + // Check that the merkle tree in storage has been correctly updated with + // the output descriptions cmu + let tree_key = Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_TREE.to_owned()) + .expect("Cannot obtain a storage key"); + let mut previous_tree: CommitmentTree = + self.ctx.read_pre(&tree_key)?.ok_or(Error::NativeVpError( + native_vp::Error::SimpleMessage("Cannot read storage"), + ))?; + let post_tree: CommitmentTree = + self.ctx.read_post(&tree_key)?.ok_or(Error::NativeVpError( + native_vp::Error::SimpleMessage("Cannot read storage"), + ))?; + + // Based on the output descriptions of the transaction, update the + // previous tree in storage + for description in transaction + .sapling_bundle() + .map_or(&vec![], |bundle| &bundle.shielded_outputs) + { + previous_tree + .append(Node::from_scalar(description.cmu)) + .map_err(|()| { + Error::NativeVpError(native_vp::Error::SimpleMessage( + "Failed to update the commitment tree", + )) + })?; + } + // Check that the updated previous tree matches the actual post tree. + // This verifies that all and only the necessary notes have been + // appended to the tree + if previous_tree != post_tree { + tracing::debug!("The note commitment tree was incorrectly updated"); + return Ok(false); + } + + // Check that only one anchor was published + if keys_changed + .iter() + .filter(|key| is_masp_anchor_key(key)) + .count() + != 1 + { + tracing::debug!( + "More than one MASP anchor keys havebeen revealed by the \ + transaction" + ); + return Ok(false); + } + + // Check that the new anchor has been written to storage + let updated_anchor_key = Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_ANCHOR_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&namada_core::types::hash::Hash( + bls12_381::Scalar::from(previous_tree.root()).to_bytes(), + )) + .expect("Cannot obtain a storage key"); + + match self.ctx.read_bytes_post(&updated_anchor_key)? { + Some(value) if value.is_empty() => (), + _ => { + tracing::debug!( + "The commitment tree anchor was not updated correctly" + ); + return Ok(false); + } + } + + Ok(true) + } + + // Check that the spend descriptions anchors of a transaction are valid + fn validate_spend_descriptions_anchor( + &self, + transaction: &Transaction, + ) -> Result { + for description in transaction + .sapling_bundle() + .map_or(&vec![], |bundle| &bundle.shielded_spends) + { + let anchor_key = Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_ANCHOR_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&namada_core::types::hash::Hash( + description.anchor.to_bytes(), + )) + .expect("Cannot obtain a storage key"); + + // Check if the provided anchor was published before + if !self.ctx.has_key_pre(&anchor_key)? { + tracing::debug!( + "Spend description refers to an invalid anchor" + ); + return Ok(false); + } + } + + Ok(true) + } +} + impl<'a, DB, H, CA> NativeVp for MaspVp<'a, DB, H, CA> where DB: 'static + storage::DB + for<'iter> storage::DBIter<'iter>, @@ -153,7 +274,8 @@ where // 2. the transparent transaction value pool's amount must equal // the containing wrapper transaction's fee // amount Satisfies 1. - // 3. The nullifiers provided by the transaction have not been + // 3. The spend descriptions' anchors are valid + // 4. The nullifiers provided by the transaction have not been // revealed previously (even in the same tx) and no unneeded // nullifier is being revealed by the tx if let Some(transp_bundle) = shielded_tx.transparent_bundle() { @@ -167,6 +289,10 @@ where } } + if !self.validate_spend_descriptions_anchor(&shielded_tx)? { + return Ok(false); + } + let mut revealed_nullifiers = HashSet::new(); for description in shielded_tx .sapling_bundle() @@ -304,6 +430,8 @@ where // Handle shielded output // The following boundary conditions must be satisfied // 1. Zero transparent output + // 2. The transaction must correctly update the note commitment tree + // and the anchor in storage with the new output descriptions // Satisfies 1. if let Some(transp_bundle) = shielded_tx.transparent_bundle() { @@ -316,6 +444,13 @@ where return Ok(false); } } + + // Satisfies 2 + if !self + .validate_note_commitment_update(keys_changed, &shielded_tx)? + { + return Ok(false); + } } match transparent_tx_pool.partial_cmp(&I128Sum::zero()) { From 61a46de2e3461fbee48e176734a43e59c628fc2c Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Sat, 2 Dec 2023 22:46:56 +0100 Subject: [PATCH 10/24] Refactors masp nullifiers check in a separate function --- shared/src/ledger/native_vp/masp.rs | 113 +++++++++++++++------------- 1 file changed, 60 insertions(+), 53 deletions(-) diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index 607843adf8..3e705f9606 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -122,9 +122,63 @@ where H: 'static + storage::StorageHasher, CA: 'static + WasmCacheAccess, { + // Check that the transaction correctly revealed the nullifiers + fn valid_nullifiers_reveal( + &self, + keys_changed: &BTreeSet, + transaction: &Transaction, + ) -> Result { + let mut revealed_nullifiers = HashSet::new(); + for description in transaction + .sapling_bundle() + .map_or(&vec![], |description| &description.shielded_spends) + { + let nullifier_key = Key::from(MASP.to_db_key()) + .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&namada_core::types::hash::Hash(description.nullifier.0)) + .expect("Cannot obtain a storage key"); + if self.ctx.has_key_pre(&nullifier_key)? + || revealed_nullifiers.contains(&nullifier_key) + { + tracing::debug!( + "MASP double spending attempt, the nullifier {:#?} has \ + already been revealed previously", + description.nullifier.0 + ); + return Ok(false); + } + + // Check that the nullifier is indeed committed (no temp write + // and no delete) and carries no associated data (the latter not + // strictly necessary for validation, but we don't expect any + // value for this key anyway) + match self.ctx.read_bytes_post(&nullifier_key)? { + Some(value) if value.is_empty() => (), + _ => return Ok(false), + } + + revealed_nullifiers.insert(nullifier_key); + } + + for nullifier_key in + keys_changed.iter().filter(|key| is_masp_nullifier_key(key)) + { + if !revealed_nullifiers.contains(nullifier_key) { + tracing::debug!( + "An unexpected MASP nullifier key {nullifier_key} has \ + been revealed by the transaction" + ); + return Ok(false); + } + } + + Ok(true) + } + // Check that a transaction carrying output descriptions correctly updates // the tree and anchor in storage - fn validate_note_commitment_update( + fn valid_note_commitment_update( &self, keys_changed: &BTreeSet, transaction: &Transaction, @@ -202,7 +256,7 @@ where } // Check that the spend descriptions anchors of a transaction are valid - fn validate_spend_descriptions_anchor( + fn valid_spend_descriptions_anchor( &self, transaction: &Transaction, ) -> Result { @@ -289,55 +343,10 @@ where } } - if !self.validate_spend_descriptions_anchor(&shielded_tx)? { - return Ok(false); - } - - let mut revealed_nullifiers = HashSet::new(); - for description in shielded_tx - .sapling_bundle() - .map_or(&vec![], |description| &description.shielded_spends) - { - let nullifier_key = Key::from(MASP.to_db_key()) - .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) - .expect("Cannot obtain a storage key") - .push(&namada_core::types::hash::Hash( - description.nullifier.0, - )) - .expect("Cannot obtain a storage key"); - if self.ctx.has_key_pre(&nullifier_key)? - || revealed_nullifiers.contains(&nullifier_key) - { - tracing::debug!( - "MASP double spending attempt, the nullifier {:#?} \ - has already been revealed previously", - description.nullifier.0 - ); - return Ok(false); - } - - // Check that the nullifier is indeed committed (no temp write - // and no delete) and carries no associated data (the latter not - // strictly necessary for validation, but we don't expect any - // value for this key anyway) - match self.ctx.read_bytes_post(&nullifier_key)? { - Some(value) if value.is_empty() => (), - _ => return Ok(false), - } - - revealed_nullifiers.insert(nullifier_key); - } - - for nullifier_key in - keys_changed.iter().filter(|key| is_masp_nullifier_key(key)) + if !(self.valid_spend_descriptions_anchor(&shielded_tx)? + && self.valid_nullifiers_reveal(keys_changed, &shielded_tx)?) { - if !revealed_nullifiers.contains(nullifier_key) { - tracing::debug!( - "An unexpected MASP nullifier key {nullifier_key} has \ - been revealed by the transaction" - ); - return Ok(false); - } + return Ok(false); } } @@ -446,9 +455,7 @@ where } // Satisfies 2 - if !self - .validate_note_commitment_update(keys_changed, &shielded_tx)? - { + if !self.valid_note_commitment_update(keys_changed, &shielded_tx)? { return Ok(false); } } From 2183aaa999e8fe8e8409a4001f644b18d6c5ad9a Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Sat, 2 Dec 2023 23:38:37 +0100 Subject: [PATCH 11/24] Updates commitment tree anchor only once per block --- Cargo.lock | 1 - .../lib/node/ledger/shell/finalize_block.rs | 25 +++++++++++++++++ core/Cargo.toml | 1 - core/src/ledger/masp_utils.rs | 23 ++++----------- shared/src/ledger/native_vp/masp.rs | 28 +++---------------- wasm/Cargo.lock | 1 - wasm_for_tests/wasm_source/Cargo.lock | 1 - 7 files changed, 35 insertions(+), 45 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c87d9e7c49..a760f39701 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3930,7 +3930,6 @@ dependencies = [ "itertools 0.10.5", "k256", "masp_primitives", - "masp_proofs", "namada_macros", "num-integer", "num-rational 0.4.1", diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 257b766a03..6c3c67d3ac 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -1,9 +1,13 @@ //! Implementation of the `FinalizeBlock` ABCI++ method for the Shell use data_encoding::HEXUPPER; +use masp_primitives::merkle_tree::CommitmentTree; +use masp_primitives::sapling::Node; +use masp_proofs::bls12_381; use namada::core::ledger::inflation; use namada::core::ledger::masp_conversions::update_allowed_conversions; use namada::core::ledger::pgf::ADDRESS as pgf_address; +use namada::core::types::storage::KeySeg; use namada::ledger::events::EventType; use namada::ledger::gas::{GasMetering, TxGasMeter}; use namada::ledger::parameters::storage as params_storage; @@ -17,9 +21,13 @@ use namada::proof_of_stake::{ find_validator_by_raw_hash, read_last_block_proposer_address, read_pos_params, read_total_stake, write_last_block_proposer_address, }; +use namada::types::address::MASP; use namada::types::dec::Dec; use namada::types::key::tm_raw_hash_to_string; use namada::types::storage::{BlockHash, BlockResults, Epoch, Header}; +use namada::types::token::{ + MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE, +}; use namada::types::transaction::protocol::{ ethereum_tx_data_variants, ProtocolTxType, }; @@ -557,6 +565,23 @@ where tracing::info!("{}", stats); tracing::info!("{}", stats.format_tx_executed()); + // Update the MASP commitment tree anchor + let tree_key = Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_TREE.to_owned()) + .expect("Cannot obtain a storage key"); + let updated_tree: CommitmentTree = self + .wl_storage + .read(&tree_key)? + .expect("Missing note commitment tree in storage"); + let anchor_key = Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_ANCHOR_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&namada::core::types::hash::Hash( + bls12_381::Scalar::from(updated_tree.root()).to_bytes(), + )) + .expect("Cannot obtain a storage key"); + self.wl_storage.write(&anchor_key, ())?; + if update_for_tendermint { self.update_epoch(&mut response); // send the latest oracle configs. These may have changed due to diff --git a/core/Cargo.toml b/core/Cargo.toml index e2e2351087..53ee824077 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -51,7 +51,6 @@ index-set.workspace = true itertools.workspace = true k256.workspace = true masp_primitives.workspace = true -masp_proofs.workspace = true num256.workspace = true num_enum = "0.7.0" num-integer = "0.1.45" diff --git a/core/src/ledger/masp_utils.rs b/core/src/ledger/masp_utils.rs index 9eaeb2faa7..eb9918a707 100644 --- a/core/src/ledger/masp_utils.rs +++ b/core/src/ledger/masp_utils.rs @@ -3,7 +3,6 @@ use masp_primitives::merkle_tree::CommitmentTree; use masp_primitives::sapling::Node; use masp_primitives::transaction::Transaction; -use masp_proofs::bls12_381; use super::storage_api::{StorageRead, StorageWrite}; use crate::ledger::storage_api::{Error, Result}; @@ -15,8 +14,7 @@ use crate::types::token::{ TX_KEY_PREFIX, }; use crate::types::token::{ - MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE, - MASP_NULLIFIERS_KEY_PREFIX, + MASP_NOTE_COMMITMENT_TREE, MASP_NULLIFIERS_KEY_PREFIX, }; // Writes the nullifiers of the provided masp transaction to storage @@ -50,31 +48,22 @@ fn update_note_commitment_tree( let tree_key = Key::from(MASP.to_db_key()) .push(&MASP_NOTE_COMMITMENT_TREE.to_owned()) .expect("Cannot obtain a storage key"); - let mut spend_tree = ctx + let mut commitment_tree = ctx .read::>(&tree_key)? .ok_or(Error::SimpleMessage( - "Missing note commitment tree in storage", - ))?; + "Missing note commitment tree in storage", + ))?; for description in &bundle.shielded_outputs { // Add cmu to the merkle tree - spend_tree + commitment_tree .append(Node::from_scalar(description.cmu)) .map_err(|_| { Error::SimpleMessage("Note commitment tree is full") })?; } - // Write the tree back to storage and update the anchor - let updated_anchor = spend_tree.root(); - ctx.write(&tree_key, spend_tree)?; - let anchor_key = Key::from(MASP.to_db_key()) - .push(&MASP_NOTE_COMMITMENT_ANCHOR_PREFIX.to_owned()) - .expect("Cannot obtain a storage key") - .push(&Hash(bls12_381::Scalar::from(updated_anchor).to_bytes())) - .expect("Cannot obtain a storage key"); - - ctx.write(&anchor_key, ())?; + ctx.write(&tree_key, commitment_tree)?; } } diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index 3e705f9606..e4b8f3f53b 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -9,7 +9,6 @@ use masp_primitives::merkle_tree::CommitmentTree; use masp_primitives::sapling::Node; use masp_primitives::transaction::components::I128Sum; use masp_primitives::transaction::Transaction; -use masp_proofs::bls12_381; use namada_core::ledger::gas::MASP_VERIFY_SHIELDED_TX_GAS; use namada_core::ledger::storage; use namada_core::ledger::storage_api::OptionExt; @@ -219,39 +218,20 @@ where return Ok(false); } - // Check that only one anchor was published + // Check that no anchor was published (this is to be done by the + // protocol) if keys_changed .iter() .filter(|key| is_masp_anchor_key(key)) .count() - != 1 + != 0 { tracing::debug!( - "More than one MASP anchor keys havebeen revealed by the \ - transaction" + "The transaction revealed one or more MASP anchor keys" ); return Ok(false); } - // Check that the new anchor has been written to storage - let updated_anchor_key = Key::from(MASP.to_db_key()) - .push(&MASP_NOTE_COMMITMENT_ANCHOR_PREFIX.to_owned()) - .expect("Cannot obtain a storage key") - .push(&namada_core::types::hash::Hash( - bls12_381::Scalar::from(previous_tree.root()).to_bytes(), - )) - .expect("Cannot obtain a storage key"); - - match self.ctx.read_bytes_post(&updated_anchor_key)? { - Some(value) if value.is_empty() => (), - _ => { - tracing::debug!( - "The commitment tree anchor was not updated correctly" - ); - return Ok(false); - } - } - Ok(true) } diff --git a/wasm/Cargo.lock b/wasm/Cargo.lock index 7c6464415f..66a3bb5817 100644 --- a/wasm/Cargo.lock +++ b/wasm/Cargo.lock @@ -3020,7 +3020,6 @@ dependencies = [ "itertools 0.10.5", "k256", "masp_primitives", - "masp_proofs", "namada_macros", "num-integer", "num-rational 0.4.1", diff --git a/wasm_for_tests/wasm_source/Cargo.lock b/wasm_for_tests/wasm_source/Cargo.lock index 5f66a044d6..133935e7f0 100644 --- a/wasm_for_tests/wasm_source/Cargo.lock +++ b/wasm_for_tests/wasm_source/Cargo.lock @@ -3020,7 +3020,6 @@ dependencies = [ "itertools 0.10.5", "k256", "masp_primitives", - "masp_proofs", "namada_macros", "num-integer", "num-rational 0.4.1", From f7da77b2d4f62d2bc09f148d6199a6766e0ad8ab Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Sun, 3 Dec 2023 18:57:53 +0100 Subject: [PATCH 12/24] Updates the merkle tree anchor only if the tree changed --- .../lib/node/ledger/shell/finalize_block.rs | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 6c3c67d3ac..077feef2ad 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -14,9 +14,10 @@ use namada::ledger::parameters::storage as params_storage; use namada::ledger::pos::{namada_proof_of_stake, staking_token_address}; use namada::ledger::protocol; use namada::ledger::storage::wl_storage::WriteLogAndStorage; +use namada::ledger::storage::write_log::StorageModification; use namada::ledger::storage::EPOCH_SWITCH_BLOCKS_DELAY; use namada::ledger::storage_api::token::credit_tokens; -use namada::ledger::storage_api::{pgf, StorageRead, StorageWrite}; +use namada::ledger::storage_api::{pgf, ResultExt, StorageRead, StorageWrite}; use namada::proof_of_stake::{ find_validator_by_raw_hash, read_last_block_proposer_address, read_pos_params, read_total_stake, write_last_block_proposer_address, @@ -565,22 +566,24 @@ where tracing::info!("{}", stats); tracing::info!("{}", stats.format_tx_executed()); - // Update the MASP commitment tree anchor + // Update the MASP commitment tree anchor if the tree was updated let tree_key = Key::from(MASP.to_db_key()) .push(&MASP_NOTE_COMMITMENT_TREE.to_owned()) .expect("Cannot obtain a storage key"); - let updated_tree: CommitmentTree = self - .wl_storage - .read(&tree_key)? - .expect("Missing note commitment tree in storage"); - let anchor_key = Key::from(MASP.to_db_key()) - .push(&MASP_NOTE_COMMITMENT_ANCHOR_PREFIX.to_owned()) - .expect("Cannot obtain a storage key") - .push(&namada::core::types::hash::Hash( - bls12_381::Scalar::from(updated_tree.root()).to_bytes(), - )) - .expect("Cannot obtain a storage key"); - self.wl_storage.write(&anchor_key, ())?; + if let Some(StorageModification::Write { value }) = + self.wl_storage.write_log.read(&tree_key).0 + { + let updated_tree = CommitmentTree::::try_from_slice(value) + .into_storage_result()?; + let anchor_key = Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_ANCHOR_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&namada::core::types::hash::Hash( + bls12_381::Scalar::from(updated_tree.root()).to_bytes(), + )) + .expect("Cannot obtain a storage key"); + self.wl_storage.write(&anchor_key, ())?; + } if update_for_tendermint { self.update_epoch(&mut response); From 694b94970fe1536b572acb045432df126034ba08 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 6 Dec 2023 11:57:58 +0100 Subject: [PATCH 13/24] Fixes commitment tree validation in masp vp. Adds a workaround to update tree from the tx --- .../lib/node/ledger/shell/finalize_block.rs | 4 +- apps/src/lib/node/ledger/shell/init_chain.rs | 4 +- core/src/ledger/masp_utils.rs | 27 ++++++------ core/src/types/token.rs | 6 +-- shared/src/ledger/native_vp/ibc/context.rs | 7 ++- shared/src/ledger/native_vp/masp.rs | 17 ++++---- shared/src/vm/host_env.rs | 43 ++++++++++++++++++- shared/src/vm/wasm/host_env.rs | 1 + tx_prelude/src/ibc.rs | 7 ++- tx_prelude/src/lib.rs | 18 ++++++++ vm_env/src/lib.rs | 5 +++ wasm/wasm_source/src/tx_transfer.rs | 1 + 12 files changed, 108 insertions(+), 32 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 077feef2ad..ca3972b560 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -27,7 +27,7 @@ use namada::types::dec::Dec; use namada::types::key::tm_raw_hash_to_string; use namada::types::storage::{BlockHash, BlockResults, Epoch, Header}; use namada::types::token::{ - MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE, + MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE_KEY, }; use namada::types::transaction::protocol::{ ethereum_tx_data_variants, ProtocolTxType, @@ -568,7 +568,7 @@ where // Update the MASP commitment tree anchor if the tree was updated let tree_key = Key::from(MASP.to_db_key()) - .push(&MASP_NOTE_COMMITMENT_TREE.to_owned()) + .push(&MASP_NOTE_COMMITMENT_TREE_KEY.to_owned()) .expect("Cannot obtain a storage key"); if let Some(StorageModification::Write { value }) = self.wl_storage.write_log.read(&tree_key).0 diff --git a/apps/src/lib/node/ledger/shell/init_chain.rs b/apps/src/lib/node/ledger/shell/init_chain.rs index 12a18469b2..18d7acd749 100644 --- a/apps/src/lib/node/ledger/shell/init_chain.rs +++ b/apps/src/lib/node/ledger/shell/init_chain.rs @@ -18,7 +18,7 @@ use namada::types::key::*; use namada::types::storage::KeySeg; use namada::types::time::{DateTimeUtc, TimeZone, Utc}; use namada::types::token::{ - MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE, + MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE_KEY, }; use namada::vm::validate_untrusted_wasm; use namada_sdk::eth_bridge::EthBridgeStatus; @@ -184,7 +184,7 @@ where CommitmentTree::empty(); let anchor = empty_commitment_tree.root(); let note_commitment_tree_key = Key::from(MASP.to_db_key()) - .push(&MASP_NOTE_COMMITMENT_TREE.to_owned()) + .push(&MASP_NOTE_COMMITMENT_TREE_KEY.to_owned()) .expect("Cannot obtain a storage key"); self.wl_storage .write(¬e_commitment_tree_key, empty_commitment_tree) diff --git a/core/src/ledger/masp_utils.rs b/core/src/ledger/masp_utils.rs index eb9918a707..d15834f7a1 100644 --- a/core/src/ledger/masp_utils.rs +++ b/core/src/ledger/masp_utils.rs @@ -10,11 +10,8 @@ use crate::types::address::MASP; use crate::types::hash::Hash; use crate::types::storage::{BlockHeight, Epoch, Key, KeySeg, TxIndex}; use crate::types::token::{ - Transfer, HEAD_TX_KEY, MASP_NULLIFIERS_KEY_PREFIX, PIN_KEY_PREFIX, - TX_KEY_PREFIX, -}; -use crate::types::token::{ - MASP_NOTE_COMMITMENT_TREE, MASP_NULLIFIERS_KEY_PREFIX, + Transfer, HEAD_TX_KEY, MASP_NOTE_COMMITMENT_TREE_KEY, + MASP_NULLIFIERS_KEY_PREFIX, PIN_KEY_PREFIX, TX_KEY_PREFIX, }; // Writes the nullifiers of the provided masp transaction to storage @@ -37,20 +34,21 @@ fn reveal_nullifiers( Ok(()) } -// Appends the note commitments of the provided transaction to the merkle tree -// and updates the anchor -fn update_note_commitment_tree( +/// Appends the note commitments of the provided transaction to the merkle tree +/// and updates the anchor +/// NOTE: this function is public as a temporary workaround because of an issue +/// when running this function in WASM +pub fn update_note_commitment_tree( ctx: &mut (impl StorageRead + StorageWrite), transaction: &Transaction, ) -> Result<()> { if let Some(bundle) = transaction.sapling_bundle() { if !bundle.shielded_outputs.is_empty() { let tree_key = Key::from(MASP.to_db_key()) - .push(&MASP_NOTE_COMMITMENT_TREE.to_owned()) + .push(&MASP_NOTE_COMMITMENT_TREE_KEY.to_owned()) .expect("Cannot obtain a storage key"); - let mut commitment_tree = ctx - .read::>(&tree_key)? - .ok_or(Error::SimpleMessage( + let mut commitment_tree: CommitmentTree = + ctx.read(&tree_key)?.ok_or(Error::SimpleMessage( "Missing note commitment tree in storage", ))?; @@ -97,7 +95,10 @@ pub fn handle_masp_tx( ); ctx.write(¤t_tx_key, record)?; ctx.write(&head_tx_key, current_tx_idx + 1)?; - update_note_commitment_tree(ctx, shielded)?; + // TODO: temporarily disabled because of the node aggregation issue in WASM. + // Using the host env tx_update_masp_note_commitment_tree or directly the + // update_note_commitment_tree function as a workaround instead + // update_note_commitment_tree(ctx, shielded)?; reveal_nullifiers(ctx, shielded)?; // If storage key has been supplied, then pin this transaction to it diff --git a/core/src/types/token.rs b/core/src/types/token.rs index 1df1ea37da..b5737120d8 100644 --- a/core/src/types/token.rs +++ b/core/src/types/token.rs @@ -903,9 +903,9 @@ pub const PIN_KEY_PREFIX: &str = "pin-"; /// Key segment prefix for the nullifiers pub const MASP_NULLIFIERS_KEY_PREFIX: &str = "nullifiers"; /// Key segment prefix for the note commitment merkle tree -pub const MASP_NOTE_COMMITMENT_TREE: &str = "spend_tree"; +pub const MASP_NOTE_COMMITMENT_TREE_KEY: &str = "commitment_tree"; /// Key segment prefix for the note commitment anchor -pub const MASP_NOTE_COMMITMENT_ANCHOR_PREFIX: &str = "spend_anchor"; +pub const MASP_NOTE_COMMITMENT_ANCHOR_PREFIX: &str = "note_commitment_anchor"; /// Last calculated inflation value handed out pub const MASP_LAST_INFLATION_KEY: &str = "last_inflation"; /// The last locked ratio @@ -1132,7 +1132,7 @@ pub fn is_masp_key(key: &Key) -> bool { || key.starts_with(TX_KEY_PREFIX) || key.starts_with(PIN_KEY_PREFIX) || key.starts_with(MASP_NULLIFIERS_KEY_PREFIX) - || key == MASP_NOTE_COMMITMENT_TREE + || key == MASP_NOTE_COMMITMENT_TREE_KEY || key.starts_with(MASP_NOTE_COMMITMENT_ANCHOR_PREFIX))) } diff --git a/shared/src/ledger/native_vp/ibc/context.rs b/shared/src/ledger/native_vp/ibc/context.rs index 1c806afe35..c8d162c000 100644 --- a/shared/src/ledger/native_vp/ibc/context.rs +++ b/shared/src/ledger/native_vp/ibc/context.rs @@ -213,7 +213,12 @@ where } fn handle_masp_tx(&mut self, shielded: &IbcShieldedTransfer) -> Result<()> { - masp_utils::handle_masp_tx(self, &shielded.transfer, &shielded.masp_tx) + masp_utils::handle_masp_tx( + self, + &shielded.transfer, + &shielded.masp_tx, + )?; + masp_utils::update_note_commitment_tree(self, &shielded.masp_tx) } fn mint_token( diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index e4b8f3f53b..1ae3b6bdae 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -19,7 +19,7 @@ use namada_core::types::address::{Address, MASP}; use namada_core::types::storage::{Epoch, Key, KeySeg}; use namada_core::types::token::{ self, is_masp_anchor_key, is_masp_nullifier_key, - MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE, + MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE_KEY, MASP_NULLIFIERS_KEY_PREFIX, }; use namada_sdk::masp::verify_shielded_tx; @@ -185,7 +185,7 @@ where // Check that the merkle tree in storage has been correctly updated with // the output descriptions cmu let tree_key = Key::from(MASP.to_db_key()) - .push(&MASP_NOTE_COMMITMENT_TREE.to_owned()) + .push(&MASP_NOTE_COMMITMENT_TREE_KEY.to_owned()) .expect("Cannot obtain a storage key"); let mut previous_tree: CommitmentTree = self.ctx.read_pre(&tree_key)?.ok_or(Error::NativeVpError( @@ -330,6 +330,12 @@ where } } + // The transaction must correctly update the note commitment tree + // and the anchor in storage with the new output descriptions + if !self.valid_note_commitment_update(keys_changed, &shielded_tx)? { + return Ok(false); + } + if transfer.target != Address::Internal(Masp) { // Handle transparent output // The following boundary conditions must be satisfied @@ -419,8 +425,6 @@ where // Handle shielded output // The following boundary conditions must be satisfied // 1. Zero transparent output - // 2. The transaction must correctly update the note commitment tree - // and the anchor in storage with the new output descriptions // Satisfies 1. if let Some(transp_bundle) = shielded_tx.transparent_bundle() { @@ -433,11 +437,6 @@ where return Ok(false); } } - - // Satisfies 2 - if !self.valid_note_commitment_update(keys_changed, &shielded_tx)? { - return Ok(false); - } } match transparent_tx_pool.partial_cmp(&I128Sum::zero()) { diff --git a/shared/src/vm/host_env.rs b/shared/src/vm/host_env.rs index 0e3fc21929..f95b8a24dd 100644 --- a/shared/src/vm/host_env.rs +++ b/shared/src/vm/host_env.rs @@ -6,6 +6,7 @@ use std::num::TryFromIntError; use borsh::BorshDeserialize; use borsh_ext::BorshSerializeExt; +use masp_primitives::transaction::Transaction; use namada_core::ledger::gas::{ GasMetering, TxGasMeter, MEMORY_ACCESS_GAS_PER_BYTE, }; @@ -2164,6 +2165,41 @@ where } } +/// Appends the new note commitments to the tree in storage +pub fn tx_update_masp_note_commitment_tree( + env: &TxVmEnv, + transaction_ptr: u64, + transaction_len: u64, +) -> TxResult +where + MEM: VmMemory, + DB: storage::DB + for<'iter> storage::DBIter<'iter>, + H: StorageHasher, + CA: WasmCacheAccess, +{ + let _sentinel = unsafe { env.ctx.sentinel.get() }; + let _gas_meter = unsafe { env.ctx.gas_meter.get() }; + let (serialized_transaction, gas) = env + .memory + .read_bytes(transaction_ptr, transaction_len as _) + .map_err(|e| TxRuntimeError::MemoryError(Box::new(e)))?; + + tx_charge_gas(env, gas)?; + let transaction = Transaction::try_from_slice(&serialized_transaction) + .map_err(TxRuntimeError::EncodingError)?; + + let mut ctx = env.ctx.clone(); + match masp_utils::update_note_commitment_tree(&mut ctx, &transaction) { + Ok(()) => Ok(HostEnvResult::Success.to_i64()), + Err(_) => { + // NOTE: sentinel for gas errors is already set by the + // update_note_commitment_tree function which in turn calls other + // host functions + Ok(HostEnvResult::Fail.to_i64()) + } + } +} + /// Evaluate a validity predicate with the given input data. pub fn vp_eval( env: &VpVmEnv<'static, MEM, DB, H, EVAL, CA>, @@ -2519,7 +2555,12 @@ where &mut self, shielded: &IbcShieldedTransfer, ) -> Result<(), storage_api::Error> { - masp_utils::handle_masp_tx(self, &shielded.transfer, &shielded.masp_tx) + masp_utils::handle_masp_tx( + self, + &shielded.transfer, + &shielded.masp_tx, + )?; + masp_utils::update_note_commitment_tree(self, &shielded.masp_tx) } fn mint_token( diff --git a/shared/src/vm/wasm/host_env.rs b/shared/src/vm/wasm/host_env.rs index 8c897f1a29..304a27a7f2 100644 --- a/shared/src/vm/wasm/host_env.rs +++ b/shared/src/vm/wasm/host_env.rs @@ -88,6 +88,7 @@ where "namada_tx_ibc_execute" => Function::new_native_with_env(wasm_store, env.clone(), host_env::tx_ibc_execute), "namada_tx_set_commitment_sentinel" => Function::new_native_with_env(wasm_store, env.clone(), host_env::tx_set_commitment_sentinel), "namada_tx_verify_tx_section_signature" => Function::new_native_with_env(wasm_store, env.clone(), host_env::tx_verify_tx_section_signature), + "namada_tx_update_masp_note_commitment_tree" => Function::new_native_with_env(wasm_store, env.clone(), host_env::tx_update_masp_note_commitment_tree) }, } } diff --git a/tx_prelude/src/ibc.rs b/tx_prelude/src/ibc.rs index 3e9382b59a..38e907a4fe 100644 --- a/tx_prelude/src/ibc.rs +++ b/tx_prelude/src/ibc.rs @@ -53,7 +53,12 @@ impl IbcStorageContext for Ctx { &mut self, shielded: &IbcShieldedTransfer, ) -> Result<(), Error> { - masp_utils::handle_masp_tx(self, &shielded.transfer, &shielded.masp_tx) + masp_utils::handle_masp_tx( + self, + &shielded.transfer, + &shielded.masp_tx, + )?; + masp_utils::update_note_commitment_tree(self, &shielded.masp_tx) } fn mint_token( diff --git a/tx_prelude/src/lib.rs b/tx_prelude/src/lib.rs index 29e9881488..fbb6e03a78 100644 --- a/tx_prelude/src/lib.rs +++ b/tx_prelude/src/lib.rs @@ -19,6 +19,7 @@ use std::marker::PhantomData; pub use borsh::{BorshDeserialize, BorshSerialize}; pub use borsh_ext; use borsh_ext::BorshSerializeExt; +use masp_primitives::transaction::Transaction; pub use namada_core::ledger::governance::storage as gov_storage; pub use namada_core::ledger::parameters::storage as parameters_storage; pub use namada_core::ledger::storage::types::encode; @@ -404,3 +405,20 @@ pub fn verify_signatures_of_pks( Ok(HostEnvResult::is_success(valid)) } + +/// Update the masp note commitment tree in storage with the new notes +pub fn update_masp_note_commitment_tree( + transaction: &Transaction, +) -> EnvResult { + // Serialize transaction + let transaction = transaction.serialize_to_vec(); + + let valid = unsafe { + namada_tx_update_masp_note_commitment_tree( + transaction.as_ptr() as _, + transaction.len() as _, + ) + }; + + Ok(HostEnvResult::is_success(valid)) +} diff --git a/vm_env/src/lib.rs b/vm_env/src/lib.rs index 3b2c5c4dcc..f46ad44b94 100644 --- a/vm_env/src/lib.rs +++ b/vm_env/src/lib.rs @@ -133,6 +133,11 @@ pub mod tx { max_signatures_len: u64, ) -> i64; + /// Update the masp note commitment tree with the new notes + pub fn namada_tx_update_masp_note_commitment_tree( + transaction_ptr: u64, + transaction_len: u64, + ) -> i64; } } diff --git a/wasm/wasm_source/src/tx_transfer.rs b/wasm/wasm_source/src/tx_transfer.rs index e473ef1d9e..e521be4f75 100644 --- a/wasm/wasm_source/src/tx_transfer.rs +++ b/wasm/wasm_source/src/tx_transfer.rs @@ -39,6 +39,7 @@ fn apply_tx(ctx: &mut Ctx, tx_data: Tx) -> TxResult { .transpose()?; if let Some(shielded) = shielded { token::masp_utils::handle_masp_tx(ctx, &transfer, &shielded)?; + update_masp_note_commitment_tree(&shielded)?; } Ok(()) } From 8841aacf624303cc11f5612531a435363c0100ca Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 6 Dec 2023 15:43:38 +0100 Subject: [PATCH 14/24] Fixes masp vp benchmark --- Cargo.lock | 2 ++ benches/Cargo.toml | 2 ++ benches/native_vps.rs | 32 +++++++++++++++++++++++++++++++- 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index a760f39701..0ed05112b6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3896,6 +3896,8 @@ dependencies = [ "borsh", "borsh-ext", "criterion", + "masp_primitives", + "masp_proofs", "namada", "namada_apps", "rand 0.8.5", diff --git a/benches/Cargo.toml b/benches/Cargo.toml index 57de47dba4..b9d15a22f4 100644 --- a/benches/Cargo.toml +++ b/benches/Cargo.toml @@ -42,6 +42,8 @@ path = "host_env.rs" [dev-dependencies] namada = { path = "../shared", features = ["testing"] } namada_apps = { path = "../apps", features = ["benches"] } +masp_primitives.workspace = true +masp_proofs.workspace = true borsh.workspace = true borsh-ext.workspace = true criterion = { version = "0.5", features = ["html_reports"] } diff --git a/benches/native_vps.rs b/benches/native_vps.rs index fdaed5d1f9..def2e3c135 100644 --- a/benches/native_vps.rs +++ b/benches/native_vps.rs @@ -4,6 +4,8 @@ use std::rc::Rc; use std::str::FromStr; use criterion::{criterion_group, criterion_main, Criterion}; +use masp_primitives::sapling::Node; +use masp_proofs::bls12_381; use namada::core::ledger::governance::storage::proposal::ProposalType; use namada::core::ledger::governance::storage::vote::{ StorageProposalVote, VoteType, @@ -40,14 +42,18 @@ use namada::ledger::native_vp::{Ctx, NativeVp}; use namada::ledger::pgf::PgfVp; use namada::ledger::pos::PosVP; use namada::namada_sdk::masp::verify_shielded_tx; +use namada::namada_sdk::masp_primitives::merkle_tree::CommitmentTree; use namada::namada_sdk::masp_primitives::transaction::Transaction; use namada::proof_of_stake; use namada::proof_of_stake::KeySeg; use namada::proto::{Code, Section, Tx}; -use namada::types::address::InternalAddress; +use namada::types::address::{InternalAddress, MASP}; use namada::types::eth_bridge_pool::{GasFee, PendingTransfer}; use namada::types::masp::{TransferSource, TransferTarget}; use namada::types::storage::{Epoch, TxIndex}; +use namada::types::token::{ + MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE_KEY, +}; use namada::types::transaction::governance::{ InitProposalData, VoteProposalData, }; @@ -502,6 +508,30 @@ fn setup_storage_for_masp_verification( ); shielded_ctx.shell.execute_tx(&shield_tx); shielded_ctx.shell.wl_storage.commit_tx(); + + // Update the anchor in storage + let tree_key = namada::core::types::storage::Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_TREE_KEY.to_owned()) + .expect("Cannot obtain a storage key"); + let updated_tree: CommitmentTree = shielded_ctx + .shell + .wl_storage + .read(&tree_key) + .unwrap() + .unwrap(); + let anchor_key = namada::core::types::storage::Key::from(MASP.to_db_key()) + .push(&MASP_NOTE_COMMITMENT_ANCHOR_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&namada::core::types::hash::Hash( + bls12_381::Scalar::from(updated_tree.root()).to_bytes(), + )) + .expect("Cannot obtain a storage key"); + shielded_ctx + .shell + .wl_storage + .write(&anchor_key, ()) + .unwrap(); + shielded_ctx.shell.commit(); let signed_tx = match bench_name { From c09875fc14edb00f77995bee1aae6b27f25be880 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 6 Dec 2023 15:48:58 +0100 Subject: [PATCH 15/24] Updates comment --- shared/src/ledger/native_vp/masp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index 1ae3b6bdae..d506da9c8f 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -331,7 +331,7 @@ where } // The transaction must correctly update the note commitment tree - // and the anchor in storage with the new output descriptions + // in storage with the new output descriptions if !self.valid_note_commitment_update(keys_changed, &shielded_tx)? { return Ok(false); } From 6276330eb964716011811540e734a979a1e428a3 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 6 Dec 2023 12:20:44 +0100 Subject: [PATCH 16/24] Changelog #2244 --- .../unreleased/bug-fixes/2244-spend-description-validation.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/2244-spend-description-validation.md diff --git a/.changelog/unreleased/bug-fixes/2244-spend-description-validation.md b/.changelog/unreleased/bug-fixes/2244-spend-description-validation.md new file mode 100644 index 0000000000..c531894144 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/2244-spend-description-validation.md @@ -0,0 +1,2 @@ +- Updates masp tx to store the notes and the native vp to validate them and the + anchors. ([\#2244](https://github.com/anoma/namada/pull/2244)) \ No newline at end of file From fdf769359820a2eb90ee4eeccb2d32f448619ace Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 6 Dec 2023 18:02:03 +0100 Subject: [PATCH 17/24] `update_allowed_conversions` to publish the updated convert anchor --- core/src/ledger/masp_conversions.rs | 15 ++++++++++++++- core/src/types/token.rs | 5 ++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/core/src/ledger/masp_conversions.rs b/core/src/ledger/masp_conversions.rs index 371e8a5ec9..c3d32ae180 100644 --- a/core/src/ledger/masp_conversions.rs +++ b/core/src/ledger/masp_conversions.rs @@ -16,7 +16,7 @@ use crate::ledger::storage_api::token::read_denom; use crate::ledger::storage_api::{StorageRead, StorageWrite}; use crate::types::address::{Address, MASP}; use crate::types::dec::Dec; -use crate::types::storage::Epoch; +use crate::types::storage::{Epoch, Key, KeySeg}; use crate::types::token; use crate::types::token::MaspDenom; use crate::types::uint::Uint; @@ -211,6 +211,7 @@ where use rayon::prelude::ParallelSlice; use crate::types::address; + use crate::types::token::MASP_CONVERT_ANCHOR_PREFIX; // The derived conversions will be placed in MASP address space let masp_addr = MASP; @@ -454,6 +455,18 @@ where // obtained wl_storage.storage.conversion_state.tree = FrozenCommitmentTree::merge(&tree_parts); + // Publish the new anchor in storage + let anchor_key = Key::from(MASP.to_db_key()) + .push(&MASP_CONVERT_ANCHOR_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&crate::types::hash::Hash( + masp_primitives::bls12_381::Scalar::from( + wl_storage.storage.conversion_state.tree.root(), + ) + .to_bytes(), + )) + .expect("Cannot obtain a storage key"); + wl_storage.write(&anchor_key, ())?; // Add purely decoding entries to the assets map. These will be // overwritten before the creation of the next commitment tree diff --git a/core/src/types/token.rs b/core/src/types/token.rs index b5737120d8..086f6b23ac 100644 --- a/core/src/types/token.rs +++ b/core/src/types/token.rs @@ -906,6 +906,8 @@ pub const MASP_NULLIFIERS_KEY_PREFIX: &str = "nullifiers"; pub const MASP_NOTE_COMMITMENT_TREE_KEY: &str = "commitment_tree"; /// Key segment prefix for the note commitment anchor pub const MASP_NOTE_COMMITMENT_ANCHOR_PREFIX: &str = "note_commitment_anchor"; +/// Key segment prefix for the convert anchor +pub const MASP_CONVERT_ANCHOR_PREFIX: &str = "convert_anchor"; /// Last calculated inflation value handed out pub const MASP_LAST_INFLATION_KEY: &str = "last_inflation"; /// The last locked ratio @@ -1133,7 +1135,8 @@ pub fn is_masp_key(key: &Key) -> bool { || key.starts_with(PIN_KEY_PREFIX) || key.starts_with(MASP_NULLIFIERS_KEY_PREFIX) || key == MASP_NOTE_COMMITMENT_TREE_KEY - || key.starts_with(MASP_NOTE_COMMITMENT_ANCHOR_PREFIX))) + || key.starts_with(MASP_NOTE_COMMITMENT_ANCHOR_PREFIX) + || key.starts_with(MASP_CONVERT_ANCHOR_PREFIX))) } /// Check if the given storage key is a masp nullifier key From 3ef207a4808947e80b7d0262b1e061a2ca21426e Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 6 Dec 2023 19:08:45 +0100 Subject: [PATCH 18/24] Masp VP verifies the anchors of convert descriptions --- shared/src/ledger/native_vp/masp.rs | 37 ++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index d506da9c8f..4694642ab0 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -19,8 +19,8 @@ use namada_core::types::address::{Address, MASP}; use namada_core::types::storage::{Epoch, Key, KeySeg}; use namada_core::types::token::{ self, is_masp_anchor_key, is_masp_nullifier_key, - MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE_KEY, - MASP_NULLIFIERS_KEY_PREFIX, + MASP_CONVERT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, + MASP_NOTE_COMMITMENT_TREE_KEY, MASP_NULLIFIERS_KEY_PREFIX, }; use namada_sdk::masp::verify_shielded_tx; use ripemd::Digest as RipemdDigest; @@ -263,6 +263,35 @@ where Ok(true) } + + // Check that the convert descriptions anchors of a transaction are valid + fn valid_convert_descriptions_anchor( + &self, + transaction: &Transaction, + ) -> Result { + for description in transaction + .sapling_bundle() + .map_or(&vec![], |bundle| &bundle.shielded_converts) + { + let anchor_key = Key::from(MASP.to_db_key()) + .push(&MASP_CONVERT_ANCHOR_PREFIX.to_owned()) + .expect("Cannot obtain a storage key") + .push(&namada_core::types::hash::Hash( + description.anchor.to_bytes(), + )) + .expect("Cannot obtain a storage key"); + + // Check if the provided anchor was published before + if !self.ctx.has_key_pre(&anchor_key)? { + tracing::debug!( + "Convert description refers to an invalid anchor" + ); + return Ok(false); + } + } + + Ok(true) + } } impl<'a, DB, H, CA> NativeVp for MaspVp<'a, DB, H, CA> @@ -309,7 +338,8 @@ where // the containing wrapper transaction's fee // amount Satisfies 1. // 3. The spend descriptions' anchors are valid - // 4. The nullifiers provided by the transaction have not been + // 4. The convert descriptions's anchors are valid + // 5. The nullifiers provided by the transaction have not been // revealed previously (even in the same tx) and no unneeded // nullifier is being revealed by the tx if let Some(transp_bundle) = shielded_tx.transparent_bundle() { @@ -324,6 +354,7 @@ where } if !(self.valid_spend_descriptions_anchor(&shielded_tx)? + && self.valid_convert_descriptions_anchor(&shielded_tx)? && self.valid_nullifiers_reveal(keys_changed, &shielded_tx)?) { return Ok(false); From 01caceb2a7ea45f459d9476f73a7588d3aba8173 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 7 Dec 2023 12:57:11 +0100 Subject: [PATCH 19/24] Improves masp vp keys verification --- core/src/types/token.rs | 36 +++++++--- shared/src/ledger/native_vp/masp.rs | 107 +++++++++++++++++++++++----- 2 files changed, 113 insertions(+), 30 deletions(-) diff --git a/core/src/types/token.rs b/core/src/types/token.rs index 086f6b23ac..8081e5ab7d 100644 --- a/core/src/types/token.rs +++ b/core/src/types/token.rs @@ -1127,6 +1127,13 @@ pub fn is_denom_key(token_addr: &Address, key: &Key) -> bool { /// Check if the given storage key is a masp key pub fn is_masp_key(key: &Key) -> bool { + matches!(&key.segments[..], + [DbKeySeg::AddressSeg(addr), ..] if *addr == MASP + ) +} + +/// Check if the given storage key is allowed to be touched by a masp transfer +pub fn is_masp_allowed_key(key: &Key) -> bool { matches!(&key.segments[..], [DbKeySeg::AddressSeg(addr), DbKeySeg::StringSeg(key)] if *addr == MASP @@ -1134,27 +1141,34 @@ pub fn is_masp_key(key: &Key) -> bool { || key.starts_with(TX_KEY_PREFIX) || key.starts_with(PIN_KEY_PREFIX) || key.starts_with(MASP_NULLIFIERS_KEY_PREFIX) - || key == MASP_NOTE_COMMITMENT_TREE_KEY - || key.starts_with(MASP_NOTE_COMMITMENT_ANCHOR_PREFIX) - || key.starts_with(MASP_CONVERT_ANCHOR_PREFIX))) + || key == MASP_NOTE_COMMITMENT_TREE_KEY)) } -/// Check if the given storage key is a masp nullifier key -pub fn is_masp_nullifier_key(key: &Key) -> bool { +/// Check if the given storage key is a masp tx prefix key +pub fn is_masp_tx_prefix_key(key: &Key) -> bool { matches!(&key.segments[..], - [DbKeySeg::AddressSeg(addr), + [DbKeySeg::AddressSeg(addr), DbKeySeg::StringSeg(prefix), .. - ] if *addr == MASP && prefix == MASP_NULLIFIERS_KEY_PREFIX) + ] if *addr == MASP && prefix.starts_with(TX_KEY_PREFIX)) } -/// Check if the given storage key is a masp anchor key -pub fn is_masp_anchor_key(key: &Key) -> bool { +/// Check if the given storage key is a masp tx pin key +pub fn is_masp_tx_pin_key(key: &Key) -> bool { matches!(&key.segments[..], - [DbKeySeg::AddressSeg(addr), + [DbKeySeg::AddressSeg(addr), DbKeySeg::StringSeg(prefix), .. - ] if *addr == MASP && prefix == MASP_NOTE_COMMITMENT_ANCHOR_PREFIX) + ] if *addr == MASP && prefix.starts_with(PIN_KEY_PREFIX)) +} + +/// Check if the given storage key is a masp nullifier key +pub fn is_masp_nullifier_key(key: &Key) -> bool { + matches!(&key.segments[..], + [DbKeySeg::AddressSeg(addr), + DbKeySeg::StringSeg(prefix), + .. + ] if *addr == MASP && prefix == MASP_NULLIFIERS_KEY_PREFIX) } /// Obtain the storage key for the last locked ratio of a token diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index 4694642ab0..3b7c96c04c 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -16,11 +16,13 @@ use namada_core::ledger::vp_env::VpEnv; use namada_core::proto::Tx; use namada_core::types::address::InternalAddress::Masp; use namada_core::types::address::{Address, MASP}; -use namada_core::types::storage::{Epoch, Key, KeySeg}; +use namada_core::types::storage::{BlockHeight, Epoch, Key, KeySeg, TxIndex}; use namada_core::types::token::{ - self, is_masp_anchor_key, is_masp_nullifier_key, + self, is_masp_allowed_key, is_masp_key, is_masp_nullifier_key, + is_masp_tx_pin_key, is_masp_tx_prefix_key, Transfer, HEAD_TX_KEY, MASP_CONVERT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, - MASP_NOTE_COMMITMENT_TREE_KEY, MASP_NULLIFIERS_KEY_PREFIX, + MASP_NOTE_COMMITMENT_TREE_KEY, MASP_NULLIFIERS_KEY_PREFIX, PIN_KEY_PREFIX, + TX_KEY_PREFIX, }; use namada_sdk::masp::verify_shielded_tx; use ripemd::Digest as RipemdDigest; @@ -179,7 +181,6 @@ where // the tree and anchor in storage fn valid_note_commitment_update( &self, - keys_changed: &BTreeSet, transaction: &Transaction, ) -> Result { // Check that the merkle tree in storage has been correctly updated with @@ -218,20 +219,6 @@ where return Ok(false); } - // Check that no anchor was published (this is to be done by the - // protocol) - if keys_changed - .iter() - .filter(|key| is_masp_anchor_key(key)) - .count() - != 0 - { - tracing::debug!( - "The transaction revealed one or more MASP anchor keys" - ); - return Ok(false); - } - Ok(true) } @@ -292,6 +279,84 @@ where Ok(true) } + + /// Check the correctness of the general storage changes that pertain to all + /// types of masp transfers + fn valid_state( + &self, + keys_changed: &BTreeSet, + transfer: &Transfer, + transaction: &Transaction, + ) -> Result { + // Check that the transaction didn't write unallowed masp keys, nor + // multiple variations of the same key prefixes + let mut found_tx_key = false; + let mut found_pin_key = false; + for key in keys_changed.iter().filter(|key| is_masp_key(key)) { + if !is_masp_allowed_key(key) { + return Ok(false); + } else if is_masp_tx_prefix_key(key) { + if found_tx_key { + return Ok(false); + } else { + found_tx_key = true; + } + } else if is_masp_tx_pin_key(key) { + if found_pin_key { + return Ok(false); + } else { + found_pin_key = true; + } + } + } + + // Validate head tx + let head_tx_key = Key::from(MASP.to_db_key()) + .push(&HEAD_TX_KEY.to_owned()) + .expect("Cannot obtain a storage key"); + let pre_head: u64 = self.ctx.read_pre(&head_tx_key)?.unwrap_or(0); + let post_head: u64 = self.ctx.read_post(&head_tx_key)?.unwrap_or(0); + + if post_head != pre_head + 1 { + return Ok(false); + } + + // Validate tx key + let current_tx_key = Key::from(MASP.to_db_key()) + .push(&(TX_KEY_PREFIX.to_owned() + &pre_head.to_string())) + .expect("Cannot obtain a storage key"); + match self + .ctx + .read_post::<(Epoch, BlockHeight, TxIndex, Transfer, Transaction)>( + ¤t_tx_key, + )? { + Some(( + epoch, + height, + tx_index, + storage_transfer, + storage_transaction, + )) if (epoch == self.ctx.get_block_epoch()? + && height == self.ctx.get_block_height()? + && tx_index == self.ctx.get_tx_index()? + && &storage_transfer == transfer + && &storage_transaction == transaction) => {} + _ => return Ok(false), + } + + // Validate pin key + if let Some(key) = &transfer.key { + let pin_key = Key::from(MASP.to_db_key()) + .push(&(PIN_KEY_PREFIX.to_owned() + key)) + .expect("Cannot obtain a storage key"); + match self.ctx.read_post::(&pin_key)? { + Some(tx_idx) if tx_idx == pre_head => (), + _ => return Ok(false), + } + } + + Ok(true) + } } impl<'a, DB, H, CA> NativeVp for MaspVp<'a, DB, H, CA> @@ -314,6 +379,10 @@ where // The Sapling value balance adds to the transparent tx pool transparent_tx_pool += shielded_tx.sapling_value_balance(); + if !self.valid_state(keys_changed, &transfer, &shielded_tx)? { + return Ok(false); + } + if transfer.source != Address::Internal(Masp) { // Handle transparent input // Note that the asset type is timestamped so shields @@ -363,7 +432,7 @@ where // The transaction must correctly update the note commitment tree // in storage with the new output descriptions - if !self.valid_note_commitment_update(keys_changed, &shielded_tx)? { + if !self.valid_note_commitment_update(&shielded_tx)? { return Ok(false); } From 751b0dfa5c5e8b9063eed8d055fb0d03b18d0cf9 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 7 Dec 2023 13:38:48 +0100 Subject: [PATCH 20/24] Removes redundant masp dependency from bench crate --- Cargo.lock | 1 - benches/Cargo.toml | 1 - benches/native_vps.rs | 2 +- core/src/ledger/masp_conversions.rs | 6 +++++- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0ed05112b6..3f90455135 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3897,7 +3897,6 @@ dependencies = [ "borsh-ext", "criterion", "masp_primitives", - "masp_proofs", "namada", "namada_apps", "rand 0.8.5", diff --git a/benches/Cargo.toml b/benches/Cargo.toml index b9d15a22f4..b467253e2d 100644 --- a/benches/Cargo.toml +++ b/benches/Cargo.toml @@ -43,7 +43,6 @@ path = "host_env.rs" namada = { path = "../shared", features = ["testing"] } namada_apps = { path = "../apps", features = ["benches"] } masp_primitives.workspace = true -masp_proofs.workspace = true borsh.workspace = true borsh-ext.workspace = true criterion = { version = "0.5", features = ["html_reports"] } diff --git a/benches/native_vps.rs b/benches/native_vps.rs index def2e3c135..29c7e16ff1 100644 --- a/benches/native_vps.rs +++ b/benches/native_vps.rs @@ -4,8 +4,8 @@ use std::rc::Rc; use std::str::FromStr; use criterion::{criterion_group, criterion_main, Criterion}; +use masp_primitives::bls12_381; use masp_primitives::sapling::Node; -use masp_proofs::bls12_381; use namada::core::ledger::governance::storage::proposal::ProposalType; use namada::core::ledger::governance::storage::vote::{ StorageProposalVote, VoteType, diff --git a/core/src/ledger/masp_conversions.rs b/core/src/ledger/masp_conversions.rs index c3d32ae180..1ca4a2c6ef 100644 --- a/core/src/ledger/masp_conversions.rs +++ b/core/src/ledger/masp_conversions.rs @@ -5,6 +5,8 @@ use std::collections::BTreeMap; use borsh::{BorshDeserialize, BorshSerialize}; use borsh_ext::BorshSerializeExt; use masp_primitives::asset_type::AssetType; +#[cfg(feature = "wasm-runtime")] +use masp_primitives::bls12_381; use masp_primitives::convert::AllowedConversion; use masp_primitives::merkle_tree::FrozenCommitmentTree; use masp_primitives::sapling::Node; @@ -16,7 +18,9 @@ use crate::ledger::storage_api::token::read_denom; use crate::ledger::storage_api::{StorageRead, StorageWrite}; use crate::types::address::{Address, MASP}; use crate::types::dec::Dec; -use crate::types::storage::{Epoch, Key, KeySeg}; +use crate::types::storage::Epoch; +#[cfg(feature = "wasm-runtime")] +use crate::types::storage::{Key, KeySeg}; use crate::types::token; use crate::types::token::MaspDenom; use crate::types::uint::Uint; From 463bfc8cdd2f1c64936261c6ffcf83841c67de91 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 7 Dec 2023 13:40:53 +0100 Subject: [PATCH 21/24] Fixes conversion anchor handling --- apps/src/lib/node/ledger/shell/init_chain.rs | 17 +++++++- core/src/ledger/masp_conversions.rs | 19 +++++---- core/src/types/token.rs | 2 +- shared/src/ledger/native_vp/masp.rs | 45 +++++++++++--------- 4 files changed, 53 insertions(+), 30 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/init_chain.rs b/apps/src/lib/node/ledger/shell/init_chain.rs index 18d7acd749..710974dd88 100644 --- a/apps/src/lib/node/ledger/shell/init_chain.rs +++ b/apps/src/lib/node/ledger/shell/init_chain.rs @@ -18,7 +18,8 @@ use namada::types::key::*; use namada::types::storage::KeySeg; use namada::types::time::{DateTimeUtc, TimeZone, Utc}; use namada::types::token::{ - MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE_KEY, + MASP_CONVERT_ANCHOR_KEY, MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, + MASP_NOTE_COMMITMENT_TREE_KEY, }; use namada::vm::validate_untrusted_wasm; use namada_sdk::eth_bridge::EthBridgeStatus; @@ -200,6 +201,20 @@ where .write(&commitment_tree_anchor_key, ()) .unwrap(); + // Init masp convert anchor + let convert_anchor_key = Key::from(MASP.to_db_key()) + .push(&MASP_CONVERT_ANCHOR_KEY.to_owned()) + .expect("Cannot obtain a storage key"); + self.wl_storage.write( + &convert_anchor_key, + namada::core::types::hash::Hash( + bls12_381::Scalar::from( + self.wl_storage.storage.conversion_state.tree.root(), + ) + .to_bytes(), + ), + )?; + // Set the initial validator set response.validators = self .get_abci_validator_updates(true, |pk, power| { diff --git a/core/src/ledger/masp_conversions.rs b/core/src/ledger/masp_conversions.rs index 1ca4a2c6ef..6983324f2b 100644 --- a/core/src/ledger/masp_conversions.rs +++ b/core/src/ledger/masp_conversions.rs @@ -215,7 +215,7 @@ where use rayon::prelude::ParallelSlice; use crate::types::address; - use crate::types::token::MASP_CONVERT_ANCHOR_PREFIX; + use crate::types::token::MASP_CONVERT_ANCHOR_KEY; // The derived conversions will be placed in MASP address space let masp_addr = MASP; @@ -459,18 +459,19 @@ where // obtained wl_storage.storage.conversion_state.tree = FrozenCommitmentTree::merge(&tree_parts); - // Publish the new anchor in storage + // Update the anchor in storage let anchor_key = Key::from(MASP.to_db_key()) - .push(&MASP_CONVERT_ANCHOR_PREFIX.to_owned()) - .expect("Cannot obtain a storage key") - .push(&crate::types::hash::Hash( - masp_primitives::bls12_381::Scalar::from( + .push(&MASP_CONVERT_ANCHOR_KEY.to_owned()) + .expect("Cannot obtain a storage key"); + wl_storage.write( + &anchor_key, + crate::types::hash::Hash( + bls12_381::Scalar::from( wl_storage.storage.conversion_state.tree.root(), ) .to_bytes(), - )) - .expect("Cannot obtain a storage key"); - wl_storage.write(&anchor_key, ())?; + ), + )?; // Add purely decoding entries to the assets map. These will be // overwritten before the creation of the next commitment tree diff --git a/core/src/types/token.rs b/core/src/types/token.rs index 8081e5ab7d..e5d6c3706b 100644 --- a/core/src/types/token.rs +++ b/core/src/types/token.rs @@ -907,7 +907,7 @@ pub const MASP_NOTE_COMMITMENT_TREE_KEY: &str = "commitment_tree"; /// Key segment prefix for the note commitment anchor pub const MASP_NOTE_COMMITMENT_ANCHOR_PREFIX: &str = "note_commitment_anchor"; /// Key segment prefix for the convert anchor -pub const MASP_CONVERT_ANCHOR_PREFIX: &str = "convert_anchor"; +pub const MASP_CONVERT_ANCHOR_KEY: &str = "convert_anchor"; /// Last calculated inflation value handed out pub const MASP_LAST_INFLATION_KEY: &str = "last_inflation"; /// The last locked ratio diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index 3b7c96c04c..7bae49dde2 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -20,7 +20,7 @@ use namada_core::types::storage::{BlockHeight, Epoch, Key, KeySeg, TxIndex}; use namada_core::types::token::{ self, is_masp_allowed_key, is_masp_key, is_masp_nullifier_key, is_masp_tx_pin_key, is_masp_tx_prefix_key, Transfer, HEAD_TX_KEY, - MASP_CONVERT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, + MASP_CONVERT_ANCHOR_KEY, MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, MASP_NOTE_COMMITMENT_TREE_KEY, MASP_NULLIFIERS_KEY_PREFIX, PIN_KEY_PREFIX, TX_KEY_PREFIX, }; @@ -256,24 +256,31 @@ where &self, transaction: &Transaction, ) -> Result { - for description in transaction - .sapling_bundle() - .map_or(&vec![], |bundle| &bundle.shielded_converts) - { - let anchor_key = Key::from(MASP.to_db_key()) - .push(&MASP_CONVERT_ANCHOR_PREFIX.to_owned()) - .expect("Cannot obtain a storage key") - .push(&namada_core::types::hash::Hash( - description.anchor.to_bytes(), - )) - .expect("Cannot obtain a storage key"); - - // Check if the provided anchor was published before - if !self.ctx.has_key_pre(&anchor_key)? { - tracing::debug!( - "Convert description refers to an invalid anchor" - ); - return Ok(false); + if let Some(bundle) = transaction.sapling_bundle() { + if !bundle.shielded_converts.is_empty() { + let anchor_key = Key::from(MASP.to_db_key()) + .push(&MASP_CONVERT_ANCHOR_KEY.to_owned()) + .expect("Cannot obtain a storage key"); + let expected_anchor = self + .ctx + .read_pre::(&anchor_key)? + .ok_or(Error::NativeVpError( + native_vp::Error::SimpleMessage("Cannot read storage"), + ))?; + + for description in &bundle.shielded_converts { + // Check if the provided anchor matches the current + // conversion tree's one + if namada_core::types::hash::Hash( + description.anchor.to_bytes(), + ) != expected_anchor + { + tracing::debug!( + "Convert description refers to an invalid anchor" + ); + return Ok(false); + } + } } } From ba7c1329bc89a9017b9418ca7beea1c3917e7957 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 7 Dec 2023 16:28:14 +0100 Subject: [PATCH 22/24] Fixes masp key validation --- core/src/ledger/masp_utils.rs | 6 +++--- core/src/types/token.rs | 24 ++++++++++++++++-------- shared/src/ledger/native_vp/masp.rs | 4 ++-- 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/core/src/ledger/masp_utils.rs b/core/src/ledger/masp_utils.rs index d15834f7a1..f37b725531 100644 --- a/core/src/ledger/masp_utils.rs +++ b/core/src/ledger/masp_utils.rs @@ -10,8 +10,8 @@ use crate::types::address::MASP; use crate::types::hash::Hash; use crate::types::storage::{BlockHeight, Epoch, Key, KeySeg, TxIndex}; use crate::types::token::{ - Transfer, HEAD_TX_KEY, MASP_NOTE_COMMITMENT_TREE_KEY, - MASP_NULLIFIERS_KEY_PREFIX, PIN_KEY_PREFIX, TX_KEY_PREFIX, + Transfer, HEAD_TX_KEY, MASP_NOTE_COMMITMENT_TREE_KEY, MASP_NULLIFIERS_KEY, + PIN_KEY_PREFIX, TX_KEY_PREFIX, }; // Writes the nullifiers of the provided masp transaction to storage @@ -24,7 +24,7 @@ fn reveal_nullifiers( .map_or(&vec![], |description| &description.shielded_spends) { let nullifier_key = Key::from(MASP.to_db_key()) - .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) + .push(&MASP_NULLIFIERS_KEY.to_owned()) .expect("Cannot obtain a storage key") .push(&Hash(description.nullifier.0)) .expect("Cannot obtain a storage key"); diff --git a/core/src/types/token.rs b/core/src/types/token.rs index e5d6c3706b..4477967c8a 100644 --- a/core/src/types/token.rs +++ b/core/src/types/token.rs @@ -901,7 +901,7 @@ pub const TX_KEY_PREFIX: &str = "tx-"; /// Key segment prefix for pinned shielded transactions pub const PIN_KEY_PREFIX: &str = "pin-"; /// Key segment prefix for the nullifiers -pub const MASP_NULLIFIERS_KEY_PREFIX: &str = "nullifiers"; +pub const MASP_NULLIFIERS_KEY: &str = "nullifiers"; /// Key segment prefix for the note commitment merkle tree pub const MASP_NOTE_COMMITMENT_TREE_KEY: &str = "commitment_tree"; /// Key segment prefix for the note commitment anchor @@ -1134,14 +1134,24 @@ pub fn is_masp_key(key: &Key) -> bool { /// Check if the given storage key is allowed to be touched by a masp transfer pub fn is_masp_allowed_key(key: &Key) -> bool { - matches!(&key.segments[..], + match &key.segments[..] { [DbKeySeg::AddressSeg(addr), DbKeySeg::StringSeg(key)] if *addr == MASP && (key == HEAD_TX_KEY || key.starts_with(TX_KEY_PREFIX) || key.starts_with(PIN_KEY_PREFIX) - || key.starts_with(MASP_NULLIFIERS_KEY_PREFIX) - || key == MASP_NOTE_COMMITMENT_TREE_KEY)) + || key == MASP_NOTE_COMMITMENT_TREE_KEY) => + { + true + } + + [ + DbKeySeg::AddressSeg(addr), + DbKeySeg::StringSeg(key), + DbKeySeg::StringSeg(_nullifier), + ] if *addr == MASP && key == MASP_NULLIFIERS_KEY => true, + _ => false, + } } /// Check if the given storage key is a masp tx prefix key @@ -1149,7 +1159,6 @@ pub fn is_masp_tx_prefix_key(key: &Key) -> bool { matches!(&key.segments[..], [DbKeySeg::AddressSeg(addr), DbKeySeg::StringSeg(prefix), - .. ] if *addr == MASP && prefix.starts_with(TX_KEY_PREFIX)) } @@ -1158,7 +1167,6 @@ pub fn is_masp_tx_pin_key(key: &Key) -> bool { matches!(&key.segments[..], [DbKeySeg::AddressSeg(addr), DbKeySeg::StringSeg(prefix), - .. ] if *addr == MASP && prefix.starts_with(PIN_KEY_PREFIX)) } @@ -1167,8 +1175,8 @@ pub fn is_masp_nullifier_key(key: &Key) -> bool { matches!(&key.segments[..], [DbKeySeg::AddressSeg(addr), DbKeySeg::StringSeg(prefix), - .. - ] if *addr == MASP && prefix == MASP_NULLIFIERS_KEY_PREFIX) + DbKeySeg::StringSeg(_nullifier) + ] if *addr == MASP && prefix == MASP_NULLIFIERS_KEY) } /// Obtain the storage key for the last locked ratio of a token diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index 7bae49dde2..b7efededcc 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -21,7 +21,7 @@ use namada_core::types::token::{ self, is_masp_allowed_key, is_masp_key, is_masp_nullifier_key, is_masp_tx_pin_key, is_masp_tx_prefix_key, Transfer, HEAD_TX_KEY, MASP_CONVERT_ANCHOR_KEY, MASP_NOTE_COMMITMENT_ANCHOR_PREFIX, - MASP_NOTE_COMMITMENT_TREE_KEY, MASP_NULLIFIERS_KEY_PREFIX, PIN_KEY_PREFIX, + MASP_NOTE_COMMITMENT_TREE_KEY, MASP_NULLIFIERS_KEY, PIN_KEY_PREFIX, TX_KEY_PREFIX, }; use namada_sdk::masp::verify_shielded_tx; @@ -135,7 +135,7 @@ where .map_or(&vec![], |description| &description.shielded_spends) { let nullifier_key = Key::from(MASP.to_db_key()) - .push(&MASP_NULLIFIERS_KEY_PREFIX.to_owned()) + .push(&MASP_NULLIFIERS_KEY.to_owned()) .expect("Cannot obtain a storage key") .push(&namada_core::types::hash::Hash(description.nullifier.0)) .expect("Cannot obtain a storage key"); From 1971cf80d1fe9567e6a6abd5c35d41847abd38c4 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 7 Dec 2023 16:29:59 +0100 Subject: [PATCH 23/24] Changelog #2248 --- .../unreleased/bug-fixes/2248-convert-description-validation.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/2248-convert-description-validation.md diff --git a/.changelog/unreleased/bug-fixes/2248-convert-description-validation.md b/.changelog/unreleased/bug-fixes/2248-convert-description-validation.md new file mode 100644 index 0000000000..2f7b72ceb2 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/2248-convert-description-validation.md @@ -0,0 +1,2 @@ +- Updates the masp vp to validate the convert description's anchor. + ([\#2248](https://github.com/anoma/namada/pull/2248)) \ No newline at end of file From 7108d5c15e83e696f8b819add78fc9ea577fc19c Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 11 Dec 2023 14:35:42 +0100 Subject: [PATCH 24/24] Stricter checks on sapling bundle components --- shared/src/ledger/native_vp/masp.rs | 53 ++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 8 deletions(-) diff --git a/shared/src/ledger/native_vp/masp.rs b/shared/src/ledger/native_vp/masp.rs index b7efededcc..57b159408a 100644 --- a/shared/src/ledger/native_vp/masp.rs +++ b/shared/src/ledger/native_vp/masp.rs @@ -130,10 +130,20 @@ where transaction: &Transaction, ) -> Result { let mut revealed_nullifiers = HashSet::new(); - for description in transaction - .sapling_bundle() - .map_or(&vec![], |description| &description.shielded_spends) - { + let shielded_spends = match transaction.sapling_bundle() { + Some(bundle) if !bundle.shielded_spends.is_empty() => { + &bundle.shielded_spends + } + _ => { + tracing::debug!( + "Missing expected spend descriptions in shielded \ + transaction" + ); + return Ok(false); + } + }; + + for description in shielded_spends { let nullifier_key = Key::from(MASP.to_db_key()) .push(&MASP_NULLIFIERS_KEY.to_owned()) .expect("Cannot obtain a storage key") @@ -227,10 +237,20 @@ where &self, transaction: &Transaction, ) -> Result { - for description in transaction - .sapling_bundle() - .map_or(&vec![], |bundle| &bundle.shielded_spends) - { + let shielded_spends = match transaction.sapling_bundle() { + Some(bundle) if !bundle.shielded_spends.is_empty() => { + &bundle.shielded_spends + } + _ => { + tracing::debug!( + "Missing expected spend descriptions in shielded \ + transaction" + ); + return Ok(false); + } + }; + + for description in shielded_spends { let anchor_key = Key::from(MASP.to_db_key()) .push(&MASP_NOTE_COMMITMENT_ANCHOR_PREFIX.to_owned()) .expect("Cannot obtain a storage key") @@ -406,6 +426,14 @@ where // Non-masp sources add to transparent tx pool transparent_tx_pool += transp_amt; } + + // No shielded spends nor shielded converts are allowed + if shielded_tx.sapling_bundle().is_some_and(|bundle| { + !(bundle.shielded_spends.is_empty() + && bundle.shielded_converts.is_empty()) + }) { + return Ok(false); + } } else { // Handle shielded input // The following boundary conditions must be satisfied @@ -532,6 +560,7 @@ where // Handle shielded output // The following boundary conditions must be satisfied // 1. Zero transparent output + // 2. At least one shielded output // Satisfies 1. if let Some(transp_bundle) = shielded_tx.transparent_bundle() { @@ -544,6 +573,14 @@ where return Ok(false); } } + + // Staisfies 2. + if shielded_tx + .sapling_bundle() + .is_some_and(|bundle| bundle.shielded_outputs.is_empty()) + { + return Ok(false); + } } match transparent_tx_pool.partial_cmp(&I128Sum::zero()) {