Skip to content
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

SIMD-0164: ExtendProgramChecked loader-v3 instruction #164

Closed

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 from 2d1a5ef to 419fe82 Compare August 6, 2024 11:09
@Lichtso Lichtso force-pushed the extend-program-checked branch 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.

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

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.

5 participants