Added support for the increase/decrease additional stake instructions in the Python client library

[kagren]

• Updated the Python stake pool client library methods increase_validator_stake and decrease_validator_stake to support the increase/decrease additional validator stake instructions using the same logic as in the corresponding methods in the JS library.
• A bugfix in the JS stake pool client code, increaseValidatorStake function

[buffalojoec]

@kagren thanks!! Do you mind splitting the JS bugfix into a separate PR for tracking, since it's not related to this PR?

[joncinque]

Thanks for your contribution! The changes mostly look good, just one missing account on the decrease instruction.

To make sure that this works, how about adding an optional ephemeral stake seed parameter to the increase and decrease actions actions.py in

solana-program-library/stake-pool/py/stake_pool/actions.py

Line 482 in ab3908b

async def increase_validator_stake(

-- when the parameter is included, the instruction becomes increase or decrease additional stake.

Then you can use that new parameter in the tests at https://github.com/solana-labs/solana-program-library/blob/master/stake-pool/py/tests/test_a_time_sensitive.py -- that way we're sure that the new instruction bindings work.

What do you think?

[kagren]

@joncinque sure I will create a separate PR for the JS bug fix, and will implement the changes as you suggest

[joncinque]

This is very close, just a few nits to work out!

[joncinque]

Looks like you'll need to reformat the code too, see the output on flake8, and maybe mypy while you're at it:

check_dirs=(
  "bot"
  "spl_token"
  "stake"
  "stake_pool"
  "system"
  "tests"
  "vote"
)
flake8 "${check_dirs[@]}"
mypy "${check_dirs[@]}"

[kagren]

Will fix the last tests tomorrow

[kagren]

@joncinque need some help to understand why the DecreaseAdditionalValidatorStake instruction fails. I am not able to issue a call to DecreaseAdditionalValidatorStake, it always fails with "stake account with transient stake cannot be merged". I did some debugging and in the stake-pool program, in the process_decrease_validator_stake, when stake::merge is called, the transient account (transient_stake_account_info) has both effective and deactivating stake -- which is why the merge call fails as it is not possible to merge a stake account that has both an effective and either activating or deactivating amount.

Assume that means that DecreaseAdditionalValidatorStake would always fail if a previous call to DecreaseValidatorStake has been made prior in the same epoch. Is my understanding correct that the right way to use DecreaseAdditionalValidatorStake for the same validator is: 1) In epoch X do a call to DecreaseValidatorStake, and then 2) in epoch X+1 do a call to DecreaseAdditionalValidatorStake?

Any guidance here is much appreciated!

[joncinque]

DecreaseAdditionalValidatorStake does work as advertised! It's possible to merge two activating or deactivating stakes. "Transient" stake in the stake program refers to a stake undergoing a multi-epoch activation or deactivation, which happens whenever more than 8% of total stake is activating or deactivating.

You can see the decrease Rust tests at https://github.com/solana-labs/solana-program-library/blob/master/stake-pool/program/tests/decrease.rs and maybe see what's going wrong. From just reading the code, this looks correct once you remove the extra wait.

[joncinque]

Is this the part that should be removed in order to make sure that the decrease additional bit is actually happening?

[kagren]

@joncinque If I remove that then the call to DecreaseAdditionalValidatorStake fails with the following message:

'...Program Stake11111111111111111111111111111111111111 invoke [2]', 'Checking if destination stake is mergeable', 'stake account with transient stake cannot be merged ...'

I have also tried changing so that the deactivating stake is < 8% of the total stake, but still get the same error. Will spend some more time debugging to figure out what is going wrong (and use the Rust test as a reference).

[bji]

I do not believe that the DecreaseAdditionalValidatorStake instruction works despite the fact that the unit test succeeds. The stake program does not allow a stake account that has active stake and deactivating stake to be merged into; it's very clear about this both in comments and in the code of the MergeKind::get_if_mergeable function.

IncreaseAdditionalValidatorStake works because the stake pool's transient stake account after an increase instruction has zero active stake (since the transient stake account was created from reserve SOL as a new stake account) so it's fine to be merged into.

But if the transient stake account were created by a Decrease instruction, then it will have active stake (having been split off of an active stake pool vote account) and deactivating stake (since the Decrease put the new stake account in a deactivating state). According to the comments and logic of MergeKind::get_if_mergeable, this transient stake account can never be merged into, so subsequent DecreaseAdditionalValidatorStake instructions fail when they try to merge the ephemeral stake account into the transient stake account.

[joncinque]

I am a bit embarrassed to admit that you are right, and that DecreaseAdditionalValidatorStake does not work. I forgot that we didn't fix the stake program to work properly when merging two deactivating stakes.

We could fix this in the stake pool program by reactivating the transient stake, merging the activated ephemeral stake, and then deactivating the merged whole. That feels pretty hacky though, and requires passing in the validator account to do the reactivation, which we don't even have during "decrease". Which means that we'd have to create a new instruction to do this correctly.

We could also modify the stake program to properly merge valid deactivating stakes, which adds one more special case to consider for merge. I don't love either solution, so suggestions are very welcome.

My apologies for misunderstanding the situation, I'll think about this some more on my side. I created #6275 to track it.

In the meantime, for this PR, feel free to remove the test for decrease-additional and add a comment saying that it doesn't work as intended

[bji]

No need to apologize, this stuff is super complicated!

I am not sure how important it is to get this to succeed. I like the idea of being able to run a rebalance a bit early just in case there is a problem and we want to leave enough time to deal with it; but then running a rebalance earlier in the epoch means that if there is a reason to eject a validator from the pool after that (maybe they have an extended delinquency and it's against pool policy to retain them), if they've already been increased, it becomes impossible to decrease their stake.

This puts something of a tension between making changes early to ensure they complete, and making changes late to ensure they are done with the most up to date information possible.

However, this is probably not a backbreaking issue and if it takes significant effort I am not sure I'd advocate for that over other things that might be more important.

[bji]

Oh another thing. Is fixing the stake program in the cards? I can't think of a reason that two deactivating stake accounts ought not to be mergeable into one, so even outside of the stake pool program, supporting this action (unless there is a reason I am not aware of that it shouldn't be allowed) could have benefits to other programs or use cases in addition to the stake pool program.

[kagren]

I am a bit embarrassed to admit that you are right, and that DecreaseAdditionalValidatorStake does not work. I forgot that we didn't fix the stake program to work properly when merging two deactivating stakes.

We could fix this in the stake pool program by reactivating the transient stake, merging the activated ephemeral stake, and then deactivating the merged whole. That feels pretty hacky though, and requires passing in the validator account to do the reactivation, which we don't even have during "decrease". Which means that we'd have to create a new instruction to do this correctly.

We could also modify the stake program to properly merge valid deactivating stakes, which adds one more special case to consider for merge. I don't love either solution, so suggestions are very welcome.

My apologies for misunderstanding the situation, I'll think about this some more on my side. I created #6275 to track it.

In the meantime, for this PR, feel free to remove the test for decrease-additional and add a comment saying that it doesn't work as intended

@joncinque done

[joncinque]

Oh another thing. Is fixing the stake program in the cards? I can't think of a reason that two deactivating stake accounts ought not to be mergeable into one

That would probably be the best option. Our plan right now is to convert the stake program to run as a BPF program, so once that's done we can consider making this change.

[joncinque]

Just some final nits, then we can get this in, thanks for your patience!

[kagren]

Just some final nits, then we can get this in, thanks for your patience!

Done @joncinque

[joncinque]

Thanks for your contribution!