From 5af1f94efa6ba76c7db03eff1cec8f73ffb74c85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Mon, 17 Jul 2023 12:29:06 +0100 Subject: [PATCH 1/6] pos: error out if validator is not in expected state for rewards --- proof_of_stake/src/lib.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index 7f7265be91..72e03f8278 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 = From ca02d265059f74df6b93060c7e67769098c642d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Mon, 17 Jul 2023 12:34:53 +0100 Subject: [PATCH 2/6] test/e2e/slashing: extend the test to discover rewards issues --- tests/src/e2e/ledger_tests.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) 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(()) } From 2afb27c925cd9a7b27129602cbae36ce3e9d236f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Mon, 17 Jul 2023 12:44:46 +0100 Subject: [PATCH 3/6] app/ledger/finalize_block: log block rewards before recording slashes --- .../lib/node/ledger/shell/finalize_block.rs | 86 +++++++++++-------- 1 file changed, 50 insertions(+), 36 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 3af8ab9e3f..ca0dbfbb2f 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 From ed57e33ce6e8c9bd70736335ab61ef1372a8caec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Tue, 18 Jul 2023 09:39:39 +0100 Subject: [PATCH 4/6] pos/slash: fix the validator state update to be in sync with set changes --- proof_of_stake/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index 72e03f8278..1699f6dee4 100644 --- a/proof_of_stake/src/lib.rs +++ b/proof_of_stake/src/lib.rs @@ -3444,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, From 0c16a199e529f5248e090b6265d794805c85126f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Tue, 18 Jul 2023 11:26:00 +0100 Subject: [PATCH 5/6] test/ledger/finalize_block: fix slashing tests --- apps/src/lib/node/ledger/shell/finalize_block.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index ca0dbfbb2f..137d4038d6 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -2420,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(), @@ -2960,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); From 4e5293625ae40c5d86a6f84de634e6b3b7d2e09d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Tue, 18 Jul 2023 11:34:02 +0100 Subject: [PATCH 6/6] changelog: add #1729 --- .../unreleased/bug-fixes/1729-pos-fix-rewards-boundary.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/1729-pos-fix-rewards-boundary.md 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