From 3a544df0d6d500f2c79b2f722609bc3edbf6cb20 Mon Sep 17 00:00:00 2001 From: Michael Birch Date: Wed, 23 Aug 2023 11:38:50 -0400 Subject: [PATCH] Fix(standalone): Do not eagerly commit transactions to the DB (#825) ## Description The [Borealis refiner](https://github.com/aurora-is-near/borealis-engine-lib/tree/main/refiner-lib) was experiencing a performance issue where it would become very slow at processing blocks. Profiling revealed the reason was increasing time to look up the `ENGINE_STATE` key when replaying transactions (this replay is necessary to correctly process batch transactions on Near). After some investigation, we realized the cause of this slow lookup was due to that key being constantly written and deleted by every transaction. The reason for this churn is because the Engine logic was changed to automatically migrate its state, but of course old transactions did not have that logic and therefore the replay would compute an incorrect state diff relative to what is reported in the Near block. In such cases the replay changes to the DB are deleted and the correct diff from the Near block is used instead. To avoid this DB churn, this PR changes the standalone engine so that it will not commit to the DB right away when consuming a block. Instead it is now up to clients of the standalone engine to commit the changes themselves (after performing any validation). A PR on Borealis Refiner will make that change there after this PR is merged. Note this does not address the larger issue of replay accuracy. In theory the Borealis Refiner should use the code that existed at the time when replaying an old transaction. However, this is not so easy to accomplish which is why we are proposing this solution instead. It is an immediate fix to the performance issue that can keep the Refiner running while we address the more fundamental problem. ## Performance / NEAR gas cost considerations No impact to on-chain Aurora contract; changes to standalone only. ## Testing Updates to existing tests --- engine-standalone-storage/src/sync/mod.rs | 59 +++++++++++++++++------ engine-tests/src/tests/standalone/sync.rs | 23 ++++++--- 2 files changed, 60 insertions(+), 22 deletions(-) diff --git a/engine-standalone-storage/src/sync/mod.rs b/engine-standalone-storage/src/sync/mod.rs index 407804dc8..1164ab5c2 100644 --- a/engine-standalone-storage/src/sync/mod.rs +++ b/engine-standalone-storage/src/sync/mod.rs @@ -1,3 +1,4 @@ +use crate::engine_state::EngineStateAccess; use aurora_engine::parameters::SubmitArgs; use aurora_engine::pausables::{ EnginePrecompilesPauser, PausedPrecompilesManager, PrecompileFlags, @@ -8,7 +9,10 @@ use aurora_engine::{ state, xcc, }; use aurora_engine_modexp::ModExpAlgorithm; -use aurora_engine_sdk::env::{self, Env, DEFAULT_PREPAID_GAS}; +use aurora_engine_sdk::{ + env::{self, Env, DEFAULT_PREPAID_GAS}, + io::IO, +}; use aurora_engine_transactions::EthTransactionKind; use aurora_engine_types::{ account_id::AccountId, @@ -21,7 +25,6 @@ use std::{io, str::FromStr}; pub mod types; -use crate::engine_state::EngineStateAccess; use crate::{error::ParseTransactionKindError, BlockMetadata, Diff, Storage}; use types::{Message, TransactionKind, TransactionKindTag, TransactionMessage}; @@ -226,6 +229,9 @@ pub fn parse_transaction_kind( Ok(tx_kind) } +/// Note: this function does not automatically commit transaction messages to the storage. +/// If you want the transaction diff committed then you must call the `commit` method on +/// the outcome of this function. pub fn consume_message( storage: &mut Storage, message: Message, @@ -255,19 +261,16 @@ pub fn consume_message( let (tx_hash, diff, result) = storage .with_engine_access(block_height, transaction_position, &[], |io| { - execute_transaction::( + execute_transaction::<_, M, _>( transaction_message.as_ref(), block_height, &block_metadata, engine_account_id, io, + EngineStateAccess::get_transaction_diff, ) }) .result; - match result.as_ref() { - Err(_) | Ok(Some(TransactionExecutionResult::Submit(Err(_)))) => (), // do not persist if Engine encounters an error - _ => storage.set_transaction_included(tx_hash, &transaction_message, &diff)?, - } let outcome = TransactionIncludedOutcome { hash: tx_hash, info: *transaction_message, @@ -291,12 +294,13 @@ pub fn execute_transaction_message( let block_metadata = storage.get_block_metadata(block_hash)?; let engine_account_id = storage.get_engine_account_id()?; let result = storage.with_engine_access(block_height, transaction_position, &[], |io| { - execute_transaction::( + execute_transaction::<_, M, _>( &transaction_message, block_height, &block_metadata, engine_account_id, io, + EngineStateAccess::get_transaction_diff, ) }); let (tx_hash, diff, maybe_result) = result.result; @@ -309,17 +313,23 @@ pub fn execute_transaction_message( Ok(outcome) } -fn execute_transaction<'db, M: ModExpAlgorithm + 'static>( +pub fn execute_transaction( transaction_message: &TransactionMessage, block_height: u64, block_metadata: &BlockMetadata, engine_account_id: AccountId, - io: EngineStateAccess<'db, 'db, 'db>, + io: I, + get_diff: F, ) -> ( H256, Diff, Result, error::Error>, -) { +) +where + I: IO + Copy, + M: ModExpAlgorithm + 'static, + F: FnOnce(&I) -> Diff, +{ let signer_account_id = transaction_message.signer.clone(); let predecessor_account_id = transaction_message.caller.clone(); let relayer_address = @@ -390,7 +400,7 @@ fn execute_transaction<'db, M: ModExpAlgorithm + 'static>( (tx_hash, result) } other => { - let result = non_submit_execute::( + let result = non_submit_execute::( other, io, env, @@ -401,7 +411,7 @@ fn execute_transaction<'db, M: ModExpAlgorithm + 'static>( } }; - let diff = io.get_transaction_diff(); + let diff = get_diff(&io); (tx_hash, diff, result) } @@ -410,9 +420,9 @@ fn execute_transaction<'db, M: ModExpAlgorithm + 'static>( /// The `submit` transaction kind is special because it is the only one where the transaction hash /// differs from the NEAR receipt hash. #[allow(clippy::too_many_lines)] -fn non_submit_execute<'db, M: ModExpAlgorithm + 'static>( +fn non_submit_execute( transaction: &TransactionKind, - mut io: EngineStateAccess<'db, 'db, 'db>, + mut io: I, env: env::Fixed, relayer_address: Address, promise_data: &[Option>], @@ -709,6 +719,15 @@ pub enum ConsumeMessageOutcome { TransactionIncluded(Box), } +impl ConsumeMessageOutcome { + pub fn commit(&self, storage: &mut Storage) -> Result<(), crate::error::Error> { + if let Self::TransactionIncluded(x) = self { + x.commit(storage)?; + } + Ok(()) + } +} + #[derive(Debug)] pub struct TransactionIncludedOutcome { pub hash: aurora_engine_types::H256, @@ -717,6 +736,16 @@ pub struct TransactionIncludedOutcome { pub maybe_result: Result, error::Error>, } +impl TransactionIncludedOutcome { + pub fn commit(&self, storage: &mut Storage) -> Result<(), crate::error::Error> { + match self.maybe_result.as_ref() { + Err(_) | Ok(Some(TransactionExecutionResult::Submit(Err(_)))) => (), // do not persist if Engine encounters an error + _ => storage.set_transaction_included(self.hash, &self.info, &self.diff)?, + }; + Ok(()) + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum TransactionExecutionResult { Submit(engine::EngineResult), diff --git a/engine-tests/src/tests/standalone/sync.rs b/engine-tests/src/tests/standalone/sync.rs index 9371bdb57..681ecc252 100644 --- a/engine-tests/src/tests/standalone/sync.rs +++ b/engine-tests/src/tests/standalone/sync.rs @@ -67,6 +67,7 @@ fn test_consume_deposit_message() { sync::ConsumeMessageOutcome::TransactionIncluded(outcome) => outcome, other => panic!("Unexpected outcome {other:?}"), }; + outcome.commit(&mut runner.storage).unwrap(); let finish_deposit_args = match outcome.maybe_result.unwrap().unwrap() { sync::TransactionExecutionResult::Promise(promise_args) => { @@ -99,6 +100,7 @@ fn test_consume_deposit_message() { sync::ConsumeMessageOutcome::TransactionIncluded(outcome) => outcome, other => panic!("Unexpected outcome {other:?}"), }; + outcome.commit(&mut runner.storage).unwrap(); let ft_on_transfer_args = match outcome.maybe_result.unwrap().unwrap() { sync::TransactionExecutionResult::Promise(promise_args) => { @@ -120,11 +122,12 @@ fn test_consume_deposit_message() { promise_data: Vec::new(), }; - sync::consume_message::( + let outcome = sync::consume_message::( &mut runner.storage, sync::types::Message::Transaction(Box::new(transaction_message)), ) .unwrap(); + outcome.commit(&mut runner.storage).unwrap(); assert_eq!(runner.get_balance(&recipient_address), deposit_amount); @@ -150,11 +153,12 @@ fn test_consume_deploy_message() { promise_data: Vec::new(), }; - sync::consume_message::( + let outcome = sync::consume_message::( &mut runner.storage, sync::types::Message::Transaction(Box::new(transaction_message)), ) .unwrap(); + outcome.commit(&mut runner.storage).unwrap(); let diff = runner .storage @@ -203,11 +207,12 @@ fn test_consume_deploy_erc20_message() { }; // Deploy ERC-20 (this would be the flow for bridging a new NEP-141 to Aurora) - sync::consume_message::( + let outcome = sync::consume_message::( &mut runner.storage, sync::types::Message::Transaction(Box::new(transaction_message)), ) .unwrap(); + outcome.commit(&mut runner.storage).unwrap(); let erc20_address = runner .storage @@ -241,11 +246,12 @@ fn test_consume_deploy_erc20_message() { }; // Mint new tokens (via ft_on_transfer flow, same as the bridge) - sync::consume_message::( + let outcome = sync::consume_message::( &mut runner.storage, sync::types::Message::Transaction(Box::new(transaction_message)), ) .unwrap(); + outcome.commit(&mut runner.storage).unwrap(); // Check balance is correct let deployed_token = ERC20( @@ -297,11 +303,12 @@ fn test_consume_ft_on_transfer_message() { promise_data: Vec::new(), }; - sync::consume_message::( + let outcome = sync::consume_message::( &mut runner.storage, sync::types::Message::Transaction(Box::new(transaction_message)), ) .unwrap(); + outcome.commit(&mut runner.storage).unwrap(); assert_eq!( runner.get_balance(&dest_address).raw().low_u128(), @@ -341,11 +348,12 @@ fn test_consume_call_message() { promise_data: Vec::new(), }; - sync::consume_message::( + let outcome = sync::consume_message::( &mut runner.storage, sync::types::Message::Transaction(Box::new(transaction_message)), ) .unwrap(); + outcome.commit(&mut runner.storage).unwrap(); assert_eq!(runner.get_balance(&recipient_address), transfer_amount); assert_eq!( @@ -391,11 +399,12 @@ fn test_consume_submit_message() { promise_data: Vec::new(), }; - sync::consume_message::( + let outcome = sync::consume_message::( &mut runner.storage, sync::types::Message::Transaction(Box::new(transaction_message)), ) .unwrap(); + outcome.commit(&mut runner.storage).unwrap(); assert_eq!(runner.get_balance(&recipient_address), transfer_amount); assert_eq!(