program: rewrite `process_split()` #90

2501babe · 2025-10-02T19:25:39Z

i rewrote Split from scratch to fix #1 plus some other bugs with minimum delegation. i also tried to make it more readable. Split has gone through several layers of changes which caused it to accumulate a lot of cruft, particularly because it originally ignored delegation, worked with lamports exclusively, and never used stake history

current Split tries match once on source stake and share code for both the Initialized and Stake cases in the unified validate_split_amount() function and then do more work for the Stake case after. it ends up being quite difficult to understand because:

when Stake is deactivated it should actually follow the same logic as Initialized, not the logic for Stake
logic for the full split is woven into all the same math as the partial split
there are many duplicated or "just in case" balance checks, particularly involving rent-exemption because feature_set::rent_exempt_split_destinations was added in a minimally invasive way
the math overall is extremely indirect. it starts with source/dest/split amount lamps and "additional required" (minimum delegation), calculates minimums from rent and additional, calculates source remaining from source and split amount, then uses these to try to calculate stake delta and split stake amounts. all through this, these values are combined and compared haphazardly to check various error conditions

as a consequence of all this, i find Split the most difficult stake operation to reason about. in rewriting process_split(), i decided to:

check signers up front for all cases
handle the full split, partial split inactive, and partial split active/activating cases separately, with their own math and update logic
tolerate matching on stake states as convenient rather than forcing the code to be structured around a single match

this results in a function that has a bit more boilerplate but none of the difficult calculations of the orginal Split

i also made it an error to split 0 from Uninitialized. i dont see any good reason to allow it

closes #1

grod220 · 2025-12-09T14:43:26Z

program/src/processor.rs

+            if source_stake_account_info.key != destination_stake_account_info.key {
+                source_stake_account_info.resize(0)?;
+            }


source_stake_account_info.key == destination_stake_account_info.key is a strange case. Should we just guard above to throw an err in that case? Can't think of a reason why someone would do that.

i dunno why we support it either tbh, other than that we always have and that there are tests for it which indicate it was on purpose. it could be that the original authors just had a preference for permissiveness

Should we just kill this? It feels more like a potential footgun hanging around.

grod220 · 2025-12-09T15:29:27Z

program/src/processor.rs

+                    0
+                }
+                (StakeStateV2::Uninitialized, None) => 0,
+                _ => unreachable!(),


Regarding the unreachable!() code paths, I wonder if it would be easier if you kept the shared checks at the top, matched based on the source state, and then split into three separate code paths that end independently. At the moment, all three paths are sort of handled in parallel. The alternative would be something like:

// shared validation return match source_stake_state { StakeStateV2::Stake() => { // A. Full split // B. Inactive partial split // C. Active partial split } StakeStateV2::Initialized() => { // Move lamports } StakeStateV2::Uninitialized => { // Move lamports } _ => Err(), }

thats almost how process_split() is presently structured, and i think conforming to a "match once" pattern is the major cause of problems with Split, but maybe im insufficiently imaginative

the thing is:

all three account types can be split fully or partially. in master the big match sets everything up right for code after it to move lamports and optionally dealloc the source. if we returned from the big match wed need to copy that logic in three places

inactive Stake types ought to be handled identically to Initialized. this means either we copy the inactive account logic in two places, or we bundle shared post-match validation that depends on Meta a la the present validate_split_amount() + post-that extra math in the Stake arm

my thinking is the full split case is basically the same for all account types and the partial inactive split is the same for all account types, so get them out of the way. then handling the most inherently complex case, partial split of an active stake, is readable and straightforward

Ah I see. Didn't see the previous duplication. Thanks for the extra context!

grod220 · 2025-12-09T15:32:38Z

program/src/processor.rs

+                debug_assert!(
+                    source_lamport_balance
+                        .saturating_sub(split_lamports)
+                        .saturating_sub(source_stake.delegation.stake)
+                        >= source_meta.rent_exempt_reserve
+                );


These assertions are cleaned out in a release build right? Do we usually keep them in the code or are they just for local dev (aka removed before shipping)?

yea these dont get built in release. debug_assert! is generally used sparingly, you see it in agave sometimes too. i dont think theres a standard style guideline other than "it should always be irrefutable" but i sometimes throw it in for tricky calculations like "ok i did this one way, assert it via a different route to make sure the logic is never refactored to be unsound." im also fine deleting it tho

grod220 · 2025-12-09T15:45:19Z

program/src/processor.rs

+                _ => unreachable!(),
+            }
+
+            let post_source_lamports = source_lamport_balance.saturating_sub(split_lamports);


I wonder if in general it's safer to transition most of these saturating methods to be checked versions. It's currently relying upon a downstream check of some kind. If it were checked it would raise early. What do you think?

i dont have a strong preference, i just did it this was since we would just map to the same error we return immediately below

grod220 · 2025-12-09T16:02:14Z

program/src/processor.rs

+                return Err(ProgramError::InsufficientFunds);
+            }
+
+            set_stake_state(destination_stake_account_info, &destination_stake_state)?;


Do we need to recompute to avoid delegation.stake from being out of sync (in both the source and destination)? If so, think we'd need to update it here and add some assertions in tests for delegate.stake values.

stake program historically has had the philosophy "if a stake is inactive its delegation.stake value is without meaning." the last time i talked to foundation about whether this was a ux issue was two months ago, they said theyd let me know if anyone had an issue with it in practice and havent pinged me about it since

