Feature - `UpgradeableLoaderInstruction::ExtendProgramChecked` by Lichtso · Pull Request #6131 · anza-xyz/agave

Lichtso · 2025-05-06T23:10:21Z

Feature Gate Issue: #87

mergify · 2025-05-06T23:10:54Z

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

joncinque

Just a fly-by review, the program logic looks great to me, just some small comments on the CLI and tests

cli/src/program.rs

programs/bpf-loader-tests/tests/common.rs

joncinque

Just a couple of last things, and thanks for fixing up the tests, they look great!

cli/src/program.rs

programs/bpf-loader-tests/tests/extend_program_ix.rs

joncinque

Just the last bit from my side, then this is ready to go in!

cli/src/program.rs

joncinque

Looks like there's just some lockfile changes to commit, then this is good to go!

codecov-commenter · 2025-05-08T12:22:47Z

Codecov Report

❌ Patch coverage is 6.02410% with 234 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.8%. Comparing base (3cf7845) to head (71e26b5).
⚠️ Report is 3351 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #6131     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         843      843             
  Lines      378109   378209    +100     
=========================================
+ Hits       313134   313197     +63     
- Misses      64975    65012     +37

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joncinque

Looks great to me, thanks for addressing all the points!

buffalojoec

Looks good, just a few small items from me!

buffalojoec · 2025-05-12T10:08:54Z

programs/bpf_loader/src/lib.rs

