program: use stake history skillfully#586
Conversation
2501babe
force-pushed
the
20260401_stakehistory
branch
from
9bc3d12 to
edbd28c
Compare
2501babe
force-pushed
the
20260401_stakehistory
branch
from
21ca8af to
d7223a7
Compare
| */ | ||
|
|
||
| t.true(true); |
There was a problem hiding this comment.
i disabled the deposit, deposit default, and withdraw js tests for now. i think we should land this pr as-is and fix js after, since this pr is important, the js is less important, and neither the js, nor the program interface these test the js adheres to, are changing
bankrun is deprecated and still on solana 1.x, so i will have to port everything to litesvm. deposit fails now because i use sol_get_sysvar for stake history, which was added in solana 2.x. the withdraw test fails because it has to deposit, withdraw itself is unchanged by this pr
2501babe
force-pushed
the
20260401_stakehistory
branch
3 times, most recently
from
b8a1fc3 to
e275f03
Compare
| // inactive deposit into deactivated pool fails | ||
| let instructions = instruction::deposit( | ||
| &id(), | ||
| &accounts.pool, | ||
| &accounts.bob_stake.pubkey(), | ||
| &accounts.bob_token, | ||
| &accounts.bob.pubkey(), | ||
| &accounts.bob.pubkey(), | ||
| ); | ||
| let transaction = Transaction::new_signed_with_payer( | ||
| &instructions, | ||
| Some(&context.payer.pubkey()), | ||
| &[&context.payer, &accounts.bob], | ||
| context.last_blockhash, | ||
| ); | ||
|
|
||
| let e = context | ||
| .banks_client | ||
| .process_transaction(transaction) | ||
| .await | ||
| .unwrap_err(); | ||
| check_error(e, SinglePoolError::ReplenishRequired); |
There was a problem hiding this comment.
you know, i really thought this would incorrectly succeed against the current version of svsp. but when i patched this test back into master, i discovered it gives SinglePoolError::DepositTooSmall. there is no svsp error blocking the inactive/inactive merge. there is no stake program error either! the merge succeeds
... but then, because merge does not modify the delegation of the inactive stake, svsp decides every lamport is an excess lamport, resulting in a zero-deposit
its times like this i appreciate what man truly hath wrought when he invented computer programming
There was a problem hiding this comment.
What's honestly more impressive is that we can make sense of these things 😅
2501babe
force-pushed
the
20260401_stakehistory
branch
from
ae62518 to
31c85c9
Compare
* `DepositStake` allows all mergeable sources when pool is activating * `DepositStake` properly aborts when pool is inactive * `DepositStake` errors helpfully on user lockup instead of falling through to `Merge` * `DepositStake` takes stake history into account correctly
2501babe
force-pushed
the
20260401_stakehistory
branch
from
31c85c9 to
ebc37b2
Compare
| // we deliberately do NOT validate the activation status of the pool account. | ||
| // neither snow nor rain nor warmup/cooldown nor validator delinquency prevents a user withdrawal | ||
| // | ||
| // NOTE this is fine for stake v4 but subtly wrong for stake v5 *if* the pool account was deactivated. | ||
| // stake v5 declines to (meaninglessly) adjust delegations of deactivated sources. | ||
| // this will (again) be correct with #581, which shifts to NEV accounting on lamports rather than stake. | ||
| // we should plan another SVSP release before stake v5 activation |
There was a problem hiding this comment.
pointing this out for attention. all the code changes in the pr concern DepositStake, this comment is in WithdrawStake
| let stake_history = &StakeHistorySysvar(clock.epoch); | ||
|
|
There was a problem hiding this comment.
this syscall really is the gift that keeps on giving 😁
| // inactive deposit into deactivated pool fails | ||
| let instructions = instruction::deposit( | ||
| &id(), | ||
| &accounts.pool, | ||
| &accounts.bob_stake.pubkey(), | ||
| &accounts.bob_token, | ||
| &accounts.bob.pubkey(), | ||
| &accounts.bob.pubkey(), | ||
| ); | ||
| let transaction = Transaction::new_signed_with_payer( | ||
| &instructions, | ||
| Some(&context.payer.pubkey()), | ||
| &[&context.payer, &accounts.bob], | ||
| context.last_blockhash, | ||
| ); | ||
|
|
||
| let e = context | ||
| .banks_client | ||
| .process_transaction(transaction) | ||
| .await | ||
| .unwrap_err(); | ||
| check_error(e, SinglePoolError::ReplenishRequired); |
There was a problem hiding this comment.
What's honestly more impressive is that we can make sense of these things 😅
| .await | ||
| .unwrap_err(); | ||
| check_error(e, SinglePoolError::WrongStakeState); | ||
| } |
There was a problem hiding this comment.
This is very slick, and covers all of the cases, from what I can see. The comment about deactivating pools being covered with replenish is extremely helpful too.
2 participants
currently
DepositStakeeyeballs whether the pool is active based onClockand relies onMergeto gatekeep bad state transitions. this pr changes it to fully rely onStakeHistoryto make its own correct decisionsthis also fixes a couple harmless but strange bugs:
StakeStateV2::Stake. this now succeedsimportantly, without this pr, it is likely that any reasonable design for #581 lamport-based accounting would allow deposits to an inactive pool, so we desire to strictly prevent this
we also test
DepositStakemuch more thoroughly. as noted below we have to disable some of our js tests but they will be reenabled with #587closes #103