Merge #4961

4961: [BUGFIX]: Bugfix for add_contract_version post migration with entity enabled r=darthsiroftardis a=darthsiroftardis

CHANGELOG:

- Fixed a bug where a 1.x user migrated to 2.x could not install a contract via the InstallUpgrader transaction variant.
- Fixed a bug where a 1.x user migrated to 2.x could not add a new version to their contract.

Co-authored-by: Karan Dhareshwar <[email protected]>

execution_engine/src/runtime/mod.rs

@@ -1507,8 +1507,7 @@ where
                             maybe_system_entity_type,
                         )
                     }
-                    Some(_) => return Err(ExecError::UnexpectedStoredValueVariant),
-                    None => {
+                    Some(_) | None => {
                         if !self.context.engine_config().enable_entity {
                             return Err(ExecError::KeyNotFound(Key::Hash(contract_hash)));
                         }
@@ -1601,8 +1600,7 @@ where
                             maybe_system_entity_type,
                         )
                     }
-                    Some(_) => return Err(ExecError::UnexpectedStoredValueVariant),
-                    None => {
+                    Some(_) | None => {
                         if !self.context.engine_config().enable_entity {
                             return Err(ExecError::KeyNotFound(Key::Hash(hash_addr)));
                         }
@@ -2642,16 +2640,24 @@ where
                 // addressable entity format
                 let account_hash = self.context.get_initiator();
 
-                let (_package_key, access_key) = match self
+                let access_key = match self
                     .context
                     .read_gs(&Key::Hash(previous_entity.package_hash().value()))?
-                    .and_then(|stored_value| stored_value.into_cl_value())
                 {
+                    Some(StoredValue::ContractPackage(contract_package)) => {
+                        contract_package.access_key()
+                    }
+                    Some(StoredValue::CLValue(cl_value)) => {
+                        let (_key, uref) = cl_value
+                            .into_t::<(Key, URef)>()
+                            .map_err(ExecError::CLValue)?;
+                        uref
+                    }
+                    Some(_other) => return Err(ExecError::UnexpectedStoredValueVariant),
                     None => {
                         return Err(ExecError::UpgradeAuthorizationFailure);
                     }
-                    Some(cl_value) => cl_value.into_t::<(Key, URef)>().map_err(ExecError::CLValue),
-                }?;
+                };
 
                 let has_access = self.context.validate_uref(&access_key).is_ok();
 

execution_engine/src/runtime_context/tests.rs

@@ -135,6 +135,14 @@ fn new_runtime_context<'a>(
     };
 
     tracking_copy.write(Key::SystemEntityRegistry, default_system_registry);
+    tracking_copy.write(
+        Key::Account(account_hash),
+        StoredValue::CLValue(CLValue::from_t(entity_address).expect("must get cl_value")),
+    );
+    tracking_copy.write(
+        entity_address,
+        StoredValue::AddressableEntity(addressable_entity.clone()),
+    );
 
     // write block time to gs
     let now = Timestamp::now();
