diff --git a/.changelog/unreleased/bug-fixes/2928-fst-addr-vps.md b/.changelog/unreleased/bug-fixes/2928-fst-addr-vps.md new file mode 100644 index 0000000000..93a750e4fc --- /dev/null +++ b/.changelog/unreleased/bug-fixes/2928-fst-addr-vps.md @@ -0,0 +1,3 @@ +- Only use addresses from first storage key segments to + determine which VPs should be triggered by storage changes. + ([\#2928](https://github.com/anoma/namada/pull/2928)) \ No newline at end of file diff --git a/crates/apps/src/lib/bench_utils.rs b/crates/apps/src/lib/bench_utils.rs index 766f73e20b..8d903b8369 100644 --- a/crates/apps/src/lib/bench_utils.rs +++ b/crates/apps/src/lib/bench_utils.rs @@ -387,7 +387,8 @@ impl BenchShell { self.generate_ibc_tx(TX_IBC_WASM, msg) } - pub fn execute_tx(&mut self, tx: &Tx) { + /// Execute the tx and retur a set of verifiers inserted by the tx. + pub fn execute_tx(&mut self, tx: &Tx) -> BTreeSet
{ let gas_meter = RefCell::new(TxGasMeter::new_from_sub_limit(u64::MAX.into())); run::tx( @@ -398,7 +399,7 @@ impl BenchShell { &mut self.inner.vp_wasm_cache, &mut self.inner.tx_wasm_cache, ) - .unwrap(); + .unwrap() } pub fn advance_epoch(&mut self) { diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index e9c9708b50..1e2dcf9992 100644 --- a/crates/benches/native_vps.rs +++ b/crates/benches/native_vps.rs @@ -190,12 +190,12 @@ fn governance(c: &mut Criterion) { }; // Run the tx to validate - shell.execute_tx(&signed_tx); + let verifiers_from_tx = shell.execute_tx(&signed_tx); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(u64::MAX.into()), @@ -265,12 +265,12 @@ fn governance(c: &mut Criterion) { // let mut shell = BenchShell::default(); // // Run the tx to validate -// shell.execute_tx(&tx); +// let verifiers_from_tx = shell.execute_tx(&tx); // let (verifiers, keys_changed) = shell // .state // .write_log -// .verifiers_and_changed_keys(&BTreeSet::default()); +// .verifiers_and_changed_keys(&verifiers_from_tx); // let slash_fund = SlashFundVp { // ctx: Ctx::new( @@ -367,11 +367,11 @@ fn ibc(c: &mut Criterion) { _ => panic!("Unexpected bench test"), } - shell.execute_tx(signed_tx); + let verifiers_from_tx = shell.execute_tx(signed_tx); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(u64::MAX.into()), @@ -435,11 +435,11 @@ fn vp_multitoken(c: &mut Criterion) { .zip(["foreign_key_write", "transfer"]) { let mut shell = BenchShell::default(); - shell.execute_tx(signed_tx); + let verifiers_from_tx = shell.execute_tx(signed_tx); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(u64::MAX.into()), @@ -475,10 +475,11 @@ fn vp_multitoken(c: &mut Criterion) { } } -// Generate and run masp transaction to be verified +// Generate and run masp transaction to be verified. Returns the verifier set +// from tx and the tx. fn setup_storage_for_masp_verification( bench_name: &str, -) -> (BenchShieldedCtx, Tx) { +) -> (BenchShieldedCtx, BTreeSet, Tx) { let amount = Amount::native_whole(500); let mut shielded_ctx = BenchShieldedCtx::default(); @@ -504,6 +505,7 @@ fn setup_storage_for_masp_verification( TransferSource::Address(defaults::albert_address()), TransferTarget::PaymentAddress(albert_payment_addr), ); + shielded_ctx.shell.execute_tx(&shield_tx); shielded_ctx.shell.commit_masp_tx(shield_tx); @@ -535,9 +537,9 @@ fn setup_storage_for_masp_verification( ), _ => panic!("Unexpected bench test"), }; - shielded_ctx.shell.execute_tx(&signed_tx); + let verifiers_from_tx = shielded_ctx.shell.execute_tx(&signed_tx); - (shielded_ctx, signed_tx) + (shielded_ctx, verifiers_from_tx, signed_tx) } fn masp(c: &mut Criterion) { @@ -545,13 +547,13 @@ fn masp(c: &mut Criterion) { for bench_name in ["shielding", "unshielding", "shielded"] { group.bench_function(bench_name, |b| { - let (shielded_ctx, signed_tx) = + let (shielded_ctx, verifiers_from_tx, signed_tx) = setup_storage_for_masp_verification(bench_name); let (verifiers, keys_changed) = shielded_ctx .shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(u64::MAX.into()), @@ -592,7 +594,7 @@ fn masp_verify_shielded_tx(c: &mut Criterion) { for bench_name in ["shielding", "unshielding", "shielded"] { group.bench_function(bench_name, |b| { - let (_, signed_tx) = + let (_, _verifiers_from_tx, signed_tx) = setup_storage_for_masp_verification(bench_name); let transaction = signed_tx @@ -663,12 +665,12 @@ fn pgf(c: &mut Criterion) { }; // Run the tx to validate - shell.execute_tx(&signed_tx); + let verifiers_from_tx = shell.execute_tx(&signed_tx); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(u64::MAX.into()), @@ -735,12 +737,12 @@ fn eth_bridge_nut(c: &mut Criterion) { }; // Run the tx to validate - shell.execute_tx(&signed_tx); + let verifiers_from_tx = shell.execute_tx(&signed_tx); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let vp_address = Address::Internal(InternalAddress::Nut(native_erc20_addres)); @@ -806,12 +808,12 @@ fn eth_bridge(c: &mut Criterion) { }; // Run the tx to validate - shell.execute_tx(&signed_tx); + let verifiers_from_tx = shell.execute_tx(&signed_tx); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let vp_address = Address::Internal(InternalAddress::EthBridge); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( @@ -902,12 +904,12 @@ fn eth_bridge_pool(c: &mut Criterion) { }; // Run the tx to validate - shell.execute_tx(&signed_tx); + let verifiers_from_tx = shell.execute_tx(&signed_tx); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let vp_address = Address::Internal(InternalAddress::EthBridgePool); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( @@ -949,12 +951,12 @@ fn parameters(c: &mut Criterion) { for bench_name in ["foreign_key_write", "parameter_change"] { let mut shell = BenchShell::default(); - let signed_tx = match bench_name { + let (verifiers_from_tx, signed_tx) = match bench_name { "foreign_key_write" => { let tx = generate_foreign_key_tx(&defaults::albert_keypair()); // Run the tx to validate - shell.execute_tx(&tx); - tx + let verifiers_from_tx = shell.execute_tx(&tx); + (verifiers_from_tx, tx) } "parameter_change" => { // Simulate governance proposal to modify a parameter @@ -971,7 +973,8 @@ fn parameters(c: &mut Criterion) { namada::tx::data::DecryptedTx::Decrypted, )); tx.set_data(namada::tx::Data::new(borsh::to_vec(&0).unwrap())); - tx + let verifiers_from_tx = BTreeSet::default(); + (verifiers_from_tx, tx) } _ => panic!("Unexpected bench test"), }; @@ -979,7 +982,7 @@ fn parameters(c: &mut Criterion) { let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let vp_address = Address::Internal(InternalAddress::Parameters); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( @@ -1024,12 +1027,12 @@ fn pos(c: &mut Criterion) { for bench_name in ["foreign_key_write", "parameter_change"] { let mut shell = BenchShell::default(); - let signed_tx = match bench_name { + let (verifiers_from_tx, signed_tx) = match bench_name { "foreign_key_write" => { let tx = generate_foreign_key_tx(&defaults::albert_keypair()); // Run the tx to validate - shell.execute_tx(&tx); - tx + let verifiers_from_tx = shell.execute_tx(&tx); + (verifiers_from_tx, tx) } "parameter_change" => { // Simulate governance proposal to modify a parameter @@ -1046,7 +1049,8 @@ fn pos(c: &mut Criterion) { namada::tx::data::DecryptedTx::Decrypted, )); tx.set_data(namada::tx::Data::new(borsh::to_vec(&0).unwrap())); - tx + let verifiers_from_tx = BTreeSet::default(); + (verifiers_from_tx, tx) } _ => panic!("Unexpected bench test"), }; @@ -1054,7 +1058,7 @@ fn pos(c: &mut Criterion) { let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let vp_address = Address::Internal(InternalAddress::PoS); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( @@ -1152,12 +1156,12 @@ fn ibc_vp_validate_action(c: &mut Criterion) { _ => panic!("Unexpected bench test"), } - shell.execute_tx(signed_tx); + let verifiers_from_tx = shell.execute_tx(signed_tx); let tx_data = signed_tx.data().unwrap(); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(u64::MAX.into()), @@ -1252,12 +1256,12 @@ fn ibc_vp_execute_action(c: &mut Criterion) { _ => panic!("Unexpected bench test"), } - shell.execute_tx(signed_tx); + let verifiers_from_tx = shell.execute_tx(signed_tx); let tx_data = signed_tx.data().unwrap(); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(u64::MAX.into()), diff --git a/crates/benches/vps.rs b/crates/benches/vps.rs index 48e9229443..e45926b511 100644 --- a/crates/benches/vps.rs +++ b/crates/benches/vps.rs @@ -1,5 +1,4 @@ use std::cell::RefCell; -use std::collections::BTreeSet; use criterion::{criterion_group, criterion_main, Criterion}; use namada::account::UpdateAccount; @@ -136,11 +135,11 @@ fn vp_user(c: &mut Criterion) { "vp", ]) { let mut shell = BenchShell::default(); - shell.execute_tx(signed_tx); + let verifiers_from_tx = shell.execute_tx(signed_tx); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); group.bench_function(bench_name, |b| { b.iter(|| { @@ -284,11 +283,11 @@ fn vp_implicit(c: &mut Criterion) { } // Run the tx to validate - shell.execute_tx(tx); + let verifiers_from_tx = shell.execute_tx(tx); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); group.bench_function(bench_name, |b| { b.iter(|| { @@ -436,11 +435,11 @@ fn vp_validator(c: &mut Criterion) { ]) { let mut shell = BenchShell::default(); - shell.execute_tx(signed_tx); + let verifiers_from_tx = shell.execute_tx(signed_tx); let (verifiers, keys_changed) = shell .state .write_log() - .verifiers_and_changed_keys(&BTreeSet::default()); + .verifiers_and_changed_keys(&verifiers_from_tx); group.bench_function(bench_name, |b| { b.iter(|| { diff --git a/crates/core/src/storage.rs b/crates/core/src/storage.rs index 51f5ce3d53..1f5a70fd7d 100644 --- a/crates/core/src/storage.rs +++ b/crates/core/src/storage.rs @@ -653,6 +653,14 @@ impl Key { self.iter_addresses().cloned().collect() } + /// Returns the address from the first key segment if it's an address. + pub fn fst_address(&self) -> Option<&Address> { + self.segments.first().and_then(|s| match s { + DbKeySeg::AddressSeg(addr) => Some(addr), + DbKeySeg::StringSeg(_) => None, + }) + } + /// Iterates over all addresses in the key segments pub fn iter_addresses<'k, 'this: 'k>( &'this self, diff --git a/crates/state/src/write_log.rs b/crates/state/src/write_log.rs index a3f25797eb..d8f9eaf57c 100644 --- a/crates/state/src/write_log.rs +++ b/crates/state/src/write_log.rs @@ -525,20 +525,17 @@ impl WriteLog { { verifiers .insert(Address::Internal(InternalAddress::Multitoken)); - } else { - for addr in key.iter_addresses() { - if verifiers_from_tx.contains(addr) - || initialized_accounts.contains(addr) - { - // We can skip this when the address has been added from - // the Tx above. - // Also skip if it's an address of a newly initialized - // account, because anything can be written into an - // account's storage in the same tx in which it's - // initialized (there is no VP in the state prior to tx - // execution). - continue; - } + } else if let Some(addr) = key.fst_address() { + // We can skip insert when the address has been added from + // the Tx above. + // Also skip if it's an address of a newly initialized + // account, because anything can be written into an + // account's storage in the same tx in which it's + // initialized (there is no VP in the state prior to tx + // execution). + if !verifiers_from_tx.contains(addr) + && !initialized_accounts.contains(addr) + { // Add the address as a verifier verifiers.insert(addr.clone()); } @@ -976,8 +973,8 @@ mod tests { /// Test [`WriteLog::verifiers_changed_keys`] that: /// 1. Every address from `verifiers_from_tx` is included in the /// verifiers set. - /// 2. Every address included in the changed storage keys is included in - /// the verifiers set. + /// 2. Every address included in the first segment of changed storage + /// keys is included in the verifiers set. /// 3. Addresses of newly initialized accounts are not verifiers, so /// that anything can be written into an account's storage in the /// same tx in which it's initialized. @@ -999,12 +996,12 @@ mod tests { let (_changed_keys, initialized_accounts) = write_log.get_partitioned_keys(); for key in changed_keys.iter() { - for addr_from_key in &key.find_addresses() { - if !initialized_accounts.contains(addr_from_key) { - // Test for 2. - assert!(verifiers.contains(addr_from_key)); - } + if let Some(addr_from_key) = key.fst_address() { + if !initialized_accounts.contains(addr_from_key) { + // Test for 2. + assert!(verifiers.contains(addr_from_key)); } + } } println!("verifiers {:#?}", verifiers); diff --git a/crates/tx_prelude/src/proof_of_stake.rs b/crates/tx_prelude/src/proof_of_stake.rs index 8e372722c7..18d748315d 100644 --- a/crates/tx_prelude/src/proof_of_stake.rs +++ b/crates/tx_prelude/src/proof_of_stake.rs @@ -26,6 +26,10 @@ impl Ctx { validator: &Address, amount: token::Amount, ) -> TxResult { + // The tx must be authorized by the source address + let verifier = source.as_ref().unwrap_or(&validator); + self.insert_verifier(verifier)?; + let current_epoch = self.get_block_epoch()?; bond_tokens(self, source, validator, amount, current_epoch, None) } @@ -39,6 +43,10 @@ impl Ctx { validator: &Address, amount: token::Amount, ) -> EnvResult