Merge pull request #159 from casper-network/add-logging-around-pruning

Clear named keys instead of prune

Author: sacherjj

execution_engine/src/core/engine_state/mod.rs

@@ -284,53 +284,7 @@ where
             return Err(Error::InvalidProtocolVersion(new_protocol_version));
         }
 
-        let system_upgrader: SystemUpgrader<S> = 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<S> = 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,72 @@ 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");
         // 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,

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<Key, StoredValue>,
-    ) -> Result<Vec<Key>, 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)
-    }
 }

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

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;