@@ -893,7 +901,7 @@ fn remove_uref_works() {
     let uref_name = "Foo".to_owned();
     let uref_key = create_uref_as_key(&mut address_generator, AccessRights::READ);
     let account_hash = AccountHash::new([0u8; 32]);
-    let entity_hash = AddressableEntityHash::new([1u8; 32]);
+    let entity_hash = AddressableEntityHash::new([0u8; 32]);
     let mut named_keys = NamedKeys::new();
     named_keys.insert(uref_name.clone(), uref_key);
     let (_, entity_key, addressable_entity) = new_addressable_entity(account_hash, entity_hash);

execution_engine_testing/test_support/src/chainspec_config.rs

@@ -233,6 +233,12 @@ impl ChainspecConfig {
         self
     }
 
+    /// Sets the enable addressable entity flag.
+    pub fn with_enable_addressable_entity(mut self, enable_addressable_entity: bool) -> Self {
+        self.core_config.enable_addressable_entity = enable_addressable_entity;
+        self
+    }
+
     /// Returns the `max_associated_keys` setting from the core config.
     pub fn max_associated_keys(&self) -> u32 {
         self.core_config.max_associated_keys

execution_engine_testing/test_support/src/wasm_test_builder.rs

@@ -17,8 +17,7 @@ use num_traits::{CheckedMul, Zero};
 use tempfile::TempDir;
 
 use casper_execution_engine::engine_state::{
-    engine_config::DEFAULT_ENABLE_ENTITY, Error, ExecutionEngineV1, WasmV1Request, WasmV1Result,
-    DEFAULT_MAX_QUERY_DEPTH,
+    Error, ExecutionEngineV1, WasmV1Request, WasmV1Result, DEFAULT_MAX_QUERY_DEPTH,
 };
 use casper_storage::{
     data_access_layer::{
@@ -312,12 +311,12 @@ impl LmdbWasmTestBuilder {
         );
 
         let max_query_depth = DEFAULT_MAX_QUERY_DEPTH;
-        let enable_addressable_entity = DEFAULT_ENABLE_ENTITY;
+        let enable_addressable_entity = chainspec.core_config.enable_addressable_entity;
         let global_state = LmdbGlobalState::empty(
             environment,
             trie_store,
             max_query_depth,
-            DEFAULT_ENABLE_ENTITY,
+            enable_addressable_entity,
         )
         .expect("should create LmdbGlobalState");
 
@@ -375,6 +374,7 @@ impl LmdbWasmTestBuilder {
 
         let max_query_depth = DEFAULT_MAX_QUERY_DEPTH;
 
+        let enable_addressable_entity = chainspec.core_config.enable_addressable_entity;
         let global_state = match mode {
             GlobalStateMode::Create(database_flags) => {
                 let trie_store = LmdbTrieStore::new(&environment, None, database_flags)
@@ -383,7 +383,7 @@ impl LmdbWasmTestBuilder {
                     Arc::new(environment),
                     Arc::new(trie_store),
                     max_query_depth,
-                    DEFAULT_ENABLE_ENTITY,
+                    enable_addressable_entity,
                 )
                 .expect("should create LmdbGlobalState")
             }
@@ -395,7 +395,7 @@ impl LmdbWasmTestBuilder {
                     Arc::new(trie_store),
                     post_state_hash,
                     max_query_depth,
-                    DEFAULT_ENABLE_ENTITY,
+                    enable_addressable_entity,
                 )
             }
         };
@@ -404,7 +404,7 @@ impl LmdbWasmTestBuilder {
             block_store: BlockStore::new(),
             state: global_state,
             max_query_depth,
-            enable_addressable_entity: DEFAULT_ENABLE_ENTITY,
+            enable_addressable_entity,
         });
         let mut engine_config = chainspec.engine_config();
         engine_config.set_protocol_version(protocol_version);
@@ -833,6 +833,7 @@ where
             U512::from(*config.core_config.validator_credit_cap.numer()),
             U512::from(*config.core_config.validator_credit_cap.denom()),
         );
+        let enable_addressable_entity = config.core_config.enable_addressable_entity;
         let native_runtime_config = casper_storage::system::runtime_native::Config::new(
             TransferConfig::Unadministered,
             fee_handling,
@@ -845,7 +846,7 @@ where
             balance_hold_interval,
             include_credits,
             credit_cap,
-            DEFAULT_ENABLE_ENTITY,
+            enable_addressable_entity,
             config.system_costs_config.mint_costs().transfer,
         );
 
@@ -935,6 +936,7 @@ where
         upgrade_config.with_pre_state_hash(pre_state_hash);
 
         let req = ProtocolUpgradeRequest::new(upgrade_config.clone());
+
         let result = self.data_access_layer.protocol_upgrade(req);
 
         if let ProtocolUpgradeResult::Success {
@@ -1012,7 +1014,7 @@ where
             self.chainspec.core_config.gas_hold_interval.millis(),
             include_credits,
             credit_cap,
-            DEFAULT_ENABLE_ENTITY,
+            self.chainspec.core_config.enable_addressable_entity,
             self.chainspec.system_costs_config.mint_costs().transfer,
         )
     }
@@ -1907,7 +1909,7 @@ where
             state_root_hash,
             ProtocolVersion::V2_0_0,
             SystemEntityRegistrySelector::auction(),
-            DEFAULT_ENABLE_ENTITY,
+            self.chainspec.core_config.enable_addressable_entity,
         );
         self.system_entity_key(request)
             .into_entity_hash()
@@ -1921,7 +1923,7 @@ where
             state_root_hash,
             ProtocolVersion::V2_0_0,
             SystemEntityRegistrySelector::mint(),
