From 20a7cdd43d1dc0fa143efae79ccaf2ab22c38699 Mon Sep 17 00:00:00 2001 From: steviez Date: Tue, 20 Jun 2023 23:44:43 -0500 Subject: [PATCH] Restrict access to Bank's HardForks (#32180) * Restrict access to Bank's HardForks Callers could previously obtain a a lock to read/write HardForks from any Bank. This would allow any caller to modify, and creates the opportunity for inconsistent handling of what is considered a valid hard fork (ie too old). This PR adds a function to Bank so consistent sanity checks can be applied; the caller will already have a Bank as that is where they would have obtained the HardForks from in the first place. Additionally, change the getter to return a copy of HardForks (simple Vec). * Allow hard fork at bank slot if bank is not yet frozen --- core/src/validator.rs | 29 +++-------------- ledger-tool/src/main.rs | 15 ++------- ledger/src/bank_forks_utils.rs | 15 ++------- program-test/src/lib.rs | 9 +++--- .../tests/sysvar_last_restart_slot.rs | 6 ++-- runtime/src/bank.rs | 32 ++++++++++++++++--- sdk/src/hard_forks.rs | 5 +++ 7 files changed, 51 insertions(+), 60 deletions(-) diff --git a/core/src/validator.rs b/core/src/validator.rs index e7c739b65..47e2897a3 100644 --- a/core/src/validator.rs +++ b/core/src/validator.rs @@ -665,19 +665,15 @@ impl Validator { entry_notifier, Some(poh_timing_point_sender.clone()), )?; + let hard_forks = bank_forks.read().unwrap().root_bank().hard_forks(); + if !hard_forks.is_empty() { + info!("Hard forks: {:?}", hard_forks); + } node.info.set_wallclock(timestamp()); node.info.set_shred_version(compute_shred_version( &genesis_config.hash(), - Some( - &bank_forks - .read() - .unwrap() - .working_bank() - .hard_forks() - .read() - .unwrap(), - ), + Some(&hard_forks), )); Self::print_node_info(&node); @@ -1674,21 +1670,6 @@ fn load_blockstore( // is processing the dropped banks from the `pruned_banks_receiver` channel. let pruned_banks_receiver = AccountsBackgroundService::setup_bank_drop_callback(bank_forks.clone()); - { - let hard_forks: Vec<_> = bank_forks - .read() - .unwrap() - .working_bank() - .hard_forks() - .read() - .unwrap() - .iter() - .copied() - .collect(); - if !hard_forks.is_empty() { - info!("Hard forks: {:?}", hard_forks); - } - } leader_schedule_cache.set_fixed_leader_schedule(config.fixed_leader_schedule.clone()); { diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index aec107484..d8af9dfe4 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -2243,15 +2243,7 @@ fn main() { "{}", compute_shred_version( &genesis_config.hash(), - Some( - &bank_forks - .read() - .unwrap() - .working_bank() - .hard_forks() - .read() - .unwrap() - ) + Some(&bank_forks.read().unwrap().working_bank().hard_forks()) ) ); } @@ -3120,10 +3112,7 @@ fn main() { println!( "Shred version: {}", - compute_shred_version( - &genesis_config.hash(), - Some(&bank.hard_forks().read().unwrap()) - ) + compute_shred_version(&genesis_config.hash(), Some(&bank.hard_forks())) ); } Err(err) => { diff --git a/ledger/src/bank_forks_utils.rs b/ledger/src/bank_forks_utils.rs index d27d9ec3e..bd5b791b3 100644 --- a/ledger/src/bank_forks_utils.rs +++ b/ledger/src/bank_forks_utils.rs @@ -172,18 +172,9 @@ pub fn load_bank_forks( if let Some(ref new_hard_forks) = process_options.new_hard_forks { let root_bank = bank_forks.read().unwrap().root_bank(); - let hard_forks = root_bank.hard_forks(); - - for hard_fork_slot in new_hard_forks.iter() { - if *hard_fork_slot > root_bank.slot() { - hard_forks.write().unwrap().register(*hard_fork_slot); - } else { - warn!( - "Hard fork at {} ignored, --hard-fork option can be removed.", - hard_fork_slot - ); - } - } + new_hard_forks + .iter() + .for_each(|hard_fork_slot| root_bank.register_hard_fork(*hard_fork_slot)); } (bank_forks, leader_schedule_cache, starting_snapshot_hashes) diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index 33899504e..86ea46371 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -1211,9 +1211,10 @@ impl ProgramTestContext { /// record a hard fork slot in working bank; should be in the past pub fn register_hard_fork(&mut self, hard_fork_slot: Slot) { - let bank_forks = self.bank_forks.write().unwrap(); - let hard_forks = bank_forks.working_bank().hard_forks(); - let mut write = hard_forks.write().unwrap(); - write.register(hard_fork_slot); + self.bank_forks + .read() + .unwrap() + .working_bank() + .register_hard_fork(hard_fork_slot) } } diff --git a/program-test/tests/sysvar_last_restart_slot.rs b/program-test/tests/sysvar_last_restart_slot.rs index 438c10485..4d04e8974 100644 --- a/program-test/tests/sysvar_last_restart_slot.rs +++ b/program-test/tests/sysvar_last_restart_slot.rs @@ -92,13 +92,13 @@ async fn get_sysvar_last_restart_slot() { context.register_hard_fork(40 as Slot); context.warp_to_slot(45).unwrap(); check_with_program(&mut context, program_id, 41).await; - context.register_hard_fork(43 as Slot); context.register_hard_fork(47 as Slot); + context.register_hard_fork(48 as Slot); context.warp_to_slot(46).unwrap(); - check_with_program(&mut context, program_id, 43).await; + check_with_program(&mut context, program_id, 41).await; context.register_hard_fork(50 as Slot); context.warp_to_slot(48).unwrap(); - check_with_program(&mut context, program_id, 47).await; + check_with_program(&mut context, program_id, 48).await; context.warp_to_slot(50).unwrap(); check_with_program(&mut context, program_id, 50).await; } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 489c831f8..c65e3da43 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2426,8 +2426,7 @@ impl Bank { if feature_flag { let last_restart_slot = { let slot = self.slot; - let hard_forks = self.hard_forks(); - let hard_forks_r = hard_forks.read().unwrap(); + let hard_forks_r = self.hard_forks.read().unwrap(); // Only consider hard forks <= this bank's slot to avoid prematurely applying // a hard fork that is set to occur in the future. @@ -7309,8 +7308,33 @@ impl Bank { *self.inflation.write().unwrap() = inflation; } - pub fn hard_forks(&self) -> Arc> { - self.hard_forks.clone() + /// Get a snapshot of the current set of hard forks + pub fn hard_forks(&self) -> HardForks { + self.hard_forks.read().unwrap().clone() + } + + pub fn register_hard_fork(&self, new_hard_fork_slot: Slot) { + let bank_slot = self.slot(); + + let lock = self.freeze_lock(); + let bank_frozen = *lock != Hash::default(); + if new_hard_fork_slot < bank_slot { + warn!( + "Hard fork at slot {new_hard_fork_slot} ignored, the hard fork is older \ + than the bank at slot {bank_slot} that attempted to register it." + ); + } else if (new_hard_fork_slot == bank_slot) && bank_frozen { + warn!( + "Hard fork at slot {new_hard_fork_slot} ignored, the hard fork is the same \ + slot as the bank at slot {bank_slot} that attempted to register it, but that \ + bank is already frozen." + ); + } else { + self.hard_forks + .write() + .unwrap() + .register(new_hard_fork_slot); + } } // Hi! leaky abstraction here.... diff --git a/sdk/src/hard_forks.rs b/sdk/src/hard_forks.rs index 878645d24..61f7d7a0d 100644 --- a/sdk/src/hard_forks.rs +++ b/sdk/src/hard_forks.rs @@ -33,6 +33,11 @@ impl HardForks { self.hard_forks.iter() } + // Returns `true` is there are currently no registered hard forks + pub fn is_empty(&self) -> bool { + self.hard_forks.is_empty() + } + // Returns data to include in the bank hash for the given slot if a hard fork is scheduled pub fn get_hash_data(&self, slot: Slot, parent_slot: Slot) -> Option<[u8; 8]> { // The expected number of hard forks in a cluster is small.