program: skip sysvars and stake config #87

2501babe · 2025-10-01T17:50:31Z

first half of #53. this updates the program to gracefully handle the old instruction builders (sysvars are passed in) and the expected new ones (they will not be). it does this by skipping them whenever they appear in the accounts list

we also take this opportunity to enforce that authorities must be present, and must be signers, for instructions that previously had sysvars before the technically optional authority

also updated some comments to reflect the fact that the program is now live on mainnet

joncinque · 2025-10-06T14:28:53Z

Given the future port to upstream BPF / Pinocchio, I'm wondering if it's still worth making this change. Since all of the Pinocchio deserializers are zero-copy, I believe it's cheaper to take in the sysvar by account rather using the syscall to get the data.

We could give people the option -- if the account is present, deserialize from that, otherwise use the syscall. @febo do you have any thoughts about the best approach here?

febo · 2025-10-06T14:50:47Z

Given the future port to upstream BPF / Pinocchio, I'm wondering if it's still worth making this change. Since all of the Pinocchio deserializers are zero-copy, I believe it's cheaper to take in the sysvar by account rather using the syscall to get the data.

We could give people the option -- if the account is present, deserialize from that, otherwise use the syscall. @febo do you have any thoughts about the best approach here?

Agree, I think having the option of passing accounts is good. Anyone that needs to save transaction size can choose not to pass accounts and "pay" for the syscall; similarly, passing sysvar accounts save CUs (when using zero-copy) by avoiding the syscall.

2501babe · 2025-10-07T08:41:59Z

i was thinking about it from the point of view of simplifying the interface, since the cost is small (a few hundred) and would be neglible (probably exactly 110) if we switch the sysvar syscall backends to all go through sol_get_sysvar. also i thought we had been planning to move away from passing sysvar accounts entirely at some point?

if we think the cost of Rent::get() and Clock::get() are material now i would rather leave the current ad-hoc usage in place, letting the accounts be optional would just make the interface even more complicated

2501babe · 2025-10-08T20:39:18Z

per our call we are going to move forward with this, but i put it in draft because i realized i can do something better to tighten the interface now, will put it back in for review after

2501babe · 2025-10-12T20:32:52Z

program/src/processor.rs

+// this unusual function exists to enforce stricter constraints on the new interface without affecting the old
+// we call this where the old interface expected sysvar/config followed by an authority it did not assert must be present
+// thus if we do *not* see sysvar/config, we *can* assert authority, knowing that there being *no* account is always an error
+// doing it this way is non-breaking, and we can consider breaking the old interface after people switch over
+// we return () to prevent refactors where the caller uses the result, because it might not be the desired account
+fn consume_next_account<'a, 'b: 'a, I: Iterator<Item = &'a AccountInfo<'b>>>(
+    iter: &mut I,
+) -> Result<(), ProgramError> {
+    let account_info = iter.next().ok_or(ProgramError::NotEnoughAccountKeys)?;
+    if is_stake_program_sysvar_or_config(*account_info.key) || account_info.is_signer {
+        Ok(())
+    } else {
+        Err(ProgramError::NotEnoughAccountKeys)
+    }
+}


not sure if this comment is clear enough because its a bit tricky and makes sense in my head. the idea is if we have an instruction like:

vote

stake

clock

authority (native didnt enforce this exists at all, and bpf emulated native exactly)

we can safely do:

vote

stake

authority (must be present, must be signer)

because if clock isnt there we know we are using our new instruction builders, and clock being there in the old instructions means we never have a valid case where there is no account. so we can gracefully add the constraint because there is always some account there

maybe a better way of putting it is, by doing this all at once we support an old 3 or 4 account case, and a new 3 account case, not creating more debt for ourselves by ever allowing a 2 account case in between

feel free to tell me im overthinking this but what im imagining is a process maybe over the next couple years like:

check Split and SetLockup transactions on mainnet to see if people actually omit the last account, if not enforce it as a breaking update

someday if/when everything is using the sysvar-less instruction builders, delete the sysvar-skipping code and enforce the accounts as defined by the new interface

get rid of the signers hashmaps and move to a fully positional program interface like a typical bpf program

its possible there are nonupgradeable cpi callers of stake, in which case we would never get to do this, but i think at least consume_next_account() moves us in the right direction

This makes sense to me overall! I know we discussed not wanting to break this during the BPF transition, so now seems like a good time to break.

