From 16f1c99d86ecf16093442b58ba4f96845d41ad4e Mon Sep 17 00:00:00 2001 From: Emma Zhong Date: Fri, 25 Oct 2024 14:44:36 -0700 Subject: [PATCH] fix off by one stake subsidy distribution counter --- .../docs/sui-system/stake_subsidy.md | 1 - .../docs/sui-system/sui_system_state_inner.md | 12 ++- .../sui-system/sources/stake_subsidy.move | 1 - .../sources/sui_system_state_inner.move | 17 +++- .../tests/rewards_distribution_tests.move | 91 ++++++++++++++++-- .../packages_compiled/sui-system | Bin 44434 -> 44433 bytes 6 files changed, 102 insertions(+), 20 deletions(-) diff --git a/crates/sui-framework/docs/sui-system/stake_subsidy.md b/crates/sui-framework/docs/sui-system/stake_subsidy.md index 61fd4cefe7efe..8147ace90ae4f 100644 --- a/crates/sui-framework/docs/sui-system/stake_subsidy.md +++ b/crates/sui-framework/docs/sui-system/stake_subsidy.md @@ -170,7 +170,6 @@ Advance the epoch counter and draw down the subsidy for the epoch. // Drawn down the subsidy for this epoch. let stake_subsidy = self.balance.split(to_withdraw); - self.distribution_counter = self.distribution_counter + 1; // Decrease the subsidy amount only when the current period ends. diff --git a/crates/sui-framework/docs/sui-system/sui_system_state_inner.md b/crates/sui-framework/docs/sui-system/sui_system_state_inner.md index 54fba5c2d65d5..ae65fd0a93402 100644 --- a/crates/sui-framework/docs/sui-system/sui_system_state_inner.md +++ b/crates/sui-framework/docs/sui-system/sui_system_state_inner.md @@ -2164,18 +2164,22 @@ gas coins. let storage_charge = storage_reward.value(); let computation_charge = computation_reward.value(); let mut stake_subsidy = balance::zero(); + + // during the transition from epoch N to epoch N + 1, ctx.epoch() will return N + let old_epoch = ctx.epoch(); // Include stake subsidy in the rewards given out to validators and stakers. // Delay distributing any stake subsidies until after `stake_subsidy_start_epoch`. // And if this epoch is shorter than the regular epoch duration, don't distribute any stake subsidy. - if (ctx.epoch() >= self.parameters.stake_subsidy_start_epoch && + if (old_epoch >= self.parameters.stake_subsidy_start_epoch && epoch_start_timestamp_ms >= prev_epoch_start_timestamp + self.parameters.epoch_duration_ms) { // special case for epoch 560 -> 561 change bug. add extra subsidies for "safe mode" // where reward distribution was skipped. use distribution counter and epoch check to // avoiding affecting devnet and testnet - if (self.stake_subsidy.get_distribution_counter() == 540 && ctx.epoch() > 560) { - let first_safe_mode_epoch = 561; - let safe_mode_epoch_count = ctx.epoch() - first_safe_mode_epoch; + if (self.stake_subsidy.get_distribution_counter() == 540 && old_epoch > 560) { + // safe mode was entered on the change from 560 to 561. so 560 was the first epoch without proper subsidy distribution + let first_safe_mode_epoch = 560; + let safe_mode_epoch_count = old_epoch - first_safe_mode_epoch; safe_mode_epoch_count.do!(|_| { stake_subsidy.join(self.stake_subsidy.advance_epoch()); }); diff --git a/crates/sui-framework/packages/sui-system/sources/stake_subsidy.move b/crates/sui-framework/packages/sui-system/sources/stake_subsidy.move index 5a91ebc7d54fd..d355a69c8489a 100644 --- a/crates/sui-framework/packages/sui-system/sources/stake_subsidy.move +++ b/crates/sui-framework/packages/sui-system/sources/stake_subsidy.move @@ -65,7 +65,6 @@ module sui_system::stake_subsidy { // Drawn down the subsidy for this epoch. let stake_subsidy = self.balance.split(to_withdraw); - self.distribution_counter = self.distribution_counter + 1; // Decrease the subsidy amount only when the current period ends. diff --git a/crates/sui-framework/packages/sui-system/sources/sui_system_state_inner.move b/crates/sui-framework/packages/sui-system/sources/sui_system_state_inner.move index b2137264c5505..4384f3fcd2599 100644 --- a/crates/sui-framework/packages/sui-system/sources/sui_system_state_inner.move +++ b/crates/sui-framework/packages/sui-system/sources/sui_system_state_inner.move @@ -866,18 +866,22 @@ module sui_system::sui_system_state_inner { let storage_charge = storage_reward.value(); let computation_charge = computation_reward.value(); let mut stake_subsidy = balance::zero(); + + // during the transition from epoch N to epoch N + 1, ctx.epoch() will return N + let old_epoch = ctx.epoch(); // Include stake subsidy in the rewards given out to validators and stakers. // Delay distributing any stake subsidies until after `stake_subsidy_start_epoch`. // And if this epoch is shorter than the regular epoch duration, don't distribute any stake subsidy. - if (ctx.epoch() >= self.parameters.stake_subsidy_start_epoch && + if (old_epoch >= self.parameters.stake_subsidy_start_epoch && epoch_start_timestamp_ms >= prev_epoch_start_timestamp + self.parameters.epoch_duration_ms) { // special case for epoch 560 -> 561 change bug. add extra subsidies for "safe mode" // where reward distribution was skipped. use distribution counter and epoch check to // avoiding affecting devnet and testnet - if (self.stake_subsidy.get_distribution_counter() == 540 && ctx.epoch() > 560) { - let first_safe_mode_epoch = 561; - let safe_mode_epoch_count = ctx.epoch() - first_safe_mode_epoch; + if (self.stake_subsidy.get_distribution_counter() == 540 && old_epoch > 560) { + // safe mode was entered on the change from 560 to 561. so 560 was the first epoch without proper subsidy distribution + let first_safe_mode_epoch = 560; + let safe_mode_epoch_count = old_epoch - first_safe_mode_epoch; safe_mode_epoch_count.do!(|_| { stake_subsidy.join(self.stake_subsidy.advance_epoch()); }); @@ -1141,6 +1145,11 @@ module sui_system::sui_system_state_inner { self.stake_subsidy.set_distribution_counter(counter) } + #[test_only] + public(package) fun epoch_duration_ms(self: &SuiSystemStateInnerV2): u64 { + self.parameters.epoch_duration_ms + } + // CAUTION: THIS CODE IS ONLY FOR TESTING AND THIS MACRO MUST NEVER EVER BE REMOVED. Creates a // candidate validator - bypassing the proof of possession check and other metadata validation // in the process. diff --git a/crates/sui-framework/packages/sui-system/tests/rewards_distribution_tests.move b/crates/sui-framework/packages/sui-system/tests/rewards_distribution_tests.move index 2cb3c0a9fd634..ec94cbf81a1bf 100644 --- a/crates/sui-framework/packages/sui-system/tests/rewards_distribution_tests.move +++ b/crates/sui-framework/packages/sui-system/tests/rewards_distribution_tests.move @@ -493,32 +493,103 @@ module sui_system::rewards_distribution_tests { test_scenario::return_shared(system_state); } - #[test] - fun test_stake_subsidy_with_safe_mode() { - use std::unit_test::assert_eq; + fun check_distribution_counter_invariant(system: &mut SuiSystemState, ctx: &TxContext) { + assert!(ctx.epoch() == system.epoch()); + // first subsidy distribution was at epoch 20, so counter should always be ahead by 20 + assert_eq(system.get_stake_subsidy_distribution_counter() + 20, ctx.epoch()); + } + #[test] + fun test_stake_subsidy_with_safe_mode_epoch_562_to_563() { set_up_sui_system_state_with_big_amounts(); let mut test = test_scenario::begin(VALIDATOR_ADDR_1); let mut sui_system = test.take_shared(); + let ctx = test.ctx(); + // mimic state during epoch 562, if we're in safe mode since the 560 -> 561 epoch change + let start_epoch: u64 = 562; + let start_distribution_counter = 540; + let epoch_start_time = 100000000000; + let epoch_duration = sui_system.inner_mut_for_testing().epoch_duration_ms(); + + // increment epoch number (safe mode emulation) + start_epoch.do!(|_| ctx.increment_epoch_number()); + sui_system.set_epoch_for_testing(start_epoch); + sui_system.set_stake_subsidy_distribution_counter(start_distribution_counter); + + assert!(ctx.epoch() == start_epoch); + assert!(ctx.epoch() == sui_system.epoch()); + assert!(sui_system.get_stake_subsidy_distribution_counter() == start_distribution_counter); + + // perform advance epoch + sui_system + .inner_mut_for_testing() + .advance_epoch(start_epoch + 1, 65, balance::zero(), balance::zero(), 0, 0, 0, 0, epoch_start_time, ctx) + .destroy_for_testing(); // balance returned from `advance_epoch` + ctx.increment_epoch_number(); + + // should distribute 3 epochs worth of subsidies: 560, 561, 562 + assert_eq(sui_system.get_stake_subsidy_distribution_counter(), start_distribution_counter + 3); + check_distribution_counter_invariant(&mut sui_system, ctx); + + // ensure that next epoch change only distributes one epoch's worth + sui_system + .inner_mut_for_testing() + .advance_epoch(start_epoch + 2, 65, balance::zero(), balance::zero(), 0, 0, 0, 0, epoch_start_time + epoch_duration, ctx) + .destroy_for_testing(); // balance returned from `advance_epoch` + ctx.increment_epoch_number(); + // should distribute 1 epoch's worth of subsidies: 563 only + assert_eq(sui_system.get_stake_subsidy_distribution_counter(), start_distribution_counter + 4); + check_distribution_counter_invariant(&mut sui_system, ctx); + + test_scenario::return_shared(sui_system); + test.end(); + } + + #[test] + fun test_stake_subsidy_with_safe_mode_epoch_563_to_564() { + set_up_sui_system_state_with_big_amounts(); + + let mut test = test_scenario::begin(VALIDATOR_ADDR_1); + let mut sui_system = test.take_shared(); let ctx = test.ctx(); + // mimic state during epoch 563, if we're in safe mode since the 560 -> 561 epoch change + let start_epoch: u64 = 563; + let start_distribution_counter = 540; + let epoch_start_time = 100000000000; + let epoch_duration = sui_system.inner_mut_for_testing().epoch_duration_ms(); // increment epoch number (safe mode emulation) - 562u64.do!(|_| ctx.increment_epoch_number()); - sui_system.set_epoch_for_testing(562); - sui_system.set_stake_subsidy_distribution_counter(540); + start_epoch.do!(|_| ctx.increment_epoch_number()); + sui_system.set_epoch_for_testing(start_epoch); + sui_system.set_stake_subsidy_distribution_counter(start_distribution_counter); - assert!(ctx.epoch() == 562); - assert!(sui_system.get_stake_subsidy_distribution_counter() == 540); + assert!(ctx.epoch() == start_epoch); + assert!(ctx.epoch() == sui_system.epoch()); + assert!(sui_system.get_stake_subsidy_distribution_counter() == start_distribution_counter); // perform advance epoch sui_system .inner_mut_for_testing() - .advance_epoch(563, 65, balance::zero(), balance::zero(), 0, 0, 0, 0, 100000000000, ctx) + .advance_epoch(start_epoch + 1, 65, balance::zero(), balance::zero(), 0, 0, 0, 0, epoch_start_time, ctx) + .destroy_for_testing(); // balance returned from `advance_epoch` + ctx.increment_epoch_number(); + + // should distribute 4 epochs worth of subsidies: 560, 561, 562, 563 + assert_eq(sui_system.get_stake_subsidy_distribution_counter(), start_distribution_counter + 4); + check_distribution_counter_invariant(&mut sui_system, ctx); + + // ensure that next epoch change only distributes one epoch's worth + sui_system + .inner_mut_for_testing() + .advance_epoch(start_epoch + 2, 65, balance::zero(), balance::zero(), 0, 0, 0, 0, epoch_start_time + epoch_duration, ctx) .destroy_for_testing(); // balance returned from `advance_epoch` + ctx.increment_epoch_number(); - assert_eq!(sui_system.get_stake_subsidy_distribution_counter(), 542); + // should distribute 1 epoch's worth of subsidies + assert_eq(sui_system.get_stake_subsidy_distribution_counter(), start_distribution_counter + 5); + check_distribution_counter_invariant(&mut sui_system, ctx); test_scenario::return_shared(sui_system); test.end(); diff --git a/crates/sui-framework/packages_compiled/sui-system b/crates/sui-framework/packages_compiled/sui-system index 579bcc4606828af1704b8be4b13a3823653e74e6..f8cb9f3ffdf5868431ed8be791fb8c0f3975b426 100644 GIT binary patch delta 613 zcmYjOzl#$=6yDjL-MyXnW`B^`B<9TJ*O}a1F1yJld$+;vKtvQfQ9FB^6v7=g2O@%v zG47wBje;Pgu}~`oE7AJE%0>{uMi2{qiQ+lM%zNK_?|t9Ad2po9j`U~Qggwp2lasag zw*A(7TwD1TU9&#K*TsweONKuNZwm6&naKMxzcG1ku{iJT*XI}ZFP5A;hR}RVVJClh z?~2-z25e=L3C(jd#7)UzGxw&a&yR_P!Bvb!4C5m1x<=+=)6&%P!3>H|Qyn4!Fe(OJH`?j}frDG@cvWs=YC z$A<|aBd9~IgbhexRKgw5iBSltkro_Ks6t4vC0oE3o?BF~PAGx_eP{_2tl%VuOFz hLGzveAuO1`2&DNR2T1%O delta 592 zcmX|8yKWOf6y4dK-ObLuvk!UfHBS6a?8J%JUdNkkL`8vUAQAAt8k11_>bwigZAqhKdG7qD+ItwG)_P=G-&qKF-YKSbuS>zfVW(UA8k?s(!QW zBkxUh;dgY``rf-IKK4H`JdKZX^7G}$JDJ`eeXv-Rb@yu1-Mwp}^VATUZz*hLub$sl zYtn$VR5GD?PKvlHIjm;>;l+cLR2baBNJNkpaLYAP7rQQ}y__=a2oVKWNW*+vwfF;Y zpf!)ADzs3U`wl8l{4Evs-QcFRq-*n4=TF!?)#{%!%a}KxF$eRI=daK2VtwUI9wJz0 zNYk(ZfE%JAHkvbGgUL>Ia#+0jk^qhG1~B&$wQ8eOzw*VCGK-8Uw*>(VknbxiQrpQX5Tj8E4~h?_kDk zLJg`R)?o;nAts=cB9|x6TCh)0s`JxqGEtR> b2KjI>XZ{0_