Merge remote-tracking branch 'namada/brent/fix-unjail-bug' (#1793) in…

…to main

* namada/brent/fix-unjail-bug:
  [ci] wasm checksums update
  changelog: add #1793
  pos/tests: unit test for unjailing
  pos/lib: fix bug in error-catching

.changelog/unreleased/bug-fixes/1793-fix-unjail-bug.md

@@ -0,0 +1,4 @@
+- Fixes buggy error handling in pos unjail_validator. Now properly enforces that
+  if an unjail tx is submitted when the validator state is something other than
+  Jailed in any of the current or future epochs, the tx will error out and fail.
+  ([\#1793](https://github.com/anoma/namada/pull/1793))

proof_of_stake/src/lib.rs

@@ -3927,11 +3927,8 @@ where
 
     // Check that the validator is jailed up to the pipeline epoch
     for epoch in current_epoch.iter_range(params.pipeline_len + 1) {
-        let state = validator_state_handle(validator).get(
-            storage,
-            current_epoch,
-            &params,
-        )?;
+        let state =
+            validator_state_handle(validator).get(storage, epoch, &params)?;
         if let Some(state) = state {
             if state != ValidatorState::Jailed {
                 return Err(UnjailValidatorError::NotJailed(

proof_of_stake/src/tests.rs

@@ -51,9 +51,10 @@ use crate::{
     staking_token_address, store_total_consensus_stake, total_deltas_handle,
     unbond_handle, unbond_tokens, unjail_validator, update_validator_deltas,
     update_validator_set, validator_consensus_key_handle,
-    validator_set_update_tendermint, validator_slashes_handle,
-    validator_state_handle, withdraw_tokens, write_validator_address_raw_hash,
-    BecomeValidator, STORE_VALIDATOR_SETS_LEN,
+    validator_set_positions_handle, validator_set_update_tendermint,
+    validator_slashes_handle, validator_state_handle, withdraw_tokens,
+    write_validator_address_raw_hash, BecomeValidator,
+    STORE_VALIDATOR_SETS_LEN,
 };
 
 proptest! {
@@ -124,6 +125,22 @@ proptest! {
     }
 }
 
+proptest! {
+    // Generate arb valid input for `test_unjail_validator_aux`
+    #![proptest_config(Config {
+        cases: 5,
+        .. Config::default()
+    })]
+    #[test]
+    fn test_unjail_validator(
+        (pos_params, genesis_validators)
+            in arb_params_and_genesis_validators(Some(4),6..9)
+    ) {
+        test_unjail_validator_aux(pos_params,
+            genesis_validators)
+    }
+}
+
 fn arb_params_and_genesis_validators(
     num_max_validator_slots: Option<u64>,
     val_size: Range<usize>,
@@ -2112,3 +2129,127 @@ fn arb_genesis_validators(
             },
         )
 }
+
+fn test_unjail_validator_aux(
+    params: PosParams,
+    mut validators: Vec<GenesisValidator>,
+) {
+    println!("\nTest inputs: {params:?}, genesis validators: {validators:#?}");
+    let mut s = TestWlStorage::default();
+
+    // Find the validator with the most stake and 100x his stake to keep the
+    // cubic slash rate small
+    let num_vals = validators.len();
+    validators.sort_by_key(|a| a.tokens);
+    validators[num_vals - 1].tokens = 100 * validators[num_vals - 1].tokens;
+
+    // Get second highest stake validator tomisbehave
+    let val_addr = &validators[num_vals - 2].address;
+    let val_tokens = validators[num_vals - 2].tokens;
+    println!(
+        "Validator that will misbehave addr {val_addr}, tokens {}",
+        val_tokens.to_string_native()
+    );
+
+    // Genesis
+    let mut current_epoch = s.storage.block.epoch;
+    init_genesis(
+        &mut s,
+        &params,
+        validators.clone().into_iter(),
+        current_epoch,
+    )
+    .unwrap();
+    s.commit_block().unwrap();
+
+    current_epoch = advance_epoch(&mut s, &params);
+    super::process_slashes(&mut s, current_epoch).unwrap();
+
+    // Discover first slash
+    let slash_0_evidence_epoch = current_epoch;
+    let evidence_block_height = BlockHeight(0); // doesn't matter for slashing logic
+    let slash_0_type = SlashType::DuplicateVote;
+    slash(
+        &mut s,
+        &params,
+        current_epoch,
+        slash_0_evidence_epoch,
+        evidence_block_height,
+        slash_0_type,
+        val_addr,
+        current_epoch.next(),
+    )
+    .unwrap();
+
+    assert_eq!(
+        validator_state_handle(val_addr)
+            .get(&s, current_epoch, &params)
+            .unwrap(),
+        Some(ValidatorState::Consensus)
+    );
+
+    for epoch in Epoch::iter_bounds_inclusive(
+        current_epoch.next(),
+        current_epoch + params.pipeline_len,
+    ) {
+        // Check the validator state
+        assert_eq!(
+            validator_state_handle(val_addr)
+                .get(&s, epoch, &params)
+                .unwrap(),
+            Some(ValidatorState::Jailed)
+        );
+        // Check the validator set positions
+        assert!(
+            validator_set_positions_handle()
+                .at(&epoch)
+                .get(&s, val_addr)
+                .unwrap()
+                .is_none(),
+        );
+    }
+
+    // Advance past an epoch in which we can unbond
+    let unfreeze_epoch =
+        slash_0_evidence_epoch + params.slash_processing_epoch_offset();
+    while current_epoch < unfreeze_epoch + 4u64 {
+        current_epoch = advance_epoch(&mut s, &params);
+        super::process_slashes(&mut s, current_epoch).unwrap();
+    }
+
+    // Unjail the validator
+    unjail_validator(&mut s, val_addr, current_epoch).unwrap();
+
+    // Check the validator state
+    for epoch in
+        Epoch::iter_bounds_inclusive(current_epoch, current_epoch.next())
+    {
+        assert_eq!(
+            validator_state_handle(val_addr)
+                .get(&s, epoch, &params)
+                .unwrap(),
+            Some(ValidatorState::Jailed)
+        );
+    }
+
+    assert_eq!(
+        validator_state_handle(val_addr)
+            .get(&s, current_epoch + params.pipeline_len, &params)
+            .unwrap(),
+        Some(ValidatorState::Consensus)
+    );
+    assert!(
+        validator_set_positions_handle()
+            .at(&(current_epoch + params.pipeline_len))
+            .get(&s, val_addr)
+            .unwrap()
+            .is_some(),
+    );
+
+    // Advance another epoch
+    current_epoch = advance_epoch(&mut s, &params);
+    super::process_slashes(&mut s, current_epoch).unwrap();
+
+    let second_att = unjail_validator(&mut s, val_addr, current_epoch);
+    assert!(second_att.is_err());
+}

wasm/checksums.json

@@ -9,7 +9,7 @@
     "tx_reveal_pk.wasm": "tx_reveal_pk.e6375abba3f750700c06fbdeb2d5edec22b6a382116a67a7f9d7f7ba49f134e1.wasm",
     "tx_transfer.wasm": "tx_transfer.e65b47bc94c5a09e3edc68298ad0e5e35041b91bc4d679bf5b7f433dffdce58e.wasm",
     "tx_unbond.wasm": "tx_unbond.4fd70d297ccedb369bf88d8a8459a47ea733d329410387a6f80a0e026c6e480d.wasm",
-    "tx_unjail_validator.wasm": "tx_unjail_validator.b8e7204b14e15a3c395432f35788eca45ef4332916d96dfba87627a5717916de.wasm",
+    "tx_unjail_validator.wasm": "tx_unjail_validator.28082f1002a97f06b52bc7a9267d1e5675a5241c5d37f7d0c58257f9fa2cc9b2.wasm",
     "tx_update_vp.wasm": "tx_update_vp.65c5ca3e48fdef70e696460eca7540cf6196511d76fb2465133b145409329b3e.wasm",
     "tx_vote_proposal.wasm": "tx_vote_proposal.e0a003d922230d32b741b57ca18913cbc4d5d2290f02cb37dfdaa7f27cebb486.wasm",
     "tx_withdraw.wasm": "tx_withdraw.40499cb0e268d3cc3d9bca5dacca05d8df35f5b90adf4087a5171505472d921a.wasm",