Would you mind adding the first part of your comment here as a code comment? Specifically:

the idea is if we have an instruction like: 1. vote 2. stake 3. clock 4. authority (native didnt enforce this exists at all, and bpf emulated native exactly) we can safely do: 1. vote 2. stake 3. authority (must be present, must be signer) because if clock isnt there we know we are using our new instruction builders, and clock being there in the old instructions means we never have a valid case where there is no account. so we can gracefully add the constraint because there is always some account there

…_next_normal_account

joncinque

Great work overall! This is probably the best way to make this kind of sensitive change. Just some minor comments

joncinque · 2025-10-14T11:15:05Z

program/src/processor.rs

+// this unusual function exists to enforce stricter constraints on the new interface without affecting the old
+// we call this where the old interface expected sysvar/config followed by an authority it did not assert must be present
+// thus if we do *not* see sysvar/config, we *can* assert authority, knowing that there being *no* account is always an error
+// doing it this way is non-breaking, and we can consider breaking the old interface after people switch over
+// we return () to prevent refactors where the caller uses the result, because it might not be the desired account
+fn consume_next_account<'a, 'b: 'a, I: Iterator<Item = &'a AccountInfo<'b>>>(
+    iter: &mut I,
+) -> Result<(), ProgramError> {
+    let account_info = iter.next().ok_or(ProgramError::NotEnoughAccountKeys)?;
+    if is_stake_program_sysvar_or_config(*account_info.key) || account_info.is_signer {
+        Ok(())
+    } else {
+        Err(ProgramError::NotEnoughAccountKeys)
+    }
+}


This makes sense to me overall! I know we discussed not wanting to break this during the BPF transition, so now seems like a good time to break.

Would you mind adding the first part of your comment here as a code comment? Specifically:

the idea is if we have an instruction like: 1. vote 2. stake 3. clock 4. authority (native didnt enforce this exists at all, and bpf emulated native exactly) we can safely do: 1. vote 2. stake 3. authority (must be present, must be signer) because if clock isnt there we know we are using our new instruction builders, and clock being there in the old instructions means we never have a valid case where there is no account. so we can gracefully add the constraint because there is always some account there

joncinque · 2025-10-14T11:23:23Z

program/src/processor.rs

+        // required accounts
+        let stake_account_info = next_account_to_use(account_info_iter)?;


I'm not 100% sure about this, but how about adding a comment for additional accounts that might be present? I'm worried about cleanup in the future that accidentally removes these.

On the other hand, is the idea to do another breaking change in the future where we replace next_account_to_use with next_account_info everywhere? And in the meantime, completely prohibit next_account_info? If so, then I'm fine with the code as is

yes, i removed all uses of next_account_info(). ideally after we release the instruction builder update, everything migrates, and we have one program interface where we go to next_account_info() and sysvars are an error. less ideally if there are important frozen programs or lagging third parties, we leave it like this

joncinque · 2025-10-14T11:24:33Z

program/src/processor.rs

-        let _stake_or_withdraw_authority_info = next_account_info(account_info_iter)?;
+        // required accounts
+        let stake_account_info = next_account_to_use(account_info_iter)?;
+        let _stake_or_withdraw_authority_info = next_account_to_use(account_info_iter)?;


Shouldn't this be consume_next_account to do the signer check?

it cant be consume_next_account(), we need next_account_to_use() to call next() twice (skip clock and grab authority)

whether to enforce this is a signer is an interesting question though... in this pr, i only used consume_next_account() for accounts we never checked must be there at all. in native, this corresponds to the asserts it did like "grab account, grab sysvar, assert length of accounts is 2, there must be an authority somewhere but stop here and rely on the signers hashset." in bpf, it means commented-out accounts like in Merge. the pattern here in Authorize, in native, continues "grab another account, assert length of accounts is 3, dont check if signer rely on hashet". we cant use either new utility function as-is to assert authority here is a signer because we would have to know if the previous next_account_to_use() skipped a clock or not

however the comments make me think in my attempt to simplify the interface for callers i have actually made it too confusing for maintainers. i think i have to rethink my approach to this

Ah sorry you're right, this might just be me being dense. I wrongly thought consume_next_account also did a loop.

Up to you how you want to do it!

joncinque · 2025-10-14T11:30:28Z

program/src/processor.rs

