diff --git a/.github/workflows/scripts/e2e.json b/.github/workflows/scripts/e2e.json index f3792dedbd7..909fa32c367 100644 --- a/.github/workflows/scripts/e2e.json +++ b/.github/workflows/scripts/e2e.json @@ -20,6 +20,7 @@ "e2e::ledger_tests::test_node_connectivity_and_consensus": 28, "e2e::ledger_tests::test_epoch_sleep": 12, "e2e::ledger_tests::wrapper_disposable_signer": 28, + "e2e::ledger_tests::pos_rewards": 44, "e2e::wallet_tests::wallet_address_cmds": 1, "e2e::wallet_tests::wallet_encrypted_key_cmds": 1, "e2e::wallet_tests::wallet_encrypted_key_cmds_env_var": 1, diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 9a9283e6231..559531269c3 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -952,6 +952,7 @@ mod test_finalize_block { read_validator_stake, rewards_accumulator_handle, unjail_validator, validator_consensus_key_handle, validator_rewards_products_handle, validator_slashes_handle, validator_state_handle, write_pos_params, + ADDRESS as pos_address, }; use namada::proto::{Code, Data, Section, Signature}; use namada::types::dec::POS_DECIMAL_PRECISION; @@ -2108,6 +2109,187 @@ mod test_finalize_block { assert!(rp3 > rp4); } + /// A unit test for PoS inflationary rewards claiming + #[test] + fn test_claim_rewards() { + let (mut shell, _recv, _, _) = setup_with_cfg(SetupCfg { + last_height: 0, + num_validators: 1, + ..Default::default() + }); + + let mut validator_set: BTreeSet = + read_consensus_validator_set_addresses_with_stake( + &shell.wl_storage, + Epoch::default(), + ) + .unwrap() + .into_iter() + .collect(); + + let params = read_pos_params(&shell.wl_storage).unwrap(); + + let validator = validator_set.pop_first().unwrap(); + + let get_pkh = |address, epoch| { + let ck = validator_consensus_key_handle(&address) + .get(&shell.wl_storage, epoch, ¶ms) + .unwrap() + .unwrap(); + let hash_string = tm_consensus_key_raw_hash(&ck); + HEXUPPER.decode(hash_string.as_bytes()).unwrap() + }; + + let pkh1 = get_pkh(validator.address.clone(), Epoch::default()); + let votes = vec![VoteInfo { + validator: Some(Validator { + address: pkh1.clone(), + power: u128::try_from(validator.bonded_stake) + .expect("Test failed") as i64, + }), + signed_last_block: true, + }]; + // let rewards_prod_1 = + // validator_rewards_products_handle(&val1.address); + + let is_reward_equal_enough = |expected: token::Amount, + actual: token::Amount, + tolerance: u64| + -> bool { + let diff = expected - actual; + diff <= tolerance.into() + }; + + let bond_id = BondId { + source: validator.address.clone(), + validator: validator.address.clone(), + }; + + let mut total_rewards = token::Amount::zero(); + + // FINALIZE BLOCK 1. Tell Namada that val1 is the block proposer. We + // won't receive votes from TM since we receive votes at a 1-block + // delay, so votes will be empty here + next_block_for_inflation(&mut shell, pkh1.clone(), vec![], None); + assert!( + rewards_accumulator_handle() + .is_empty(&shell.wl_storage) + .unwrap() + ); + + let (current_epoch, inflation) = + advance_epoch(&mut shell, &pkh1, &votes, None); + total_rewards += inflation; + + println!("\nMINTED INFLATION: {}", inflation.to_string_native()); + + // Claim the rewards from the initial epoch + let reward_1 = namada_proof_of_stake::claim_reward_tokens( + &mut shell.wl_storage, + None, + &validator.address, + current_epoch, + ) + .unwrap(); + assert!(is_reward_equal_enough(total_rewards, reward_1, 1)); + + // Try a claim the next block and ensure we get 0 tokens back + next_block_for_inflation(&mut shell, pkh1.clone(), votes.clone(), None); + let att = namada_proof_of_stake::claim_reward_tokens( + &mut shell.wl_storage, + None, + &validator.address, + current_epoch, + ) + .unwrap(); + assert_eq!(att, token::Amount::zero()); + + // Go to the next epoch + let (current_epoch, inflation) = + advance_epoch(&mut shell, &pkh1, &votes, None); + total_rewards += inflation; + + // Unbond some tokens + let unbond_amount = token::Amount::native_whole(50_000); + let unbond_res = namada_proof_of_stake::unbond_tokens( + &mut shell.wl_storage, + None, + &validator.address, + unbond_amount, + current_epoch, + false, + ) + .unwrap(); + assert_eq!(unbond_res.sum, unbond_amount); + + // Check the bond amounts for rewards up thru the withdrawable epoch + let withdraw_epoch = current_epoch + params.withdrawable_epoch_offset(); + let last_claim_epoch = + namada_proof_of_stake::get_last_reward_claim_epoch( + &shell.wl_storage, + &validator.address, + &validator.address, + ) + .unwrap(); + let bond_amounts = namada_proof_of_stake::bond_amounts_for_rewards( + &shell.wl_storage, + &bond_id, + last_claim_epoch.unwrap_or_default(), + withdraw_epoch, + ) + .unwrap(); + + let mut exp_bond_amounts = BTreeMap::::new(); + for epoch in Epoch::iter_bounds_inclusive( + last_claim_epoch.unwrap_or_default(), + withdraw_epoch, + ) { + if epoch < current_epoch + params.pipeline_len { + exp_bond_amounts.insert(epoch, validator.bonded_stake); + } else { + exp_bond_amounts + .insert(epoch, validator.bonded_stake - unbond_amount); + } + } + assert_eq!(exp_bond_amounts, bond_amounts); + + // Advance to the withdrawable epoch + let mut current_epoch = current_epoch; + while current_epoch < withdraw_epoch { + let votes = get_default_true_votes( + &shell.wl_storage, + shell.wl_storage.storage.block.epoch, + ); + let (new_epoch, inflation) = + advance_epoch(&mut shell, &pkh1, &votes, None); + current_epoch = new_epoch; + total_rewards += inflation; + } + + // Withdraw tokens + let withdraw_amount = namada_proof_of_stake::withdraw_tokens( + &mut shell.wl_storage, + None, + &validator.address, + current_epoch, + ) + .unwrap(); + assert_eq!(withdraw_amount, unbond_amount); + + // Claim tokens + let reward_2 = namada_proof_of_stake::claim_reward_tokens( + &mut shell.wl_storage, + None, + &validator.address, + current_epoch, + ) + .unwrap(); + + // Hacky solution until I come up with smth better + let diff = total_rewards - reward_2 - reward_1; + assert!(diff < 2 * token::Amount::from(current_epoch.0)); + } + fn get_rewards_acc(storage: &S) -> HashMap where S: StorageRead, @@ -2972,7 +3154,7 @@ mod test_finalize_block { // Advance to epoch 1 and // 1. Delegate 67231 NAM to validator // 2. Validator self-unbond 154654 NAM - let current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + let (current_epoch, _) = advance_epoch(&mut shell, &pkh1, &votes, None); assert_eq!(shell.wl_storage.storage.block.epoch.0, 1_u64); // Make an account with balance and delegate some tokens @@ -3038,7 +3220,7 @@ mod test_finalize_block { &shell.wl_storage, shell.wl_storage.storage.block.epoch, ); - let current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + let (current_epoch, _) = advance_epoch(&mut shell, &pkh1, &votes, None); println!("\nUnbonding in epoch 2"); let del_unbond_1_amount = token::Amount::native_whole(18_000); namada_proof_of_stake::unbond_tokens( @@ -3083,7 +3265,7 @@ mod test_finalize_block { &shell.wl_storage, shell.wl_storage.storage.block.epoch, ); - let current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + let (current_epoch, _) = advance_epoch(&mut shell, &pkh1, &votes, None); println!("\nBonding in epoch 3"); let self_bond_1_amount = token::Amount::native_whole(9_123); @@ -3103,7 +3285,7 @@ mod test_finalize_block { &shell.wl_storage, shell.wl_storage.storage.block.epoch, ); - let current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + let (current_epoch, _) = advance_epoch(&mut shell, &pkh1, &votes, None); assert_eq!(current_epoch.0, 4_u64); let self_unbond_2_amount = token::Amount::native_whole(15_000); @@ -3123,7 +3305,7 @@ mod test_finalize_block { &shell.wl_storage, shell.wl_storage.storage.block.epoch, ); - let current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + let (current_epoch, _) = advance_epoch(&mut shell, &pkh1, &votes, None); assert_eq!(current_epoch.0, 5_u64); println!("Delegating in epoch 5"); @@ -3146,7 +3328,7 @@ mod test_finalize_block { &shell.wl_storage, shell.wl_storage.storage.block.epoch, ); - let current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + let (current_epoch, _) = advance_epoch(&mut shell, &pkh1, &votes, None); assert_eq!(current_epoch.0, 6_u64); // Discover a misbehavior committed in epoch 3 @@ -3211,7 +3393,7 @@ mod test_finalize_block { println!("Advancing to epoch 7"); // Advance to epoch 7 - let current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + let (current_epoch, _) = advance_epoch(&mut shell, &pkh1, &votes, None); // Discover two more misbehaviors, one committed in epoch 3, one in // epoch 4 @@ -3315,7 +3497,7 @@ mod test_finalize_block { &shell.wl_storage, shell.wl_storage.storage.block.epoch, ); - let current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + let (current_epoch, _) = advance_epoch(&mut shell, &pkh1, &votes, None); assert_eq!(current_epoch.0, 9_u64); let val_stake_3 = namada_proof_of_stake::read_validator_stake( @@ -3443,7 +3625,7 @@ mod test_finalize_block { &shell.wl_storage, shell.wl_storage.storage.block.epoch, ); - let current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + let (current_epoch, _) = advance_epoch(&mut shell, &pkh1, &votes, None); assert_eq!(current_epoch.0, 10_u64); // Check the balance of the Slash Pool @@ -3806,7 +3988,7 @@ mod test_finalize_block { &shell.wl_storage, shell.wl_storage.storage.block.epoch, ); - current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None).0; } assert_eq!(shell.wl_storage.storage.block.epoch.0, default_past_epochs); assert_eq!(current_epoch.0, default_past_epochs); @@ -3823,7 +4005,7 @@ mod test_finalize_block { &shell.wl_storage, shell.wl_storage.storage.block.epoch, ); - current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None).0; assert_eq!(current_epoch.0, default_past_epochs + 1); check_is_data( @@ -3862,7 +4044,7 @@ mod test_finalize_block { &shell.wl_storage, shell.wl_storage.storage.block.epoch, ); - current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None).0; if current_epoch.0 == consensus_val_set_len + 1 { break; } @@ -3880,7 +4062,7 @@ mod test_finalize_block { &shell.wl_storage, shell.wl_storage.storage.block.epoch, ); - current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); + current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None).0; for ep in Epoch::default().iter_range(2) { assert!( consensus_val_set @@ -3935,8 +4117,21 @@ mod test_finalize_block { proposer_address: &[u8], consensus_votes: &[VoteInfo], misbehaviors: Option>, - ) -> Epoch { + ) -> (Epoch, token::Amount) { let current_epoch = shell.wl_storage.storage.block.epoch; + let staking_token = staking_token_address(&shell.wl_storage); + + // NOTE: assumed that the only change in pos address balance by + // advancing to the next epoch is minted inflation - no change occurs + // due to slashing + let pos_balance_pre = shell + .wl_storage + .read::(&token::balance_key( + &staking_token, + &pos_address, + )) + .unwrap() + .unwrap_or_default(); loop { next_block_for_inflation( shell, @@ -3948,7 +4143,19 @@ mod test_finalize_block { break; } } - shell.wl_storage.storage.block.epoch + let pos_balance_post = shell + .wl_storage + .read::(&token::balance_key( + &staking_token, + &pos_address, + )) + .unwrap() + .unwrap_or_default(); + + ( + shell.wl_storage.storage.block.epoch, + pos_balance_post - pos_balance_pre, + ) } /// Test that updating the ethereum bridge params via governance works. diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index 57500024fa9..334534571b9 100644 --- a/proof_of_stake/src/lib.rs +++ b/proof_of_stake/src/lib.rs @@ -1675,17 +1675,19 @@ pub fn unbond_tokens( where S: StorageRead + StorageWrite, { - tracing::debug!( - "Unbonding token amount {} at epoch {}", - amount.to_string_native(), - current_epoch - ); if amount.is_zero() { return Ok(ResultSlashing::default()); } let params = read_pos_params(storage)?; let pipeline_epoch = current_epoch + params.pipeline_len; + let withdrawable_epoch = current_epoch + params.withdrawable_epoch_offset(); + tracing::debug!( + "Unbonding token amount {} at epoch {}, withdrawable at epoch {}", + amount.to_string_native(), + current_epoch, + withdrawable_epoch + ); // Make sure source is not some other validator if let Some(source) = source { @@ -1727,7 +1729,6 @@ where } let unbonds = unbond_handle(source, validator); - let withdrawable_epoch = current_epoch + params.withdrawable_epoch_offset(); let redelegated_bonds = delegator_redelegated_bonds_handle(source).at(validator); @@ -2801,6 +2802,7 @@ where let unbond_handle: Unbonds = unbond_handle(source, validator); let redelegated_unbonds = delegator_redelegated_unbonds_handle(source).at(validator); + let rewards_products = validator_rewards_products_handle(validator); // Check that there are unbonded tokens available for withdrawal if unbond_handle.is_empty(storage)? { @@ -2811,6 +2813,11 @@ where .into()); } + // Last claimed epoch for rewards + let last_claim_epoch = + get_last_reward_claim_epoch(storage, source, validator)? + .unwrap_or_default(); + let mut unbonds_and_redelegated_unbonds: BTreeMap< (Epoch, Epoch), (token::Amount, EagerRedelegatedBondsMap), @@ -2831,6 +2838,7 @@ where "Unbond delta ({start_epoch}..{withdraw_epoch}), amount {}", amount.to_string_native() ); + // Consider only unbonds that are eligible to be withdrawn if withdraw_epoch > current_epoch { tracing::debug!( "Not yet withdrawable until epoch {withdraw_epoch}" @@ -2856,7 +2864,19 @@ where .or_insert(amount); } - rewards += amount; + // Tally rewards due to unbonds + // TODO: check this logic and the bounds (esp the end epoch)! + // NOTE: may have been an issue with trying to claim rewards for epochs + // after the unbond was initiated + let end = withdraw_epoch - params.withdrawable_epoch_offset() + + params.pipeline_len; + for ep in Epoch::iter_bounds_inclusive(start_epoch, end.prev()) { + if ep < last_claim_epoch { + continue; + } + let rp = rewards_products.get(storage, &ep)?.unwrap_or_default(); + rewards += rp * amount; + } unbonds_and_redelegated_unbonds.insert( (start_epoch, withdraw_epoch), @@ -3315,7 +3335,7 @@ where if start <= claim_start { // A bond that wasn't unbonded is added to all epochs up to // `claim_end` - for ep in Epoch::iter_bounds_inclusive(start, claim_end) { + for ep in Epoch::iter_bounds_inclusive(claim_start, claim_end) { let amount = amounts.entry(ep).or_default().entry(start).or_default(); *amount += delta; @@ -3333,11 +3353,13 @@ where }, delta, ) = next?; + // This is the first epoch in which the unbond stops contributing to + // voting power let end = withdrawable_epoch - params.withdrawable_epoch_offset() + params.pipeline_len; if start <= claim_start && end > claim_start { - for ep in Epoch::iter_bounds_inclusive(start, end) { + for ep in Epoch::iter_bounds_inclusive(claim_start, end.prev()) { let amount = amounts.entry(ep).or_default().entry(start).or_default(); *amount += delta; @@ -3370,7 +3392,8 @@ where && start <= claim_start && end > claim_start { - for ep in Epoch::iter_bounds_inclusive(start, end) { + for ep in Epoch::iter_bounds_inclusive(claim_start, end.prev()) + { let amount = amounts .entry(ep) .or_default() @@ -4395,14 +4418,17 @@ where )?; if reward_tokens_remaining > token::Amount::zero() { - let amount = - token::Amount::from_uint(reward_tokens_remaining, 0).unwrap(); tracing::info!( "Minting tokens remaining from PoS rewards distribution into the \ Governance account. Amount: {}.", - amount.to_string_native() + reward_tokens_remaining.to_string_native() ); - token::credit_tokens(storage, staking_token, &address::GOV, amount)?; + token::credit_tokens( + storage, + staking_token, + &address::GOV, + reward_tokens_remaining, + )?; } // Clear validator rewards accumulators @@ -5715,10 +5741,17 @@ pub fn claim_reward_tokens( where S: StorageRead + StorageWrite, { - let rewards_products = validator_rewards_products_handle(validator); + tracing::debug!("Claiming rewards in epoch {current_epoch}"); + let rewards_products = validator_rewards_products_handle(validator); let source = source.cloned().unwrap_or_else(|| validator.clone()); - // let params = read_pos_params(storage)?; + tracing::debug!("Source {} --> Validator {}", source, validator); + + if current_epoch == Epoch::default() { + // Nothing to claim in the first epoch + return Ok(token::Amount::zero()); + } + let last_claim_epoch = get_last_reward_claim_epoch(storage, &source, validator)?; if let Some(last_epoch) = last_claim_epoch { @@ -5728,19 +5761,10 @@ where } } - // let bonds = bond_handle(&source, validator); - - // TODO: decide if this way or the typical F1 method is better (tho F1 would - // require a bit fancier impl for not much gained prob) - // TODO: different approach to avoid the nested repeated loops over bonds - // for each epoch that we iterate let mut reward_tokens = token::Amount::zero(); + // Want to claim from `last_claim_epoch` to `current_epoch.prev()` since // rewards are computed at the end of an epoch - if current_epoch == Epoch::default() { - // Nothing to claim in the first epoch - return Ok(token::Amount::zero()); - } let (claim_start, claim_end) = ( last_claim_epoch.unwrap_or_default(), // Safe because of the check above @@ -5763,8 +5787,10 @@ where reward_tokens += reward; } + // Add reward tokens tallied during previous withdrawals reward_tokens += take_rewards_from_counter(storage, &source, validator)?; + // Update the last claim epoch in storage write_last_reward_claim_epoch(storage, &source, validator, current_epoch)?; // Transfer the bonded tokens from PoS to the source @@ -5774,7 +5800,8 @@ where Ok(reward_tokens) } -fn get_last_reward_claim_epoch( +/// Get the last epoch in which rewards were claimed from storage, if any +pub fn get_last_reward_claim_epoch( storage: &S, delegator: &Address, validator: &Address, diff --git a/proof_of_stake/src/tests.rs b/proof_of_stake/src/tests.rs index c55de43cc8f..276fea4cf33 100644 --- a/proof_of_stake/src/tests.rs +++ b/proof_of_stake/src/tests.rs @@ -31,7 +31,6 @@ use namada_core::types::key::RefTo; use namada_core::types::storage::{BlockHeight, Epoch, Key}; use namada_core::types::token::testing::arb_amount_non_zero_ceiled; use namada_core::types::token::NATIVE_MAX_DECIMAL_PLACES; -use namada_core::types::uint::Uint; use namada_core::types::{address, key, token}; use proptest::prelude::*; use proptest::test_runner::Config; @@ -6562,10 +6561,7 @@ fn test_slashed_bond_amount_aux(validators: Vec) { dbg!(&del_bond_amount); dbg!(&self_bond_amount); - let diff = (val_stake.change() - - self_bond_amount.change() - - del_bond_amount.change()) - .abs(); - assert!(diff <= Uint::from(2u64)); + let diff = val_stake - self_bond_amount - del_bond_amount; + assert!(diff <= 2.into()); } } diff --git a/tests/src/e2e/ledger_tests.rs b/tests/src/e2e/ledger_tests.rs index 1075cfa506e..2eb296607d4 100644 --- a/tests/src/e2e/ledger_tests.rs +++ b/tests/src/e2e/ledger_tests.rs @@ -1162,7 +1162,13 @@ fn pos_bonds() -> Result<()> { Ok(()) } -/// TODO +/// Test for claiming PoS inflationary rewards +/// +/// 1. Run the ledger node +/// 2. Wait some epochs while inflationary rewards accumulate in the PoS system +/// 3. Submit a claim-rewards tx +/// 4. Query the validator's balance before and after the claim tx to ensure +/// that reward tokens were actually transferred #[test] fn pos_rewards() -> Result<()> { let test = setup::network( @@ -1240,6 +1246,13 @@ fn pos_rewards() -> Result<()> { &validator_0_rpc, ]; let mut client = run!(test, Bin::Client, query_balance_args, Some(40))?; + let (_, res) = client.exp_regex(r"nam: [0-9\.]+").unwrap(); + let amount_pre = token::Amount::from_str( + res.split(' ').last().unwrap(), + NATIVE_MAX_DECIMAL_PLACES, + ) + .unwrap(); + dbg!(&amount_pre); client.assert_success(); let tx_args = vec![ @@ -1267,8 +1280,17 @@ fn pos_rewards() -> Result<()> { &validator_0_rpc, ]; let mut client = run!(test, Bin::Client, query_balance_args, Some(40))?; + let (_, res) = client.exp_regex(r"nam: [0-9\.]+").unwrap(); + let amount_post = token::Amount::from_str( + res.split(' ').last().unwrap(), + NATIVE_MAX_DECIMAL_PLACES, + ) + .unwrap(); + dbg!(&amount_post); client.assert_success(); + assert!(amount_post > amount_pre); + Ok(()) }