-            DEFAULT_ENABLE_ENTITY,
+            self.chainspec.core_config.enable_addressable_entity,
         );
         self.system_entity_key(request)
             .into_entity_hash()
@@ -1939,7 +1941,7 @@ where
             state_root_hash,
             protocol_version,
             SystemEntityRegistrySelector::handle_payment(),
-            DEFAULT_ENABLE_ENTITY,
+            self.chainspec.core_config.enable_addressable_entity,
         );
         self.system_entity_key(request)
             .into_entity_hash()

execution_engine_testing/tests/fixtures/release_1_5_8/state.json

@@ -0,0 +1,6 @@
+{
+  "genesis_request": {
+    "protocol_version": "1.0.0"
+  },
+  "post_state_hash": "29bbd3e40c68462422db2a7bb144e71e53607a1b7d9bcbdacecef22c998de8e3"
+}

execution_engine_testing/tests/src/lmdb_fixture.rs

@@ -22,6 +22,7 @@ pub const RELEASE_1_4_2: &str = "release_1_4_2";
 pub const RELEASE_1_4_3: &str = "release_1_4_3";
 pub const RELEASE_1_4_4: &str = "release_1_4_4";
 pub const RELEASE_1_4_5: &str = "release_1_4_5";
+pub const RELEASE_1_5_8: &str = "release_1_5_8";
 const STATE_JSON_FILE: &str = "state.json";
 const FIXTURES_DIRECTORY: &str = "fixtures";
 const GENESIS_PROTOCOL_VERSION_FIELD: &str = "protocol_version";
@@ -96,6 +97,32 @@ pub fn builder_from_global_state_fixture(
     )
 }
 
+pub fn builder_from_global_state_fixture_with_enable_ae(
+    fixture_name: &str,
+    enable_addressable_entity: bool,
+) -> (LmdbWasmTestBuilder, LmdbFixtureState, TempDir) {
+    let source = path_to_lmdb_fixtures().join(fixture_name);
+    let to = tempfile::tempdir().expect("should create temp dir");
+    fs_extra::copy_items(&[source], &to, &dir::CopyOptions::default())
+        .expect("should copy global state fixture");
+
+    let path_to_state = to.path().join(fixture_name).join(STATE_JSON_FILE);
+    let lmdb_fixture_state: LmdbFixtureState =
+        serde_json::from_reader(File::open(path_to_state).unwrap()).unwrap();
+    let path_to_gs = to.path().join(fixture_name);
+
+    (
+        LmdbWasmTestBuilder::open(
+            &path_to_gs,
+            ChainspecConfig::default().with_enable_addressable_entity(enable_addressable_entity),
+            lmdb_fixture_state.genesis_protocol_version(),
+            lmdb_fixture_state.post_state_hash,
+        ),
+        lmdb_fixture_state,
+        to,
+    )
+}
+
 /// Creates a new fixture with a name.
 ///
 /// This process is currently manual. The process to do this is to check out a release branch, call

execution_engine_testing/tests/src/test/contract_api/add_contract_version.rs

@@ -1,14 +1,15 @@
+use crate::lmdb_fixture;
 use casper_engine_test_support::{
-    utils, ExecuteRequestBuilder, LmdbWasmTestBuilder, DEFAULT_ACCOUNT_ADDR,
+    utils, ExecuteRequestBuilder, LmdbWasmTestBuilder, UpgradeRequestBuilder, DEFAULT_ACCOUNT_ADDR,
     DEFAULT_ACCOUNT_SECRET_KEY, LOCAL_GENESIS_REQUEST,
 };
 use casper_execution_engine::{
     engine_state::{Error as StateError, SessionDataV1, SessionInputData},
     execution::ExecError,
 };
 use casper_types::{
-    ApiError, BlockTime, InitiatorAddr, PricingMode, RuntimeArgs, Transaction, TransactionArgs,
-    TransactionEntryPoint, TransactionTarget, TransactionV1Builder,
+    ApiError, BlockTime, EraId, InitiatorAddr, Key, PricingMode, ProtocolVersion, RuntimeArgs,
+    Transaction, TransactionArgs, TransactionEntryPoint, TransactionTarget, TransactionV1Builder,
 };
 
 const CONTRACT: &str = "do_nothing_stored.wasm";
@@ -32,10 +33,11 @@ fn should_allow_add_contract_version_via_deploy() {
     builder.exec(deploy_request).expect_success().commit();
 }
 
