Skip to content

SIMD-0164: ExtendProgramChecked loader-v3 instruction #164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Aug 5, 2024

No description provided.

Copy link

@topointon-jump topointon-jump left a comment

Choose a reason for hiding this comment

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

Looks good, would be great to just add a bit more detail. Thanks so much :)

stable), thus a new instruction needs to be added, similar to `SetAuthority`
and `SetAuthorityChecked`.

Adds `ExtendProgramChecked` which is exactly the same as `ExtendProgram`,

Choose a reason for hiding this comment

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

Could we describe:

  • The discriminant of the new instruction.
  • The data structure of the new instruction.
  • What the new instruction will do - does it just call the ExtendProgram logic?

I know it's identical to ExtendProgram with some extra checks, but SIMD's should be able to be implemented without looking at the Agave source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a description of the interface of the new instruction. However, as we don't have a complete specification defining the exact behavior of the entire instruction is out-of-scope for this PR, in my opinion. I will gladly do so for the upcoming loader-v4 as that is more symmetric.

@Lichtso Lichtso force-pushed the extend-program-checked branch 2 times, most recently from 419fe82 to 2870ad5 Compare August 6, 2024 11:12
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

This proposal looks good to me. I'm just a bit hung up on the new instruction's name.

We use the *Checked suffix across programs in SPL to typically imply that there's some kind of check being applied on the account data. Since the original ExtendProgram does this already (finalized check), naming the new one ExtendProgramChecked implies that there's been an additive check to the program's account data. However, this is not the case. It expects a signature now.

Is it possible to just change the instruction itself, since the feature gate you've proposed would disable the old one and enable the new one? Specifically, can't we add a feature gate to ExtendProgram that, once activated, requires this signer and performs the check?

This would be akin to the reverse of the Relax Authority feature gate for the Lookup Table builtin.

@Lichtso
Copy link
Contributor Author

Lichtso commented Aug 8, 2024

We can't modify / add stuff to existing instructions. That would break block explorer instruction parsing and the CLI tools.

The reason the new instruction is named Checked is because it adds an authority check, the same way we extended SetAuthority to SetAuthorityChecked.

@buffalojoec
Copy link
Contributor

We can't modify / add stuff to existing instructions. That would break block explorer instruction parsing and the CLI tools.

The reason the new instruction is named Checked is because it adds an authority check, the same way we extended SetAuthority to SetAuthorityChecked.

Hm, okay - if we definitely need to do a new instruction and you've already used the *Checked suffix for an authority check before, then I'm good with it.

buffalojoec
buffalojoec previously approved these changes Aug 8, 2024
@smcio
Copy link

smcio commented Oct 8, 2024

The SIMD looks good to me.

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.

Note that for tools like multisigs (squads, etc) and DAOs (realms, etc), this constitutes a breaking change, since the upgrade authority is typically a PDA, and those programs give limited control over what instructions they will sign with the DAO / multisig's PDA.

Before, it was very easy to just increase the size of the program permissionlessly if needed on a new deployment, and stick with the old process for upgrading the program. With this change, they'll need to check the size and optionally add the new instruction to extend whenever needed.

As another option, is it possible to simply add a lower bound for the extension? For example, just require that a program must be extended by at least 10k bytes.

@Lichtso
Copy link
Contributor Author

Lichtso commented Dec 30, 2024

We will concentrate our efforts on loader-v4 (and migrating loader-v3 programs to it). That combined with the concerns raised here made me retract this proposal.

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 good to me! I've come around since my last comment, and think that this is the best solution to the problem.

Comment on lines 73 to 75
Almost none, the CLI will send `ExtendProgramChecked` instead of
`ExtendProgram` once the feature is active. Thus dapp developers should not
notice any difference.
Copy link
Contributor

Choose a reason for hiding this comment

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

People using older versions of the CLI and external teams using the ExtendProgram instruction directly will be broken by the change, and need to be updated to use the new one. It might be worth specifying that impact.

@Nicola-Osec
Copy link

Since ExtendProgramChecked is being implemented to replace ExtendProgram, it's important that this instruction is also allowed in CPI. As pointed out by @joncinque some programs rely on PDAs to (self-)upgrade, and these programs will need to be updated to invoke the new instruction in CPI with their PDA authority.

@johnsaigle
Copy link

johnsaigle commented May 14, 2025

Hi, I'd like to get some clarification on the case where a program is relying on PDAs and CPI for upgrading ('self-upgrade'), building on the points raised by @Nicola-Osec and @joncinque.

It seems that the current state of the new ExtendProgramChecked instruction poses a serious operational risk
to existing and future deployments of self-upgrading programs. I don't believe the Impact section here
takes this problem into account.

Could you please clarify all of the points below? If so, we can head off these potential problems and find a safe upgrade path.

Assumptions

From my understanding:

  1. ExtendProgram will be replaced by ExtendProgramChecked, which is feature-gated.
  2. The new instruction requires another account -- the upgrade authority -- in order to extend the program size.

Assuming ExtendProgram is NOT accessible via CPI:

  1. Already deployed 'self-upgrading' programs will be unable to call ExtendProgram after the release.
  2. Any upgrades post-release would need to be done within the bounds of whatever allocations the programs already have.
  3. The programs would need a new entrypoint to call ExtendProgramChecked with an additional account (?)
  4. New self-upgrading programs must pre-emptively over-allocate the program size to account for potential increases in size in future upgrades.

