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.

Sign up for GitHub

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

Jump to bottom

Bumps solana-sbpf to v0.9.0 #3830

Merged
merged 3 commits into from
Dec 17, 2024
Merged

Bumps solana-sbpf to v0.9.0 #3830

merged 3 commits into from
Dec 17, 2024

Conversation

Lichtso
Copy link

@Lichtso Lichtso commented Nov 28, 2024 •
edited
Loading

Problem

solana-sbpf v0.9.0 was released.

Summary of Changes

  • Renames solana_rbpf => solana-sbpf
  • Renames disable_sbpf_v1_execution => disable_sbpf_v0_execution
  • Renames reenable_sbpf_v1_execution => reenable_sbpf_v0_execution
  • Adds feature gate enable_sbpf_v1_deployment_and_execution
  • Adds feature gate enable_sbpf_v2_deployment_and_execution
  • Adds feature gate enable_sbpf_v3_deployment_and_execution
  • Defines the dense keys for the syscall registry

Not included in this PR

  • Adopt the newest toolchain release
  • Build all program tests with the newest SBPF version
  • Only accept the newest SBPF version in re/deployments, reject all older versions

@mergify Mergify
Copy link

mergify bot commented Nov 28, 2024

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.

@Lichtso Lichtso force-pushed the rbpf/v0.9.0 branch 3 times, most recently from 2c0e201 to a9c8f59 Compare November 28, 2024 14:14
@0x0ece 0x0ece mentioned this pull request Nov 29, 2024
Merged
@Lichtso Lichtso force-pushed the rbpf/v0.9.0 branch 2 times, most recently from ed52de2 to 9e667e9 Compare December 2, 2024 11:29
@Lichtso Lichtso force-pushed the rbpf/v0.9.0 branch 2 times, most recently from bd28e00 to 8bed666 Compare December 9, 2024 16:35
@Lichtso Lichtso mentioned this pull request Dec 12, 2024
Open
@Lichtso Lichtso force-pushed the rbpf/v0.9.0 branch 2 times, most recently from 4161b3a to 690c82a Compare December 13, 2024 18:33
@Lichtso Lichtso changed the title Bumps solana_rbpf to v0.9.0 Bumps solana-sbpf to v0.9.0 Dec 13, 2024
@Lichtso Lichtso force-pushed the rbpf/v0.9.0 branch 2 times, most recently from e7a4940 to 32909f0 Compare December 13, 2024 18:58
@Lichtso Lichtso marked this pull request as ready for review December 13, 2024 19:10
@Lichtso Lichtso requested a review from a team as a code owner December 13, 2024 19:10
@Lichtso Lichtso requested a review from LucasSte December 13, 2024 19:10
LucasSte
LucasSte reviewed Dec 16, 2024
View reviewed changes
programs/bpf_loader/test_elfs/out/sbpfv0_verifier_err.so Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we have a test for each one of the SBPF versions? At some point, we'll have all four of them running online.

I believe that at least such a test should exist for the program in programs/sbf.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, however currently these test binaries are manually constructed. We can revisit this topic once we have actual toolchain support for all these SBPF versions.

@Lichtso Lichtso force-pushed the rbpf/v0.9.0 branch 4 times, most recently from 1a4b7f2 to d1b8459 Compare December 16, 2024 20:17
LucasSte
LucasSte reviewed Dec 16, 2024
View reviewed changes
program-runtime/src/invoke_context.rs Outdated
@@ -555,13 +555,13 @@ impl<'a> InvokeContext<'a> {
// For now, only built-ins are invoked from here, so the VM and its Config are irrelevant.
let mock_config = Config::default();
let empty_memory_mapping =
MemoryMapping::new(Vec::new(), &mock_config, &SBPFVersion::V1).unwrap();
MemoryMapping::new(Vec::new(), &mock_config, SBPFVersion::V1).unwrap();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be V0?

programs/bpf_loader/src/syscalls/mod.rs
SBPFVersion::V1
} else {
SBPFVersion::V0
};
Copy link

@LucasSte LucasSte Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a debug assert here to prevent min_sbpf_version from being greater than the max_sbpf_version.

I know we plan on enabling enable_sbpf_v3_deployment_and_execution before activating disable_sbpf_v0_execution, but any case different than that should error out (at least in debug mode).

LucasSte
LucasSte previously approved these changes Dec 16, 2024
View reviewed changes
Lichtso
Lichtso commented Dec 17, 2024
View reviewed changes
runtime/src/bank/tests.rs
@@ -12183,7 +12182,7 @@ fn test_feature_activation_loaded_programs_cache_preparation_phase() {
result_with_feature_enabled,
Err(TransactionError::InstructionError(
0,
InstructionError::InvalidAccountData
InstructionError::UnsupportedProgramId
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling all features with let mut feature_set = FeatureSet::all_enabled(); did also activate remove_accounts_executable_flag_checks which changes the error message here.

@Lichtso Lichtso merged commit 54a73ce into anza-xyz:master Dec 17, 2024
52 checks passed
@Lichtso Lichtso deleted the rbpf/v0.9.0 branch December 17, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LucasSte LucasSte LucasSte approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Lichtso @LucasSte