Merge branch 'brent/fix-validator-set-update-with-ck' (#2563)

* brent/fix-validator-set-update-with-ck:
  changelog: add #2563
  refactor conversion of token amount into voting power
  refactor validator set update to consensus engine
  simplest fix
  test a validator that becomes consensus and gets new consensus key at same epoch (fails)

.changelog/unreleased/bug-fixes/2653-fix-validator-set-update-with-ck.md

@@ -0,0 +1,4 @@
+- Fixed a bug in the communication of validator set updates to
+  CometBFT after a change of validator consensus key that occurs
+  at the same epoch as a validator entering the consensus set.
+  ([\#2653](https://github.com/anoma/namada/pull/2653))

crates/apps/src/lib/node/ledger/shell/mod.rs

@@ -38,7 +38,6 @@ use namada::ethereum_bridge::protocol::validation::validator_set_update::validat
 use namada::ledger::events::log::EventLog;
 use namada::ledger::events::Event;
 use namada::ledger::gas::{Gas, TxGasMeter};
-use namada::ledger::pos::into_tm_voting_power;
 use namada::ledger::pos::namada_proof_of_stake::types::{
     ConsensusValidator, ValidatorSetUpdate,
 };
@@ -1294,7 +1293,7 @@ where
         let validator_set_update_fn = if is_genesis {
             namada_proof_of_stake::genesis_validator_set_tendermint
         } else {
-            namada_proof_of_stake::validator_set_update::validator_set_update_tendermint
+            namada_proof_of_stake::validator_set_update::validator_set_update_comet
         };
 
         validator_set_update_fn(
@@ -1305,14 +1304,8 @@ where
                 let (consensus_key, power) = match update {
                     ValidatorSetUpdate::Consensus(ConsensusValidator {
                         consensus_key,
-                        bonded_stake,
-                    }) => {
-                        let power: i64 = into_tm_voting_power(
-                            pos_params.tm_votes_per_token,
-                            bonded_stake,
-                        );
-                        (consensus_key, power)
-                    }
+                        bonded_stake: power,
+                    }) => (consensus_key, power),
                     ValidatorSetUpdate::Deactivated(consensus_key) => {
                         // Any validators that have been dropped from the
                         // consensus set must have voting power set to 0 to

crates/proof_of_stake/src/lib.rs

@@ -36,6 +36,7 @@ use namada_storage::collections::lazy_map::{self, Collectable, LazyMap};
 use namada_storage::{StorageRead, StorageWrite};
 pub use namada_trans_token as token;
 pub use parameters::{OwnedPosParams, PosParams};
+use types::into_tm_voting_power;
 
 use crate::queries::{find_bonds, has_bonds};
 use crate::rewards::{
@@ -1843,9 +1844,11 @@ where
         let consensus_key = validator_consensus_key_handle(&address)
             .get(storage, current_epoch, params)?
             .unwrap();
+        let new_tm_voting_power =
+            into_tm_voting_power(params.tm_votes_per_token, new_stake);
         let converted = f(ValidatorSetUpdate::Consensus(ConsensusValidator {
             consensus_key,
-            bonded_stake: new_stake,
+            bonded_stake: new_tm_voting_power,
         }));
         Ok(converted)
     })

crates/proof_of_stake/src/tests/helpers.rs

@@ -15,7 +15,7 @@ use proptest::strategy::{Just, Strategy};
 use crate::parameters::testing::arb_pos_params;
 use crate::types::{GenesisValidator, ValidatorSetUpdate};
 use crate::validator_set_update::{
-    copy_validator_sets_and_positions, validator_set_update_tendermint,
+    copy_validator_sets_and_positions, validator_set_update_comet,
 };
 use crate::{
     compute_and_store_total_consensus_stake, OwnedPosParams, PosParams,
@@ -57,7 +57,7 @@ pub fn get_tendermint_set_updates(
     // the start of a new one too and so we give it the predecessor of the
     // current epoch here to actually get the update for the current epoch.
     let epoch = Epoch(epoch - 1);
-    validator_set_update_tendermint(s, params, epoch, |update| update).unwrap()
+    validator_set_update_comet(s, params, epoch, |update| update).unwrap()
 }
 
 /// Advance to the next epoch. Returns the new epoch.

crates/proof_of_stake/src/tests/test_validator.rs

@@ -42,8 +42,9 @@ use crate::validator_set_update::{
     insert_validator_into_validator_set, update_validator_set,
 };
 use crate::{
-    become_validator, bond_tokens, is_validator, staking_token_address,
-    unbond_tokens, withdraw_tokens, BecomeValidator, OwnedPosParams,
+    become_validator, bond_tokens, change_consensus_key, is_validator,
+    staking_token_address, unbond_tokens, withdraw_tokens, BecomeValidator,
+    OwnedPosParams,
 };
 
 proptest! {
@@ -555,7 +556,10 @@ fn test_validator_sets() {
         tm_updates[0],
         ValidatorSetUpdate::Consensus(ConsensusValidator {
             consensus_key: pk3,
-            bonded_stake: stake3,
+            bonded_stake: into_tm_voting_power(
+                params.tm_votes_per_token,
+                stake3
+            ),
         })
     );
 
@@ -610,7 +614,10 @@ fn test_validator_sets() {
         tm_updates[0],
         ValidatorSetUpdate::Consensus(ConsensusValidator {
             consensus_key: pk5,
-            bonded_stake: stake5,
+            bonded_stake: into_tm_voting_power(
+                params.tm_votes_per_token,
+                stake5
+            ),
         })
     );
     assert_eq!(tm_updates[1], ValidatorSetUpdate::Deactivated(pk2));
@@ -811,7 +818,10 @@ fn test_validator_sets() {
         tm_updates[0],
         ValidatorSetUpdate::Consensus(ConsensusValidator {
             consensus_key: pk4.clone(),
-            bonded_stake: stake4,
+            bonded_stake: into_tm_voting_power(
+                params.tm_votes_per_token,
+                stake4
+            ),
         })
     );
     assert_eq!(tm_updates[1], ValidatorSetUpdate::Deactivated(pk1));
@@ -926,7 +936,10 @@ fn test_validator_sets() {
         tm_updates[0],
         ValidatorSetUpdate::Consensus(ConsensusValidator {
             consensus_key: pk6,
-            bonded_stake: stake6,
+            bonded_stake: into_tm_voting_power(
+                params.tm_votes_per_token,
+                stake6
+            ),
         })
     );
     assert_eq!(tm_updates[1], ValidatorSetUpdate::Deactivated(pk4));
@@ -1185,10 +1198,64 @@ fn test_validator_sets_swap() {
     assert_eq!(
         tm_updates[0],
         ValidatorSetUpdate::Consensus(ConsensusValidator {
-            consensus_key: pk3,
-            bonded_stake: stake3,
+            consensus_key: pk3.clone(),
+            bonded_stake: into_tm_voting_power(
+                params.tm_votes_per_token,
+                stake3
+            ),
         })
     );
+
+    // Now give val2 stake such that it bumps val3 out of the consensus set, and
+    // also change val2's consensus key
+    let pipeline_epoch = epoch + params.pipeline_len;
+    let bonds_epoch_3 = pipeline_epoch;
+    let bonds = token::Amount::native_whole(1);
+    let stake2 = stake2 + bonds;
+
+    update_validator_set(&mut s, &params, &val2, bonds.change(), epoch, None)
+        .unwrap();
+    update_validator_deltas(
+        &mut s,
+        &params,
+        &val2,
+        bonds.change(),
+        epoch,
+        None,
+    )
+    .unwrap();
+
+    sk_seed += 1;
+    let new_ck2 = key::testing::common_sk_from_simple_seed(sk_seed).to_public();
+    change_consensus_key(&mut s, &val2, &new_ck2, epoch).unwrap();
+
+    // Advance to EPOCH 5
+    let epoch = advance_epoch(&mut s, &params);
+
+    // Check tendermint validator set updates
+    let tm_updates = get_tendermint_set_updates(&s, &params, epoch);
+    assert!(tm_updates.is_empty());
+
+    // Advance to EPOCH 6
+    let epoch = advance_epoch(&mut s, &params);
+    assert_eq!(epoch, bonds_epoch_3);
+
+    let tm_updates = get_tendermint_set_updates(&s, &params, epoch);
+    // dbg!(&tm_updates);
+    assert_eq!(tm_updates.len(), 2);
+    assert_eq!(
+        tm_updates,
+        vec![
+            ValidatorSetUpdate::Consensus(ConsensusValidator {
+                consensus_key: new_ck2,
+                bonded_stake: into_tm_voting_power(
+                    params.tm_votes_per_token,
+                    stake2
+                ),
+            }),
+            ValidatorSetUpdate::Deactivated(pk3),
+        ]
+    );
 }
 
 proptest! {

crates/proof_of_stake/src/types/mod.rs

@@ -388,13 +388,13 @@ pub enum ValidatorSetUpdate {
     Deactivated(common::PublicKey),
 }
 
-/// Consensus validator's consensus key and its bonded stake.
+/// Newly updated consensus validator's consensus key and bonded stake.
 #[derive(Debug, Clone, PartialEq)]
 pub struct ConsensusValidator {
     /// A public key used for signing validator's consensus actions
     pub consensus_key: common::PublicKey,
     /// Total bonded stake of the validator
-    pub bonded_stake: token::Amount,
+    pub bonded_stake: i64,
 }
 
 /// ID of a bond and/or an unbond.