Fix bug of same-epoch stake deactivation after stake redelegation #32606

HaoranYi · 2023-07-24T14:45:26Z

Problem

Normally, activated stake has to wait at least one epoch after being deactivated, before it becomes deactivated and withdrawable.

However, there is a bug in redelegation which could lead to same-epoch stake deactivation after stake redelegation. By redelegating the activated stake to another validator and deactivating the redelegated stake, the stake becomes deactivated and withdrawable in the same epoch.

Part of #31876

Summary of Changes

Fix bug of same-epoch stake deactivation after stake redelegation

Fixes #

codecov · 2023-07-24T15:47:06Z

Codecov Report

Merging #32606 (7c45985) into master (57e78a1) will increase coverage by 0.0%.
Report is 8 commits behind head on master.
The diff coverage is 96.4%.

@@           Coverage Diff            @@
##           master   #32606    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         798      798            
  Lines      216577   216980   +403     
========================================
+ Hits       177537   177874   +337     
- Misses      39040    39106    +66

mvines

this PR is so much clearer now that the other refactoring landed earlier!
it looks like there's no test for the condition we're fixing here though, we'll definitely want to add one, or several, to demonstrate the fix

HaoranYi · 2023-07-24T16:23:26Z

this PR is so much clearer now that the other refactoring landed earlier! it looks like there's no test for the condition we're fixing here though, we'll definitely want to add one, or several, to demonstrate the fix

Yeah. Added.

t-nelson · 2023-07-24T18:39:09Z

can we spruce up the description to indicate that this bug is restricted to the Redelegate operation?

programs/stake/src/stake_state.rs

t-nelson · 2023-07-25T01:17:25Z

cli/tests/stake.rs

@@ -2320,3 +2320,757 @@ fn test_stake_minimum_delegation() {
    let result = process_command(&config);
    assert!(matches!(result, Ok(..)));
 }
