diff --git a/.changelog/unreleased/bug-fixes/1793-fix-unjail-bug.md b/.changelog/unreleased/bug-fixes/1793-fix-unjail-bug.md new file mode 100644 index 0000000000..11a2b1e545 --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index 4c087dbbd6..d84c538571 100644 --- a/proof_of_stake/src/lib.rs +++ b/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, - ¶ms, - )?; + let state = + validator_state_handle(validator).get(storage, epoch, ¶ms)?; if let Some(state) = state { if state != ValidatorState::Jailed { return Err(UnjailValidatorError::NotJailed( diff --git a/proof_of_stake/src/tests.rs b/proof_of_stake/src/tests.rs index 6476827417..b7463c8ea5 100644 --- a/proof_of_stake/src/tests.rs +++ b/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, val_size: Range, @@ -2112,3 +2129,127 @@ fn arb_genesis_validators( }, ) } + +fn test_unjail_validator_aux( + params: PosParams, + mut validators: Vec, +) { + 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, + ¶ms, + validators.clone().into_iter(), + current_epoch, + ) + .unwrap(); + s.commit_block().unwrap(); + + current_epoch = advance_epoch(&mut s, ¶ms); + 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, + ¶ms, + 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, ¶ms) + .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, ¶ms) + .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, ¶ms); + 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, ¶ms) + .unwrap(), + Some(ValidatorState::Jailed) + ); + } + + assert_eq!( + validator_state_handle(val_addr) + .get(&s, current_epoch + params.pipeline_len, ¶ms) + .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, ¶ms); + super::process_slashes(&mut s, current_epoch).unwrap(); + + let second_att = unjail_validator(&mut s, val_addr, current_epoch); + assert!(second_att.is_err()); +} diff --git a/wasm/checksums.json b/wasm/checksums.json index cb9ca8ed52..29b6d2ea6f 100644 --- a/wasm/checksums.json +++ b/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",