From d0f8337350df48deade661f930a8fb28bcd836d6 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 14 Sep 2023 13:45:10 +0100 Subject: [PATCH 1/9] New method to append segments to a storage key --- core/src/types/storage.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/core/src/types/storage.rs b/core/src/types/storage.rs index b6467b8a00..e94f1280b8 100644 --- a/core/src/types/storage.rs +++ b/core/src/types/storage.rs @@ -497,6 +497,13 @@ impl Key { Ok(Key { segments }) } + /// Takes ownership of the key, appends a new segment to it, + /// and returns the modified key. + pub fn with_segment(mut self, other: T) -> Self { + self.segments.push(other.to_db_key()); + self + } + /// Returns a new key with segments of `Self` and the given key pub fn join(&self, other: &Key) -> Self { let mut segments = self.segments.clone(); From 589f5118358e0c3578adc12273c417b5f35038f4 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 14 Sep 2023 13:48:41 +0100 Subject: [PATCH 2/9] Parameterize signed Bridge pool root by its block height --- .../transactions/bridge_pool_roots.rs | 52 ++++++++++--------- ethereum_bridge/src/storage/vote_tallies.rs | 34 ++++++------ shared/src/ledger/protocol/mod.rs | 7 +-- 3 files changed, 50 insertions(+), 43 deletions(-) diff --git a/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs b/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs index 8571881453..cee70fcf4d 100644 --- a/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs +++ b/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs @@ -49,20 +49,17 @@ where let (partial_proof, seen_by) = parse_vexts(wl_storage, vext); // apply updates to the bridge pool root. - let (mut changed, confirmed) = apply_update( + let (mut changed, confirmed_update) = apply_update( wl_storage, - partial_proof.clone(), + root_height, + partial_proof, seen_by, &voting_powers, )?; // if the root is confirmed, update storage and add // relevant key to changed. - if confirmed { - let proof = votes::storage::read_body( - wl_storage, - &vote_tallies::Keys::from(&partial_proof), - )?; + if let Some(proof) = confirmed_update { wl_storage .write_bytes( &get_signed_root_key(), @@ -138,15 +135,16 @@ where /// In all instances, the changed storage keys are returned. fn apply_update( wl_storage: &mut WlStorage, + root_height: BlockHeight, mut update: BridgePoolRoot, seen_by: Votes, voting_powers: &HashMap<(Address, BlockHeight), FractionalVotingPower>, -) -> Result<(ChangedKeys, bool)> +) -> Result<(ChangedKeys, Option)> where D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, { - let bp_key = vote_tallies::Keys::from(&update); + let bp_key = vote_tallies::Keys::from((&update, root_height)); let partial_proof = votes::storage::read_body(wl_storage, &bp_key); let (vote_tracking, changed, confirmed, already_present) = if let Ok( partial, @@ -162,7 +160,7 @@ where let (vote_tracking, changed) = votes::update::calculate(wl_storage, &bp_key, new_votes)?; if changed.is_empty() { - return Ok((changed, false)); + return Ok((changed, None)); } let confirmed = vote_tracking.seen && changed.contains(&bp_key.seen()); (vote_tracking, changed, confirmed, true) @@ -181,7 +179,7 @@ where &vote_tracking, already_present, )?; - Ok((changed, confirmed)) + Ok((changed, confirmed.then_some(update))) } #[cfg(test)] @@ -292,8 +290,9 @@ mod test_apply_bp_roots_to_storage { let TxResult { changed_keys, .. } = apply_derived_tx(&mut wl_storage, vext.into()) .expect("Test failed"); - let bp_root_key = vote_tallies::Keys::from(BridgePoolRoot( - BridgePoolRootProof::new((root, nonce)), + let bp_root_key = vote_tallies::Keys::from(( + &BridgePoolRoot(BridgePoolRootProof::new((root, nonce))), + 100.into(), )); let expected: BTreeSet = bp_root_key.into_iter().collect(); assert_eq!(expected, changed_keys); @@ -349,8 +348,9 @@ mod test_apply_bp_roots_to_storage { vexts.insert(vext); let TxResult { changed_keys, .. } = apply_derived_tx(&mut wl_storage, vexts).expect("Test failed"); - let bp_root_key = vote_tallies::Keys::from(BridgePoolRoot( - BridgePoolRootProof::new((root, nonce)), + let bp_root_key = vote_tallies::Keys::from(( + &BridgePoolRoot(BridgePoolRootProof::new((root, nonce))), + 100.into(), )); let mut expected: BTreeSet = bp_root_key.into_iter().collect(); @@ -392,8 +392,9 @@ mod test_apply_bp_roots_to_storage { let TxResult { changed_keys, .. } = apply_derived_tx(&mut wl_storage, vext.into()) .expect("Test failed"); - let bp_root_key = vote_tallies::Keys::from(BridgePoolRoot( - BridgePoolRootProof::new((root, nonce)), + let bp_root_key = vote_tallies::Keys::from(( + &BridgePoolRoot(BridgePoolRootProof::new((root, nonce))), + 100.into(), )); let expected: BTreeSet = [ bp_root_key.seen(), @@ -417,8 +418,9 @@ mod test_apply_bp_roots_to_storage { let root = wl_storage.ethbridge_queries().get_bridge_pool_root(); let nonce = wl_storage.ethbridge_queries().get_bridge_pool_nonce(); let to_sign = keccak_hash([root.0, nonce.to_bytes()].concat()); - let bp_root_key = vote_tallies::Keys::from(BridgePoolRoot( - BridgePoolRootProof::new((root, nonce)), + let bp_root_key = vote_tallies::Keys::from(( + &BridgePoolRoot(BridgePoolRootProof::new((root, nonce))), + 100.into(), )); let hot_key = &keys[&validators[0]].eth_bridge; @@ -471,8 +473,9 @@ mod test_apply_bp_roots_to_storage { let to_sign = keccak_hash([root.0, nonce.to_bytes()].concat()); let hot_key = &keys[&validators[0]].eth_bridge; - let bp_root_key = vote_tallies::Keys::from(BridgePoolRoot( - BridgePoolRootProof::new((root, nonce)), + let bp_root_key = vote_tallies::Keys::from(( + &BridgePoolRoot(BridgePoolRootProof::new((root, nonce))), + 100.into(), )); let vext = bridge_pool_roots::Vext { @@ -529,8 +532,9 @@ mod test_apply_bp_roots_to_storage { let to_sign = keccak_hash([root.0, nonce.to_bytes()].concat()); let hot_key = &keys[&validators[0]].eth_bridge; - let bp_root_key = vote_tallies::Keys::from(BridgePoolRoot( - BridgePoolRootProof::new((root, nonce)), + let bp_root_key = vote_tallies::Keys::from(( + &BridgePoolRoot(BridgePoolRootProof::new((root, nonce))), + 100.into(), )); let vext = bridge_pool_roots::Vext { @@ -593,7 +597,7 @@ mod test_apply_bp_roots_to_storage { let hot_key = &keys[&validators[0]].eth_bridge; let mut expected = BridgePoolRoot(BridgePoolRootProof::new((root, nonce))); - let bp_root_key = vote_tallies::Keys::from(&expected); + let bp_root_key = vote_tallies::Keys::from((&expected, 100.into())); let vext = bridge_pool_roots::Vext { validator_addr: validators[0].clone(), diff --git a/ethereum_bridge/src/storage/vote_tallies.rs b/ethereum_bridge/src/storage/vote_tallies.rs index 084746adbb..4b2d26c5c4 100644 --- a/ethereum_bridge/src/storage/vote_tallies.rs +++ b/ethereum_bridge/src/storage/vote_tallies.rs @@ -8,8 +8,8 @@ use namada_core::ledger::eth_bridge::ADDRESS; use namada_core::types::address::Address; use namada_core::types::ethereum_events::{EthereumEvent, Uint}; use namada_core::types::hash::Hash; -use namada_core::types::keccak::KeccakHash; -use namada_core::types::storage::{DbKeySeg, Epoch, Key}; +use namada_core::types::keccak::{keccak_hash, KeccakHash}; +use namada_core::types::storage::{BlockHeight, DbKeySeg, Epoch, Key}; use namada_core::types::vote_extensions::validator_set_update::VotingPowersMap; use namada_macros::StorageKeys; @@ -207,15 +207,23 @@ impl BorshDeserialize for BridgePoolRoot { } } -impl<'a> From<&'a BridgePoolRoot> for Keys { - fn from(bp_root: &BridgePoolRoot) -> Self { - let hash = [bp_root.0.data.0.to_string(), bp_root.0.data.1.to_string()] - .concat(); +impl From<(&BridgePoolRoot, BlockHeight)> for Keys { + fn from( + (BridgePoolRoot(bp_root), root_height): (&BridgePoolRoot, BlockHeight), + ) -> Self { + let hash = { + let (KeccakHash(root), nonce) = &bp_root.data; + + let mut to_hash = [0u8; 64]; + to_hash[..32].copy_from_slice(root); + to_hash[32..].copy_from_slice(&nonce.to_bytes()); + + keccak_hash(to_hash).to_string() + }; let prefix = super::prefix() - .push(&BRIDGE_POOL_ROOT_PREFIX_KEY_SEGMENT.to_owned()) - .expect("should always be able to construct this key") - .push(&hash) - .expect("should always be able to construct this key"); + .with_segment(BRIDGE_POOL_ROOT_PREFIX_KEY_SEGMENT.to_owned()) + .with_segment(root_height) + .with_segment(hash); Keys { prefix, _phantom: std::marker::PhantomData, @@ -223,12 +231,6 @@ impl<'a> From<&'a BridgePoolRoot> for Keys { } } -impl From for Keys { - fn from(bp_root: BridgePoolRoot) -> Self { - Self::from(&bp_root) - } -} - /// Get the key prefix corresponding to the storage location of validator set /// updates whose "seen" state is being tracked. pub fn valset_upds_prefix() -> Key { diff --git a/shared/src/ledger/protocol/mod.rs b/shared/src/ledger/protocol/mod.rs index c6ed0814b1..2891f1f0fb 100644 --- a/shared/src/ledger/protocol/mod.rs +++ b/shared/src/ledger/protocol/mod.rs @@ -1210,9 +1210,10 @@ mod tests { apply_eth_tx(tx.clone(), &mut wl_storage)?; apply_eth_tx(tx, &mut wl_storage)?; - let bp_root_keys = vote_tallies::Keys::from( - vote_tallies::BridgePoolRoot(EthereumProof::new((root, nonce))), - ); + let bp_root_keys = vote_tallies::Keys::from(( + &vote_tallies::BridgePoolRoot(EthereumProof::new((root, nonce))), + 100.into(), + )); let root_seen_by_bytes = wl_storage.read_bytes(&bp_root_keys.seen_by())?; assert_eq!( From cd8a0c4b5ff8a6c7d52e2d390271af5621be2582 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 14 Sep 2023 14:01:04 +0100 Subject: [PATCH 3/9] Only update a signed BP root in storage if it is newer than old root --- .../transactions/bridge_pool_roots.rs | 38 +++++++++++++------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs b/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs index cee70fcf4d..34a9de3dba 100644 --- a/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs +++ b/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs @@ -1,10 +1,9 @@ use std::collections::{HashMap, HashSet}; -use borsh::BorshSerialize; use eyre::Result; use namada_core::ledger::eth_bridge::storage::bridge_pool::get_signed_root_key; use namada_core::ledger::storage::{DBIter, StorageHasher, WlStorage, DB}; -use namada_core::ledger::storage_api::StorageWrite; +use namada_core::ledger::storage_api::{StorageRead, StorageWrite}; use namada_core::types::address::Address; use namada_core::types::storage::BlockHeight; use namada_core::types::transaction::TxResult; @@ -60,17 +59,32 @@ where // if the root is confirmed, update storage and add // relevant key to changed. if let Some(proof) = confirmed_update { - wl_storage - .write_bytes( - &get_signed_root_key(), - (proof, root_height) - .try_to_vec() - .expect("Serializing a Bridge pool root shouldn't fail."), - ) + let signed_root_key = get_signed_root_key(); + let should_write_root = wl_storage + .read::<(BridgePoolRoot, BlockHeight)>(&signed_root_key) .expect( - "Writing a signed bridge pool root to storage should not fail.", - ); - changed.insert(get_signed_root_key()); + "Reading a signed Bridge pool root from storage should not \ + fail", + ) + .map(|(_, existing_root_height)| { + // only write the newly confirmed signed root if + // it is more recent than the existing root in + // storage + existing_root_height < root_height + }) + .unwrap_or({ + // if no signed root was present in storage, write the new one + true + }); + if should_write_root { + wl_storage + .write(&signed_root_key, (proof, root_height)) + .expect( + "Writing a signed Bridge pool root to storage should not \ + fail.", + ); + changed.insert(get_signed_root_key()); + } } Ok(TxResult { From b28aaf2007bbdcfd0848ababd6bc0f355ab07384 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 14 Sep 2023 14:28:44 +0100 Subject: [PATCH 4/9] Add Debug to Bridge pool root tallies --- ethereum_bridge/src/storage/vote_tallies.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum_bridge/src/storage/vote_tallies.rs b/ethereum_bridge/src/storage/vote_tallies.rs index 4b2d26c5c4..09c9278d43 100644 --- a/ethereum_bridge/src/storage/vote_tallies.rs +++ b/ethereum_bridge/src/storage/vote_tallies.rs @@ -189,7 +189,7 @@ impl From<&Hash> for Keys { /// A wrapper struct for managing keys related to /// tracking signatures over bridge pool roots and nonces. -#[derive(Clone)] +#[derive(Debug, Clone)] pub struct BridgePoolRoot(pub BridgePoolRootProof); impl BorshSerialize for BridgePoolRoot { From d1ab2e12dbe423d9f917b811d8969e0f86baaefd Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 14 Sep 2023 14:29:15 +0100 Subject: [PATCH 5/9] Write test_more_recent_signed_root_not_overwritten() unit test --- .../transactions/bridge_pool_roots.rs | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs b/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs index 34a9de3dba..6b36aa4773 100644 --- a/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs +++ b/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs @@ -200,6 +200,7 @@ where mod test_apply_bp_roots_to_storage { use std::collections::BTreeSet; + use assert_matches::assert_matches; use borsh::{BorshDeserialize, BorshSerialize}; use namada_core::ledger::eth_bridge::storage::bridge_pool::{ get_key_from_hash, get_nonce_key, @@ -253,6 +254,11 @@ mod test_apply_bp_roots_to_storage { ]), ); bridge_pool_vp::init_storage(&mut wl_storage); + test_utils::commit_bridge_pool_root_at_height( + &mut wl_storage.storage, + &KeccakHash([1; 32]), + 99.into(), + ); test_utils::commit_bridge_pool_root_at_height( &mut wl_storage.storage, &KeccakHash([1; 32]), @@ -835,4 +841,79 @@ mod test_apply_bp_roots_to_storage { let root_epoch_validators = query_validators(root_epoch.0); assert_eq!(epoch_0_validators, root_epoch_validators); } + + #[test] + /// Test that a signed root is not overwritten in storage + /// if a signed root is decided that had been signed at a + /// less recent block height. + fn test_more_recent_signed_root_not_overwritten() { + let TestPackage { + validators, + keys, + mut wl_storage, + } = setup(); + + let root = wl_storage.ethbridge_queries().get_bridge_pool_root(); + let nonce = wl_storage.ethbridge_queries().get_bridge_pool_nonce(); + let to_sign = keccak_hash([root.0, nonce.to_bytes()].concat()); + + macro_rules! decide_at_height { + ($block_height:expr) => { + let hot_key = &keys[&validators[0]].eth_bridge; + let vext = bridge_pool_roots::Vext { + validator_addr: validators[0].clone(), + block_height: $block_height.into(), + sig: Signed::<_, SignableEthMessage>::new( + hot_key, + to_sign.clone(), + ) + .sig, + } + .sign(&keys[&validators[0]].protocol); + _ = apply_derived_tx(&mut wl_storage, vext.into()) + .expect("Test failed"); + let hot_key = &keys[&validators[1]].eth_bridge; + let vext = bridge_pool_roots::Vext { + validator_addr: validators[1].clone(), + block_height: $block_height.into(), + sig: Signed::<_, SignableEthMessage>::new( + hot_key, + to_sign.clone(), + ) + .sig, + } + .sign(&keys[&validators[1]].protocol); + _ = apply_derived_tx(&mut wl_storage, vext.into()) + .expect("Test failed"); + }; + } + + // decide bridge pool root signed at block height 100 + decide_at_height!(100); + + // check the signed root in storage + let root_in_storage = wl_storage + .read::<(BridgePoolRoot, BlockHeight)>(&get_signed_root_key()) + .expect("Test failed - storage read failed") + .expect("Test failed - no signed root in storage"); + assert_matches!( + root_in_storage, + (BridgePoolRoot(r), BlockHeight(100)) + if r.data.0 == root && r.data.1 == nonce + ); + + // decide bridge pool root signed at block height 99 + decide_at_height!(99); + + // check the signed root in storage is unchanged + let root_in_storage = wl_storage + .read::<(BridgePoolRoot, BlockHeight)>(&get_signed_root_key()) + .expect("Test failed - storage read failed") + .expect("Test failed - no signed root in storage"); + assert_matches!( + root_in_storage, + (BridgePoolRoot(r), BlockHeight(100)) + if r.data.0 == root && r.data.1 == nonce + ); + } } From 299fbc8a1a573de259f1a03060ac045fce411c86 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 14 Sep 2023 15:24:26 +0100 Subject: [PATCH 6/9] Return immediately if a BP proof is already available --- .../protocol/transactions/bridge_pool_roots.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs b/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs index 6b36aa4773..4def446412 100644 --- a/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs +++ b/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs @@ -47,10 +47,23 @@ where let root_height = vext.iter().next().unwrap().data.block_height; let (partial_proof, seen_by) = parse_vexts(wl_storage, vext); + // return immediately if a complete proof has already been acquired + let bp_key = vote_tallies::Keys::from((&partial_proof, root_height)); + let seen = + votes::storage::maybe_read_seen(wl_storage, &bp_key)?.unwrap_or(false); + if seen { + tracing::debug!( + ?root_height, + ?partial_proof, + "Bridge pool root tally is already complete" + ); + return Ok(TxResult::default()); + } + // apply updates to the bridge pool root. let (mut changed, confirmed_update) = apply_update( wl_storage, - root_height, + bp_key, partial_proof, seen_by, &voting_powers, @@ -149,7 +162,7 @@ where /// In all instances, the changed storage keys are returned. fn apply_update( wl_storage: &mut WlStorage, - root_height: BlockHeight, + bp_key: vote_tallies::Keys, mut update: BridgePoolRoot, seen_by: Votes, voting_powers: &HashMap<(Address, BlockHeight), FractionalVotingPower>, @@ -158,7 +171,6 @@ where D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, { - let bp_key = vote_tallies::Keys::from((&update, root_height)); let partial_proof = votes::storage::read_body(wl_storage, &bp_key); let (vote_tracking, changed, confirmed, already_present) = if let Ok( partial, From c92f8be85dad2cd8ba09fa597019089dc49fd5ac Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 14 Sep 2023 15:34:31 +0100 Subject: [PATCH 7/9] Add debug logs for discarded Bridge pool root proofs --- .../src/protocol/transactions/bridge_pool_roots.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs b/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs index 4def446412..cd28eeb8c7 100644 --- a/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs +++ b/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs @@ -90,6 +90,10 @@ where true }); if should_write_root { + tracing::debug!( + ?root_height, + "New Bridge pool root proof acquired" + ); wl_storage .write(&signed_root_key, (proof, root_height)) .expect( @@ -97,6 +101,11 @@ where fail.", ); changed.insert(get_signed_root_key()); + } else { + tracing::debug!( + ?root_height, + "Discarding outdated Bridge pool root proof" + ); } } From 59809d17c1d7a0a44229a0d2baabe3c021beacc1 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Tue, 19 Sep 2023 08:37:05 +0100 Subject: [PATCH 8/9] Declare returned storage key as must use Co-authored-by: Tomas Zemanovic --- core/src/types/storage.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/types/storage.rs b/core/src/types/storage.rs index e94f1280b8..a4fd16977a 100644 --- a/core/src/types/storage.rs +++ b/core/src/types/storage.rs @@ -499,6 +499,7 @@ impl Key { /// Takes ownership of the key, appends a new segment to it, /// and returns the modified key. + #[must_use] pub fn with_segment(mut self, other: T) -> Self { self.segments.push(other.to_db_key()); self From d854be06611bcb2b91c9488bb7b7513d7cdf05d1 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 14 Sep 2023 15:28:36 +0100 Subject: [PATCH 9/9] Changelog for #1893 --- .../unreleased/bug-fixes/1893-bridge-pool-roots-signing.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/1893-bridge-pool-roots-signing.md diff --git a/.changelog/unreleased/bug-fixes/1893-bridge-pool-roots-signing.md b/.changelog/unreleased/bug-fixes/1893-bridge-pool-roots-signing.md new file mode 100644 index 0000000000..70e8e3d662 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1893-bridge-pool-roots-signing.md @@ -0,0 +1,2 @@ +- Never overwrite recent Bridge pool proofs in storage + ([\#1893](https://github.com/anoma/namada/pull/1893)) \ No newline at end of file