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

There's an earlier check that throws if split_lamports > source_lamport_balance, so I would go a step further and make this source_lamport_balance.checked_sub(split_lamports).unwrap() since we know for sure this is ok

tbh i like that the program code has zero unwrap()s...

edit: as noted below this is now checked_sub()

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

Are the debug_asserts ever even used? Since program builds are optimized, they would only ever get run if someone calls process_split or process directly in a test, which I don't see in any of the tests, since they either use program-test or mollusk.

If that's the case, I would prefer to add these asserts directly to the tests

oh yes, i was thinking about tests being debug but there is no debug build for sbf

3859a55 there isnt really a way to test this since it can never happen, but i changed the tricky debug_assert! to a normal error because i like having the refactor safety of it. i removed the one this comment points to, its trivially true because we are in a if split_lamports == source_lamport_balance { ... } block

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

joncinque

Sorry for the slow review, but this looks great! I'm a big fan of the more linear flow with less nesting.

Mostly small questions and nits, nothing showstopping

joncinque · 2026-01-08T19:07:43Z

program/src/processor.rs

+        if source_stake_account_info.key == destination_stake_account_info.key {
+            return Err(ProgramError::InvalidArgument);


I can't think of a reason of doing this, I'm happy to kill it

joncinque · 2026-01-09T23:42:33Z

program/src/processor.rs

+            return Err(ProgramError::InsufficientFunds);
+        }
+
+        let source_rent_exempt_reserve = rent.minimum_balance(source_stake_account_info.data_len());


Sorry, I'm pretty sure I've asked before, but I can't remember the answer: are we sure that all stake accounts have the correct meta.rent_exempt_reserve? The old code was using the field instead. If all stake accounts are correct, then great! I'm happy to start soft-deprecating rent_exempt_reserve

we take it for granted that meta.rent_exempt_reserve is always correct, i just do it this way because source can be Uninitialized. rereading the version in master, i think this condition was never checked for that case, allowing a split to leave an uninitialized source below rent exemption and letting the instruction die on validator postchecks

1135c6d however i noticed i only need this value in the partial inactive block so i moved it inside there

joncinque · 2026-01-10T01:24:15Z

program/src/processor.rs

+        // this prevents state changes in exceptional cases where a once-valid source has become invalid
+        // relocate lamports, copy data, and close the original account
+        if split_lamports == source_lamport_balance {
+            let mut destination_stake_state = source_stake_state;


nit: since StakeStateV2 is Copy, this performs a copy, even though source_stake_state isn't used afterwards for this branch. I don't feel that strongly about it though, so it's probably something we can optimize later by just getting a mut reference to source_stake_state. Does it matter enough to warrant a comment?

mm i think im happy leaving it as-is, a 200 byte memset isnt a big deal compared to the cleanliness of letting source_stake_state be immutable

joncinque · 2026-01-10T01:36:54Z

program/src/processor.rs

+            let mut destination_stake_state = source_stake_state;
+            let delegation = match (&mut destination_stake_state, option_dest_meta) {
+                (StakeStateV2::Stake(meta, stake, _), Some(dest_meta)) => {
+                    *meta = dest_meta;


If I'm understanding this correctly, option_dest_meta just copies source_meta and updates rent_exempt_reserve, and then this code is copying the modified meta back into source_stake_state.

So if rent_exempt_reserve is the same (and correct) on all accounts, this part shouldn't be necessary. Do I have that right?

correct, option_dest_meta is the source meta with the destination rent exemption, which we write into the new destination stake state. this exists to preserve the logic in master...

let mut destination_meta = source_meta; destination_meta.rent_exempt_reserve = validated_split_info.destination_rent_exempt_reserve;

...which i interpreted as a mechanism to allow splitting from a StakeStateV2 to a hypothetical larger StakeStateV3. theres an assert in a test test_split_rent_exemptness() from old monorepo that requires it

joncinque · 2026-01-10T01:43:24Z

program/src/processor.rs

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


Are the debug_asserts ever even used? Since program builds are optimized, they would only ever get run if someone calls process_split or process directly in a test, which I don't see in any of the tests, since they either use program-test or mollusk.

If that's the case, I would prefer to add these asserts directly to the tests

joncinque · 2026-01-10T01:49:43Z

program/src/processor.rs

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


There's an earlier check that throws if split_lamports > source_lamport_balance, so I would go a step further and make this source_lamport_balance.checked_sub(split_lamports).unwrap() since we know for sure this is ok

joncinque · 2026-01-10T01:51:43Z

program/src/processor.rs

+            let post_destination_lamports =
+                destination_lamport_balance.saturating_add(split_lamports);


nit: since split_lamports is checked earlier, this can't overflow, but let's do checked_add (with unwrap if you want) for this one too

joncinque · 2026-01-10T02:09:03Z

program/src/processor.rs

+                // 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


If you end up removing the debug assert, can you keep this comment? All the safety hinges on it 😅

as noted above i kept this as a normal error now

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 previously approved these changes Dec 15, 2025

View reviewed changes

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

joncinque reviewed Jan 10, 2026

View reviewed changes

remove debug asserts

3859a55

2501babe dismissed grod220’s stale review via 3859a55 January 13, 2026 11:10

2501babe added 2 commits January 13, 2026 03:29

move source rent into block

1135c6d

use checked math

22b892c

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

		if source_stake_account_info.key == destination_stake_account_info.key {
		return Err(ProgramError::InvalidArgument);

		let post_destination_lamports =
		destination_lamport_balance.saturating_add(split_lamports);

		// 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

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 Jan 13, 2026 • 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!

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!

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!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

program: rewrite `process_split()` #90

program: rewrite `process_split()` #90

2501babe commented Oct 2, 2025 •

edited

Loading

2501babe Jan 13, 2026 •

edited

Loading

2501babe Dec 9, 2025 •

edited

Loading

2501babe Dec 10, 2025 •

edited

Loading