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 #35341

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions sdk/src/feature_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,10 @@ pub mod allow_commission_decrease_at_any_time {
solana_sdk::declare_id!("decoMktMcnmiq6t3u7g5BfgcQu91nKZr6RvMYf9z1Jb");
}

pub mod add_new_reserved_account_keys {
solana_sdk::declare_id!("8U4skmMVnF6k2kMvrWbQuRUT3qQSiTYpSjqmhmgfthZu");
}

pub mod consume_blockstore_duplicate_proofs {
solana_sdk::declare_id!("6YsBCejwK96GZCkJ6mkZ4b68oP63z2PLoQmWjC7ggTqZ");
}
Expand Down Expand Up @@ -965,6 +969,7 @@ lazy_static! {
(drop_legacy_shreds::id(), "drops legacy shreds #34328"),
(allow_commission_decrease_at_any_time::id(), "Allow commission decrease at any time in epoch #33843"),
(consume_blockstore_duplicate_proofs::id(), "consume duplicate proofs from blockstore in consensus #34372"),
(add_new_reserved_account_keys::id(), "add new unwritable reserved accounts #34899"),
(index_erasure_conflict_duplicate_proofs::id(), "generate duplicate proofs for index and erasure conflicts #34360"),
(merkle_conflict_duplicate_proofs::id(), "generate duplicate proofs for merkle root conflicts #34270"),
(disable_bpf_loader_instructions::id(), "disable bpf loader management instructions #34194"),
Expand Down
1 change: 1 addition & 0 deletions sdk/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ pub mod quic;
pub mod recent_blockhashes_account;
pub mod rent_collector;
pub mod rent_debits;
pub mod reserved_account_keys;
pub mod reward_info;
pub mod reward_type;
pub mod rpc_port;
Expand Down
202 changes: 202 additions & 0 deletions sdk/src/reserved_account_keys.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
//! Collection of reserved account keys that cannot be write-locked by transactions.
//! New reserved account keys may be added as long as they specify a feature
//! gate that transitions the key into read-only at an epoch boundary.

#![cfg(feature = "full")]

use {
crate::{
address_lookup_table, bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable,
compute_budget, config, ed25519_program, feature,
feature_set::{self, FeatureSet},
loader_v4, native_loader,
pubkey::Pubkey,
secp256k1_program, stake, system_program, sysvar, vote,
},
lazy_static::lazy_static,
std::collections::HashSet,
};

// Temporary until a zk token program module is added to the sdk
Copy link
Member Author

Choose a reason for hiding this comment

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

Added separately in this PR: #35339

mod zk_token_proof_program {
solana_sdk::declare_id!("ZkTokenProof1111111111111111111111111111111");
}

pub struct ReservedAccountKeys;
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
Contributor

Choose a reason for hiding this comment

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

any reason to prefer a singleton over making ReservedAccountKeys hold a Vec directly. then Bank can just pass an Arc around.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Bank uses an Arc wrapped HashSet. This module lives in solana-sdk because feature set is in solana-sdk but the message types live in solana-program still. So since the message types all operate over a HashSet, the Bank does so too.

Copy link
Contributor

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

.iter()
.map(|reserved_key| reserved_key.key)
.collect()
Copy link
Contributor

Choose a reason for hiding this comment

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

how are these used? if we instead used a HashMap (probably in conjunction with the above change), can we move whatever tests these outputs are used for internally and directly query the map instead of allocating?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only allocated once per bank. What are the keys and values of your proposed HashMap and what does it give us over a HashSet?

Copy link
Contributor

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<Pubkey>>. we get the HashSet-like accessors for free. no allocations other than passing the Arc on to each successor bank

}

/// Compute the active set of reserved keys based on activated features
pub fn active(feature_set: &FeatureSet) -> HashSet<Pubkey> {
Self::compute_active(&RESERVED_ACCOUNT_KEYS, feature_set)
}

// Method only exists for unit testing
fn compute_active(
account_keys: &[ReservedAccountKey],
feature_set: &FeatureSet,
) -> HashSet<Pubkey> {
account_keys
.iter()
.filter(|reserved_key| reserved_key.is_active(feature_set))
.map(|reserved_key| reserved_key.key)
.collect()
}

/// Return an empty list of reserved keys for visibility when using in
/// places where the dynamic reserved key set is not available
pub fn empty() -> HashSet<Pubkey> {
HashSet::default()
}
}

/// `ReservedAccountKey` represents a reserved account key that will not be
/// write-lockable by transactions. If a feature id is set, the account key will
/// become read-only only after the feature has been activated.
#[derive(Debug, Clone, Eq, PartialEq)]
struct ReservedAccountKey {
key: Pubkey,
feature_id: Option<Pubkey>,
}

impl ReservedAccountKey {
fn new_pending(key: Pubkey, feature_id: Pubkey) -> Self {
Self {
key,
feature_id: Some(feature_id),
}
}

fn new_active(key: Pubkey) -> Self {
Self {
key,
feature_id: None,
}
}

/// Returns true if no feature id is set or if the feature id is activated
/// in the feature set.
fn is_active(&self, feature_set: &FeatureSet) -> bool {
self.feature_id
.map_or(true, |feature_id| feature_set.is_active(&feature_id))
}
}

