Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert descriptions anchor validation #2248

Merged
merged 24 commits into from
Dec 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d8b5114
Updates masp tx to reveal nullifiers
grarco Nov 30, 2023
9f855dc
Masp VP checks uniqueness of nullifiers
grarco Nov 30, 2023
5cc290c
Removes wrong comment
grarco Nov 30, 2023
9ed6d52
Changelog #2240
grarco Nov 30, 2023
f1156c1
Fixes masp vp nullifier validation
grarco Dec 1, 2023
144536f
Reduce code duplication to reveal masp nullifiers
grarco Dec 1, 2023
39e00c5
Refactors `handle_masp_tx`
grarco Dec 4, 2023
4fb5142
Updates masp tx with note commitment tree and anchor
grarco Dec 1, 2023
cf1ad17
Updates masp vp to validate note commitment tree and anchor
grarco Dec 2, 2023
61a46de
Refactors masp nullifiers check in a separate function
grarco Dec 2, 2023
2183aaa
Updates commitment tree anchor only once per block
grarco Dec 2, 2023
f7da77b
Updates the merkle tree anchor only if the tree changed
grarco Dec 3, 2023
694b949
Fixes commitment tree validation in masp vp. Adds a workaround to upd…
grarco Dec 6, 2023
8841aac
Fixes masp vp benchmark
grarco Dec 6, 2023
c09875f
Updates comment
grarco Dec 6, 2023
6276330
Changelog #2244
grarco Dec 6, 2023
fdf7693
`update_allowed_conversions` to publish the updated convert anchor
grarco Dec 6, 2023
3ef207a
Masp VP verifies the anchors of convert descriptions
grarco Dec 6, 2023
01caceb
Improves masp vp keys verification
grarco Dec 7, 2023
751b0df
Removes redundant masp dependency from bench crate
grarco Dec 7, 2023
463bfc8
Fixes conversion anchor handling
grarco Dec 7, 2023
ba7c132
Fixes masp key validation
grarco Dec 7, 2023
1971cf8
Changelog #2248
grarco Dec 7, 2023
7108d5c
Stricter checks on sapling bundle components
grarco Dec 11, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .changelog/unreleased/bug-fixes/2240-nullifier-uniqueness.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Prevents double-spending in masp by adding a nullifier set.
([\#2240](https://github.com/anoma/namada/pull/2240))
Original file line number Diff line number Diff line change
@@ -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))
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Updates the masp vp to validate the convert description's anchor.
([\#2248](https://github.com/anoma/namada/pull/2248))
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 29 additions & 1 deletion apps/src/lib/node/ledger/shell/finalize_block.rs
Original file line number Diff line number Diff line change
@@ -1,25 +1,34 @@
//! 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;
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,
};
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_KEY,
};
use namada::types::transaction::protocol::{
ethereum_tx_data_variants, ProtocolTxType,
};
Expand Down Expand Up @@ -557,6 +566,25 @@ where
tracing::info!("{}", stats);
tracing::info!("{}", stats.format_tx_executed());

// Update the MASP commitment tree anchor if the tree was updated
cwgoes marked this conversation as resolved.
Show resolved Hide resolved
let tree_key = Key::from(MASP.to_db_key())
.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
{
let updated_tree = CommitmentTree::<Node>::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);
// send the latest oracle configs. These may have changed due to
Expand Down
43 changes: 43 additions & 0 deletions apps/src/lib/node/ledger/shell/init_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,25 @@
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};
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_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;
use namada_sdk::proof_of_stake::types::ValidatorMetaData;
Expand Down Expand Up @@ -172,6 +180,41 @@ where

ibc::init_genesis_storage(&mut self.wl_storage);

