From 4bfe14f04527efdb36b72d6eb764425c94b46ad3 Mon Sep 17 00:00:00 2001 From: Jernej Kos Date: Wed, 28 Sep 2022 09:07:49 +0200 Subject: [PATCH] runtime-sdk: Better handle nonces in the future --- runtime-sdk/src/context.rs | 12 +++- runtime-sdk/src/dispatcher.rs | 55 ++++++++++++++---- runtime-sdk/src/modules/accounts/mod.rs | 76 ++++++++++++++++++++----- runtime-sdk/src/modules/core/mod.rs | 4 ++ 4 files changed, 119 insertions(+), 28 deletions(-) diff --git a/runtime-sdk/src/context.rs b/runtime-sdk/src/context.rs index f79101042f..089b05b50c 100644 --- a/runtime-sdk/src/context.rs +++ b/runtime-sdk/src/context.rs @@ -36,11 +36,13 @@ pub enum Mode { ExecuteTx, CheckTx, SimulateTx, + PreScheduleTx, } const MODE_CHECK_TX: &str = "check_tx"; const MODE_EXECUTE_TX: &str = "execute_tx"; const MODE_SIMULATE_TX: &str = "simulate_tx"; +const MODE_PRE_SCHEDULE_TX: &str = "pre_schedule_tx"; impl fmt::Display for Mode { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -54,6 +56,7 @@ impl From<&Mode> for &'static str { Mode::CheckTx => MODE_CHECK_TX, Mode::ExecuteTx => MODE_EXECUTE_TX, Mode::SimulateTx => MODE_SIMULATE_TX, + Mode::PreScheduleTx => MODE_PRE_SCHEDULE_TX, } } } @@ -89,7 +92,12 @@ pub trait Context { /// Whether the transaction is just being checked for validity. fn is_check_only(&self) -> bool { - self.mode() == Mode::CheckTx + self.mode() == Mode::CheckTx || self.mode() == Mode::PreScheduleTx + } + + /// Whether the transaction is just being checked for validity before being scheduled. + fn is_pre_schedule(&self) -> bool { + self.mode() == Mode::PreScheduleTx } /// Whether the transaction is just being simulated. @@ -122,7 +130,7 @@ pub trait Context { .unwrap_or_default() } // When just checking a transaction, we always want to be fast and skip contracts. - Mode::CheckTx => false, + Mode::CheckTx | Mode::PreScheduleTx => false, } } diff --git a/runtime-sdk/src/dispatcher.rs b/runtime-sdk/src/dispatcher.rs index 3a05d4bea2..ae0a16550f 100644 --- a/runtime-sdk/src/dispatcher.rs +++ b/runtime-sdk/src/dispatcher.rs @@ -115,6 +115,8 @@ pub struct DispatchOptions<'a> { pub tx_index: usize, /// Optionally only allow methods for which the provided authorizer closure returns true. pub method_authorizer: Option<&'a dyn Fn(&str) -> bool>, + /// Optionally skip authentication. + pub skip_authentication: bool, } /// The runtime dispatcher. @@ -244,8 +246,10 @@ impl Dispatcher { opts: &DispatchOptions<'_>, ) -> Result { // Run pre-processing hooks. - if let Err(err) = R::Modules::authenticate_tx(ctx, &tx) { - return Ok(err.into_call_result().into()); + if !opts.skip_authentication { + if let Err(err) = R::Modules::authenticate_tx(ctx, &tx) { + return Ok(err.into_call_result().into()); + } } let tx_auth_info = tx.auth_info.clone(); let is_read_only = tx.call.read_only; @@ -656,21 +660,50 @@ impl transaction::dispatcher::Dispatcher for Dispatche } // Determine the current transaction index. - let index = new_batch.len(); + let tx_index = new_batch.len(); // First run the transaction in check tx mode in a separate subcontext. If - // that fails, skip and reject transaction. - let check_result = ctx.with_child(Mode::CheckTx, |mut ctx| { - Self::dispatch_tx(&mut ctx, tx_size, tx.clone(), index) - })?; - if let module::CallResult::Failed { .. } = check_result.result { - // Skip and reject transaction. - tx_reject_hashes.push(Hash::digest_bytes(&raw_tx)); + // that fails, skip and (sometimes) reject transaction. + let skip = + ctx.with_child(Mode::PreScheduleTx, |mut ctx| -> Result<_, Error> { + // First authenticate the transaction to get any nonce related errors. + match R::Modules::authenticate_tx(&mut ctx, &tx) { + Err(modules::core::Error::FutureNonce) => { + // Only skip transaction as it may become valid in the future. + return Ok(true); + } + Err(_) => { + // Skip and reject the transaction. + } + Ok(_) => { + // Run additional checks on the transaction. + let check_result = Self::dispatch_tx_opts( + &mut ctx, + tx.clone(), + &DispatchOptions { + tx_size, + tx_index, + skip_authentication: true, // Already done. + ..Default::default() + }, + )?; + if check_result.result.is_success() { + // Checks successful, execute transaction as usual. + return Ok(false); + } + } + } + + // Skip and reject the transaction. + tx_reject_hashes.push(Hash::digest_bytes(&raw_tx)); + Ok(true) + })?; + if skip { continue; } new_batch.push(raw_tx); - results.push(Self::execute_tx(ctx, tx_size, tx, index)?); + results.push(Self::execute_tx(ctx, tx_size, tx, tx_index)?); } // If there's more room in the block and we got the maximum number of diff --git a/runtime-sdk/src/modules/accounts/mod.rs b/runtime-sdk/src/modules/accounts/mod.rs index 593a80d9ac..30fd53024b 100644 --- a/runtime-sdk/src/modules/accounts/mod.rs +++ b/runtime-sdk/src/modules/accounts/mod.rs @@ -1,5 +1,6 @@ //! Accounts module. use std::{ + cmp::Ordering, collections::{BTreeMap, BTreeSet}, convert::TryInto, }; @@ -16,7 +17,9 @@ use crate::{ modules, modules::core::{Error as CoreError, API as _}, runtime::Runtime, - sdk_derive, storage, + sdk_derive, + sender::SenderMeta, + storage, storage::Prefix, types::{ address::{Address, SignatureAddressSpec}, @@ -32,6 +35,10 @@ pub mod types; /// Unique module name. const MODULE_NAME: &str = "accounts"; +/// Maximum delta that the transaction nonce can be in the future from the current nonce to still +/// be accepted during transaction checks. +const MAX_CHECK_NONCE_FUTURE_DELTA: u64 = 10; + /// Errors emitted by the accounts module. #[derive(Error, Debug, oasis_runtime_sdk_macros::Error)] pub enum Error { @@ -230,7 +237,7 @@ pub trait API { fn check_signer_nonces( ctx: &mut C, tx_auth_info: &AuthInfo, - ) -> Result, modules::core::Error>; + ) -> Result; /// Update transaction signer account nonces. fn update_signer_nonces( @@ -623,7 +630,10 @@ impl API for Module { fn check_signer_nonces( ctx: &mut C, auth_info: &AuthInfo, - ) -> Result, modules::core::Error> { + ) -> Result { + let is_pre_schedule = ctx.is_pre_schedule(); + let is_check_only = ctx.is_check_only(); + // TODO: Optimize the check/update pair so that the accounts are // fetched only once. let params = Self::params(ctx.runtime_state()); @@ -631,23 +641,62 @@ impl API for Module { let mut store = storage::PrefixStore::new(ctx.runtime_state(), &MODULE_NAME); let accounts = storage::TypedStore::new(storage::PrefixStore::new(&mut store, &state::ACCOUNTS)); - let mut payer = None; + let mut sender = None; for si in auth_info.signer_info.iter() { let address = si.address_spec.address(); let account: types::Account = accounts.get(&address).unwrap_or_default(); - if account.nonce != si.nonce { - // Reject unles nonce checking is disabled. - if !params.debug_disable_nonce_check { + + // First signer pays for the fees and is considered the sender. + if sender.is_none() { + sender = Some(SenderMeta { + address, + tx_nonce: si.nonce, + state_nonce: account.nonce, + }); + } + + // When nonce checking is disabled, skip the rest of the checks. + if params.debug_disable_nonce_check { + continue; + } + + // Check signer nonce against the corresponding account nonce. + match si.nonce.cmp(&account.nonce) { + Ordering::Less => { + // In the past and will never become valid, reject. return Err(modules::core::Error::InvalidNonce); } - } + Ordering::Equal => {} // Ok. + Ordering::Greater => { + // If too much in the future, reject. + if si.nonce - account.nonce > MAX_CHECK_NONCE_FUTURE_DELTA { + return Err(modules::core::Error::InvalidNonce); + } - // First signer pays for the fees. - if payer.is_none() { - payer = Some(address); + // If in the future and this is before scheduling, reject with separate error + // that will make the scheduler skip the transaction. + if is_pre_schedule { + return Err(modules::core::Error::FutureNonce); + } + + // If in the future and this is during execution, reject. + if !is_check_only { + return Err(modules::core::Error::InvalidNonce); + } + + // If in the future and this is during checks, accept. + } } } - Ok(payer) + + // Configure the sender. + let sender = sender.expect("at least one signer is always present"); + let sender_address = sender.address; + if is_check_only { + ::Core::set_sender_meta(ctx, sender); + } + + Ok(sender_address) } fn update_signer_nonces( @@ -869,8 +918,6 @@ impl module::TransactionHandler for Module { // Charge the specified amount of fees. if !tx.auth_info.fee.amount.amount().is_zero() { - let payer = payer.expect("at least one signer is always present"); - if ctx.is_check_only() { // Do not update balances during transaction checks. In case of checks, only do it // after all the other checks have already passed as otherwise retrying the @@ -918,7 +965,6 @@ impl module::TransactionHandler for Module { // Update payer balance. let payer = Self::check_signer_nonces(ctx, tx_auth_info).unwrap(); // Already checked. - let payer = payer.unwrap(); // Already checked. let amount = &tx_auth_info.fee.amount; Self::sub_amount(ctx.runtime_state(), payer, amount).unwrap(); // Already checked. diff --git a/runtime-sdk/src/modules/core/mod.rs b/runtime-sdk/src/modules/core/mod.rs index cde9b33373..a3e8bf024e 100644 --- a/runtime-sdk/src/modules/core/mod.rs +++ b/runtime-sdk/src/modules/core/mod.rs @@ -130,6 +130,10 @@ pub enum Error { #[sdk_error(code = 25)] ReadOnlyTransaction, + #[error("future nonce")] + #[sdk_error(code = 26)] + FutureNonce, + #[error("{0}")] #[sdk_error(transparent)] TxSimulationFailed(#[from] TxSimulationFailure),