SIMD-0431: Loader V3: Minimum Extend Program Size

[deanmlittle]

No description provided.

[simd-bot]

Hello deanmlittle! Welcome to the SIMD process. By opening this PR you are affirming that your SIMD has been thoroughly discussed and vetted in the SIMD discussion section. The SIMD PR section should only be used to submit a final technical specification for review. If your design / idea still needs discussion, please close this PR and create a new discussion here.

This PR requires the following approvals before it can be merged:

• At least one approval from a Anza team member: t-nelson, sakridge, or bw-solana
• At least one approval from an Firedancer team member: ptaffet-jump, topointon-jump, 0x0ece, lidatong, or ripatel-fd

Once all requirements are met, you can merge this PR by commenting /merge.

[buffalojoec]

Lgtm

[deanmlittle]

@0xRigel requesting your review as this pertains to multisig wallets like Squads. I know we discussed at breakpoint but worth double checking you don't foresee any issues. I think it is actually as simple as throwing a lighthouse instruction at the end to ensure the authority doesn't change.

[0xRigel]

Whats the current reason for ExtendProgram not supporting inner instructions? If we could get rid of that while simultaneously adding this permissioned check, that would be fab.

Aside from that, this looks good. We'll just have to make a few adjusments to the UI's under the hood logic for extend program.

[deanmlittle]

Whats the current reason for ExtendProgram not supporting inner instructions? If we could get rid of that while simultaneously adding this permissioned check, that would be fab.

Aside from that, this looks good. We'll just have to make a few adjusments to the UI's under the hood logic for extend program.

ACK, thanks for reviewing! There are a many dumb restrictions on LoaderV3 due to security issues that can arise as a result of problematic account midstates created during the upgrade process. Hopefully we can ship a few more fixes that would allow us to relax these constraints in the future!

[buffalojoec]

I think this proposal could actually use a bit of fleshing out for context.

I've written a draft implementation here: anza-xyz/agave#9859

---

The first major issue I found is that we'd actually need to break the
ExtendProgram instruction's accounts list beyond just requiring a signature.
The authority is actually not a member of the required accounts for
ExtendProgram. We would need to add it.

https://github.com/anza-xyz/solana-sdk/blob/7b95685a9c3ddc4312d8522618cf4c1595accb0c/loader-v3-interface/src/instruction.rs#L150-L156

For this reason, I'm actually considering SIMD-0164 as a preferred solution. We
would introduce a new instruction with a new accounts list and deprecate the
old one.

Since callers will have to modify their code anyway in order to support the
ExtendProgram instruction after this feature, are we certain changing the
existing instruction is cleaner? It seems like maybe not.

---

Additionally, this proposal can probably add more details about some of the
deliberation that occurred in SIMD-0164. After all, it's going to be the exact
same design, just a different rollout strategy.

There was a bit of discussion about how this change affects multisigs. However,
in SIMD-0164 and also here in this proposal (via ack by @0xRigel) these
concerns were curtailed. We can probably at least mention how this affects
multisigs in one of the lower sections of the SIMD.

@Lichtso also added context to SIMD-0164 about already-deployed, possibly
frozen programs. This proposal will have the same issues.

We probably have to add that ExtendProgram will be accessible via CPI as a
result of this proposal's feature activation. If we don't, self-upgrading
programs won't be able to invoke ExtendProgram via CPI with their PDA
signers.

[deanmlittle]

For this reason, I'm actually considering SIMD-0164 as a preferred solution. We
would introduce a new instruction with a new accounts list and deprecate the
old one.

I am of the opinion that simply modifying the existing instruction makes more sense for several reasons:

• We don't want the current behavior to exist in the first place, so there is no need to preserve it
• Maintaining two instructions in the enum just to display an error message in one of them is, in my opinion, less sensible than simply introducing an error message into the existing instruction in the case that the upgrade authority is not a signer
• There are two main happy paths here, one being the feepayer already being the authority, and the other being Squads handling it all for you in their frontend. As such I think the chances of users actually hitting this error are already relatively low