// New reserved account keys should be added in alphabetical order and must
// specify a feature id for activation.
lazy_static! {
static ref RESERVED_ACCOUNT_KEYS: Vec<ReservedAccountKey> = [
// builtin programs
ReservedAccountKey::new_pending(address_lookup_table::program::id(), feature_set::add_new_reserved_account_keys::id()),
ReservedAccountKey::new_active(bpf_loader::id()),
ReservedAccountKey::new_active(bpf_loader_deprecated::id()),
ReservedAccountKey::new_active(bpf_loader_upgradeable::id()),
ReservedAccountKey::new_pending(compute_budget::id(), feature_set::add_new_reserved_account_keys::id()),
ReservedAccountKey::new_active(config::program::id()),
ReservedAccountKey::new_pending(ed25519_program::id(), feature_set::add_new_reserved_account_keys::id()),
ReservedAccountKey::new_active(feature::id()),
ReservedAccountKey::new_pending(loader_v4::id(), feature_set::add_new_reserved_account_keys::id()),
ReservedAccountKey::new_pending(secp256k1_program::id(), feature_set::add_new_reserved_account_keys::id()),
#[allow(deprecated)]
ReservedAccountKey::new_active(stake::config::id()),
ReservedAccountKey::new_active(stake::program::id()),
ReservedAccountKey::new_active(system_program::id()),
ReservedAccountKey::new_active(vote::program::id()),
ReservedAccountKey::new_pending(zk_token_proof_program::id(), feature_set::add_new_reserved_account_keys::id()),

// sysvars
ReservedAccountKey::new_active(sysvar::clock::id()),
ReservedAccountKey::new_pending(sysvar::epoch_rewards::id(), feature_set::add_new_reserved_account_keys::id()),
ReservedAccountKey::new_active(sysvar::epoch_schedule::id()),
#[allow(deprecated)]
ReservedAccountKey::new_active(sysvar::fees::id()),
ReservedAccountKey::new_active(sysvar::instructions::id()),
ReservedAccountKey::new_pending(sysvar::last_restart_slot::id(), feature_set::add_new_reserved_account_keys::id()),
#[allow(deprecated)]
ReservedAccountKey::new_active(sysvar::recent_blockhashes::id()),
ReservedAccountKey::new_active(sysvar::rent::id()),
ReservedAccountKey::new_active(sysvar::rewards::id()),
ReservedAccountKey::new_active(sysvar::slot_hashes::id()),
ReservedAccountKey::new_active(sysvar::slot_history::id()),
ReservedAccountKey::new_active(sysvar::stake_history::id()),

// other
ReservedAccountKey::new_active(native_loader::id()),
ReservedAccountKey::new_pending(sysvar::id(), feature_set::add_new_reserved_account_keys::id()),
].to_vec();
}

#[cfg(test)]
mod tests {
use {
super::*,
solana_program::{message::legacy::BUILTIN_PROGRAMS_KEYS, sysvar::ALL_IDS},
};

#[test]
fn test_reserved_account_key_is_active() {
let feature_id = Pubkey::new_unique();
let old_reserved_account_key = ReservedAccountKey::new_active(Pubkey::new_unique());
let new_reserved_account_key =
ReservedAccountKey::new_pending(Pubkey::new_unique(), feature_id);

let mut feature_set = FeatureSet::default();
assert!(
old_reserved_account_key.is_active(&feature_set),
"if not feature id is set, the key should be active"
);
assert!(
!new_reserved_account_key.is_active(&feature_set),
"if feature id is set but not in the feature set, the key should not be active"
);

feature_set.active.insert(feature_id, 0);
assert!(
new_reserved_account_key.is_active(&feature_set),
"if feature id is set and is in the feature set, the key should be active"
);
}

#[test]
fn test_reserved_account_keys_compute_active() {
let feature_id = Pubkey::new_unique();
let key0 = Pubkey::new_unique();
let key1 = Pubkey::new_unique();
let test_account_keys = vec![
ReservedAccountKey::new_active(key0),
ReservedAccountKey::new_pending(key1, feature_id),
ReservedAccountKey::new_pending(Pubkey::new_unique(), Pubkey::new_unique()),
];

let mut feature_set = FeatureSet::default();
assert_eq!(
HashSet::from_iter([key0].into_iter()),
ReservedAccountKeys::compute_active(&test_account_keys, &feature_set),
"should only contain keys without feature ids"
);

feature_set.active.insert(feature_id, 0);
assert_eq!(
HashSet::from_iter([key0, key1].into_iter()),
ReservedAccountKeys::compute_active(&test_account_keys, &feature_set),
"should only contain keys without feature ids or with active feature ids"
);
}

#[test]
fn test_static_list_compat() {
let mut static_set = HashSet::new();
static_set.extend(ALL_IDS.iter().cloned());
static_set.extend(BUILTIN_PROGRAMS_KEYS.iter().cloned());

let initial_dynamic_set = ReservedAccountKeys::active(&FeatureSet::default());

assert_eq!(initial_dynamic_set, static_set);
}
}
Loading