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 diff --git a/core/src/types/storage.rs b/core/src/types/storage.rs index b2793a8d62..4aae00b42e 100644 --- a/core/src/types/storage.rs +++ b/core/src/types/storage.rs @@ -497,6 +497,14 @@ impl Key { Ok(Key { segments }) } + /// 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 + } + /// 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(); diff --git a/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs b/ethereum_bridge/src/protocol/transactions/bridge_pool_roots.rs index 0c3ba5c34c..3271efeed5 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::token::Amount; @@ -48,32 +47,66 @@ 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) = apply_update( + let (mut changed, confirmed_update) = apply_update( wl_storage, - partial_proof.clone(), + bp_key, + 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), - )?; - wl_storage - .write_bytes( - &get_signed_root_key(), - (proof, root_height) - .try_to_vec() - .expect("Serializing a Bridge pool root shouldn't fail."), - ) + if let Some(proof) = confirmed_update { + 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.", + "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 { + tracing::debug!( + ?root_height, + "New Bridge pool root proof acquired" ); - changed.insert(get_signed_root_key()); + 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()); + } else { + tracing::debug!( + ?root_height, + "Discarding outdated Bridge pool root proof" + ); + } } Ok(TxResult { @@ -138,15 +171,15 @@ where /// In all instances, the changed storage keys are returned. fn apply_update( wl_storage: &mut WlStorage, + bp_key: vote_tallies::Keys, mut update: BridgePoolRoot, seen_by: Votes, voting_powers: &HashMap<(Address, BlockHeight), Amount>, -) -> 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 partial_proof = votes::storage::read_body(wl_storage, &bp_key); let (vote_tracking, changed, confirmed, already_present) = if let Ok( partial, @@ -162,7 +195,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,13 +214,14 @@ where &vote_tracking, already_present, )?; - Ok((changed, confirmed)) + Ok((changed, confirmed.then_some(update))) } #[cfg(test)] 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, @@ -237,6 +271,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]), @@ -288,8 +327,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); @@ -345,8 +385,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(); @@ -388,8 +429,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(), @@ -413,8 +455,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; @@ -467,8 +510,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 { @@ -525,8 +569,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 { @@ -589,7 +634,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(), @@ -809,4 +854,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 + ); + } } diff --git a/ethereum_bridge/src/storage/vote_tallies.rs b/ethereum_bridge/src/storage/vote_tallies.rs index 23ec02bfec..ec03c498d6 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; @@ -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 { @@ -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 1f2883fdea..ceb41a36af 100644 --- a/shared/src/ledger/protocol/mod.rs +++ b/shared/src/ledger/protocol/mod.rs @@ -1152,9 +1152,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!(