Merge pull request #676 from anoma/tiago+james/ethbridge/blockheight-…

…refactor

Record the block heights at which validators vote for Ethereum events

apps/src/lib/node/ledger/protocol/transactions/ethereum_events/eth_msgs.rs

@@ -1,12 +1,8 @@
-use std::collections::BTreeSet;
-
 use borsh::{BorshDeserialize, BorshSchema, BorshSerialize};
-use namada::types::address::Address;
 use namada::types::ethereum_events::EthereumEvent;
-use namada::types::storage::BlockHeight;
 use namada::types::vote_extensions::ethereum_events::MultiSignedEthEvent;
 
-use crate::node::ledger::protocol::transactions::votes::Tally;
+use crate::node::ledger::protocol::transactions::votes::{Tally, Votes};
 
 /// Represents an Ethereum event being seen by some validators
 #[derive(
@@ -21,14 +17,12 @@ use crate::node::ledger::protocol::transactions::votes::Tally;
     BorshDeserialize,
 )]
 pub struct EthMsgUpdate {
-    /// The event being seen
+    /// The event being seen.
     pub body: EthereumEvent,
-    /// Addresses of the validators who have just seen this event. We use
-    /// [`BTreeSet`] even though ordering is not important here, so that we
-    /// can derive [`Hash`] for [`EthMsgUpdate`].
+    /// New votes for this event.
     // NOTE(feature = "abcipp"): This can just become BTreeSet<Address> because
     // BlockHeight will always be the previous block
-    pub seen_by: BTreeSet<(Address, BlockHeight)>,
+    pub seen_by: Votes,
 }
 
 impl From<MultiSignedEthEvent> for EthMsgUpdate {
@@ -80,10 +74,7 @@ mod tests {
         };
         let expected = EthMsgUpdate {
             body: event,
-            seen_by: BTreeSet::from_iter(vec![(
-                sole_validator,
-                BlockHeight(100),
-            )]),
+            seen_by: Votes::from([(sole_validator, BlockHeight(100))]),
         };
 
         let update: EthMsgUpdate = with_signers.into();

apps/src/lib/node/ledger/protocol/transactions/ethereum_events/mod.rs

@@ -12,21 +12,19 @@ use namada::ledger::eth_bridge::storage::vote_tallies;
 use namada::ledger::storage::traits::StorageHasher;
 use namada::ledger::storage::{DBIter, Storage, DB};
 use namada::types::address::Address;
-use namada::types::storage::{self, BlockHeight};
+use namada::types::storage::BlockHeight;
 use namada::types::transaction::TxResult;
 use namada::types::vote_extensions::ethereum_events::MultiSignedEthEvent;
 use namada::types::voting_power::FractionalVotingPower;
 
+use super::ChangedKeys;
 use crate::node::ledger::protocol::transactions::utils::{
     self, get_active_validators,
 };
 use crate::node::ledger::protocol::transactions::votes::{
-    calculate_new, calculate_updated, write,
+    calculate_new, calculate_updated, write, Votes,
 };
 
-/// The keys changed while applying a protocol transaction
-type ChangedKeys = BTreeSet<storage::Key>;
-
 /// Applies derived state changes to storage, based on Ethereum `events` which
 /// were newly seen by some active validator(s) in the last epoch. For `events`
 /// which have been seen by enough voting power, extra state changes may take
@@ -157,9 +155,17 @@ where
     // is a less arbitrary way to do this
     let (exists_in_storage, _) = storage.has_key(&eth_msg_keys.seen())?;
 
+    let mut seen_by = Votes::default();
+    for (address, block_height) in update.seen_by.into_iter() {
+        // TODO: more deterministic deduplication
+        if let Some(present) = seen_by.insert(address, block_height) {
+            tracing::warn!(?present, "Duplicate vote in digest");
+        }
+    }
+
     let (vote_tracking, changed, confirmed) = if !exists_in_storage {
         tracing::debug!(%eth_msg_keys.prefix, "Ethereum event not seen before by any validator");
-        let vote_tracking = calculate_new(&update.seen_by, voting_powers)?;
+        let vote_tracking = calculate_new(seen_by, voting_powers)?;
         let changed = eth_msg_keys.into_iter().collect();
         let confirmed = vote_tracking.seen;
         (vote_tracking, changed, confirmed)
@@ -168,9 +174,24 @@ where
             %eth_msg_keys.prefix,
             "Ethereum event already exists in storage",
         );
-        let vote_tracking =
-            calculate_updated(storage, &eth_msg_keys, voting_powers)?;
-        let changed = BTreeSet::default(); // TODO(namada#515): calculate changed keys
+        let mut votes = HashMap::default();
+        seen_by.iter().for_each(|(address, block_height)| {
+            let voting_power = voting_powers
+                .get(&(address.to_owned(), block_height.to_owned()))
+                .unwrap();
+            if let Some(already_present_voting_power) =
+                votes.insert(address.to_owned(), voting_power.to_owned())
+            {
+                tracing::warn!(
+                    ?address,
+                    ?already_present_voting_power,
+                    new_voting_power = ?voting_power,
+                    "Validator voted more than once, arbitrarily using later value",
+                )
+            }
+        });
+        let (vote_tracking, changed) =
+            calculate_updated(storage, &eth_msg_keys, &votes)?;
         let confirmed =
             vote_tracking.seen && changed.contains(&eth_msg_keys.seen());
         (vote_tracking, changed, confirmed)
@@ -208,7 +229,6 @@ mod tests {
     };
     use namada::types::ethereum_events::{EthereumEvent, TransferToNamada};
     use namada::types::token::Amount;
-    use storage::BlockHeight;
 
     use super::*;
 
@@ -230,10 +250,7 @@ mod tests {
         };
         let update = EthMsgUpdate {
             body: body.clone(),
-            seen_by: BTreeSet::from_iter(vec![(
-                sole_validator.clone(),
-                BlockHeight(100),
-            )]),
+            seen_by: Votes::from([(sole_validator.clone(), BlockHeight(100))]),
         };
         let updates = HashSet::from_iter(vec![update]);
         let voting_powers = HashMap::from_iter(vec![(
@@ -269,8 +286,8 @@ mod tests {
         let (seen_by_bytes, _) = storage.read(&eth_msg_keys.seen_by())?;
         let seen_by_bytes = seen_by_bytes.unwrap();
         assert_eq!(
-            Vec::<Address>::try_from_slice(&seen_by_bytes)?,
-            vec![sole_validator]
+            Votes::try_from_slice(&seen_by_bytes)?,
+            Votes::from([(sole_validator, BlockHeight(100))])
         );
 
         let (voting_power_bytes, _) =

apps/src/lib/node/ledger/protocol/transactions/mod.rs

@@ -4,8 +4,14 @@
 //! to update their blockchain state in a deterministic way. This can be done
 //! natively rather than via the wasm environment as happens with regular
 //! transactions.
+
+use std::collections::BTreeSet;
+
+use namada::types::storage;
+
 #[cfg(not(feature = "abcipp"))]
 pub(super) mod ethereum_events;
+
 #[cfg(not(feature = "abcipp"))]
 mod votes;
 
@@ -17,3 +23,6 @@ mod update;
 
 #[cfg(not(feature = "abcipp"))]
 mod utils;
+
+/// The keys changed while applying a protocol transaction.
+pub type ChangedKeys = BTreeSet<storage::Key>;

apps/src/lib/node/ledger/protocol/transactions/votes.rs

@@ -1,7 +1,7 @@
 //! Logic and data types relating to tallying validators' votes for pieces of
 //! data stored in the ledger, where those pieces of data should only be acted
 //! on once they have received enough votes
-use std::collections::{BTreeSet, HashMap};
+use std::collections::{BTreeMap, HashMap};
 
 use borsh::{BorshDeserialize, BorshSchema, BorshSerialize};
 use eyre::{eyre, Result};
@@ -12,8 +12,17 @@ use namada::types::address::Address;
 use namada::types::storage::BlockHeight;
 use namada::types::voting_power::FractionalVotingPower;
 
+use super::ChangedKeys;
 use crate::node::ledger::protocol::transactions::read;
 
+/// The addresses of validators that voted for something, and the block
+/// heights at which they voted. We use a [`BTreeMap`] to enforce that a
+/// validator (as uniquely identified by an [`Address`]) may vote at most once,
+/// and their vote must be associated with a specific [`BlockHeight`]. Their
+/// voting power at that block height is what is used when calculating whether
+/// something has enough voting power behind it or not.
+pub type Votes = BTreeMap<Address, BlockHeight>;
+
 #[derive(
     Clone, Debug, PartialEq, Eq, BorshSerialize, BorshDeserialize, BorshSchema,
 )]
@@ -22,11 +31,11 @@ use crate::node::ledger::protocol::transactions::read;
 pub struct Tally {
     /// The total voting power that's voted for this event across all epochs
     pub voting_power: FractionalVotingPower,
-    /// The addresses of validators that voted for this event. We use a
-    /// set type as validators should only be able to vote at most once,
-    /// and [`BTreeSet`] specifically as we want this field to be
-    /// deterministically ordered for storage.
-    pub seen_by: BTreeSet<Address>,
+    /// The votes which have been counted towards `voting_power`. Note that
+    /// validators may submit multiple votes at different block heights for
+    /// the same thing, but ultimately only one vote per validator will be
+    /// used when tallying voting power.
+    pub seen_by: Votes,
     /// Whether this event has been acted on or not - this should only ever
     /// transition from `false` to `true`, once there is enough voting power
     pub seen: bool,
@@ -35,11 +44,11 @@ pub struct Tally {
 /// Calculate a new [`Tally`] based on some validators' fractional voting powers
 /// as specific block heights
 pub fn calculate_new(
-    seen_by: &BTreeSet<(Address, BlockHeight)>,
+    seen_by: Votes,
     voting_powers: &HashMap<(Address, BlockHeight), FractionalVotingPower>,
 ) -> Result<Tally> {
     let mut seen_by_voting_power = FractionalVotingPower::default();
-    for (validator, block_height) in seen_by {
+    for (validator, block_height) in seen_by.iter() {
         match voting_powers
             .get(&(validator.to_owned(), block_height.to_owned()))
         {
@@ -57,21 +66,18 @@ pub fn calculate_new(
         seen_by_voting_power > FractionalVotingPower::TWO_THIRDS;
     Ok(Tally {
         voting_power: seen_by_voting_power,
-        seen_by: seen_by
-            .iter()
-            .map(|(validator, _)| validator.to_owned())
-            .collect(),
+        seen_by,
         seen: newly_confirmed,
     })
 }
 
 /// Calculate an updated [`Tally`] based on one that is in storage under `keys`,
-/// and some new votes
+/// with some new `voters`.
 pub fn calculate_updated<D, H, T>(
     store: &mut Storage<D, H>,
     keys: &vote_tallies::Keys<T>,
-    _voting_powers: &HashMap<(Address, BlockHeight), FractionalVotingPower>,
-) -> Result<Tally>
+    _voters: &HashMap<Address, FractionalVotingPower>,
+) -> Result<(Tally, ChangedKeys)>
 where
     D: 'static + DB + for<'iter> DBIter<'iter> + Sync,
     H: 'static + StorageHasher + Sync,
@@ -80,7 +86,7 @@ where
     // TODO(namada#515): implement this
     let _body: T = read::value(store, &keys.body())?;
     let seen: bool = read::value(store, &keys.seen())?;
-    let seen_by: BTreeSet<Address> = read::value(store, &keys.seen_by())?;
+    let seen_by: Votes = read::value(store, &keys.seen_by())?;
     let voting_power: FractionalVotingPower =
         read::value(store, &keys.voting_power())?;
 
@@ -95,7 +101,7 @@ where
         "Updating events is not implemented yet, so the returned vote tally \
          will be identical to the one in storage",
     );
-    Ok(tally)
+    Ok((tally, ChangedKeys::default()))
 }
 
 pub fn write<D, H, T>(