Thus 164 is actually doing more work to achieve the same outcome. For that reason, I do not think 164 is a better solution.

[Lichtso]

We don't want the current behavior to exist in the first place, so there is no need to preserve it
Maintaining two instructions in the enum

I re-read SIMD-0164 any yes you are right, it does not mention the old one being disabled, but that would be my intention. Can update that SIMD.

introducing an error message into the existing instruction in the case that the upgrade authority is not a signer

It is not about displaying error messages but preventing unauthorized access from others. Currently ExtendProgram is completely lacking the [signer] authority parameter, that is the issue that got SIMD-0164 started.

https://github.com/anza-xyz/solana-sdk/blob/b0f5666fa6ccd99c47f656d6d19ac16bf8d2030f/loader-v3-interface/src/instruction.rs#L151-L156

[deanmlittle]

I believe I have addressed all outstanding feedback. It is really just a matter of making the decision of whether we go with #164 or #431. As we would be throwing a missing signer error either way, I find no justification to pollute our Enum with an additional instruction when we can simply modify the existing one to achieve the same result.

[buffalojoec]

I am of the opinion that simply modifying the existing instruction makes more sense for several reasons:

• We don't want the current behavior to exist in the first place, so there is no need to preserve it
• Maintaining two instructions in the enum just to display an error message in one of them is, in my opinion, less sensible than simply introducing an error message into the existing instruction in the case that the upgrade authority is not a signer
• There are two main happy paths here, one being the feepayer already being the authority, and the other being Squads handling it all for you in their frontend. As such I think the chances of users actually hitting this error are already relatively low

Thus 164 is actually doing more work to achieve the same outcome. For that reason, I do not think 164 is a better solution.

OK, I can appreciate these points. I did some digging, and it seems like we've used both methods, but it depends on what we've historically done with the previous wire format.

For example, with System's CreateAccountPrefunded and Token's InitializeMint2, we introduced new versions since we planned to support their previous versions. Here we do not wish to do this.

With ALT, we relaxed signer requirements for CreateLookupTable, but did not modify the instruction accounts list. That's because the wire format was still backwards compatible. We know both options (SIMD-0164 or this one) for ExtendProgram will not be backwards compatible.

So, since we don't wish to support the old version (as @deanmlittle said above) and we will break the program's interface either way (as I said in the second block of this comment), this proposal does in fact appear to be cleaner to me.

[mjain-jump]

Suggested change

	top-level signer and reclaim it in the same transaction. The `ExtendProgram`
	top-level signer and extend it in the same transaction. The `ExtendProgram`

think this is what you meant here?

[buffalojoec]

That line means reclaim the authority after extending at top-level with the temporary authority.

[Lichtso]

The only reason why Extend was ever separated from Upgrade was because it was explicitly designed to be permission-less as I understood it. Idea being one would first Extend on top-level and then Upgrade in CPI.

If we make it permissioned self upgrading programs would have to use the transient (but atomic) authority shuffle pattern to sign Extend and/or Upgrade on the top-level. Thus, if we go with an entirely permissioned workflow, there is no use-case for a stand-alone Extend anymore. Extend can implicitly be part of Upgrade.

In that sense neither SIMD-0431 nor SIMD-0164 are needed, we can just disable Extend altogether.

[deanmlittle]

The only reason why Extend was ever separated from Upgrade was because it was explicitly designed to be permission-less as I understood it. Idea being one would first Extend on top-level and then Upgrade in CPI.

If we make it permissioned self upgrading programs would have to use the transient (but atomic) authority shuffle pattern to sign Extend and/or Upgrade on the top-level. Thus, if we go with an entirely permissioned workflow, there is no use-case for a stand-alone Extend anymore. Extend can implicitly be part of Upgrade.

In that sense neither SIMD-0431 nor SIMD-0164 are needed, we can just disable Extend altogether.

