Fix: remove unused state variable bridge_prover_id (#749)

This PR is a modified version of #726 

Normally I would not make a whole separate PR, but I ended up starting
this work from scratch off of `develop` because I could not understand
why the gas costs in `repro` were increased so much in the old PR. I
have since figured out that it was some change in `Cargo.lock` that was
causing the problem, but by that point I already had all this code
finished in a separate branch so I thought it would be easier to make a
separate PR at that point.

Key differences from #726 :

1. The `repro` tests pass without any changes to the gas values (i.e.
this implementation does not create a significant gas increase). This is
mostly due to leaving `Cargo.lock` untouched, but there are a few other
improvements as well (see below).
2. Automatically migrate the state if the V1 state is found. This change
means that after the first transaction, the fallback deserialization
logic will not be executed and hence reading the state will be as
efficient as it always was (as exemplified by the `repro` gas costs
remaining unchanged).
3. Use `Cow` in `BorshableEngineState` so that `From<&EngineState> for
BorshableEngineState` does not need to clone the owner account id.
4. Ensure removing the `bridge_prover_id` field from `NewCallArgs` is
done in a backwards compatible way. This is not a performance
improvement, but it is still a critical change. We must ensure that
changes to the format of top-level contract functions are always
backwards compatible otherwise the Borealis Engine and Borealis Refiner
will not be able to parse old transactions (it will fail to deserialize
the arguments).

engine-standalone-storage/src/relayer_db/mod.rs

@@ -218,7 +218,6 @@ mod test {
         let engine_state = state::EngineState {
             chain_id: aurora_engine_types::types::u256_to_arr(&1_313_161_555.into()),
             owner_id: "aurora".parse().unwrap(),
-            bridge_prover_id: "prover.bridge.near".parse().unwrap(),
             upgrade_delay_blocks: 0,
         };
 
@@ -241,7 +240,7 @@ mod test {
                     io,
                     &engine_state.owner_id,
                     parameters::InitCallArgs {
-                        prover_account: engine_state.bridge_prover_id.clone(),
+                        prover_account: "prover.bridge.near".parse().unwrap(),
                         eth_custodian_address: "6bfad42cfc4efc96f529d786d643ff4a8b89fa52"
                             .to_string(),
                         metadata: FungibleTokenMetadata::default(),

engine-tests/src/test_utils/mod.rs

@@ -20,7 +20,9 @@ use rlp::RlpStream;
 use std::borrow::Cow;
 
 use crate::prelude::fungible_token::{FungibleToken, FungibleTokenMetadata};
-use crate::prelude::parameters::{InitCallArgs, NewCallArgs, SubmitResult, TransactionStatus};
+use crate::prelude::parameters::{
+    InitCallArgs, LegacyNewCallArgs, SubmitResult, TransactionStatus,
+};
 use crate::prelude::transactions::{
     eip_1559::{self, SignedTransaction1559, Transaction1559},
     eip_2930::{self, SignedTransaction2930, Transaction2930},
@@ -635,7 +637,7 @@ impl ExecutionProfile {
 
 pub fn deploy_evm() -> AuroraRunner {
     let mut runner = AuroraRunner::default();
-    let args = NewCallArgs {
+    let args = LegacyNewCallArgs {
         chain_id: crate::prelude::u256_to_arr(&U256::from(runner.chain_id)),
         owner_id: str_to_account_id(runner.aurora_account_id.as_str()),
         bridge_prover_id: str_to_account_id("bridge_prover.near"),
@@ -872,12 +874,18 @@ pub fn panic_on_fail(status: TransactionStatus) {
     }
 }
 
+/// Checks if `total_gas` is within 1 Tgas of `tgas_bound`.
 pub fn assert_gas_bound(total_gas: u64, tgas_bound: u64) {
-    // Add 1 to round up
-    let tgas_used = (total_gas / 1_000_000_000_000) + 1;
+    const TERA: i128 = 1_000_000_000_000;
+    let total_gas: i128 = total_gas.into();
+    let tgas_bound: i128 = i128::from(tgas_bound) * TERA;
+    let diff = (total_gas - tgas_bound).abs() / TERA;
     assert_eq!(
-        tgas_used, tgas_bound,
-        "{tgas_used} Tgas is not equal to {tgas_bound} Tgas",
+        diff,
+        0,
+        "{} Tgas is not equal to {} Tgas",
+        total_gas / TERA,
+        tgas_bound / TERA,
     );
 }
 

engine-tests/src/test_utils/standalone/mocks/mod.rs

@@ -1,7 +1,7 @@
 use crate::test_utils;
 use aurora_engine::fungible_token::FungibleTokenMetadata;
 use aurora_engine::parameters::{
-    FinishDepositCallArgs, InitCallArgs, NEP141FtOnTransferArgs, NewCallArgs,
+    FinishDepositCallArgs, InitCallArgs, NEP141FtOnTransferArgs, NewCallArgsV2,
 };
 use aurora_engine::{engine, state};
 use aurora_engine_sdk::env::{Env, DEFAULT_PREPAID_GAS};
@@ -49,10 +49,9 @@ pub fn default_env(block_height: u64) -> aurora_engine_sdk::env::Fixed {
 }
 
 pub fn init_evm<I: IO + Copy, E: Env>(mut io: I, env: &E, chain_id: u64) {
-    let new_args = NewCallArgs {
+    let new_args = NewCallArgsV2 {
         chain_id: aurora_engine_types::types::u256_to_arr(&U256::from(chain_id)),
         owner_id: env.current_account_id(),
-        bridge_prover_id: test_utils::str_to_account_id("bridge_prover.near"),
         upgrade_delay_blocks: 1,
     };
 

engine-tests/src/tests/eth_connector.rs

@@ -7,7 +7,7 @@ use aurora_engine::connector::{
 };
 use aurora_engine::fungible_token::FungibleTokenMetadata;
 use aurora_engine::parameters::{
-    InitCallArgs, NewCallArgs, RegisterRelayerCallArgs, WithdrawResult,
+    InitCallArgs, LegacyNewCallArgs, RegisterRelayerCallArgs, WithdrawResult,
 };
 use aurora_engine_types::types::{Fee, NEP141Wei};
 use borsh::{BorshDeserialize, BorshSerialize};
@@ -67,7 +67,7 @@ fn init_contract(
         .call(
             contract_name.parse().unwrap(),
             "new",
-            &NewCallArgs {
+            &LegacyNewCallArgs {
                 chain_id: [0u8; 32],
                 owner_id: str_to_account_id(master_account.account_id.clone().as_str()),
                 bridge_prover_id: str_to_account_id("bridge.prover.near"),

engine-tests/src/tests/repro.rs

@@ -152,6 +152,11 @@ fn repro_common(context: &ReproContext) {
     let tx_hex = std::fs::read_to_string(input_path).unwrap();
     let tx_bytes = hex::decode(tx_hex.trim()).unwrap();
 
+    // Make a random call that touches the Engine state to force the lazy migration
+    runner
+        .call("get_chain_id", "relay.aurora", Vec::new())
+        .unwrap();
+    // Run benchmark post-migration
     let outcome = runner.call("submit", "relay.aurora", tx_bytes).unwrap();
     let profile = ExecutionProfile::new(&outcome);
     let submit_result =

engine-tests/src/tests/sanity.rs

@@ -140,21 +140,19 @@ fn test_state_format() {
     // change the binary format of the `EngineState` then we will know
     // about it. This is important because changing the state format will
     // break the contract unless we do a state migration.
-    let args = aurora_engine::parameters::NewCallArgs {
+    let args = aurora_engine::parameters::NewCallArgsV2 {
         chain_id: aurora_engine_types::types::u256_to_arr(&666.into()),
         owner_id: "boss".parse().unwrap(),
-        bridge_prover_id: "prover_mcprovy_face".parse().unwrap(),
         upgrade_delay_blocks: 3,
     };
     let state: aurora_engine::state::EngineState = args.into();
     let expected_hex: String = [
-        "000000000000000000000000000000000000000000000000000000000000029a",
+        "01000000000000000000000000000000000000000000000000000000000000029a",
         "04000000626f7373",
-        "1300000070726f7665725f6d6370726f76795f66616365",
         "0300000000000000",
     ]
     .concat();
-    assert_eq!(hex::encode(state.try_to_vec().unwrap()), expected_hex);
+    assert_eq!(hex::encode(state.borsh_serialize().unwrap()), expected_hex);
 }
 
 fn generate_code(len: usize) -> Vec<u8> {

engine-tests/src/tests/standalone/json_snapshot.rs

@@ -1,4 +1,5 @@
 use crate::test_utils::{self, standalone};
+use aurora_engine_types::storage::{self, KeyPrefix};
 use aurora_engine_types::types::{Address, Wei};
 use aurora_engine_types::{H160, U256};
 use engine_standalone_storage::json_snapshot;
@@ -92,13 +93,16 @@ fn test_produce_snapshot() {
         .get_snapshot(runner.env.block_height)
         .unwrap();
 
+    let state_key = storage::bytes_to_key(KeyPrefix::Config, aurora_engine::state::STATE_KEY);
     // New snapshot should still contain all keys from initial snapshot
     for entry in snapshot.result.values {
         let key = aurora_engine_sdk::base64::decode(entry.key).unwrap();
         // skip the eth-connector keys; they were changed by minting the new account
-        if key[0..3] == [7, 6, 1] {
+        // also skip the state since it was automatically migrated from the old format
+        if (key[0..3] == [7, 6, 1]) || (key == state_key) {
             continue;
         }
+        println!("{}", hex::encode(&key));
         let value = aurora_engine_sdk::base64::decode(entry.value).unwrap();
         assert_eq!(computed_snapshot.get(&key).unwrap(), &value);
     }

engine-tests/src/tests/standalone/sanity.rs

@@ -18,7 +18,6 @@ fn test_deploy_code() {
     let state = state::EngineState {
         chain_id,
         owner_id: owner_id.clone(),
-        bridge_prover_id: "mr_the_prover".parse().unwrap(),
         upgrade_delay_blocks: 0,
     };
     let origin = Address::new(H160([0u8; 20]));

engine-tests/src/tests/state_migration.rs

@@ -1,7 +1,7 @@
 use crate::prelude::U256;
 use crate::test_utils::{self, str_to_account_id, AuroraRunner};
 use aurora_engine::fungible_token::FungibleTokenMetadata;
-use aurora_engine::parameters::{InitCallArgs, NewCallArgs};
+use aurora_engine::parameters::{InitCallArgs, NewCallArgs, NewCallArgsV2};
 use borsh::BorshSerialize;
 use near_sdk_sim::{ExecutionResult, UserAccount};
 use std::fs;
@@ -56,12 +56,11 @@ pub fn deploy_evm() -> AuroraAccount {
         5 * near_sdk_sim::STORAGE_AMOUNT,
     );
     let prover_account = str_to_account_id("prover.near");
-    let new_args = NewCallArgs {
+    let new_args = NewCallArgs::V2(NewCallArgsV2 {
         chain_id: crate::prelude::u256_to_arr(&U256::from(aurora_runner.chain_id)),
         owner_id: str_to_account_id(main_account.account_id.as_str()),
-        bridge_prover_id: prover_account.clone(),
         upgrade_delay_blocks: 1,
-    };
+    });
     main_account
         .call(
             contract_account.account_id.clone(),

engine/src/engine.rs

@@ -1023,7 +1023,7 @@ pub fn compute_block_hash(chain_id: [u8; 32], block_height: u64, account_id: &[u
 }
 
 #[must_use]
-pub fn get_authorizer<I: IO>(io: &I) -> EngineAuthorizer {
+pub fn get_authorizer<I: IO + Copy>(io: &I) -> EngineAuthorizer {
     // TODO: a temporary use the owner account only until the engine adapts std with near-plugins
     state::get_state(io)
         .map(|state| EngineAuthorizer::from_accounts(once(state.owner_id)))

engine/src/lib.rs

@@ -127,7 +127,8 @@ mod contract {
             sdk::panic_utf8(b"ERR_ALREADY_INITIALIZED");
         }
 
-        let args: NewCallArgs = io.read_input_borsh().sdk_unwrap();
+        let bytes = io.read_input().to_vec();
+        let args = NewCallArgs::deserialize(&bytes).sdk_expect(errors::ERR_BORSH_DESERIALIZE);
         state::set_state(&mut io, &args.into()).sdk_unwrap();
     }
 

engine/src/parameters.rs

@@ -11,9 +11,25 @@ pub use aurora_engine_types::parameters::engine::{
 use aurora_engine_types::types::{Fee, NEP141Wei, Yocto};
 use serde::{Deserialize, Serialize};
 
-/// Borsh-encoded parameters for the `new` function.
+/// Parameters for the `new` function.
 #[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)]
-pub struct NewCallArgs {
+pub enum NewCallArgs {
+    V1(LegacyNewCallArgs),
+    V2(NewCallArgsV2),
+}
+
+impl NewCallArgs {
+    pub fn deserialize(bytes: &[u8]) -> Result<Self, borsh::maybestd::io::Error> {
+        Self::try_from_slice(bytes).map_or_else(
+            |_| LegacyNewCallArgs::try_from_slice(bytes).map(Self::V1),
+            Ok,
+        )
+    }
+}
+
+/// Old Borsh-encoded parameters for the `new` function.
+#[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)]
+pub struct LegacyNewCallArgs {
     /// Chain id, according to the EIP-115 / ethereum-lists spec.
     pub chain_id: RawU256,
     /// Account which can upgrade this contract.
@@ -26,6 +42,17 @@ pub struct NewCallArgs {
     pub upgrade_delay_blocks: u64,
 }
 
+#[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)]
+pub struct NewCallArgsV2 {
+    /// Chain id, according to the EIP-115 / ethereum-lists spec.
+    pub chain_id: RawU256,
+    /// Account which can upgrade this contract.
+    /// Use empty to disable updatability.
+    pub owner_id: AccountId,
+    /// How many blocks after staging upgrade can deploy it.
+    pub upgrade_delay_blocks: u64,
+}
+
 /// Borsh-encoded parameters for the `set_owner` function.
 #[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize, Serialize, Deserialize)]
 pub struct SetOwnerArgs {