If projects are unaware of these constraints, their programs may effectively become un-upgradeable post-patch without warning.

Assuming ExtendProgram IS accessible via CPI:

I see the new PR anza-xyz/agave#6245 that allows for CPI on the new Extend Program Checked
instruction. This should help, but in any case I wanted to clarify the plan for the release that includes this feature.

  • What is required for existing programs using the self-upgrade pattern?
  • Do self-upgrading programs need new code to handle the new ExtendProgramChecked instructions?
  • Is the intended upgrade path in this case to first call ExtendProgramChecked and then proceed as normally with the upgrade?

Over-allocations, truncation, and refunds

Assuming a program needs to over-allocate as described above:

  • what is the intended path for projects to be able to reallocate/truncate the excess size post-patch?
    which address has the authority to reallocate/truncate?
  • which address does the refunded rent go to?
    • For a self-upgrading PDA set-up, the funds may be transferred to a PDA which may not have any ability to transfer lamports out.

@jacobcreech
Copy link
Contributor

some programs rely on PDAs to (self-)upgrade, and these programs will need to be updated to invoke the new instruction in CPI with their PDA authority.

@Nicola-Osec do you know what programs are self upgrading today? I've reached out to squads and everything using their program seems fine.

@Lichtso Lichtso force-pushed the extend-program-checked branch 3 times, most recently from 10ea57c to ccb5e96 Compare May 15, 2025 11:53
@Lichtso Lichtso force-pushed the extend-program-checked branch from ccb5e96 to 0b43467 Compare May 15, 2025 11:57
@Lichtso
Copy link
Contributor Author

Lichtso commented May 15, 2025

@johnsaigle, I wrote four more paragraphs in the "Impact" section, hope they cover all your questions.

@johnsaigle
Copy link

johnsaigle commented May 15, 2025

@Lichtso Thanks for providing more information about the above. That did address many of my points.

Here's my understanding based on the new post:

Summary of changes

  • Existing self-upgrading programs must upgrade to support several new instructions. (ExtendProgramChecked, Migrate, and the new program management instructions.)
  • If they do not do this before the simd is applied, they will be permanently unable to extend their program length.
  • New self-upgrading programs must support ExtendProgramChecked and the program management instructions, or else they will be permanently unable to extend their program length.
  • To mitigate this, these programs could be extended now if they have the money to spend on rent. This will allow them to upgrade.
  • It will be possible for programs to truncate and reclaim the rent, and this can be sent to an arbitrary address.

Follow-up questions

  • Is the new truncate/refund instruction part of the 'program management' instructions you mentioned?

  • Do you have a sense of a rough deadline for when this change would be merged? A resolution of weeks is fine if there's no precise date. If the changes go through as planned, teams will be required to scramble to write, test, and deploy patches to support the instructions new instructions. Having an idea of the timeline would help here.

  • Are you considering publicizing this patch and providing some instructions to assist with the upgrade and new deployments?

Given the above, I still feel that the description is under-stating the impact to existing projects. It seems that all projects using the self-upgrade pattern need to scramble to perform the upgrades/extensions ASAP. Otherwise, they face the very serious consequence that their programs will be unable to be upgraded.


@jacobcreech FWIW I know several major programs using this pattern. I'm trying to advocate for them by posting here.

@Lichtso
Copy link
Contributor Author

Lichtso commented Jul 1, 2025

Is the new truncate/refund instruction part of the 'program management' instructions you mentioned?

That is part of loader-v4, see #167.

Do you have a sense of a rough deadline for when this change would be merged? A resolution of weeks is fine if there's no precise date. If the changes go through as planned, teams will be required to scramble to write, test, and deploy patches to support the instructions new instructions. Having an idea of the timeline would help here.

SIMD can be merged soon (maybe even this week), but feature activation will take months.

Are you considering publicizing this patch and providing some instructions to assist with the upgrade and new deployments?

https://solana.com/docs/programs/deploying#loader-v3-and-loader-v4

joncinque
joncinque previously approved these changes Jul 1, 2025
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 good to me!

Comment on lines 75 to 77
does require an upgrade authority anyway. Thus the additional requirement of a
upgrade authority should not impose any new requirements on the extend-upgrade-
workflow.
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole reason the extend instruction was permissionless + non-cpi-able in the first place was because cpi instructions can only extend the programdata by 10KiB at a time. However, I don't see that discussed here in the impact section at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, added it to the section.

@jstarry
Copy link
Contributor

jstarry commented Jul 7, 2025

Because of the 10KiB account size increase limit in CPI's, I think it would be better to explore other options as @joncinque mentioned

@jstarry
Copy link
Contributor

jstarry commented Jul 31, 2025

I think it's better to go with a minimum extension length. That way the extend instruction doesn't need to be deprecated and self upgrading programs don't need to migrate

@jstarry
Copy link
Contributor

jstarry commented Jul 31, 2025

Or maybe we do both? Add the minimum extension to the legacy instruction and this new checked instruction without the minimum?

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.

9 participants