Skip to content

deps: disable default features for solana-sbpf in solana-transaction-context crate#7575

Merged
Lichtso merged 3 commits intoanza-xyz:masterfrom
dfinity:lpahlavi/default-sbpf-features-in-transaction-context
Aug 19, 2025
Merged

deps: disable default features for solana-sbpf in solana-transaction-context crate#7575
Lichtso merged 3 commits intoanza-xyz:masterfrom
dfinity:lpahlavi/default-sbpf-features-in-transaction-context

Conversation

@lpahlavi
Copy link
Copy Markdown

@lpahlavi lpahlavi commented Aug 18, 2025

Problem

The solana-sbpf dependency in the solana-transaction-context crate (added in #5871) implicitly enables the default features which include the jit feature of solana-sbpf and thus a transitive dependency to the rand and getrandom crates. This also extends to e.g. solana-transaction-status-client-types which depends on solana-transaction-context.

Summary of Changes

Set default-features = false for the solana-sbpf dependency at the workspace level, and explicitly enable the jit feature all crates using solana-sbpf except the solana-transaction-context crate.

Fixes #

@lpahlavi lpahlavi requested a review from a team as a code owner August 18, 2025 15:56
@0xbrw 0xbrw added the CI Pull Request is ready to enter CI label Aug 18, 2025
@lpahlavi
Copy link
Copy Markdown
Author

@joncinque I discovered this issue while trying out the changes merged (and later reverted) in #7492. This is unfortunately a pretty big deal for us and would be amazing if we could include this in the upcoming 3.0.0 releases!

@anza-team anza-team removed the CI Pull Request is ready to enter CI label Aug 18, 2025
Lichtso
Lichtso previously approved these changes Aug 18, 2025
@mergify mergify Bot requested a review from a team August 18, 2025 16:30
@lpahlavi lpahlavi requested a review from Lichtso August 18, 2025 16:31
@lpahlavi
Copy link
Copy Markdown
Author

lpahlavi commented Aug 18, 2025

@Lichtso Thank you so much for the review! It seems my first solution didn't quite work and I actually have to set default-features = false at the workspace level for this to work. I've now verified that I indeed don't have any unwanted transitive dependencies on getrandom if I just add solana-transaction-status-client-types as a dependency.

For now, I've just enabled the jit feature in all crates except solana-transaction-context. I could also set default-features = true in those other crates but I thought it was more explicit like this. One optimization here would be to check which crates actually use the jit feature, but I'm not quite sure if there is an easy way to do this. WDYT?

Edit: it seems I can compile fine with jit disabled for all crates except agave-syscalls but I don't have enough domain knowledge here to accurately determine if that's OK.

@Lichtso
Copy link
Copy Markdown

Lichtso commented Aug 18, 2025

How exactly are you consuming the solana-transaction-context crate? Because all feature flags within a workspace are consolidated before the build. So, I think as long as you build within the workspace you will always have the jit feature enabled.

@lpahlavi
Copy link
Copy Markdown
Author

lpahlavi commented Aug 18, 2025

I have a separate project, which depends on some crates from this repository and of which only solana-transaction-status-client-types depends (transitively) on solana-sbpf through solana-transaction-context.

Because all feature flags within a workspace are consolidated before the build.

IIUC you are correct that feature flags are consolidated, however only crates being compiled are considered.

So, I think as long as you build within the workspace you will always have the jit feature enabled.

Since I am only building crates where the jit is not enabled, the feature is disabled during compilation.

Does this make sense?

@Lichtso Lichtso added the CI Pull Request is ready to enter CI label Aug 18, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Aug 18, 2025
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.4%. Comparing base (0aed0c5) to head (3b2c35b).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7575   +/-   ##
=======================================
  Coverage    83.4%    83.4%           
=======================================
  Files         812      812           
  Lines      365102   365102           
=======================================
+ Hits       304650   304682   +32     
+ Misses      60452    60420   -32     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread program-test/Cargo.toml Outdated
solana-rent = { workspace = true }
solana-runtime = { workspace = true }
solana-sbpf = { workspace = true }
solana-sbpf = { workspace = true, features = ["jit"] }
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Think this crate might not need it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done!

@lpahlavi lpahlavi requested a review from Lichtso August 18, 2025 20:08
@lpahlavi
Copy link
Copy Markdown
Author

Could I please get a CI tag? (cc @Lichtso @0xbrw)

@joncinque joncinque added the CI Pull Request is ready to enter CI label Aug 19, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Aug 19, 2025
@lpahlavi
Copy link
Copy Markdown
Author

@Lichtso I guess this is ready to be merged 🎉

@Lichtso Lichtso merged commit 9d8bf06 into anza-xyz:master Aug 19, 2025
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants