Skip to content

Remove SVM dependency on agave-feature-set #5841

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.

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

Merged
merged 3 commits into from
Apr 18, 2025

Conversation

pgarg66
Copy link

@pgarg66 pgarg66 commented Apr 16, 2025

Problem

SVM and runtime currently depends on agave-feature-set. Not all of the features are relevant for SVM. Also, this is causing a tight dependency between SVM and monorepo. It'll be ideal if the SVM features could be configured when SVM is instantiated.

Summary of Changes

Add a new config struct in SVM, that gets initialized/configured as part of SVM instantiation.

Fixes #

Copy link

mergify bot commented Apr 16, 2025

If this PR represents a change to the public RPC API:

  1. Make sure it includes a complementary update to rpc-client/ (example)
  2. Open a follow-up PR to update the JavaScript client @solana/web3.js (example)

Thank you for keeping the RPC clients in sync with the server API @pgarg66.

Copy link

mergify bot commented Apr 16, 2025

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.

@pgarg66 pgarg66 force-pushed the runtime-features branch 3 times, most recently from 00b6ddb to e4af604 Compare April 16, 2025 15:47
@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 96.15385% with 14 lines in your changes missing coverage. Please review.

Project coverage is 83.0%. Comparing base (7bb8e2d) to head (7281edb).
Report is 8 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #5841    +/-   ##
========================================
  Coverage    83.0%    83.0%            
========================================
  Files         828      829     +1     
  Lines      375826   375994   +168     
========================================
+ Hits       311963   312246   +283     
+ Misses      63863    63748   -115     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pgarg66 pgarg66 marked this pull request as ready for review April 16, 2025 16:42
@pgarg66 pgarg66 requested a review from a team as a code owner April 16, 2025 16:42
@pgarg66 pgarg66 requested review from LucasSte and buffalojoec April 16, 2025 16:43
Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Nice work! Overall, I think the concept of a separate feature set for SVM over a callback is a very nice change. Let's see if we can just tidy it up a bit and reduce bloat.

@@ -147,15 +225,15 @@ pub struct EnvironmentConfig<'a> {
pub blockhash: Hash,
pub blockhash_lamports_per_signature: u64,
epoch_stake_callback: &'a dyn InvokeContextCallback,
pub feature_set: Arc<FeatureSet>,
feature_set: Arc<RuntimeFeatures>,

Choose a reason for hiding this comment

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

Any chance we can take this opportunity to get rid of the Arc? I can't think of anywhere we actually need a thread-safe atomic reference to a set of Option<Slot>. The feature set should be frozen from the time SVM is instantiated to to the time it's torn down.

Copy link
Author

Choose a reason for hiding this comment

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

I was keeping this change for a follow on PR, as it'll increase the size of this PR. Let me see how much additional changes it'll cause. If it's reasonable, I will update it here.

Choose a reason for hiding this comment

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

Ok, follow-up PR is also okay, but I definitely think we should take the opportunity to eliminate it while our attention is here.


pub fn runtime_features(&self) -> RuntimeFeatures {
RuntimeFeatures {
lift_cpi_caller_restriction: self.activated_slot(&lift_cpi_caller_restriction::id()),

Choose a reason for hiding this comment

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

This seems like a lot of repeated code. Do you think we can abstract this initialization in a macro? The struct members are named the same as the features.

Copy link
Author

Choose a reason for hiding this comment

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

It'll remove the possibility of naming the feature ID with a different name than what we call in SVM repo. It's easier to call it the same since the features already exist. But, when SVM is a separate repo, it might be extra work to enforce it.

We can probably add a macro in a separate PR, so if we don't like the rigidity in the future we can just revert that PR.

@pgarg66 pgarg66 force-pushed the runtime-features branch 2 times, most recently from ea83949 to d2c9f8c Compare April 17, 2025 20:51
@pgarg66
Copy link
Author

pgarg66 commented Apr 17, 2025

Hey @yihau, could you please reserve solana-svm-feature-set crate (newly created in this PR)?

@pgarg66 pgarg66 force-pushed the runtime-features branch 4 times, most recently from 36c5660 to 7dc5b17 Compare April 17, 2025 21:26
@pgarg66 pgarg66 merged commit bb5a6e7 into anza-xyz:master Apr 18, 2025
41 checks passed
@pgarg66 pgarg66 deleted the runtime-features branch April 18, 2025 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants