SIMD-0430: Loader V3: Relax Program Buffer Constraints

[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

[Lichtso]

This should probably specify that we would still expect the layout (specifically the ELF starting offset in the account) to be the same, even if the supplied buffer was not created by loader-v3.

[Lichtso]

Overall looking good. I think the wording would benefit from distinguishing whether the SIMD is talking about initial deployments (Deploy), redeployments (Upgrade) or both in the various places these are mentioned.

[Lichtso]

This should specify that it only removes the InstructionError::IncorrectAuthority check of the authority matching the program, not the InstructionError::MissingRequiredSignature check of the authority being a signer.

Both happen right next to one another:
https://github.com/anza-xyz/agave/blob/ec8ba132a605eee519effa7c7ea2bdc1bc55b151/programs/bpf_loader/src/lib.rs#L534-L541

[buffalojoec]

@Lichtso is correct - we can definitely make this proposal more clear.

I wrote up a draft implementation here: anza-xyz/agave#9854

You can see the feature changes only the IncorrectAuthority error case, not the missing signature.
https://github.com/buffalojoec/solana/blob/bd7db804ac3cbd72276d4edac45c2b4ab62ffda3/programs/bpf_loader/src/lib.rs#L534-L541
https://github.com/buffalojoec/solana/blob/bd7db804ac3cbd72276d4edac45c2b4ab62ffda3/programs/bpf_loader/src/lib.rs#L709-L716

I left some comments about two things. For starters, I think it would be really good to mention that the signature is stil required, obviously since Upgrade needs to match it to the program's upgrade authority but also for DeployWithMaxDataLen as a sanity check for the to-be upgrade authority.

The second thing is that we can't actually remove the ownership check, since both instructions will modify the buffer account, and the program can't do that if the buffer is not owned by Loader V3. We can either keep the ownership check (as per my comment) or change the behavior to not modify buffer accounts, so they can be reused.
https://github.com/anza-xyz/agave/blob/7b6683976906e298e869c680a58f14035c114b06/programs/bpf_loader/src/lib.rs#L578-L584
https://github.com/anza-xyz/agave/blob/7b6683976906e298e869c680a58f14035c114b06/programs/bpf_loader/src/lib.rs#L823-L834

[buffalojoec]

@deanmlittle one suggestion for the ownership check is to only partially relax it.

We could add an argument to both deployment instructions that tells Loader V3 what to do with the buffer account.

enum LoaderV3Instruction {
    DeployWithMaxDataLen { reuse_buffer: bool },
    Upgrade { reuse_buffer: bool },
}

• reuse_buffer=true:
  • No signature required.
  • No ownership check required.
  • Buffer account is not modified.
• reuse_buffer=false:
  • Signature of the buffer authority is required.
  • Buffer must by owned by Loader V3.
  • Buffer account is drained according to existing behavior.

---

Personally I like more imperative arguments, but we'd have to treat the absence of that byte as false, which is a bit uglier, but doable. Obviously the control flow would be reversed.

enum LoaderV3Instruction {
    DeployWithMaxDataLen { close_buffer: bool },
    Upgrade { close_buffer: bool },
}

[deanmlittle]

Personally I like more imperative arguments, but we'd have to treat the absence of that byte as false, which is a bit uglier, but doable. Obviously the control flow would be reversed.

I also prefer the idea of close_buffer as the additional argument. I think it is actually fine to change the default behavior to not close the buffer by default (in the program). The worse case scenario here would be that program deploys temporarily take up more rent for users who don't upgrade their CLI which is in no way a dealbreaker imo. As for the default behavior in the CLI, I am ambivalent about that. @jacobcreech do you have any thoughts on this?

[buffalojoec]

The worse case scenario here would be that program deploys temporarily take up more rent for users who don't upgrade their CLI which is in no way a dealbreaker imo.

The CLI can be updated ASAP to try to mitigate this. We can add the extra byte to pass close_buffer=true by default before the feature even goes live.

for instruction in [
    UpgradeableLoaderInstruction::DeployWithMaxDataLen { max_data_len: 1000 },
    UpgradeableLoaderInstruction::Upgrade,
] {
    let mut data = bincode::serialize(&instruction).unwrap();
    let data_len_original = data.len();
    
    // Add trailing bytes.
    data.extend_from_slice(&[0xDE, 0xAD, 0xBE, 0xEF]);
    let data_len_with_trailing = data.len();

    // Deserialize.
    let result: UpgradeableLoaderInstruction =
        limited_deserialize(&data, solana_packet::PACKET_DATA_SIZE as u64)
        .unwrap();

    assert_eq!(
        result,
        instruction,
        "Deserialized instruction should match original"
    );

    assert_eq!(
        data_len_with_trailing,
        data_len_original + 4,
    );
}

[mjain-jump]

lgtm!

[Lichtso]

I also prefer the idea of close_buffer as the additional argument. I think it is actually fine to change the default behavior to not close the buffer by default (in the program). The worse case scenario here would be that program deploys temporarily take up more rent for users who don't upgrade their CLI which is in no way a dealbreaker imo. As for the default behavior in the CLI, I am ambivalent about that.

I agree with @dean that closing the buffer does not need to be part of Upgrade, the accidental omission is easily corrected. Furthermore, this begs the question: Why even add the option to close the buffer as a parameter to the instruction interface? We can add an explicit Close of the buffer in the same TX in the CLI.

Since we split the authorities of program and buffer in this SIMD anyway, this also opens up work flows in which the buffer is recycled in a different TX by a different key pair altogether. In that sense separating Upgrade and Close buffer is a natural consequence.

[simd-bot]

Thanks, topointon-jump!

⚠️ Status: Cannot merge yet

• Missing approval from: Anza (t-nelson, sakridge, or bw-solana)

[buffalojoec]

I agree with @dean that closing the buffer does not need to be part of Upgrade, the accidental omission is easily corrected. Furthermore, this begs the question: Why even add the option to close the buffer as a parameter to the instruction interface? We can add an explicit Close of the buffer in the same TX in the CLI.

Since we split the authorities of program and buffer in this SIMD anyway, this also opens up work flows in which the buffer is recycled in a different TX by a different key pair altogether. In that sense separating Upgrade and Close buffer is a natural consequence.

I'm hesitant to add more changes to this proposal. The goal for this and the other Loader V3 SIMDs was to keep the change spread to a minimum and minimize disruptions to developers.

You're correct that we could include a Close instruction in the same transaction and that would preserve existing CLI workflows. However, removing automatic closure in Upgrade would disrupt anyone who does not use the CLI - such as CPI-based workflows.

I would personally rather stick to the original goal and focus on the wins we want to see, rather than add breaking changes to obtain modularity. Save that for Loader V4 IMO.

[deanmlittle]

I also prefer the idea of close_buffer as the additional argument. I think it is actually fine to change the default behavior to not close the buffer by default (in the program). The worse case scenario here would be that program deploys temporarily take up more rent for users who don't upgrade their CLI which is in no way a dealbreaker imo. As for the default behavior in the CLI, I am ambivalent about that.

I agree with @dean that closing the buffer does not need to be part of Upgrade, the accidental omission is easily corrected. Furthermore, this begs the question: Why even add the option to close the buffer as a parameter to the instruction interface? We can add an explicit Close of the buffer in the same TX in the CLI.

Since we split the authorities of program and buffer in this SIMD anyway, this also opens up work flows in which the buffer is recycled in a different TX by a different key pair altogether. In that sense separating Upgrade and Close buffer is a natural consequence.

If we were designing from first principles with a blank slate, I would totally agree with you! The reality today is that separating them entirely in the current loader has a few undesirable outcomes:

1. It would cease to be backwards -compatible with expected behavior in existing CPI/CLI workflows.

2. Splitting out the Close instruction means that, although overall cost is the same, upfront capital required for a deployment is actually doubled as you cannot directly reuse the lamports from closing the buffer to deploy the program.

While these may not lead to catastrophic failure, they do represent major pain points for developers today and thus lead to undesirable outcomes.

For these two reasons, in the context trying to achieve an easy win of added flexibility without breaking existing functionality, I believe the current design is more suitable.