From 8059c702bf960bb3370b793e1d34743839c32460 Mon Sep 17 00:00:00 2001 From: Karan Dhareshwar Date: Tue, 30 Jul 2024 14:29:51 -0500 Subject: [PATCH 1/2] Clear named keys instead of prune --- execution_engine/src/core/engine_state/mod.rs | 142 ++++++++---------- .../src/core/engine_state/upgrade.rs | 58 ------- .../test/regression/regression_20240726.rs | 23 ++- types/src/contracts.rs | 5 + 4 files changed, 86 insertions(+), 142 deletions(-) diff --git a/execution_engine/src/core/engine_state/mod.rs b/execution_engine/src/core/engine_state/mod.rs index dc004080d2..6cc3a91a99 100644 --- a/execution_engine/src/core/engine_state/mod.rs +++ b/execution_engine/src/core/engine_state/mod.rs @@ -284,53 +284,7 @@ where return Err(Error::InvalidProtocolVersion(new_protocol_version)); } - let system_upgrader: SystemUpgrader = SystemUpgrader::new( - new_protocol_version, - current_protocol_version, - tracking_copy.clone(), - ); - - info!("attempting prune of contract packages"); - - // Generate the list of keys to prune - let keys_to_prune = system_upgrader - .get_contracts_prune_list(correlation_id, upgrade_config.global_state_update()) - .unwrap_or_else(|upgrade_error| { - warn!("error in generating prune list: {:?}", upgrade_error); - vec![] - }); - - info!("keys to prune {:?}", keys_to_prune); - - let prune_config = PruneConfig::new(pre_state_hash, keys_to_prune); - let post_prune_state_root_hash = match self.commit_prune(correlation_id, prune_config) { - Ok(PruneResult::Success { - post_state_hash: new_state_root_hash, - .. - }) => { - info!( - "prune success with new state root hash {:?}", - new_state_root_hash - ); - new_state_root_hash - } - Ok(_) | Err(_) => { - warn!("failed to prune, reusing previous state root hash"); - pre_state_hash - } - }; - - info!( - "post-prune: pre-state-hash: {:?} post-state-hash: {:?}", - pre_state_hash, post_prune_state_root_hash, - ); - - let post_prune_tracking_copy = match self.tracking_copy(post_prune_state_root_hash)? { - Some(tracking_copy) => Rc::new(RefCell::new(tracking_copy)), - None => return Err(Error::RootNotFound(post_prune_state_root_hash)), - }; - - let registry = if let Ok(registry) = post_prune_tracking_copy + let registry = if let Ok(registry) = tracking_copy .borrow_mut() .get_system_contracts(correlation_id) { @@ -383,7 +337,7 @@ where // We write the checksums of the chainspec settings to global state // allowing verification of the chainspec data reported via the RPC. - post_prune_tracking_copy.borrow_mut().write( + tracking_copy.borrow_mut().write( Key::ChainspecRegistry, StoredValue::CLValue(cl_value_chainspec_registry), ); @@ -393,7 +347,7 @@ where let system_upgrader: SystemUpgrader = SystemUpgrader::new( new_protocol_version, current_protocol_version, - post_prune_tracking_copy.clone(), + tracking_copy.clone(), ); system_upgrader @@ -417,7 +371,7 @@ where // 3.1.1.1.1.7 new total validator slots is optional if let Some(new_validator_slots) = upgrade_config.new_validator_slots() { // 3.1.2.4 if new total validator slots is provided, update auction contract state - let auction_contract = post_prune_tracking_copy + let auction_contract = tracking_copy .borrow_mut() .get_contract(correlation_id, *auction_hash)?; @@ -426,14 +380,12 @@ where CLValue::from_t(new_validator_slots) .map_err(|_| Error::Bytesrepr("new_validator_slots".to_string()))?, ); - post_prune_tracking_copy - .borrow_mut() - .write(validator_slots_key, value); + tracking_copy.borrow_mut().write(validator_slots_key, value); } if let Some(new_auction_delay) = upgrade_config.new_auction_delay() { debug!(%new_auction_delay, "Auction delay changed as part of the upgrade"); - let auction_contract = post_prune_tracking_copy + let auction_contract = tracking_copy .borrow_mut() .get_contract(correlation_id, *auction_hash)?; @@ -442,13 +394,11 @@ where CLValue::from_t(new_auction_delay) .map_err(|_| Error::Bytesrepr("new_auction_delay".to_string()))?, ); - post_prune_tracking_copy - .borrow_mut() - .write(auction_delay_key, value); + tracking_copy.borrow_mut().write(auction_delay_key, value); } if let Some(new_locked_funds_period) = upgrade_config.new_locked_funds_period_millis() { - let auction_contract = post_prune_tracking_copy + let auction_contract = tracking_copy .borrow_mut() .get_contract(correlation_id, *auction_hash)?; @@ -457,7 +407,7 @@ where CLValue::from_t(new_locked_funds_period) .map_err(|_| Error::Bytesrepr("new_locked_funds_period".to_string()))?, ); - post_prune_tracking_copy + tracking_copy .borrow_mut() .write(locked_funds_period_key, value); } @@ -468,7 +418,7 @@ where Ratio::new(numer.into(), denom.into()) }; - let mint_contract = post_prune_tracking_copy + let mint_contract = tracking_copy .borrow_mut() .get_contract(correlation_id, *mint_hash)?; @@ -477,13 +427,15 @@ where CLValue::from_t(new_round_seigniorage_rate) .map_err(|_| Error::Bytesrepr("new_round_seigniorage_rate".to_string()))?, ); - post_prune_tracking_copy + tracking_copy .borrow_mut() .write(locked_funds_period_key, value); } + // We insert the new unbonding delay once the purses to be paid out have been transformed + // based on the previous unbonding delay. if let Some(new_unbonding_delay) = upgrade_config.new_unbonding_delay() { - let auction_contract = post_prune_tracking_copy + let auction_contract = tracking_copy .borrow_mut() .get_contract(correlation_id, *auction_hash)?; @@ -492,48 +444,80 @@ where CLValue::from_t(new_unbonding_delay) .map_err(|_| Error::Bytesrepr("new_unbonding_delay".to_string()))?, ); - post_prune_tracking_copy - .borrow_mut() - .write(unbonding_delay_key, value); + tracking_copy.borrow_mut().write(unbonding_delay_key, value); } - info!("apply the accepted modifications to global state"); + // match system_upgrader + // .clear_named_keys_for_packages(correlation_id, upgrade_config.global_state_update()) + // { Ok(()) => { info!("cleared named keys for contracts")} + // Err(error) => { + // error!("error in clearing named keys: {:?}", error); + // } + // } + // apply the accepted modifications to global state. for (key, value) in upgrade_config.global_state_update() { // Skip the key hashes associated with values as we mark these as records to be // pruned. let is_unit_value = value.is_unit_cl_value(); if key.into_hash().is_some() && is_unit_value { - debug!( - "found marker record for package prune; skipping normal operation of write {}", + info!( + "found marker record for package overwrite {}", key.to_formatted_string() ); - continue; + { + let contract_package = + match tracking_copy.borrow_mut().read(correlation_id, key) { + Ok(Some(StoredValue::ContractPackage(contract_package))) => { + contract_package + } + Ok(_) | Err(_) => { + error!( + "error in retrieving contract package: {}", + key.to_formatted_string() + ); + continue; + } + }; + for contract_hash in contract_package.versions().values() { + let contract_key = Key::Hash(contract_hash.value()); + let mut contract = match tracking_copy + .borrow_mut() + .read(correlation_id, &contract_key) + { + Ok(Some(StoredValue::Contract(contract))) => contract, + Ok(_) | Err(_) => { + error!( + "error in retrieving contract : {}", + contract_key.to_formatted_string() + ); + continue; + } + }; + contract.clear_named_keys(); + tracking_copy + .borrow_mut() + .write(contract_key, StoredValue::Contract(contract)); + } + }; } else { info!("update for key: {}", key.to_formatted_string()); - post_prune_tracking_copy - .borrow_mut() - .write(*key, value.clone()); + tracking_copy.borrow_mut().write(*key, value.clone()); } } - info!("finished applying accepted modifications to global state"); - let execution_effect = post_prune_tracking_copy.borrow().effect(); + let execution_effect = tracking_copy.borrow().effect(); // commit let post_state_hash = self .state .commit( correlation_id, - post_prune_state_root_hash, + pre_state_hash, execution_effect.transforms.to_owned(), ) .map_err(Into::into)?; - info!( - "upgrade success with post state hash: {:?}", - post_state_hash - ); // return result and effects Ok(UpgradeSuccess { post_state_hash, diff --git a/execution_engine/src/core/engine_state/upgrade.rs b/execution_engine/src/core/engine_state/upgrade.rs index 975112a34f..ce0295a0ee 100644 --- a/execution_engine/src/core/engine_state/upgrade.rs +++ b/execution_engine/src/core/engine_state/upgrade.rs @@ -1,5 +1,4 @@ //! Support for applying upgrades on the execution engine. -use log::{info, warn}; use std::{cell::RefCell, collections::BTreeMap, fmt, rc::Rc}; use num_rational::Ratio; @@ -412,61 +411,4 @@ where Ok(()) } - - pub(crate) fn get_contracts_prune_list( - &self, - correlation_id: CorrelationId, - global_state_update: &BTreeMap, - ) -> Result, ProtocolUpgradeError> { - let mut keys_to_prune = vec![]; - for (key, _) in global_state_update.iter() { - if key.into_hash().is_none() { - continue; - } - - let maybe_contract_package = self - .tracking_copy - .borrow_mut() - .read(correlation_id, key) - .ok(); - if let Some(Some(StoredValue::ContractPackage(contract_package))) = - maybe_contract_package - { - info!("Found contract package {:?}", key.to_formatted_string()); - let contract_versions = contract_package.versions(); - let mut found_all_keys = true; - let mut keys_for_package = vec![]; - for contract_hash in contract_versions.values() { - let contract = self - .tracking_copy - .borrow_mut() - .read(correlation_id, &Key::Hash(contract_hash.value())) - .ok(); - if let Some(Some(StoredValue::Contract(contract))) = contract { - let contract_wasm_hash = contract.contract_wasm_hash(); - keys_for_package.push(Key::Hash(contract_hash.value())); - keys_for_package.push(Key::Hash(contract_wasm_hash.value())) - } else { - found_all_keys = false; - warn!( - "Unable to find contract record for contract hash: {:?}", - contract_hash - ); - break; - } - } - if found_all_keys { - keys_to_prune.extend(keys_for_package); - } - keys_to_prune.push(*key); - } else { - warn!( - "Expected Contract package found either None or different stored value {:?}", - key - ); - } - } - - Ok(keys_to_prune) - } } diff --git a/execution_engine_testing/tests/src/test/regression/regression_20240726.rs b/execution_engine_testing/tests/src/test/regression/regression_20240726.rs index 6c3aa7960e..3eb4e722af 100644 --- a/execution_engine_testing/tests/src/test/regression/regression_20240726.rs +++ b/execution_engine_testing/tests/src/test/regression/regression_20240726.rs @@ -6,7 +6,9 @@ use casper_engine_test_support::{ use once_cell::sync::Lazy; use std::collections::BTreeMap; -use casper_types::{runtime_args, CLValue, EraId, ProtocolVersion, RuntimeArgs, StoredValue, URef}; +use casper_types::{ + runtime_args, CLValue, EraId, Key, ProtocolVersion, RuntimeArgs, StoredValue, URef, +}; const PURSE_FIXTURE: &str = "purse_fixture"; const PURSE_WASM: &str = "regression-20240726.wasm"; @@ -77,10 +79,21 @@ fn should_not_allow_forged_urefs_to_be_saved_to_named_keys() { .expect_upgrade_success() .commit(); - if let Err(error) = builder.query(None, contract_to_prune, &[]) { - assert!(error.contains("ValueNotFound")) - } else { - panic!("does not contain value not found"); + let contract_package = builder + .query(None, contract_to_prune, &[]) + .expect("must get stored value") + .as_contract_package() + .cloned() + .expect("must get package"); + + for hash in contract_package.versions().values() { + let contract = builder + .query(None, Key::Hash(hash.value()), &[]) + .expect("must get stored value") + .as_contract() + .cloned() + .expect("must get contract"); + assert!(contract.named_keys().is_empty()); } let hardcoded_uref = builder diff --git a/types/src/contracts.rs b/types/src/contracts.rs index 4c39a7983d..f2abe985c0 100644 --- a/types/src/contracts.rs +++ b/types/src/contracts.rs @@ -1173,6 +1173,11 @@ impl Contract { self.named_keys.remove(key) } + /// Clears the named keys for the contract. + pub fn clear_named_keys(&mut self) { + self.named_keys = NamedKeys::new(); + } + /// Set protocol_version. pub fn set_protocol_version(&mut self, protocol_version: ProtocolVersion) { self.protocol_version = protocol_version; From 2bc125d830da5a8ba29ba1c2a4852ac145933ec3 Mon Sep 17 00:00:00 2001 From: Karan Dhareshwar Date: Tue, 30 Jul 2024 14:32:36 -0500 Subject: [PATCH 2/2] Clear named keys instead of prune --- execution_engine/src/core/engine_state/mod.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/execution_engine/src/core/engine_state/mod.rs b/execution_engine/src/core/engine_state/mod.rs index 6cc3a91a99..84f950a032 100644 --- a/execution_engine/src/core/engine_state/mod.rs +++ b/execution_engine/src/core/engine_state/mod.rs @@ -447,14 +447,6 @@ where tracking_copy.borrow_mut().write(unbonding_delay_key, value); } - // match system_upgrader - // .clear_named_keys_for_packages(correlation_id, upgrade_config.global_state_update()) - // { Ok(()) => { info!("cleared named keys for contracts")} - // Err(error) => { - // error!("error in clearing named keys: {:?}", error); - // } - // } - // apply the accepted modifications to global state. for (key, value) in upgrade_config.global_state_update() { // Skip the key hashes associated with values as we mark these as records to be