+// thus if we do *not* see sysvar/config, we *can* assert authority, knowing that there being *no* account is always an error
+// doing it this way is non-breaking, and we can consider breaking the old interface after people switch over
+// we return () to prevent refactors where the caller uses the result, because it might not be the desired account
+fn consume_next_account<'a, 'b: 'a, I: Iterator<Item = &'a AccountInfo<'b>>>(


nit: maybe we can name this a bit more clearly. How about consume_sysvar_account_or_signer?

hmm i feel like that would imply it doesnt consume the account in other cases. next() is unconditional and then the behavior difference is ok/err

joncinque · 2025-10-14T11:31:38Z

program/src/processor.rs


        // other accounts
-        // let _stake_authority_info = next_account_info(account_info_iter);
+        let _stake_authority_info = consume_next_account(account_info_iter)?;


nit: consume_next_account doesn't return an account info, so we should either change it to return the account info or change callers to not assign a named value

joncinque · 2025-10-14T11:36:18Z

program/src/processor.rs

+        // NOTE we cannot consume this account without a breaking change
+        // its presence was never enforced and `Split` never accepted sysvars as args
+        // we may decide to enforce this as a breaking change if the pattern is not used on mainnet
+        // let _stake_authority_info = next_account_to_use(account_info_iter)?;


We could go halfway and say "if this is account is present, it must be a signer". What do you think?

joncinque · 2025-10-14T11:37:19Z

program/src/processor.rs

+        // NOTE we cannot consume this account without a breaking change
+        // its presence was never enforced and `SetLockup` never accepted sysvars as args
+        // we may decide to enforce this as a breaking change if the pattern is not used on mainnet
+        // let _old_withdraw_or_lockup_authority_info = next_account_to_use(account_info_iter)?;


Same with this one, how about we enforce "if it's present, it must be a signer"

2501babe · 2025-10-16T20:29:52Z

doing this in #126 instead

2501babe force-pushed the ignore-sysvars branch from bfe5f39 to 59839ec Compare October 1, 2025 17:55

2501babe self-assigned this Oct 2, 2025

2501babe marked this pull request as ready for review October 2, 2025 09:40

2501babe requested a review from joncinque October 2, 2025 09:40

2501babe mentioned this pull request Oct 2, 2025

program: rewrite process_split() #90

Draft

2501babe force-pushed the ignore-sysvars branch from 4f1629f to 2d53d1d Compare October 3, 2025 18:14

2501babe force-pushed the ignore-sysvars branch 2 times, most recently from a2e1d6d to fd859ba Compare October 8, 2025 20:30

2501babe marked this pull request as draft October 8, 2025 20:35

2501babe force-pushed the ignore-sysvars branch from da2907f to 4f8fca7 Compare October 10, 2025 11:03

2501babe commented Oct 12, 2025

View reviewed changes

2501babe marked this pull request as ready for review October 13, 2025 10:35

2501babe mentioned this pull request Oct 13, 2025

remove all sysvar pubkeys from instructions interface #53

Open

2501babe added 9 commits October 14, 2025 03:25

program: skip sysvars and stake config

c7b0a52

rename next_non_sysvar_account to next_account_to_use and add consume…

2b62d2f

…_next_normal_account

test the new guard behavior

b5b4dab

minor cleanup

4e8f0f2

this may not be so normal after all!

ec5b167

flew too close to the sun

b098ab2

actually im so smart

9fa0ca2

dammit we have to skip this one too

e295905

ok im done

f9be148

2501babe force-pushed the ignore-sysvars branch from 2945f85 to f9be148 Compare October 14, 2025 10:26

joncinque reviewed Oct 14, 2025

View reviewed changes

2501babe marked this pull request as draft October 16, 2025 17:23

2501babe closed this Oct 16, 2025

2501babe deleted the ignore-sysvars branch October 16, 2025 20:30

		// required accounts
		let stake_account_info = next_account_to_use(account_info_iter)?;

program: skip sysvars and stake config #87

program: skip sysvars and stake config #87

Uh oh!

Conversation

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

Uh oh!

Uh oh!

joncinque commented Oct 6, 2025

Uh oh!

febo commented Oct 6, 2025

Uh oh!

2501babe commented Oct 7, 2025

Uh oh!

2501babe commented Oct 8, 2025

Uh oh!

2501babe Oct 12, 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!

joncinque 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2501babe commented Oct 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

2501babe commented Oct 1, 2025 •

edited

Loading

2501babe Oct 12, 2025 •

edited

Loading