Skip to content

Conversation

2501babe
Copy link
Member

@2501babe 2501babe commented Aug 13, 2025

instead of zeroing account data, we should realloc accounts to 0 size, to ease headaches of downstream programs dealing with stake accounts across multiple instructions in the same transaction

closes #71

@2501babe 2501babe self-assigned this Aug 13, 2025
@2501babe
Copy link
Member Author

this might also be a good time to think about how we are going to do fuzzing, because it is a small simple change that slihgtly changes behavior. ideally we have some kind of runner via mollusk that tells us something changed between the two versions, we can say "ok this is fine" and adjust the things we expect to be different, and confirm nothing unexpected changed by accident. i have no idea yet what impling this kind of flow would be like since it depends on multiple versions of the program and idk if it can really be part of ci... idk think about it

@felix-asym
Copy link

I believe this change would break the stake cache logic in Agave.
See the comment here and the discussion here

The code for updating the stake cache currently assumes that the staking program doesn't change the owner of an account when it's lamports are zeroed out.

@2501babe 2501babe changed the title program: deallocate stake account on zero lamports program: reallocate stake account on zero lamports Aug 15, 2025
@2501babe 2501babe marked this pull request as ready for review August 28, 2025 11:08
@2501babe 2501babe requested a review from joncinque August 28, 2025 11:08
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! I took another looks through the stakes cache, and I believe this will be safe.

The stakes cache only stores stake accounts with delegations, and if lamports go to 0 OR if the account can no longer be deserialized as a stake account, it gets removed from the cache. So essentially we'll go from deserializing StakeState::Uninitialized and then failing to get its delegation.

We should be aware that this does create a DOS opportunity on stake accounts. If someone closes a stake account and then tops it up with lamports in the same transaction, it will be impossible to ever reuse it. We could relax the logic in withdraw to treat a zero-length account the same as a StakeState::Uninitialized -- what do you think?

@2501babe 2501babe marked this pull request as draft September 2, 2025 05:24
@2501babe
Copy link
Member Author

2501babe commented Sep 2, 2025

im gonna leave this in draft until im back, i havent thought deeply about downstream effects the zero-len accounts or the ability to withdraw from them. at least we need to add test cases to make sure some of the other instructions refuse to interact with them. its probably fine since i believe everything goes through get_stake_state() but yea

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new commit looks great to me! And yeah definitely good to think about all of the other interactions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

program: Properly delete accounts
3 participants