we wouldnt want to recompute it, if we were going to change it we would probably want to reset deactivated stakes to Initialized in Split, Withdraw, and MoveLamports. but i think its debatable... the thing is that since an account can be deactivated and then not used again, and since we wouldnt want to rewrite account state if some preliminary checks fail, it is potentially a worse footgun if someone doesnt realize that stake accounts are often but not invariably reset to an Initialized state

Gotcha. With new eyes, it's hard to know a field like that is inert depending on other fields. But helpful to know it's safe.

(insert stake activation state machine pitch)

technically delegation.stake never means anything on its own. it is only in combination with activation epoch, deactivation epoch, clock, and stake history that you know whether there is any stake or not. its just that a lot of consumers play "good enough for government work" with it

grod220

The answers have been helpful 🙏

Just a few more questions.

grod220 · 2025-12-10T17:26:31Z

program/src/processor.rs

+                destination_stake_account_info,
+                split_lamports,
+            )?;
+            debug_assert!(source_stake_account_info.lamports() == 0);


Won't this assertion fail for a full split into itself?

youre right, but i agree now on just removing self-split so ill do that instead

grod220 · 2025-12-10T17:32:56Z

program/src/processor.rs

+                let is_active_or_activating =
+                    source_status.effective > 0 || source_status.activating > 0;


an aside: I wish activation status was an enum and not inferred state based on integer values of fields. We are relying upon proper handling to not end up with say effective == 0, activating == 0, deactivating > 0. But if we leveraged the type system, it would be an impossible state (aka finite state machine pattern).

yea something like this is what i was envisioning with #75, im pretty sure no one who deals with this regularly has made zero lifetime mistakes with it, myself included. i took an abortive attempt at it a couple months ago but dont remember if i ran into a serious problem or if i just had something higher priority i had to drop it for

grod220 · 2025-12-10T17:44:23Z

program/src/processor.rs

+                    0
+                }
+                (StakeStateV2::Uninitialized, None) => 0,
+                _ => unreachable!(),


Ah I see. Didn't see the previous duplication. Thanks for the extra context!

grod220 · 2025-12-10T17:56:53Z

program/src/processor.rs

+                if destination_lamport_balance < destination_rent_exempt_reserve {
+                    return Err(ProgramError::InsufficientFunds);
+                }


Post relocate_lamports() below, should we be doing this check for the source account as well?

no, it can never be violated unless DelegateStake is unsound. this is what i meant when i called this...

// minimum delegation is by definition nonzero, and we remove one delegated lamport per split lamport // since the remaining source delegation > 0, it is impossible that we took from its rent-exempt reserve debug_assert!( source_lamport_balance .saturating_sub(split_lamports) .saturating_sub(source_stake.delegation.stake) >= source_meta.rent_exempt_reserve );

...irrefutable

the check on destination rent is to ensure the account is pre-funded per what used to be feature_set::rent_exempt_split_destinations. then we take split_lamports directly from the source delegation, and enforce the delegation remains above the minimum delegation (presently 1 lamport). rent lamports, or any other undelegated lamports, never move in a partial active split

that is, the important check is this one

if source_stake.delegation.stake < minimum_delegation { return Err(StakeError::InsufficientDelegation.into()); }

grod220 · 2025-12-10T17:58:30Z

program/src/processor.rs

+            if source_stake_account_info.key != destination_stake_account_info.key {
+                source_stake_account_info.resize(0)?;
+            }


Should we just kill this? It feels more like a potential footgun hanging around.

grod220 · 2025-12-10T18:10:59Z

program/src/processor.rs

+                return Err(ProgramError::InsufficientFunds);
+            }
+
+            set_stake_state(destination_stake_account_info, &destination_stake_state)?;


Gotcha. With new eyes, it's hard to know a field like that is inert depending on other fields. But helpful to know it's safe.

(insert stake activation state machine pitch)

2501babe · 2025-12-16T10:41:09Z

@joncinque feel free to review as much or as little as you like, the main reason i added you is to sign off on banning self-split, in case there is deep lore on why it is allowed in the first place

2501babe self-assigned this Oct 2, 2025

2501babe force-pushed the split-rewrite branch from 49c9214 to 879491e Compare October 3, 2025 18:12

2501babe force-pushed the split-rewrite branch from 879491e to 4dd0e7c Compare December 8, 2025 13:07

program: rewrite Split

66bd197

2501babe force-pushed the split-rewrite branch from 4dd0e7c to 66bd197 Compare December 8, 2025 13:10

2501babe marked this pull request as ready for review December 8, 2025 13:35

2501babe requested a review from grod220 December 8, 2025 13:35

grod220 reviewed Dec 9, 2025

View reviewed changes

grod220 reviewed Dec 10, 2025

View reviewed changes

ban self-split

855fd28

grod220 approved these changes Dec 15, 2025

View reviewed changes

2501babe requested a review from joncinque December 16, 2025 10:40

		let is_active_or_activating =
		source_status.effective > 0 \|\| source_status.activating > 0;

program: rewrite process_split() #90

Are you sure you want to change the base?

program: rewrite process_split() #90

Uh oh!

Conversation

2501babe commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2501babe Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grod220 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2501babe Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2501babe commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

program: rewrite `process_split()` #90

program: rewrite `process_split()` #90

2501babe commented Oct 2, 2025 •

edited

Loading

2501babe Dec 9, 2025 •

edited

Loading

2501babe Dec 10, 2025 •

edited

Loading