-fn try_add_contract_version(is_install_upgrade: bool, should_succeed: bool) {
-    let mut builder = LmdbWasmTestBuilder::default();
-    builder.run_genesis(LOCAL_GENESIS_REQUEST.clone()).commit();
-
+fn try_add_contract_version(
+    is_install_upgrade: bool,
+    should_succeed: bool,
+    mut builder: LmdbWasmTestBuilder,
+) {
     let module_bytes = utils::read_wasm_file(CONTRACT);
 
     let txn = TransactionV1Builder::new_session(
@@ -150,11 +152,46 @@ fn to_v1_session_input_data<'a>(
 #[ignore]
 #[test]
 fn should_allow_add_contract_version_via_transaction_v1_installer_upgrader() {
-    try_add_contract_version(true, true)
+    let mut builder = LmdbWasmTestBuilder::default();
+    builder.run_genesis(LOCAL_GENESIS_REQUEST.clone()).commit();
+    try_add_contract_version(true, true, builder)
 }
 
 #[ignore]
 #[test]
 fn should_disallow_add_contract_version_via_transaction_v1_standard() {
-    try_add_contract_version(false, false)
+    let mut builder = LmdbWasmTestBuilder::default();
+    builder.run_genesis(LOCAL_GENESIS_REQUEST.clone()).commit();
+    try_add_contract_version(false, false, builder)
+}
+
+#[ignore]
+#[test]
+fn should_allow_1x_user_to_add_contract_version_via_transaction_v1_installer_upgrader() {
+    let (mut builder, lmdb_fixture_state, _temp_dir) =
+        lmdb_fixture::builder_from_global_state_fixture_with_enable_ae(
+            lmdb_fixture::RELEASE_1_5_8,
+            true,
+        );
+    let old_protocol_version = lmdb_fixture_state.genesis_protocol_version();
+
+    let mut upgrade_request = UpgradeRequestBuilder::new()
+        .with_current_protocol_version(old_protocol_version)
+        .with_new_protocol_version(ProtocolVersion::from_parts(2, 0, 0))
+        .with_activation_point(EraId::new(1))
+        .with_enable_addressable_entity(true)
+        .build();
+
+    builder
+        .upgrade(&mut upgrade_request)
+        .expect_upgrade_success();
+
+    let account_as_1x = builder
+        .query(None, Key::Account(*DEFAULT_ACCOUNT_ADDR), &[])
+        .expect("must have stored value")
+        .as_account()
+        .is_some();
+
+    assert!(account_as_1x);
+    try_add_contract_version(true, true, builder)
 }

execution_engine_testing/tests/src/test/upgrade.rs

@@ -851,7 +851,10 @@ fn setup_upgrade_threshold_state() -> (LmdbWasmTestBuilder, AccountHash) {
     const UPGRADE_THRESHOLDS_FIXTURE: &str = "upgrade_thresholds";
 
     let (mut builder, lmdb_fixture_state, _temp_dir) =
-        crate::lmdb_fixture::builder_from_global_state_fixture(UPGRADE_THRESHOLDS_FIXTURE);
+        crate::lmdb_fixture::builder_from_global_state_fixture_with_enable_ae(
+            UPGRADE_THRESHOLDS_FIXTURE,
+            true,
+        );
 
     let current_protocol_version = lmdb_fixture_state.genesis_protocol_version();
 
@@ -866,7 +869,7 @@ fn setup_upgrade_threshold_state() -> (LmdbWasmTestBuilder, AccountHash) {
         .with_activation_point(activation_point)
         .with_new_gas_hold_handling(HoldBalanceHandling::Accrued)
         .with_new_gas_hold_interval(24 * 60 * 60 * 60)
-        .with_migrate_legacy_contracts(true)
+        .with_enable_addressable_entity(true)
         .build();
 
     builder
@@ -1001,16 +1004,6 @@ fn call_and_migrate_purse_holder_contract(migration_scenario: MigrationScenario)
         .map(PackageHash::new)
         .unwrap();
 
-    // There is only one version present, post migration there should also
-    // be only one.
-    let version_count = builder
-        .get_package(package_hash)
-        .expect("must have package")
-        .versions()
-        .version_count();
-
-    assert_eq!(version_count, 1usize);
-
     let execute_request = match migration_scenario {
         MigrationScenario::ByPackageName(maybe_contract_version) => {
             ExecuteRequestBuilder::versioned_contract_call_by_name(