-
Notifications
You must be signed in to change notification settings - Fork 179
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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`, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
419fe82
to
2870ad5
Compare
There was a problem hiding this 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.
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 |
Hm, okay - if we definitely need to do a new instruction and you've already used the |
The SIMD looks good to me. |
There was a problem hiding this 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.
We will concentrate our efforts on loader-v4 (and migrating loader-v3 programs to it). |
There was a problem hiding this 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.
Almost none, the CLI will send `ExtendProgramChecked` instead of | ||
`ExtendProgram` once the feature is active. Thus dapp developers should not | ||
notice any difference. |
There was a problem hiding this comment.
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.
Since |
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 Could you please clarify all of the points below? If so, we can head off these potential problems and find a safe upgrade path. AssumptionsFrom my understanding:
Assuming
|
@Nicola-Osec do you know what programs are self upgrading today? I've reached out to squads and everything using their program seems fine. |
10ea57c
to
ccb5e96
Compare
ccb5e96
to
0b43467
Compare
@johnsaigle, I wrote four more paragraphs in the "Impact" section, hope they cover all your questions. |
@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
Follow-up questions
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. |
That is part of loader-v4, see #167.
SIMD can be merged soon (maybe even this week), but feature activation will take months.
https://solana.com/docs/programs/deploying#loader-v3-and-loader-v4 |
There was a problem hiding this 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!
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Because of the 10KiB account size increase limit in CPI's, I think it would be better to explore other options as @joncinque mentioned |
0b43467
to
04019d2
Compare
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 |
Or maybe we do both? Add the minimum extension to the legacy instruction and this new checked instruction without the minimum? |
No description provided.