-
Notifications
You must be signed in to change notification settings - Fork 223
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
Conversation
There was a problem hiding this 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
sdk/src/reserved_account_keys.rs
Outdated
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 |
There was a problem hiding this comment.
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
sdk/src/reserved_account_keys.rs
Outdated
RESERVED_ACCOUNT_KEYS | ||
.iter() | ||
.map(|reserved_key| reserved_key.key) | ||
.collect() |
There was a problem hiding this comment.
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)
}
```
0c98be2
to
34f90e6
Compare
@t-nelson @CriesofCarrots when you get the time can you take another look at this? |
@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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
6533d30
to
d341e16
Compare
Codecov ReportAttention: Patch coverage is
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) { |
There was a problem hiding this comment.
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.
i still believe that this being in public interface is a bug, as is everything depending on it being there |
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