From 84f1840e99d23966111a2c0cd0cd7fac6be4eb7a Mon Sep 17 00:00:00 2001 From: m-lord-renkse <160488334+m-lord-renkse@users.noreply.github.com> Date: Mon, 9 Sep 2024 17:02:52 +0200 Subject: [PATCH] Speed up signature check for orders with no pre-interactions (#2953) # Description Speed up signature check for orders with no pre-interactions by directly calling `isValidSignature` instead of our `Signatures` contract. # Changes - In the cases with no pre-interactions, directly call `isValidSignature` - Modify the eht safe e2e tests to use properly a SAFE transaction using the corresponding signature and forcing the new code to be tested/executed ## How to test 1. Acceptance tests # Performance improvement By looking at the [Signatures contract](https://github.com/cowprotocol/services/blob/main/crates/contracts/solidity/Signatures.sol), it is apparent that the speed it is apparent that the improvement won't be significant. We are mostly passing some small checks from the smart contract to Rust code, but the heavy operations are still done in the smart contract. I did some speed tests **locally** (I know the numbers will differ in real production, but I wanted to get some approximation). What I did is to do `Instant::now()` + `now.elapse()` wrapping the critical code, and after many tests, and I got in average these results: - Previous implementation: 2982196 nanoseconds - New implementation: 2388463 nanoseconds There is definitely an improvement. In production the improvement should be greater, because I was using anvil in localhost which reduces the latency significantly. --------- Co-authored-by: Martin Beckmann --- crates/e2e/tests/e2e/eth_safe.rs | 19 +++++----- .../src/signature_validator/simulation.rs | 37 ++++++++++++++++++- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/crates/e2e/tests/e2e/eth_safe.rs b/crates/e2e/tests/e2e/eth_safe.rs index 516cb2e9f7..43d0e40dd1 100644 --- a/crates/e2e/tests/e2e/eth_safe.rs +++ b/crates/e2e/tests/e2e/eth_safe.rs @@ -6,11 +6,9 @@ use { ethcontract::U256, model::{ order::{OrderCreation, OrderKind, BUY_ETH_ADDRESS}, - signature::EcdsaSigningScheme, + signature::{hashed_eip712_message, Signature}, }, - secp256k1::SecretKey, shared::ethrpc::Web3, - web3::signing::SecretKeyRef, }; #[tokio::test] @@ -31,6 +29,9 @@ async fn test(web3: Web3) { .await; token.mint(trader.address(), to_wei(4)).await; + safe.exec_call(token.approve(onchain.contracts().allowance, to_wei(4))) + .await; + token.mint(safe.address(), to_wei(4)).await; tx!( trader.account(), token.approve(onchain.contracts().allowance, to_wei(4)) @@ -49,7 +50,8 @@ async fn test(web3: Web3) { .await .unwrap(); assert_eq!(balance, 0.into()); - let order = OrderCreation { + let mut order = OrderCreation { + from: Some(safe.address()), sell_token: token.address(), sell_amount: to_wei(4), buy_token: BUY_ETH_ADDRESS, @@ -59,12 +61,11 @@ async fn test(web3: Web3) { kind: OrderKind::Sell, receiver: Some(safe.address()), ..Default::default() - } - .sign( - EcdsaSigningScheme::Eip712, + }; + order.signature = Signature::Eip1271(safe.sign_message(&hashed_eip712_message( &onchain.contracts().domain_separator, - SecretKeyRef::from(&SecretKey::from_slice(trader.private_key()).unwrap()), - ); + &order.data().hash_struct(), + ))); services.create_order(&order).await.unwrap(); tracing::info!("Waiting for trade."); diff --git a/crates/shared/src/signature_validator/simulation.rs b/crates/shared/src/signature_validator/simulation.rs index 34d7dc9db9..086ff0a113 100644 --- a/crates/shared/src/signature_validator/simulation.rs +++ b/crates/shared/src/signature_validator/simulation.rs @@ -7,6 +7,7 @@ use { super::{SignatureCheck, SignatureValidating, SignatureValidationError}, crate::ethcontract_error::EthcontractErrorType, anyhow::Result, + contracts::ERC1271SignatureValidator, ethcontract::Bytes, ethrpc::Web3, futures::future, @@ -18,16 +19,46 @@ pub struct Validator { signatures: contracts::support::Signatures, settlement: H160, vault_relayer: H160, + web3: Web3, } impl Validator { + /// The result returned from `isValidSignature` if the signature is correct + const IS_VALID_SIGNATURE_MAGIC_BYTES: &'static str = "1626ba7e"; + pub fn new(web3: &Web3, settlement: H160, vault_relayer: H160) -> Self { let web3 = ethrpc::instrumented::instrument_with_label(web3, "signatureValidation".into()); Self { signatures: contracts::support::Signatures::at(&web3, settlement), settlement, vault_relayer, + web3: web3.clone(), + } + } + + /// Simulate isValidSignature for the cases in which the order does not have + /// pre-interactions + async fn simulate_without_pre_interactions( + &self, + check: &SignatureCheck, + ) -> Result<(), SignatureValidationError> { + // Since there are no interactions (no dynamic conditions / complex pre-state + // change), the order's validity can be directly determined by whether + // the signature matches the expected hash of the order data, checked + // with isValidSignature method called on the owner's contract + let contract = ERC1271SignatureValidator::at(&self.web3, check.signer); + let magic_bytes = contract + .methods() + .is_valid_signature(Bytes(check.hash), Bytes(check.signature.clone())) + .call() + .await + .map(|value| hex::encode(value.0))?; + + if magic_bytes != Self::IS_VALID_SIGNATURE_MAGIC_BYTES { + return Err(SignatureValidationError::Invalid); } + + Ok(()) } async fn simulate( @@ -73,7 +104,11 @@ impl SignatureValidating for Validator { checks: Vec, ) -> Vec> { future::join_all(checks.into_iter().map(|check| async move { - self.simulate(&check).await?; + if check.interactions.is_empty() { + self.simulate_without_pre_interactions(&check).await?; + } else { + self.simulate(&check).await?; + } Ok(()) })) .await