diff --git a/.changelog/unreleased/bug-fixes/1729-pos-fix-rewards-boundary.md b/.changelog/unreleased/bug-fixes/1729-pos-fix-rewards-boundary.md new file mode 100644 index 0000000000..3b6fcaa903 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1729-pos-fix-rewards-boundary.md @@ -0,0 +1,3 @@ +- PoS: Fixed an epoch boundary issue in which a validator who's being slashed + on a start of a new epoch is disregarded during processing of block votes. + ([\#1729](https://github.com/anoma/namada/pull/1729)) \ No newline at end of file diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 3af8ab9e3f..137d4038d6 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -110,6 +110,14 @@ where )?; } + // Invariant: Has to be applied before `record_slashes_from_evidence` + // because it potentially needs to be able to read validator state from + // previous epoch and jailing validator removes the historical state + self.log_block_rewards(&req.votes, height, current_epoch, new_epoch)?; + if new_epoch { + self.apply_inflation(current_epoch)?; + } + // Invariant: This has to be applied after // `copy_validator_sets_and_positions` and before `self.update_epoch`. self.record_slashes_from_evidence(); @@ -530,42 +538,6 @@ where self.update_eth_oracle(); } - // Read the block proposer of the previously committed block in storage - // (n-1 if we are in the process of finalizing n right now). - match read_last_block_proposer_address(&self.wl_storage)? { - Some(proposer_address) => { - tracing::debug!( - "Found last block proposer: {proposer_address}" - ); - let votes = pos_votes_from_abci(&self.wl_storage, &req.votes); - namada_proof_of_stake::log_block_rewards( - &mut self.wl_storage, - if new_epoch { - current_epoch.prev() - } else { - current_epoch - }, - &proposer_address, - votes, - )?; - } - None => { - if height > BlockHeight::default().next_height() { - tracing::error!( - "Can't find the last block proposer at height {height}" - ); - } else { - tracing::debug!( - "No last block proposer at height {height}" - ); - } - } - } - - if new_epoch { - self.apply_inflation(current_epoch)?; - } - if !req.proposer_address.is_empty() { let tm_raw_hash_string = tm_raw_hash_to_string(req.proposer_address); @@ -887,6 +859,48 @@ where Ok(()) } + + // Process the proposer and votes in the block to assign their PoS rewards. + fn log_block_rewards( + &mut self, + votes: &[VoteInfo], + height: BlockHeight, + current_epoch: Epoch, + new_epoch: bool, + ) -> Result<()> { + // Read the block proposer of the previously committed block in storage + // (n-1 if we are in the process of finalizing n right now). + match read_last_block_proposer_address(&self.wl_storage)? { + Some(proposer_address) => { + tracing::debug!( + "Found last block proposer: {proposer_address}" + ); + let votes = pos_votes_from_abci(&self.wl_storage, votes); + namada_proof_of_stake::log_block_rewards( + &mut self.wl_storage, + if new_epoch { + current_epoch.prev() + } else { + current_epoch + }, + &proposer_address, + votes, + )?; + } + None => { + if height > BlockHeight::default().next_height() { + tracing::error!( + "Can't find the last block proposer at height {height}" + ); + } else { + tracing::debug!( + "No last block proposer at height {height}" + ); + } + } + } + Ok(()) + } } /// Convert ABCI vote info to PoS vote info. Any info which fails the conversion @@ -2406,8 +2420,11 @@ mod test_finalize_block { ); // Advance to the processing epoch - let votes = get_default_true_votes(&shell.wl_storage, Epoch::default()); loop { + let votes = get_default_true_votes( + &shell.wl_storage, + shell.wl_storage.storage.block.epoch, + ); next_block_for_inflation( &mut shell, pkh1.clone(), @@ -2946,10 +2963,14 @@ mod test_finalize_block { total_voting_power: Default::default(), }, ]; + let votes = get_default_true_votes( + &shell.wl_storage, + shell.wl_storage.storage.block.epoch, + ); next_block_for_inflation( &mut shell, pkh1.clone(), - votes.clone(), + votes, Some(misbehaviors), ); assert_eq!(current_epoch.0, 7_u64); diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index 7f7265be91..1699f6dee4 100644 --- a/proof_of_stake/src/lib.rs +++ b/proof_of_stake/src/lib.rs @@ -96,6 +96,8 @@ pub enum GenesisError { pub enum InflationError { #[error("Error in calculating rewards: {0}")] Rewards(rewards::RewardsError), + #[error("Expected validator {0} to be in consensus set but got: {1:?}")] + ExpectedValidatorInConsensus(Address, Option), } #[allow(missing_docs)] @@ -3136,7 +3138,11 @@ where let state = validator_state_handle(&validator_address) .get(storage, epoch, ¶ms)?; if state != Some(ValidatorState::Consensus) { - continue; + return Err(InflationError::ExpectedValidatorInConsensus( + validator_address, + state, + )) + .into_storage_result(); } let stake_from_deltas = @@ -3438,8 +3444,10 @@ where } } } + // Safe sub cause `validator_set_update_epoch > current_epoch` + let start_offset = validator_set_update_epoch.0 - current_epoch.0; // Set the validator state as `Jailed` thru the pipeline epoch - for offset in 1..=params.pipeline_len { + for offset in start_offset..=params.pipeline_len { validator_state_handle(validator).set( storage, ValidatorState::Jailed, diff --git a/tests/src/e2e/ledger_tests.rs b/tests/src/e2e/ledger_tests.rs index 9b579bef92..e08fd174c2 100644 --- a/tests/src/e2e/ledger_tests.rs +++ b/tests/src/e2e/ledger_tests.rs @@ -4308,6 +4308,7 @@ fn test_genesis_validators() -> Result<()> { /// 4. Run it to get it to double vote and sign blocks /// 5. Submit a valid token transfer tx to validator 0 /// 6. Wait for double signing evidence +/// 7. Make sure the the first validator can proceed to the next epoch #[test] fn double_signing_gets_slashed() -> Result<()> { use std::net::SocketAddr; @@ -4319,7 +4320,13 @@ fn double_signing_gets_slashed() -> Result<()> { // Setup 2 genesis validator nodes let test = setup::network( - |genesis| setup::set_validators(2, genesis, default_port_offset), + |genesis| { + let mut genesis = + setup::set_validators(2, genesis, default_port_offset); + // Make faster epochs to be more likely to discover boundary issues + genesis.parameters.min_num_of_blocks = 2; + genesis + }, None, )?; @@ -4468,6 +4475,18 @@ fn double_signing_gets_slashed() -> Result<()> { let mut validator_1 = bg_validator_1.foreground(); validator_1.exp_string("Processing evidence")?; validator_1.exp_string("Slashing")?; + let bg_validator_1 = validator_1.background(); + + // 7. Make sure the the first validator can proceed to the next epoch + epoch_sleep(&test, &validator_one_rpc, 120)?; + + // Make sure there are no errors + let mut validator_1 = bg_validator_1.foreground(); + validator_1.interrupt()?; + // Wait for the node to stop running to finish writing the state and tx + // queue + validator_1.exp_string("Namada ledger node has shut down.")?; + validator_1.assert_success(); Ok(()) }