Skip to content

Commit

Permalink
stake: fix missing index and simplify state machine (#3928)
Browse files Browse the repository at this point in the history
  • Loading branch information
erwanor authored Mar 10, 2024
1 parent c3419c2 commit dbba4fa
Show file tree
Hide file tree
Showing 4 changed files with 124 additions and 182 deletions.
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use anyhow::{Context, Result};
use anyhow::{ensure, Context, Result};
use async_trait::async_trait;
use cnidarium::StateWrite;
use penumbra_sct::component::clock::EpochRead;
Expand Down Expand Up @@ -73,36 +73,39 @@ impl ActionHandler for validator::Definition {
{
// Ensure that the highest existing sequence number is less than
// the new sequence number.
let current_seq = existing_v.sequence_number;
if v.validator.sequence_number <= current_seq {
anyhow::bail!(
"expected sequence numbers to be increasing: current sequence number is {}",
current_seq
);
}
// Ensure that the sequence number keeps increasing.
let old_seq = existing_v.sequence_number;
let new_seq = v.validator.sequence_number;
ensure!(
new_seq > old_seq,
"definition sequence number must increase (given {}, but previous definition sequence number is {})",
new_seq,
old_seq,
);
}

// Check whether the consensus key has already been used by another validator.
if let Some(existing_v) = state
// Check if the consensus key is known, and if so, that it is by the
// validator that declares it in this definition.
if let Some(ck_owner) = state
.get_validator_by_consensus_key(&v.validator.consensus_key)
.await?
{
if v.validator.identity_key != existing_v.identity_key {
// This is a new validator definition, but the consensus key it declares
// is used by another validator. We MUST reject this definition:
//
// 1. It prevents someone from declaring an (app-level) validator that
// "piggybacks" on the actual behavior of someone else's validator.
//
// 2. If we submit a validator update to Tendermint that
// includes duplicate consensus keys, Tendermint gets confused
// and hangs.
anyhow::bail!(
"consensus key {:?} is already in use by validator {}",
v.validator.consensus_key,
existing_v.identity_key,
);
}
// If we detect that the new definition tries to squat someone else's
// consensus key, we MUST reject this definition:
//
// 1. It prevents someone from declaring an (app-level) validator that
// "piggybacks" on the actual behavior of someone else's validator.
//
// 2. If we submit a validator update to CometBFT that
// includes duplicate consensus keys, CometBFT gets confused
// and hangs.
// Note: This is currently vulnerable to a TOCTOU hazard. Tracked in #3858.
ensure!(
ck_owner.identity_key == v.validator.identity_key,
"consensus key {:?} is already in use by validator {}",
v.validator.consensus_key,
ck_owner.identity_key,
);
}

// (end of former check_historical impl)
Expand Down
1 change: 1 addition & 0 deletions crates/core/component/stake/src/component/epoch_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ pub trait EpochHandler: StateWriteExt + ConsensusIndexRead {
Ok(reward_queue_entry)
}

/// Compute and return the chain base rate ("L1BOR").
async fn process_chain_base_rate(&mut self) -> Result<BaseRateData> {
// We are transitioning to the next epoch, so the "current" base rate in
// the state is now the previous base rate.
Expand Down
4 changes: 3 additions & 1 deletion crates/core/component/stake/src/component/stake.rs
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,9 @@ pub trait ConsensusIndexRead: StateRead {
.boxed())
}

/// Returns whether the given validator should be indexed in the consensus set.
/// Returns whether a validator should be indexed in the consensus set.
/// Here, "consensus set" refers to the set of active validators as well as
/// the "inactive" validators which could be promoted during a view change.
#[instrument(level = "error", skip(self))]
async fn belongs_in_index(&self, validator_id: &IdentityKey) -> bool {
let Some(state) = self
Expand Down
Loading

0 comments on commit dbba4fa

Please sign in to comment.