program: implement stricter instruction processors #126

2501babe · 2025-10-16T19:55:16Z

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). 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

in a previous pr, we attempted to do this with two magic functions for skipping accounts. this proved to be quite obtuse and difficult to understand why one was used rather than the other in specific circumstances. in this pr, we do it by hand on an instruction-by-instruction basis, making it clear when account selection branches and why. this also makes it easier to assert positional signers in all cases, when before it was tricky because to do it would depend on accounts we had already skipped

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

joncinque · 2025-10-22T17:33:21Z

Note for onlookers: we're going to hold off on this PR for the next release, and get everything before this audited.

2501babe · 2025-10-28T20:39:00Z

added @grod220 as a secondary reviewer per the last onchain team call, since this has a lot of nuance to replicate in p-stake

grod220

This is a tricky problem! Provided some initial thoughts and perhaps an alt design to consider (if it makes sense).

grod220 · 2025-11-19T13:07:56Z

program/src/processor.rs

 // NOTE our usage of the accounts iter is idiosyncratic, in imitation of the native stake program
-// native stake typically accumulates all signers from the accounts array indiscriminately
-// each instruction processor also asserts a required number of instruction accounts
-// but this is extremely ad hoc, essentially allowing any account to act as a signing authority
-// when lengths are asserted in setup, accounts are retrieved via hardcoded index from InstructionContext
-// but after control is passed to main processing functions, they are pulled from the TransactionContext
+// native stake typically, but not always, accumulated signers from the accounts array indiscriminately
+// this essentially allowed any account to act as a stake account signing authority
+// instruction processors also asserted a required number of instruction accounts, often fewer than the actual number
+// when lengths were asserted in setup, accounts were retrieved via hardcoded index from InstructionContext
+// but after control was passed to main processing functions, they were pulled from the TransactionContext


Is it possible to get a few periods in these paragraphs? 😅

I think the combination of no capitalization + no periods is making it hard for me to tell where new clauses end/begin. Sometimes it's end of line, but sometimes not.

grod220 · 2025-11-19T13:12:01Z

program/src/processor.rs

    Ok((source_merge_kind, destination_merge_kind))
 }

 // NOTE our usage of the accounts iter is idiosyncratic, in imitation of the native stake program


Could we get a simpler word than idiosyncratic here? Also, sounds like that first part before the comma is an independent clause? Aka can be idiosyncratic.. Or am I reading that wrong and the clause continues at , but not always?

grod220 · 2025-11-19T13:45:24Z

program/src/processor.rs

+//
+// new interface signer checks may duplicate later signer hashset checks. this is intended and harmless
+// `ok()` account retrievals (lockup custodians) were, are, and will always be optional by design
 pub struct Processor {}


Any thoughts on a more top-level design such as this?