// Init masp commitment tree and anchor
let empty_commitment_tree: CommitmentTree<Node> =
CommitmentTree::empty();
let anchor = empty_commitment_tree.root();
let note_commitment_tree_key = Key::from(MASP.to_db_key())
.push(&MASP_NOTE_COMMITMENT_TREE_KEY.to_owned())
.expect("Cannot obtain a storage key");
self.wl_storage
.write(&note_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();

// 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| {
Expand Down
1 change: 1 addition & 0 deletions benches/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ path = "host_env.rs"
[dev-dependencies]
namada = { path = "../shared", features = ["testing"] }
namada_apps = { path = "../apps", features = ["benches"] }
masp_primitives.workspace = true
borsh.workspace = true
borsh-ext.workspace = true
criterion = { version = "0.5", features = ["html_reports"] }
Expand Down
32 changes: 31 additions & 1 deletion benches/native_vps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +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 namada::core::ledger::governance::storage::proposal::ProposalType;
use namada::core::ledger::governance::storage::vote::{
StorageProposalVote, VoteType,
Expand Down Expand Up @@ -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,
};
Expand Down Expand Up @@ -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<Node> = 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 {
Expand Down
18 changes: 18 additions & 0 deletions core/src/ledger/masp_conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -17,6 +19,8 @@ use crate::ledger::storage_api::{StorageRead, StorageWrite};
use crate::types::address::{Address, MASP};
use crate::types::dec::Dec;
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;
Expand Down Expand Up @@ -211,6 +215,7 @@ where
use rayon::prelude::ParallelSlice;

use crate::types::address;
use crate::types::token::MASP_CONVERT_ANCHOR_KEY;

// The derived conversions will be placed in MASP address space
let masp_addr = MASP;
Expand Down Expand Up @@ -454,6 +459,19 @@ where
// obtained
wl_storage.storage.conversion_state.tree =
FrozenCommitmentTree::merge(&tree_parts);
// Update the anchor in storage
let anchor_key = Key::from(MASP.to_db_key())
.push(&MASP_CONVERT_ANCHOR_KEY.to_owned())
.expect("Cannot obtain a storage key");
wl_storage.write(
&anchor_key,
crate::types::hash::Hash(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can abstract all of this out into a hash function on the tree type, perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah the FrozenCommitmentTree is imported from the masp crate but we can probably refactor this part of the code in other ways

bls12_381::Scalar::from(
wl_storage.storage.conversion_state.tree.root(),
)
.to_bytes(),
),
)?;

// Add purely decoding entries to the assets map. These will be
// overwritten before the creation of the next commitment tree
Expand Down
113 changes: 113 additions & 0 deletions core/src/ledger/masp_utils.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
//! MASP utilities

use masp_primitives::merkle_tree::CommitmentTree;
use masp_primitives::sapling::Node;
use masp_primitives::transaction::Transaction;

use super::storage_api::{StorageRead, StorageWrite};
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};
use crate::types::token::{
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
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.to_owned())
.expect("Cannot obtain a storage key")
.push(&Hash(description.nullifier.0))
.expect("Cannot obtain a storage key");
ctx.write(&nullifier_key, ())?;
}

Ok(())
}

/// 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_KEY.to_owned())
.expect("Cannot obtain a storage key");
let mut commitment_tree: CommitmentTree<Node> =
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
commitment_tree
.append(Node::from_scalar(description.cmu))
.map_err(|_| {
Error::SimpleMessage("Note commitment tree is full")
})?;
}

ctx.write(&tree_key, commitment_tree)?;
}
}

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 =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to write transactions to storage like this, all of this incrementing current tx index logic should be abstracted into a common function.

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() + &current_tx_idx.to_string()))
.expect("Cannot obtain a storage key");
// Save the Transfer object and its location within the blockchain
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to store the entire transaction data? Seems wasteful. Also, can't this be queried from Tendermint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this data in protocol but we need it for the clients to scan transactions and generate a valid internal state to produce future transactions. I agree with you that this could bloat the storage pretty quick (together with the nullifier set), but I think there are a few concerns with querying this data from comet:

  • I'm not sure we can assume that the blocks will be always available (in case of a chain restart for example if we decided to prune history)
  • Requesting blocks from full nodes could take a long time/bandwidth
  • The time to process the blocks filtering out only masp transactions could be long

Still we might prefer this solution over writing to storage. Wdyt? Alternatively we could think about a way to prune transactions from storage when they are not useful anymore (e.g. all the output descriptions have been spent)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to use the indexer to pre-filter for MASP transaction data so that clients need only download and scan that. It's true that we need to keep around data after chain upgrades, but this doesn't seem too difficult.

// 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(&current_tx_key, record)?;
ctx.write(&head_tx_key, current_tx_idx + 1)?;
// 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
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(())
}
1 change: 1 addition & 0 deletions core/src/ledger/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion core/src/ledger/storage/write_log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading
Loading