PoS: fix rewards boundary

.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))

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);

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<ValidatorState>),
 }
 
 #[allow(missing_docs)]
@@ -3136,7 +3138,11 @@ where
         let state = validator_state_handle(&validator_address)
             .get(storage, epoch, &params)?;
         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,

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(())
 }