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 40e808d02f..6c069e7309 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -114,6 +114,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(); @@ -526,42 +534,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); @@ -847,6 +819,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 @@ -2400,8 +2414,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(), @@ -2940,10 +2957,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 e8f2cf45d7..4c087dbbd6 100644 --- a/proof_of_stake/src/lib.rs +++ b/proof_of_stake/src/lib.rs @@ -101,6 +101,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)] @@ -3281,7 +3283,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 = @@ -3580,8 +3586,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 e333949316..9bcd3f098f 100644 --- a/tests/src/e2e/ledger_tests.rs +++ b/tests/src/e2e/ledger_tests.rs @@ -4309,6 +4309,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; @@ -4330,7 +4331,11 @@ fn double_signing_gets_slashed() -> Result<()> { genesis.pos_params.unbonding_len, genesis.pos_params.cubic_slashing_window_length, ); - setup::set_validators(4, genesis, default_port_offset) + let mut genesis = + setup::set_validators(4, genesis, default_port_offset); + // Make faster epochs to be more likely to discover boundary issues + genesis.parameters.min_num_of_blocks = 2; + genesis }, None, )?; @@ -4492,14 +4497,13 @@ fn double_signing_gets_slashed() -> Result<()> { // 6. Wait for double signing evidence let mut validator_1 = bg_validator_1.foreground(); validator_1.exp_string("Processing evidence")?; - // validator_1.exp_string("Slashing")?; println!("\nPARSING SLASH MESSAGE\n"); let (_, res) = validator_1 .exp_regex(r"Slashing [a-z0-9]+ for Duplicate vote in epoch [0-9]+") .unwrap(); println!("\n{res}\n"); - let _bg_validator_1 = validator_1.background(); + let bg_validator_1 = validator_1.background(); let exp_processing_epoch = Epoch::from_str(res.split(' ').last().unwrap()) .unwrap() @@ -4605,6 +4609,17 @@ fn double_signing_gets_slashed() -> Result<()> { .exp_regex(r"Validator [a-z0-9]+ is in the .* set") .unwrap(); + // 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(()) }