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.

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

Add reserved_account_keys module to sdk #84

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

jstarry
Copy link

@jstarry jstarry commented Mar 5, 2024

Problem

No way to introduce new reserved account keys which cannot be write locked by transactions.

Summary of Changes

Add a ReservedAccountKeys module to sdk which facilitates adding feature-gated reserved account keys like new sysvars and new enshrined programs.

Note that the new module is not currently used anywhere. It's added here on its own to facilitate faster reviews over smaller patchsets of code. To see the full changes, look here: solana-labs#34901

Feature Gate Issue: #540

@jstarry jstarry added the feature-gate Pull Request adds or modifies a runtime feature gate label Mar 5, 2024
Copy link
Author

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

@CriesofCarrots @t-nelson could you please take another look? Note that this PR illustrates more clearly how this module will be used: solana-labs#34901

Comment on lines 26 to 29
impl ReservedAccountKeys {
/// Compute a set of all reserved keys, regardless of if they are active or not
pub fn active_and_inactive() -> HashSet<Pubkey> {
RESERVED_ACCOUNT_KEYS
Copy link
Author

Choose a reason for hiding this comment

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

is the Messages referencing this list not the primary bug here? doesn't seem like something we'd want to constrain the solution by. that is, we should invert the test such that this list doesn't need to be public and can be totally encapsulated in runtime/bank

@t-nelson this list should not be used by the runtime but is very useful for other areas of the codebase like ledger-tool, rpc, snapshot minimizer, cli, etc.

The runtime should only be using ReservedAccountKeys::active

Comment on lines 29 to 32
RESERVED_ACCOUNT_KEYS
.iter()
.map(|reserved_key| reserved_key.key)
.collect()
Copy link
Author

Choose a reason for hiding this comment

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

instead of the global Vec, HashMap<Pubkey, Option>. we get the HashSet-like accessors for free. no allocations other than passing the Arc on to each successor bank

@t-nelson yes, it could be implemented that way as well but it's not as convenient. The end consumer of this reserved account keys list only wants to know if a key is reserved or not. This is what the usage of the HashSet looks like in Message:

 pub fn is_writable(&self, i: usize, reserved_account_keys: &HashSet<Pubkey>) -> bool {
        (self.is_writable_index(i))
            && !reserved_account_keys.contains(&self.account_keys[i])
            && !self.demote_program_id(i)
    }
    ```

@jstarry jstarry force-pushed the feat/reserved-accounts-mod branch from 0c98be2 to 34f90e6 Compare March 5, 2024 09:22
@jstarry
Copy link
Author

jstarry commented Mar 7, 2024

@t-nelson @CriesofCarrots when you get the time can you take another look at this?

@jstarry
Copy link
Author

jstarry commented Mar 8, 2024

@t-nelson @CriesofCarrots still waiting on a review here :)


/// Move inactive reserved account keys to the active set if their feature
/// is active.
pub fn update_active_set(&mut self, feature_set: &FeatureSet) {
Copy link
Author

Choose a reason for hiding this comment

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

@CriesofCarrots this is the new method to add new reserved account keys that are activated by the feature set. It's used like this now in the bank:

        // Update active set of reserved account keys which are not allowed to be write locked
        self.reserved_account_keys = {
            let mut reserved_keys = ReservedAccountKeys::clone(&self.reserved_account_keys);
            reserved_keys.update_active_set(&self.feature_set);
            Arc::new(reserved_keys)
        };

I know you asked for methods to add / remove keys from this struct but I think those can be added later when needed. The new struct supports those operations more readily than the previous implementation

Choose a reason for hiding this comment

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

Thank you! I'm much happier with this, and I agree that add/remove methods can easily be added later, given the new design.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 80.61224% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (e682fec) to head (d341e16).

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #84   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         837      838    +1     
  Lines      226539   226637   +98     
=======================================
+ Hits       185548   185642   +94     
- Misses      40991    40995    +4     


/// Move inactive reserved account keys to the active set if their feature
/// is active.
pub fn update_active_set(&mut self, feature_set: &FeatureSet) {

Choose a reason for hiding this comment

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

Thank you! I'm much happier with this, and I agree that add/remove methods can easily be added later, given the new design.

@jstarry jstarry merged commit 72d6d78 into anza-xyz:master Mar 14, 2024
45 checks passed
@jstarry jstarry deleted the feat/reserved-accounts-mod branch March 14, 2024 07:03
@t-nelson
Copy link

i still believe that this being in public interface is a bug, as is everything depending on it being there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants