Never overwrite recent Bridge pool proofs in storage

.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))

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<T: KeySeg>(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();

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;
@@ -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<D, H>(
     wl_storage: &mut WlStorage<D, H>,
+    bp_key: vote_tallies::Keys<BridgePoolRoot>,
     mut update: BridgePoolRoot,
     seen_by: Votes,
     voting_powers: &HashMap<(Address, BlockHeight), FractionalVotingPower>,
-) -> Result<(ChangedKeys, bool)>
+) -> Result<(ChangedKeys, Option<BridgePoolRoot>)>
 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,
@@ -241,6 +275,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]),
@@ -292,8 +331,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<Key> = bp_root_key.into_iter().collect();
         assert_eq!(expected, changed_keys);
@@ -349,8 +389,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<Key> = bp_root_key.into_iter().collect();
@@ -392,8 +433,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<Key> = [
             bp_root_key.seen(),
@@ -417,8 +459,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 +514,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 +573,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 +638,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(),
@@ -817,4 +862,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
+        );
+    }
 }

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<EthereumEvent> {
 
 /// 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,28 +207,30 @@ impl BorshDeserialize for BridgePoolRoot {
     }
 }
 
-impl<'a> From<&'a BridgePoolRoot> for Keys<BridgePoolRoot> {
-    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<BridgePoolRoot> {
+    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,
         }
     }
 }
 
-impl From<BridgePoolRoot> for Keys<BridgePoolRoot> {
-    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 {