The authority shuffle is very clunky, and I agree with you that if we do have to shuffle, why not just combine this into upgrade directly? Upon reevaluating our options, I think I actually have an even better solution to this. I think we should instead:

1. Remove cache invalidation on Extend.
2. Add a check to inspect InvokeContext for an Upgrade instruction immediately proceeding the Extend instruction for the same program account.

By doing so, Extend can remain "permissionless" as it is today, so none of our workflows change, but it can piggyback on the safety of the Extend instruction for the same program, handling for permissions and cache invalidation without ever having to change the authority. I believe this is a much more elegant solution.

[Lichtso]

Add a check to inspect InvokeContext for an Upgrade instruction immediately proceeding the Extend instruction for the same program account.

Only top-level instructions are known ahead of time. For self upgrading programs the Upgrade is in CPI, thus unpredictable. We would need some other mechanism to make sure every Extend is followed by an Upgrade of the same program in the same transaction. Essentially a transient program state, which only exists during the transaction.

By then it becomes very similar to the retract-modify-redeploy-workflow in the early designs of loader-v4. With the difference being that loader-v4 explicitly persisted the retracted state in the program account, while this dirty flag here would be implicit and only exist during the transaction.

[deanmlittle]

Only top-level instructions are known ahead of time. For self upgrading programs the Upgrade is in CPI, thus unpredictable. We would need some other mechanism to make sure every Extend is followed by an Upgrade of the same program in the same transaction. Essentially a transient program state, which only exists during the transaction.

Ahh yeah, you're right. Though I suppose we could go the other way and have the upgrade instruction introspect for the Extend? I'll give this some more thought. It would be great to avoid the authority shuffle if possible.

[buffalojoec]

@Lichtso we've reworked this proposal to use a minimum extension length, as proposed by jstarry here on SIMD-0164. Please give this another review, and if it's acceptable by all parties, we'll update dependent SIMDs like SIMD-0430 and SIMD-0433.

[Lichtso]

I am still in favor of "Disable ExtendProgram entirely", as I think that being able to manipulate other peoples programs at all is fishy. However, in the interest of getting this fixed, I will approve this simpler yet potentially incomplete solution.

[buffalojoec]

I am still in favor of "Disable ExtendProgram entirely", as I think that being able to manipulate other peoples programs at all is fishy. However, in the interest of getting this fixed, I will approve this simpler yet potentially incomplete solution.

Yeah, disabling permissionless resizing of program accounts would be the most complete fix, but at the cost of breaking many long-existing workflows for self-upgrading programs and multisig upgrades. I documented this out in the Alternatives Considered section.

[bw-solana]

Does this mean ExtendProgramChecked will be removed from Loader v3 ixs? Might be nice to restate that explicitly in the summary

Edit: I see SIMD-0164 was never actually approved but it's in the code ☹️

[deanmlittle]

many such cases 🙃

[buffalojoec]

Yeah, merging and implementing this effectively nulls that SIMD anyway, even though it was closed out.
If this lands, we can land my two PRs to clean up ExtendProgramChecked:
anza-xyz/solana-sdk#656
anza-xyz/agave#11685

[bw-solana]

do we need to specify precedence of errors?

[buffalojoec]

Technically no, since the specific codes are not relevant to consensus, and it gives us a bit of flexibility for program performance improvements.

[bw-solana]

outside the scope of this SIMD, but if the account is overfunded for whatever reason, can anyone extend the size and "lock up" that SOL?

[buffalojoec]

Yep, but I'm about to update SIMD-0430 and SIMD-0433 so that the program owner can just resize it down to the correct length and they'll get all of the attacker's funds. So it's a non-issue IMO.

[bw-solana]

might want to update tools to enforce this minimum resize up the stack

[deanmlittle]

while not relevant to consensus, that's true. will add a comment about this.

[buffalojoec]

I don't think we need to add anything to the proposal for non-protocol tooling. I try to avoid doing so.