Skip to content

Commit

Permalink
Speed up signature check for orders with no pre-interactions (#2953)
Browse files Browse the repository at this point in the history
# 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 <martin.beckmann@protonmail.com>
  • Loading branch information
m-lord-renkse and MartinquaXD committed Sep 9, 2024
1 parent 067f45f commit 84f1840
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 10 deletions.
19 changes: 10 additions & 9 deletions crates/e2e/tests/e2e/eth_safe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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))
Expand All @@ -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,
Expand All @@ -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.");
Expand Down
37 changes: 36 additions & 1 deletion crates/shared/src/signature_validator/simulation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use {
super::{SignatureCheck, SignatureValidating, SignatureValidationError},
crate::ethcontract_error::EthcontractErrorType,
anyhow::Result,
contracts::ERC1271SignatureValidator,
ethcontract::Bytes,
ethrpc::Web3,
futures::future,
Expand All @@ -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(
Expand Down Expand Up @@ -73,7 +104,11 @@ impl SignatureValidating for Validator {
checks: Vec<SignatureCheck>,
) -> Vec<Result<(), SignatureValidationError>> {
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
Expand Down

0 comments on commit 84f1840

Please sign in to comment.