Skip to content

Conversation

@pgarg66
Copy link

@pgarg66 pgarg66 commented Apr 10, 2025

Problem

stake_raise_minimum_delegation_to_1_sol feature will not be used as part of the builtin stake program. But, this is causing a dependency on InvokeContext::get_feature_set(). This dependency needs to be removed as part of SVM repo migration.

Summary of Changes

Remove the feature usage from the builtin stake program.

Fixes #

@mergify
Copy link

mergify bot commented Apr 10, 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.

@mergify
Copy link

mergify bot commented Apr 10, 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.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.9%. Comparing base (0d19c55) to head (4d2c5a8).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #5754     +/-   ##
=========================================
- Coverage    82.9%    82.9%   -0.1%     
=========================================
  Files         826      826             
  Lines      375390   375207    -183     
=========================================
- Hits       311525   311273    -252     
- Misses      63865    63934     +69     
🚀 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 10, 2025 22:54
@pgarg66 pgarg66 requested a review from brooksprumo April 10, 2025 22:54
Comment on lines -28 to -34
pub fn get_minimum_delegation(feature_set: &FeatureSet) -> u64 {
if feature_set.is_active(&feature_set::stake_raise_minimum_delegation_to_1_sol::id()) {
const MINIMUM_DELEGATION_SOL: u64 = 1;
MINIMUM_DELEGATION_SOL * LAMPORTS_PER_SOL
} else {
1
}

Choose a reason for hiding this comment

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

So if we merge this PR, there will be a chance that testnet halts. Since this feature gate is active, if some nodes run this version and some run the previous version, then they could disagree on if a new stake delegation succeeds or not.

Can you link to the SVM code that needs to remove the feature set lookup? I dunno if there's a chicken-and-egg problem here. I.e. we can't (shouldn't?) remove the code here without the bpf stake program going live first, but we also can't have the bpf stake program go live without removing the code here.

We may say it's OK to have testnet halt for the sake of simplicity, but I'm not sure who should make that call. Otherwise we'd need another feature gate to flip only testnet back to 1 lamport.

Copy link
Author

Choose a reason for hiding this comment

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

We want to remove FeatureSet from SVM. So, this function will need to be deleted.

pub fn get_feature_set(&self) -> &FeatureSet {

This is being called from a few builtin programs (including stake program) to check if a feature has been activated.

Is this particular feature on hold for MNB? If so, would it be deactivated on testnet?

Choose a reason for hiding this comment

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

Yeah, stake_raise_minimum_delegation_to_1_sol isn't going to be activated on mnb in its current form. Trent suggested over in Discord that we deactivate this feature from testnet on the next testnet cluster restart. That'll allow us to then delete the code here.

Copy link
Author

Choose a reason for hiding this comment

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

This PR will not be backported. So it won't reach testnet anytime soon. Should we merge it on master? Or, would you rather wait for the feature to be deactivated before merging it?

Choose a reason for hiding this comment

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

We still run edge builds against testnet though. If it were me, I'd wait until the feature was deactivated before merging.

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.

3 participants