+
+#[test]
+fn test_stake_redelegation_then_deactivation_withdraw_not_permitted() {


these don't seem to be testing any new cli behavior, they'd be better implemented against ~~SolanaTestValidator~~ TestValidator (or even Bank) directly than with all of the superfluous cli plumbing in the middle

I am not sure that I understand your comments.

Inside cli/tests, it uses TestValidator class. Is that not the SolanaTestValidator?

The test scenario for this bug is very complex. It involves multiple stake accounts and votes accounts, and a sequence of very specific actions, i.e. delegate, redelegate, merge, deactivate, withdraw etc. It also requires stake_history mutation. I am not sure how this can be modeled on just bank itself. I think bank tests are more suitable to test individual instruction. Do you know any examples on bank test to model stake deactivation, activation and withdraw?

It seems like cli/stake.rs is where we are testing all other existing scenarios of stake manipulation, such as redelegation, withdraw etc. Therefore, I feel it is natural to extend those tests to cover the new behavior.

i'm saying that the cli crate is not a test framework, that these tests belong elsewhere and implemented without the cli details

To me, the cli/tests looks more or less like a test framework. It has been used to test stake, vote, transfer, and other programs. And it has very useful test_util.rs, which make it very convenient to model complex test scenario there.

I don't know if we should "reinvent the wheel" to write another test framework to model the lifetime of a test validator. Or maybe I am missing something about the other test framework?

Do you know where the other test framework/utility is, which could be used to better model these tests, and where should those tests be?

To me, the cli/tests looks more or less like a test framework.

this is a function of those tests existing before TestValidator and us for some reason being very paranoid about cli behaving as expected. most, if not all of them should really be modernized at some point to avoid leading folks astray

I don't know if we should "reinvent the wheel" to write another test framework to model the lifetime of a test validator.

we don't need to, that's literally what TestValidator was for. the tests will be similar but won't a) live under the totally unrelated cli crate and b) have all of the superfluous cli machination in the way and confounding tests. you'll grab an RpcClient handle from the TestValidator instance, build the txes up and rpc_client.send_and_confirm_transaction() your way to victory. these should probably live under programs/stake, either a new tests directory or if one of existing source files seems appropriate

it should be a red flag when tests are being added to a crate that was otherwise unaffected by the change set that something isn't right organizationally

which has a separate crate for tests

oh thank god. i missed that this wasn't implementing the tests directory as a standalone crate. just realized i was about to get screwed by the instruction processor being globbed into BUILTINS. this is The Way ™️

the program crates like solana-stake-program should function like normal program crates

generally agree, but there's a bunch of crap in this program crate that makes zero sense. even have comments about being external helpers with uses listed that are... not the program itself!

It seems that moving the stake tests out of cli is not as straightforward as it looks.

How about keeping these 3 tests in cli for this PR, and moving all stake tests out of cli in a separated follow-up PR after we fixes the circular dependencies issue?

take a crack at making the tests dir a crate as jon proposes first. doing the easy thing our of impatience is how the code got into the shape it is today. that followup pr rarely happens

ok. I will create a new test crate solana-stake-program-tests for those tests.

generally agree, but there's a bunch of crap in this program crate that makes zero sense. even have comments about being external helpers with uses listed that are... not the program itself!

That's totally fair... the native program crates are probably the most problematic with dependencies. We could eventually make it a policy that all native stuff goes into the sdk, but I would prefer to thin out the sdk whenever possible.

Note: I'm the one who moved a lot of the stake types into the sdk in the first place while developing stake pools, because I didn't think to use target_os guards in solana-stake-program, so I bear a lot of the responsibility there 😓

sdk/program/src/stake/state.rs

joncinque

We're getting really close! Just a few bits to make the tests better, which @t-nelson can shoot down if he wishes.

joncinque · 2023-07-28T17:21:26Z

Cargo.toml

@@ -88,6 +88,7 @@ members = [
    "sdk/program",
    "send-transaction-service",
    "stake-accounts",
+    "stake-program-test",


nit: let's put this in programs/stake-tests for consistency with address lookup table tests

joncinque · 2023-07-28T17:23:36Z

stake-program-test/Cargo.toml

@@ -0,0 +1,21 @@
+[package]
+name = "stake-program-test"


nit:

Suggested change

name = "stake-program-test"

name = "solana-stake-program-tests"

joncinque · 2023-07-28T17:23:55Z

stake-program-test/Cargo.toml

+license = { workspace = true }
+edition = { workspace = true }
+
+[dependencies]


These should all be dev-dependencies

joncinque · 2023-07-28T17:24:06Z

stake-program-test/src/lib.rs

nit: you should be able to delete this file

joncinque · 2023-07-28T17:30:59Z

stake-program-test/tests/stake.rs

Although it's better factored, I don't think this addresses @t-nelson 's point about using the CLI as a test framework, since it's still using CLI commands.

Rather than CLI helpers, how about using solana-program-test? To make it easier, you can copy these helpers for creating vote accounts https://github.com/solana-labs/solana-program-library/blob/8f1a8c6e9c58c8710f0655d5e54d3f07bfacf003/stake-pool/single-pool/tests/helpers/mod.rs#L349 and stake accounts https://github.com/solana-labs/solana-program-library/blob/8f1a8c6e9c58c8710f0655d5e54d3f07bfacf003/stake-pool/single-pool/tests/helpers/stake.rs#L66

I know it's annoying to change the test framework, but it'll make maintenance a lot easier going forward, and you can use warp_to_slot on ProgramTestContext to simulate advancing epochs, which will make the test run much more quickly.

yeah. I was debating about whether to use or not to use those CLI helpers yesterday too. Basically, the cli functions used in the tests are just wrappers for rpc_clients. We can certainly reimplement those helpers for tests, which would end up inlining all the cli functions in those tests. They wouldn't make much difference except duplicated code.

I will take a look at the program-test and see if rewrite those tests in program-tests is better.

Done! Rewrite the tests in program-test.

joncinque

The test looks great! Mainly some nits around it, so this is extremely close

program-test/tests/test_utils.rs

joncinque · 2023-08-03T23:50:32Z

program-test/tests/test_utils.rs

nit: I usually dislike vague names for helper files, how about something like setup.rs since there's just setup stuff in there?

program-test/tests/stake.rs

joncinque

Looks setup.rs wasn't committed, but the rest of the changes look good

HaoranYi · 2023-08-04T21:44:15Z

Looks setup.rs wasn't committed, but the rest of the changes look good

oops. added.

joncinque

Looks great! Be sure to get at least one more approval on this since it touches the stake program

HaoranYi · 2023-08-29T15:23:03Z

@t-nelson Can you review this again and let me know if you are okay with this PR? Thanks!

joncinque

Still looks good to me!

program-test/tests/warp.rs

joncinque · 2023-09-22T00:34:32Z

program-test/tests/stake.rs

+#[tokio::test]
+async fn test_stake_redelegation_then_deactivation_withdraw_not_permitted() {
+    test_stake_redelegation_pending_activation(PendingStakeActivationTestFlag::NoMerge).await
+}
+
+#[tokio::test]
+async fn test_stake_redelegation_then_merge_active_then_deactivation_withdraw_not_permitted() {
+    test_stake_redelegation_pending_activation(PendingStakeActivationTestFlag::MergeActive).await
+}
+
+#[tokio::test]
+async fn test_stake_redelegation_then_merge_inactive_then_deactivation_withdraw_not_permitted() {
+    test_stake_redelegation_pending_activation(PendingStakeActivationTestFlag::MergeInactive).await
+}


nit: rather than having all of these, you could have them all as separate test_cases

good idea. updated.

CriesofCarrots

One suggestion to move sysvar load

programs/stake/src/stake_instruction.rs

programs/stake/src/stake_state.rs

CriesofCarrots · 2023-09-25T19:39:29Z

program-test/tests/warp.rs

 #[tokio::test]
 async fn stake_rewards_from_warp() {
    // Initialize and start the test network
    let program_test = ProgramTest::default();
    let mut context = program_test.start_with_context().await;
+
+    context.warp_to_slot(100).unwrap();


To be clear, Trent isn't objecting to the change, just requesting that the move and the edit be noted in separate commits. So the commits in this PR might be something like:

Add deactivate_stake() helper

Return error when feature activated and StakeFlag set

Add stake_flag unit test

Move setup_stake to its own mod

Only warp in warp.rs tests

Add stake.rs tests

I don't want to bog this down in a reorg, so just a consideration for next time.

program-test/tests/stake.rs

add tests refactor common code into fn avoid early return add feature gate for the new stake redelegate behavior move stake tests out of cli add stake-program-test crate reimplemnt stake test with program-test remove stake-program-test crate reviews add setup.rs remove clippy reveiws

This is done.

HaoranYi changed the title ~~fix stake deactivation in the same epoch after redelegation bug~~ Fix stake deactivation in the same epoch after redelegation bug Jul 24, 2023

HaoranYi changed the title ~~Fix stake deactivation in the same epoch after redelegation bug~~ Fix bug of same-epoch stake deatviation after stake delegation Jul 24, 2023

HaoranYi requested review from joncinque, mvines, t-nelson and 2501babe July 24, 2023 15:50

mvines reviewed Jul 24, 2023

View reviewed changes

mvines changed the title ~~Fix bug of same-epoch stake deatviation after stake delegation~~ Fix bug of same-epoch stake deactivation after stake delegation Jul 24, 2023

HaoranYi changed the title ~~Fix bug of same-epoch stake deactivation after stake delegation~~ Fix bug of same-epoch stake deactivation after stake redelegation Jul 24, 2023

t-nelson previously requested changes Jul 24, 2023

View reviewed changes

programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved

t-nelson reviewed Jul 25, 2023

View reviewed changes

sdk/program/src/stake/state.rs Outdated Show resolved Hide resolved

sdk/program/src/stake/state.rs Outdated Show resolved Hide resolved

HaoranYi marked this pull request as draft July 26, 2023 01:25

HaoranYi force-pushed the stake_fix branch from c8021c6 to e207e2e Compare July 26, 2023 01:48

HaoranYi marked this pull request as ready for review July 26, 2023 02:05

HaoranYi force-pushed the stake_fix branch 5 times, most recently from 0583f66 to 1a2de7b Compare July 28, 2023 01:43

joncinque reviewed Jul 28, 2023

View reviewed changes

joncinque reviewed Aug 4, 2023

View reviewed changes

HaoranYi force-pushed the stake_fix branch from 0451018 to 1f05ac6 Compare August 4, 2023 18:59

joncinque reviewed Aug 4, 2023

View reviewed changes

joncinque previously approved these changes Aug 4, 2023

View reviewed changes

HaoranYi requested a review from t-nelson August 4, 2023 21:58

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 28, 2023

mvines mentioned this pull request Aug 29, 2023

Feature Gate: StakeInstruction::Redelegate #26765

Open

HaoranYi force-pushed the stake_fix branch from fbd3507 to ca150b4 Compare August 29, 2023 15:21

HaoranYi mentioned this pull request Aug 29, 2023

rekey stake_redelegate_instruction feature #33056

Merged

github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 30, 2023

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 14, 2023

HaoranYi dismissed joncinque’s stale review via ff6561e September 20, 2023 18:16

HaoranYi force-pushed the stake_fix branch from ca150b4 to ff6561e Compare September 20, 2023 18:16

HaoranYi requested a review from joncinque September 20, 2023 18:19

HaoranYi force-pushed the stake_fix branch from ff6561e to 2886661 Compare September 20, 2023 18:23

github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 21, 2023

joncinque previously approved these changes Sep 22, 2023

View reviewed changes

HaoranYi dismissed joncinque’s stale review via b7e53a1 September 22, 2023 02:19

HaoranYi force-pushed the stake_fix branch 4 times, most recently from a9bbf47 to c2b889e Compare September 22, 2023 23:48

HaoranYi requested a review from joncinque September 22, 2023 23:54

joncinque previously approved these changes Sep 25, 2023

View reviewed changes

CriesofCarrots self-requested a review September 25, 2023 19:13

CriesofCarrots reviewed Sep 25, 2023

View reviewed changes

HaoranYi added 3 commits September 25, 2023 20:11

reviews

935df43

review comments

7c45985

HaoranYi dismissed joncinque’s stale review via 7c45985 September 25, 2023 20:11

HaoranYi force-pushed the stake_fix branch from c2b889e to 7c45985 Compare September 25, 2023 20:11

CriesofCarrots approved these changes Sep 25, 2023

View reviewed changes

HaoranYi merged commit d25d53e into solana-labs:master Sep 25, 2023
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug of same-epoch stake deactivation after stake redelegation #32606

Fix bug of same-epoch stake deactivation after stake redelegation #32606

HaoranYi commented Jul 24, 2023 •

edited

Loading

codecov bot commented Jul 24, 2023 •

edited

Loading

mvines left a comment

HaoranYi commented Jul 24, 2023

t-nelson commented Jul 24, 2023

t-nelson Jul 25, 2023 •

edited

Loading

HaoranYi Jul 25, 2023

t-nelson Jul 25, 2023

HaoranYi Jul 25, 2023 •

edited

Loading

t-nelson Jul 25, 2023

t-nelson Jul 27, 2023

t-nelson Jul 27, 2023

HaoranYi Jul 27, 2023

joncinque Jul 27, 2023

HaoranYi Jul 28, 2023

joncinque left a comment

joncinque Jul 28, 2023

joncinque Jul 28, 2023

joncinque Jul 28, 2023

joncinque Jul 28, 2023

joncinque Jul 28, 2023

HaoranYi Jul 28, 2023 •

edited

Loading

HaoranYi Aug 3, 2023

joncinque left a comment

joncinque Aug 3, 2023

HaoranYi Aug 4, 2023

joncinque left a comment

HaoranYi commented Aug 4, 2023

joncinque left a comment

HaoranYi commented Aug 29, 2023

joncinque left a comment

joncinque Sep 22, 2023

HaoranYi Sep 22, 2023

CriesofCarrots left a comment

CriesofCarrots Sep 25, 2023

	name = "stake-program-test"
	name = "solana-stake-program-tests"

Fix bug of same-epoch stake deactivation after stake redelegation #32606

Fix bug of same-epoch stake deactivation after stake redelegation #32606

Conversation

HaoranYi commented Jul 24, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Jul 24, 2023 • edited Loading

Codecov Report

mvines left a comment

Choose a reason for hiding this comment

HaoranYi commented Jul 24, 2023

t-nelson commented Jul 24, 2023

t-nelson Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoranYi Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoranYi Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

HaoranYi commented Aug 4, 2023

joncinque left a comment

Choose a reason for hiding this comment

HaoranYi commented Aug 29, 2023

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoranYi commented Jul 24, 2023 •

edited

Loading

codecov bot commented Jul 24, 2023 •

edited

Loading

t-nelson Jul 25, 2023 •

edited

Loading

HaoranYi Jul 25, 2023 •

edited

Loading

HaoranYi Jul 28, 2023 •

edited

Loading