diff --git a/.changelog/unreleased/bug-fixes/2653-fix-validator-set-update-with-ck.md b/.changelog/unreleased/bug-fixes/2653-fix-validator-set-update-with-ck.md new file mode 100644 index 0000000000..d88bc87bb2 --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/crates/apps/src/lib/node/ledger/shell/mod.rs b/crates/apps/src/lib/node/ledger/shell/mod.rs index d33656aa9d..28a2cc006a 100644 --- a/crates/apps/src/lib/node/ledger/shell/mod.rs +++ b/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 diff --git a/crates/proof_of_stake/src/lib.rs b/crates/proof_of_stake/src/lib.rs index 558c40e0ea..6fb618db4f 100644 --- a/crates/proof_of_stake/src/lib.rs +++ b/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) }) diff --git a/crates/proof_of_stake/src/tests/helpers.rs b/crates/proof_of_stake/src/tests/helpers.rs index 96f6ff7d11..d66b64c504 100644 --- a/crates/proof_of_stake/src/tests/helpers.rs +++ b/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. diff --git a/crates/proof_of_stake/src/tests/test_validator.rs b/crates/proof_of_stake/src/tests/test_validator.rs index ebcaec7703..4ddb17c7c6 100644 --- a/crates/proof_of_stake/src/tests/test_validator.rs +++ b/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, ¶ms, &val2, bonds.change(), epoch, None) + .unwrap(); + update_validator_deltas( + &mut s, + ¶ms, + &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, ¶ms); + + // Check tendermint validator set updates + let tm_updates = get_tendermint_set_updates(&s, ¶ms, epoch); + assert!(tm_updates.is_empty()); + + // Advance to EPOCH 6 + let epoch = advance_epoch(&mut s, ¶ms); + assert_eq!(epoch, bonds_epoch_3); + + let tm_updates = get_tendermint_set_updates(&s, ¶ms, 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! { diff --git a/crates/proof_of_stake/src/types/mod.rs b/crates/proof_of_stake/src/types/mod.rs index 282ab6a6d4..b2a0d176ad 100644 --- a/crates/proof_of_stake/src/types/mod.rs +++ b/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. diff --git a/crates/proof_of_stake/src/validator_set_update.rs b/crates/proof_of_stake/src/validator_set_update.rs index 3897697a01..474af4ddda 100644 --- a/crates/proof_of_stake/src/validator_set_update.rs +++ b/crates/proof_of_stake/src/validator_set_update.rs @@ -561,7 +561,7 @@ where /// Communicate imminent validator set updates to Tendermint. This function is /// called two blocks before the start of a new epoch because Tendermint /// validator updates become active two blocks after the updates are submitted. -pub fn validator_set_update_tendermint( +pub fn validator_set_update_comet( storage: &S, params: &PosParams, current_epoch: Epoch, @@ -570,9 +570,11 @@ pub fn validator_set_update_tendermint( where S: StorageRead, { - tracing::debug!("Communicating validator set updates to Tendermint."); - // Because this is called 2 blocks before a start on an epoch, we're gonna - // give Tendermint updates for the next epoch + tracing::debug!( + "Communicating post-genesis validator set updates to CometBFT." + ); + // Because this is called 2 blocks before the start on an epoch, we will + // give CometBFT the updates for the next epoch let next_epoch = current_epoch.next(); let new_consensus_validator_handle = @@ -580,6 +582,7 @@ where let prev_consensus_validator_handle = consensus_validator_set_handle().at(¤t_epoch); + tracing::debug!("Iterating over new consensus validators:"); let new_consensus_validators = new_consensus_validator_handle .iter(storage)? .map(|validator| { @@ -596,100 +599,95 @@ where new_stake.to_string_native() ); + let prev_tm_voting_power = Lazy::new(|| { + let prev_validator_stake = read_validator_stake( + storage, + params, + &address, + current_epoch, + ) + .unwrap(); + into_tm_voting_power( + params.tm_votes_per_token, + prev_validator_stake, + ) + }); + let new_tm_voting_power = Lazy::new(|| { + into_tm_voting_power(params.tm_votes_per_token, new_stake) + }); + + // If both previous and current voting powers are 0, and the + // validator_stake_threshold is 0, skip update + if params.validator_stake_threshold.is_zero() + && *prev_tm_voting_power == 0 + && *new_tm_voting_power == 0 + { + tracing::debug!( + "Skipping CometBFT validator set update, {address} is in \ + consensus set but without voting power" + ); + return vec![]; + } + let new_consensus_key = validator_consensus_key_handle(&address) .get(storage, next_epoch, params) .unwrap() .unwrap(); - let old_consensus_key = validator_consensus_key_handle(&address) + let prev_consensus_key = validator_consensus_key_handle(&address) .get(storage, current_epoch, params) .unwrap(); - // Check if the validator was consensus in the previous epoch with - // the same stake. If so, no updated is needed. - // Look up previous state and prev and current voting powers - if !prev_consensus_validator_handle.is_empty(storage).unwrap() { - let prev_state = validator_state_handle(&address) - .get(storage, current_epoch, params) - .unwrap(); - let prev_tm_voting_power = Lazy::new(|| { - let prev_validator_stake = read_validator_stake( - storage, - params, - &address, - current_epoch, - ) - .unwrap(); - into_tm_voting_power( - params.tm_votes_per_token, - prev_validator_stake, - ) - }); - let new_tm_voting_power = Lazy::new(|| { - into_tm_voting_power(params.tm_votes_per_token, new_stake) - }); - - // If it was in `Consensus` before and voting power has not - // changed, skip the update - if matches!(prev_state, Some(ValidatorState::Consensus)) - && *prev_tm_voting_power == *new_tm_voting_power - { - if old_consensus_key.as_ref().unwrap() == &new_consensus_key - { - tracing::debug!( - "skipping validator update, {address} is in \ - consensus set but voting power hasn't changed" - ); - return vec![]; - } else { - return vec![ - ValidatorSetUpdate::Consensus(ConsensusValidator { - consensus_key: new_consensus_key, - bonded_stake: new_stake, - }), - ValidatorSetUpdate::Deactivated( - old_consensus_key.unwrap(), - ), - ]; - } - } - // If both previous and current voting powers are 0, and the - // validator_stake_threshold is 0, skip update - if params.validator_stake_threshold.is_zero() - && *prev_tm_voting_power == 0 - && *new_tm_voting_power == 0 - { - tracing::info!( - "skipping validator update, {address} is in consensus \ - set but without voting power" - ); - return vec![]; - } - } + let prev_state = validator_state_handle(&address) + .get(storage, current_epoch, params) + .unwrap(); tracing::debug!( - "{address} consensus key {}", + "{address} new consensus key {}", new_consensus_key.tm_raw_hash() ); - if old_consensus_key.as_ref() == Some(&new_consensus_key) - || old_consensus_key.is_none() - { - vec![ValidatorSetUpdate::Consensus(ConsensusValidator { - consensus_key: new_consensus_key, - bonded_stake: new_stake, - })] - } else { + // If the old state was not Consensus, then just a + // ConsensusValidator update is required + if !matches!(prev_state, Some(ValidatorState::Consensus)) { + return vec![ValidatorSetUpdate::Consensus( + ConsensusValidator { + consensus_key: new_consensus_key, + bonded_stake: *new_tm_voting_power, + }, + )]; + } + + // Now we've matched validators that were previously Consensus and + // remain so, which also implies that an old consensus key exists + + // If the consensus key has changed, then both ConsensusValidator + // and DeactivatedValidator updates are required + if prev_consensus_key.as_ref() != Some(&new_consensus_key) { vec![ ValidatorSetUpdate::Consensus(ConsensusValidator { consensus_key: new_consensus_key, - bonded_stake: new_stake, + bonded_stake: *new_tm_voting_power, }), - ValidatorSetUpdate::Deactivated(old_consensus_key.unwrap()), + ValidatorSetUpdate::Deactivated( + prev_consensus_key.unwrap(), + ), ] + } else if *prev_tm_voting_power == *new_tm_voting_power { + tracing::debug!( + "Skipping CometBFT validator set update; {address} \ + remains in consensus set but voting power hasn't changed" + ); + vec![] + } else { + vec![ValidatorSetUpdate::Consensus(ConsensusValidator { + consensus_key: new_consensus_key, + bonded_stake: *new_tm_voting_power, + })] } }); + tracing::debug!("Iterating over previous consensus validators:"); let prev_consensus_validators = prev_consensus_validator_handle .iter(storage)? .map(|validator| { @@ -719,7 +717,7 @@ where ) }); - let old_consensus_key = validator_consensus_key_handle(&address) + let prev_consensus_key = validator_consensus_key_handle(&address) .get(storage, current_epoch, params) .unwrap() .unwrap(); @@ -734,24 +732,25 @@ where // If the new state is not Consensus but its prev voting power // was 0 and the stake threshold is 0, we can also skip the // update - tracing::info!( - "skipping validator update, {address} is in consensus set \ - but without voting power" + tracing::debug!( + "Skipping CometBFT validator set update, {address} is in \ + not in consensus set anymore, but it previously had no \ + voting power" ); return vec![]; } // The remaining validators were previously Consensus but no longer // are, so they must be deactivated - let consensus_key = validator_consensus_key_handle(&address) + let new_consensus_key = validator_consensus_key_handle(&address) .get(storage, next_epoch, params) .unwrap() .unwrap(); tracing::debug!( - "{address} consensus key {}", - consensus_key.tm_raw_hash() + "{address} new consensus key {}", + new_consensus_key.tm_raw_hash() ); - vec![ValidatorSetUpdate::Deactivated(old_consensus_key)] + vec![ValidatorSetUpdate::Deactivated(prev_consensus_key)] }); Ok(new_consensus_validators