From 88578bfa1b48d6587bc18c112f31611c829dea38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Mon, 20 Nov 2023 10:35:05 +0100 Subject: [PATCH] refactor sig verification gas handling --- benches/host_env.rs | 7 ++++++- core/src/proto/types.rs | 38 ++++++++++++++++++------------------ core/src/types/key/mod.rs | 2 +- shared/src/vm/host_env.rs | 11 +++-------- tests/src/vm_host_env/mod.rs | 9 ++------- 5 files changed, 31 insertions(+), 36 deletions(-) diff --git a/benches/host_env.rs b/benches/host_env.rs index 14f4a8ca71..3b096f11ec 100644 --- a/benches/host_env.rs +++ b/benches/host_env.rs @@ -48,7 +48,12 @@ fn tx_section_signature_validation(c: &mut Criterion) { c.bench_function("tx_section_signature_validation", |b| { b.iter(|| { multisig - .verify_signature(&mut HashSet::new(), &pkim, &None, &mut None) + .verify_signature( + &mut HashSet::new(), + &pkim, + &None, + &mut || Ok(()), + ) .unwrap() }) }); diff --git a/core/src/proto/types.rs b/core/src/proto/types.rs index 5f6ded8e7c..1ac9830ad6 100644 --- a/core/src/proto/types.rs +++ b/core/src/proto/types.rs @@ -20,7 +20,7 @@ use sha2::{Digest, Sha256}; use thiserror::Error; use super::generated::types; -use crate::ledger::gas::{self, GasMetering, VpGasMeter, VERIFY_TX_SIG_GAS}; +use crate::ledger::gas; use crate::ledger::storage::{KeccakHasher, Sha256Hasher, StorageHasher}; use crate::types::account::AccountPublicKeysMap; use crate::types::address::Address; @@ -552,13 +552,16 @@ impl Signature { } /// Verify that the signature contained in this section is valid - pub fn verify_signature( + pub fn verify_signature( &self, verified_pks: &mut HashSet, public_keys_index_map: &AccountPublicKeysMap, signer: &Option
, - gas_meter: &mut Option<&mut VpGasMeter>, - ) -> std::result::Result { + consume_verify_sig_gas: &mut F, + ) -> std::result::Result + where + F: FnMut() -> std::result::Result<(), crate::ledger::gas::Error>, + { // Records whether there are any successful verifications let mut verifications = 0; match &self.signer { @@ -569,11 +572,7 @@ impl Signature { if let Some(pk) = public_keys_index_map.get_public_key_from_index(*idx) { - if let Some(meter) = gas_meter { - meter - .consume(VERIFY_TX_SIG_GAS) - .map_err(VerifySigError::OutOfGas)?; - } + consume_verify_sig_gas()?; common::SigScheme::verify_signature( &pk, &self.get_raw_hash(), @@ -594,11 +593,7 @@ impl Signature { if let Some(map_idx) = public_keys_index_map.get_index_from_public_key(pk) { - if let Some(meter) = gas_meter { - meter - .consume(VERIFY_TX_SIG_GAS) - .map_err(VerifySigError::OutOfGas)?; - } + consume_verify_sig_gas()?; common::SigScheme::verify_signature( pk, &self.get_raw_hash(), @@ -1245,15 +1240,18 @@ impl Tx { /// Verify that the section with the given hash has been signed by the given /// public key - pub fn verify_signatures( + pub fn verify_signatures( &self, hashes: &[crate::types::hash::Hash], public_keys_index_map: AccountPublicKeysMap, signer: &Option
, threshold: u8, max_signatures: Option, - gas_meter: &mut Option<&mut VpGasMeter>, - ) -> std::result::Result, Error> { + mut consume_verify_sig_gas: F, + ) -> std::result::Result, Error> + where + F: FnMut() -> std::result::Result<(), crate::ledger::gas::Error>, + { let max_signatures = max_signatures.unwrap_or(u8::MAX); // Records the public key indices used in successful signatures let mut verified_pks = HashSet::new(); @@ -1284,7 +1282,7 @@ impl Tx { &mut verified_pks, &public_keys_index_map, signer, - gas_meter, + &mut consume_verify_sig_gas, ) .map_err(|e| { if let VerifySigError::OutOfGas(inner) = e { @@ -1314,6 +1312,8 @@ impl Tx { /// Verify that the sections with the given hashes have been signed together /// by the given public key. I.e. this function looks for one signature that /// covers over the given slice of hashes. + /// Note that this method doesn't consider gas cost and hence it shouldn't + /// be used from txs or VPs. pub fn verify_signature( &self, public_key: &common::PublicKey, @@ -1325,7 +1325,7 @@ impl Tx { &None, 1, None, - &mut None, + || Ok(()), ) .map(|x| *x.first().unwrap()) .map_err(|_| Error::InvalidWrapperSignature) diff --git a/core/src/types/key/mod.rs b/core/src/types/key/mod.rs index 6f1a0eca68..b7a426607a 100644 --- a/core/src/types/key/mod.rs +++ b/core/src/types/key/mod.rs @@ -123,7 +123,7 @@ pub enum VerifySigError { #[error("Signature belongs to a different scheme from the public key.")] MismatchedScheme, #[error("Signature verification went out of gas: {0}")] - OutOfGas(crate::ledger::gas::Error), + OutOfGas(#[from] crate::ledger::gas::Error), } #[allow(missing_docs)] diff --git a/shared/src/vm/host_env.rs b/shared/src/vm/host_env.rs index 9f9ed07a18..4533cef72f 100644 --- a/shared/src/vm/host_env.rs +++ b/shared/src/vm/host_env.rs @@ -1928,7 +1928,7 @@ where &Some(signer), threshold, max_signatures, - &mut Some(gas_meter), + || gas_meter.consume(gas::VERIFY_TX_SIG_GAS), ) { Ok(_) => Ok(HostEnvResult::Success.to_i64()), Err(err) => match err { @@ -2100,6 +2100,7 @@ where .map_err(|e| TxRuntimeError::MemoryError(Box::new(e)))?; let sentinel = unsafe { env.ctx.sentinel.get() }; + let gas_meter = unsafe { env.ctx.gas_meter.get() }; tx_charge_gas(env, gas)?; let hashes = <[Hash; 1]>::try_from_slice(&hash_list) .map_err(TxRuntimeError::EncodingError)?; @@ -2127,19 +2128,13 @@ where let tx = unsafe { env.ctx.tx.get() }; - tx_charge_gas( - env, - gas::VERIFY_TX_SIG_GAS * public_keys_map.idx_to_pk.len() as u64, - )?; match tx.verify_signatures( &hashes, public_keys_map, &None, threshold, max_signatures, - // This uses VpGasMeter, so we're charging the gas here manually - // instead - &mut None, + || gas_meter.consume(gas::VERIFY_TX_SIG_GAS), ) { Ok(_) => Ok(HostEnvResult::Success.to_i64()), Err(err) => match err { diff --git a/tests/src/vm_host_env/mod.rs b/tests/src/vm_host_env/mod.rs index 820624b5a9..b5895a7d26 100644 --- a/tests/src/vm_host_env/mod.rs +++ b/tests/src/vm_host_env/mod.rs @@ -36,7 +36,6 @@ mod tests { use namada::types::time::DateTimeUtc; use namada::types::token::{self, Amount}; use namada::types::{address, key}; - use namada_core::ledger::gas::{TxGasMeter, VpGasMeter}; use namada_core::ledger::ibc::context::transfer_mod::testing::DummyTransferModule; use namada_core::ledger::ibc::Error as IbcActionError; use namada_test_utils::TestWasms; @@ -479,9 +478,7 @@ mod tests { &None, 1, None, - &mut Some(&mut VpGasMeter::new_from_tx_meter( - &TxGasMeter::new_from_sub_limit(u64::MAX.into()) - )) + || Ok(()) ) .is_ok() ); @@ -497,9 +494,7 @@ mod tests { &None, 1, None, - &mut Some(&mut VpGasMeter::new_from_tx_meter( - &TxGasMeter::new_from_sub_limit(u64::MAX.into()) - )) + || Ok(()) ) .is_err() );