+            if invoke_context
+                .get_feature_set()
+                .enable_extend_program_checked
+            {


It might be nice to add a log message for the developer! I guess for either one.

buffalojoec · 2025-05-12T10:24:29Z

programs/bpf_loader/src/lib.rs

+    const PROGRAM_DATA_ACCOUNT_INDEX: IndexOfAccount = 0;
+    const PROGRAM_ACCOUNT_INDEX: IndexOfAccount = 1;
+    const AUTHORITY_ACCOUNT_INDEX: IndexOfAccount = 2;
+    let optional_payer_account_index = if check_authority { 4 } else { 3 };


Some kind of note here about the system program being at index 2 or 3, based on the authority check, would be nice for other maintainers.

buffalojoec · 2025-05-12T10:27:26Z

transaction-status/src/parse_bpf_loader.rs

+                    "additionalBytes": additional_bytes,
+                    "programDataAccount": account_keys[instruction.accounts[0] as usize].to_string(),
+                    "programAccount": account_keys[instruction.accounts[1] as usize].to_string(),
+                    "authority": account_keys[instruction.accounts[2] as usize].to_string(),


I think you need a .len() > 2 check here, since the call to check_num_bpf_upgradeable_loader_accounts will only check loader v3 accounts, not the authority, which is most likely owned by System.

The extra check isn't necessary, the function is just poorly named. It checks for all accounts, not just ones owned by the loader program:

agave/transaction-status/src/parse_bpf_loader.rs

Lines 208 to 213 in a9bb1ba

fn check_num_bpf_upgradeable_loader_accounts(

accounts: &[u8],

num: usize,

) -> Result<(), ParseInstructionError> {

check_num_accounts(accounts, num, ParsableProgram::BpfUpgradeableLoader)

}

cli/src/program.rs

buffalojoec

Nice!

mergify · 2025-05-13T13:58:30Z

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

* Bumps solana-loader-v3-interface version. * Adds the feature. * Adds feature gating logic. * Adjusts transaction-status parser. * Adjusts CLI manual extend and auto extend. * Adds authority_signer_index to ProgramCliCommand::ExtendProgramChecked. * Adjusts solana-bpf-loader-program-tests. (cherry picked from commit 94d70cd) # Conflicts: # Cargo.lock # feature-set/src/lib.rs # genesis/Cargo.toml # programs/bpf-loader-tests/tests/extend_program_ix.rs # programs/sbf/Cargo.lock # programs/sbf/Cargo.toml # runtime/Cargo.toml # svm-feature-set/src/lib.rs # svm/Cargo.toml # svm/examples/Cargo.lock # transaction-status/Cargo.toml

* Bumps solana-loader-v3-interface version. * Adds the feature. * Adds feature gating logic. * Adjusts transaction-status parser. * Adjusts CLI manual extend and auto extend. * Adds authority_signer_index to ProgramCliCommand::ExtendProgramChecked. * Adjusts solana-bpf-loader-program-tests. (cherry picked from commit 94d70cd)

…(backport of #6131) (#6217) Feature - `UpgradeableLoaderInstruction::ExtendProgramChecked` (#6131) * Bumps solana-loader-v3-interface version. * Adds the feature. * Adds feature gating logic. * Adjusts transaction-status parser. * Adjusts CLI manual extend and auto extend. * Adds authority_signer_index to ProgramCliCommand::ExtendProgramChecked. * Adjusts solana-bpf-loader-program-tests. (cherry picked from commit 94d70cd) Co-authored-by: Alexander Meißner <AlexanderMeissner@gmx.net>

Lichtso force-pushed the feature/extend_program_checked branch 6 times, most recently from e93ec72 to 435b3c6 Compare May 6, 2025 23:52

joncinque reviewed May 7, 2025

View reviewed changes

cli/src/program.rs Show resolved Hide resolved

programs/bpf-loader-tests/tests/common.rs Outdated Show resolved Hide resolved

Lichtso force-pushed the feature/extend_program_checked branch 3 times, most recently from 8e7ef1f to c79ac7a Compare May 7, 2025 14:00

joncinque reviewed May 7, 2025

View reviewed changes

cli/src/program.rs Outdated Show resolved Hide resolved

cli/src/program.rs Show resolved Hide resolved

programs/bpf-loader-tests/tests/extend_program_ix.rs Outdated Show resolved Hide resolved

Lichtso force-pushed the feature/extend_program_checked branch from c79ac7a to baba6ec Compare May 8, 2025 08:56

joncinque reviewed May 8, 2025

View reviewed changes

cli/src/program.rs Outdated Show resolved Hide resolved

Lichtso force-pushed the feature/extend_program_checked branch 2 times, most recently from edf3150 to 6f70d23 Compare May 8, 2025 10:29

joncinque reviewed May 8, 2025

View reviewed changes

Lichtso force-pushed the feature/extend_program_checked branch 3 times, most recently from 042ef6f to f64f03c Compare May 8, 2025 11:28

joncinque previously approved these changes May 8, 2025

View reviewed changes

Lichtso marked this pull request as ready for review May 12, 2025 09:03

Lichtso requested a review from a team as a code owner May 12, 2025 09:03

buffalojoec reviewed May 12, 2025

View reviewed changes

Lichtso dismissed joncinque’s stale review via dae745e May 12, 2025 15:53

Lichtso force-pushed the feature/extend_program_checked branch from dae745e to 019ec1d Compare May 12, 2025 16:23

buffalojoec previously approved these changes May 13, 2025

View reviewed changes

Lichtso dismissed buffalojoec’s stale review via eb3e328 May 13, 2025 08:46

Lichtso force-pushed the feature/extend_program_checked branch from 019ec1d to eb3e328 Compare May 13, 2025 08:46

Lichtso force-pushed the feature/extend_program_checked branch 2 times, most recently from 9f721b9 to fc57f4a Compare May 13, 2025 09:59

Lichtso added 7 commits May 13, 2025 10:29

Bumps solana-loader-v3-interface version.

cef2dd2

Adds the feature.

8a5ed8c

Adds feature gating logic.

7e7a6ed

Adjusts transaction-status parser.

d5122cf

Adjusts CLI manual extend and auto extend.

d68ea04

Adds authority_signer_index to ProgramCliCommand::ExtendProgramChecked.

393678b

Adjusts solana-bpf-loader-program-tests.

71e26b5

Lichtso force-pushed the feature/extend_program_checked branch from fc57f4a to 71e26b5 Compare May 13, 2025 10:30

joncinque approved these changes May 13, 2025

View reviewed changes

buffalojoec approved these changes May 13, 2025

View reviewed changes

Lichtso merged commit 94d70cd into anza-xyz:master May 13, 2025
58 checks passed

Lichtso deleted the feature/extend_program_checked branch May 13, 2025 12:01

Lichtso added the v2.2 label May 13, 2025

mergify bot mentioned this pull request May 13, 2025

v2.2: Feature - UpgradeableLoaderInstruction::ExtendProgramChecked (backport of #6131) #6217

Merged

Lichtso added the feature-gate label May 13, 2025

	fn check_num_bpf_upgradeable_loader_accounts(
	accounts: &[u8],
	num: usize,
	) -> Result<(), ParseInstructionError> {
	check_num_accounts(accounts, num, ParsableProgram::BpfUpgradeableLoader)
	}

Conversation

Lichtso commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented May 6, 2025

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joncinque left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

buffalojoec left a comment

Choose a reason for hiding this comment

Uh oh!

buffalojoec May 12, 2025

Choose a reason for hiding this comment

Uh oh!

buffalojoec May 12, 2025

Choose a reason for hiding this comment

Uh oh!

buffalojoec May 12, 2025

Choose a reason for hiding this comment

Uh oh!

joncinque May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

buffalojoec left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mergify bot commented May 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Lichtso commented May 6, 2025 •

edited

Loading

joncinque left a comment •

edited

Loading

codecov-commenter commented May 8, 2025 •

edited

Loading

joncinque May 12, 2025 •

edited

Loading