fn is_legacy_sysvar(key: &Pubkey) -> bool { key == &sysvar::clock::id() || key == &sysvar::rent::id() || key == &sysvar::stake_history::id() } fn canonicalize_accounts<'a>( accounts: &'a [AccountInfo<'a>], ) -> Vec<&'a AccountInfo<'a>> { accounts .iter() .filter(|a| !is_legacy_sysvar(a.key)) .collect() } pub fn process(program_id: &Pubkey, accounts: &[AccountInfo], data: &[u8]) -> ProgramResult { let filtered_accounts = canonicalize_accounts(accounts); match instruction { StakeInstruction::Initialize(authorize, lockup) => { Self::process_initialize(filtered_accounts, authorize, lockup) ...

This way, you wouldn't need the “invariant / diverge / converge” logic per instruction (I think).

i did that here #87 but it proved impossible to understand (see #87 (comment)), hence the approach this pr takes to explicitly handle the specific accounts each instruction expects

it would have been lighter reading if i just used next_account_to_use() (which is the same as your canonicalize_accounts() without collecting into a new vector) with no new requirements that accounts exist or be signers, but this would mean the interface gets even more lax

Helpful context, thank you 🙏

grod220 · 2025-11-19T13:49:19Z

program/src/processor.rs

+                if !withdraw_authority_info.is_signer {
+                    return Err(ProgramError::MissingRequiredSignature);
+                }


Wondering if it's better to split the new checks into it's own PR. Don't feel strongly about it though.

Realizing the signer checks is integral with the new branching logic. Can ignore 👍

grod220 · 2025-11-19T13:50:32Z

program/src/processor.rs

+        // NOTE we cannot check this account without a breaking change
+        // we may decide to enforce this if the pattern is not used on mainnet


Can you provide more context why? Some folks are passing this and others not at all?

we dont really know what people are doing (shamefacedly i confess because i dont have the mainnet node plus ledgertool experience to pull a month of stake transactions to analyze), its about what they can do

i tried to explain in the big comment but succinctly:

native stake interface was insane. most instructions would make that signers hashset, so a signer could be anywhere in the transaction. then it would somewhat arbitrarily:

take accounts by absolute position, occassionaly checking they specifically are signers but usually not

assert there must be at least n accounts. this tended to break down near the end of the list because:

it was allegedly common before the unified "wallet as authority" pattern to retain stake or token account keypairs and have them sign for themselves, so authorities may be omitted

custodians are always optional, but sometimes it would handle the pre-custodian accounts more strictly if there was an account in the custodian position

bpf stake was written to be logically equivalent

the tao of this pr is to incrementally (nonbreaking) add stricter checks under the logic that the sysvars were always strictly enforced, so the absence of a sysvar lets us know it cannot be an old format and we are free to enforce any new constraints we want. but the instructions with notes like this have no sysvars that let us do this

we are likely free to add some breaking changes in future prs for things that are technically valid but not done, after looking at mainnet traffic

someday, after the new instruction builders that will follow this version of the stake program landing on mainnet, enough of the ecosystem may be switched over that we can delete the old interface entirely

Gotcha! Wow the native stake version had some quite lax requirements compared to normal onchain program position args.

grod220 · 2025-11-19T13:59:01Z

program/src/processor.rs

+        // diverge
+        {
+            let branch_account = next_account_info(account_info_iter)?;
+            if Clock::check_id(branch_account.key) {
+                let _stake_history_info = next_account_info(account_info_iter)?;
+                let _stake_config_info = next_account_info(account_info_iter)?;
+                // let _stake_authority_info = next_account_info(account_info_iter);
+            } else {
+                let stake_authority_info = branch_account;
+                if !stake_authority_info.is_signer {
+                    return Err(ProgramError::MissingRequiredSignature);
+                }
+            }
+        };


Wonder if a code comment table may help as documentation?

// ┌───────────────────────────────┬─────────────────────────────┐ // │ Old layout │ New layout │ // ├───────────────────────────────┼─────────────────────────────┤ // │ [0] Stake │ [0] Stake │ // │ [1] Vote Account │ [1] Vote Account │ // │ [2] Clock Sysvar │ [2] Stake Authority (signer)│ // │ [3] Stake History Sysvar │ │ // │ [4] Config Account (unused) │ │ // │ [5] Stake Authority (signer) │ │ // └───────────────────────────────┴─────────────────────────────┘

grod220

Thanks for the additional context! Would be great to deprecate the old interface entirely one day. Think this approach seems a good step to get there. 👍 from me (but merge is still blocked by next release per above)

grod220 · 2025-11-21T13:45:05Z

program/src/processor.rs

+//
+// new interface signer checks may duplicate later signer hashset checks. this is intended and harmless
+// `ok()` account retrievals (lockup custodians) were, are, and will always be optional by design
 pub struct Processor {}


Helpful context, thank you 🙏

grod220 · 2025-11-21T13:46:20Z

program/src/processor.rs

+                if !withdraw_authority_info.is_signer {
+                    return Err(ProgramError::MissingRequiredSignature);
+                }


Realizing the signer checks is integral with the new branching logic. Can ignore 👍

grod220 · 2025-11-21T13:49:21Z

program/src/processor.rs

+        // NOTE we cannot check this account without a breaking change
+        // we may decide to enforce this if the pattern is not used on mainnet


Gotcha! Wow the native stake version had some quite lax requirements compared to normal onchain program position args.

joncinque

Looks great! Thanks for redoing the logic, this is much more legible.

2501babe self-assigned this Oct 16, 2025

2501babe force-pushed the gently-handle-sysvars branch from 957ee04 to 5971b45 Compare October 16, 2025 19:57

2501babe marked this pull request as draft October 16, 2025 19:58

This was referenced Oct 16, 2025

program: skip sysvars and stake config #87

Closed

remove all sysvar pubkeys from instructions interface #53

Open

2501babe marked this pull request as ready for review October 16, 2025 21:32

2501babe requested a review from joncinque October 16, 2025 21:43

2501babe force-pushed the gently-handle-sysvars branch from 1ad9ee7 to 2c19b7f Compare October 17, 2025 13:28

2501babe requested a review from grod220 October 28, 2025 20:38

2501babe added 2 commits November 14, 2025 09:45

program: implement stricter instruction processors

bb4e9d5

second pass on correctness, normalize style

1133631

2501babe force-pushed the gently-handle-sysvars branch from 2c19b7f to 1133631 Compare November 14, 2025 17:45

grod220 reviewed Nov 19, 2025

View reviewed changes

reformat interface explanation

ff88a40

grod220 approved these changes Nov 21, 2025

View reviewed changes

joncinque approved these changes Nov 24, 2025

View reviewed changes

		// NOTE we cannot check this account without a breaking change
		// we may decide to enforce this if the pattern is not used on mainnet

program: implement stricter instruction processors #126

Are you sure you want to change the base?

program: implement stricter instruction processors #126

Uh oh!

Conversation

2501babe commented Oct 16, 2025

Uh oh!

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

Uh oh!

Uh oh!

2501babe commented Oct 28, 2025

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!

2501babe Nov 20, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2501babe Nov 20, 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!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joncinque commented Oct 22, 2025 •

edited

Loading

2501babe Nov 20, 2025 •

edited

Loading

2501babe Nov 20, 2025 •

edited

Loading