cosmos / cosmjs

The Swiss Army knife to power JavaScript based client solutions ranging from Web apps/explorers over browser extensions to server-side clients like faucets/scrapers.
https://cosmos.github.io/cosmjs/
Apache License 2.0
645 stars 330 forks source link

DirectSecp256k1HdWallet slow to sign a transaction #1467

Open tuloski opened 1 year ago

tuloski commented 1 year ago

Im using SigningCosmWasmClient with .connectWithSigner and passing as signer a DirectSecp256k1HdWallet. I'm using .sign() of SigningCosmWasmClient passing the fees and the explicitSignerData so the network doesn't has to be fetched to get account number, sequence and chainID. Still the signing takes around 1000 ms which looks an eternity. It seems most of the time is due to the getAccountsWithPrivkeys() of DirectSecp256k1HdWallet, which is called twice (one in the signDirect() of SigningCosmWasmClient and one in signDirect() of DirectSecp256k1HdWallet). Is it really that slow to derive private and public key?

webmaster128 commented 1 year ago

I came across something like this as well recently. A purely offline signing steps took between 12 and 26ms using latest deno as an execution environment, which I consider too slow as well.

I wonder if the two observations are for the same reason:

  1. How many accounts does your DirectSecp256k1HdWallet have?
  2. Which version of CosmJS do you use?
  3. Which runtime and version do you use?
  4. What operating system and hardware is this running on?
tuloski commented 1 year ago

DirectSecp256k1HdWallet has 1 account. Version 0.30.0 Ubuntu 16.04 core i7 16Gb ram

Moreover why are all async functions when they have nothing async?

webmaster128 commented 1 year ago

Ubuntu 16.04 core i7 16Gb ram

16.04 is very old. Are you using node? Which node --version?

Moreover why are all async functions when they have nothing async?

This often happens for interfaces where there can be asynchonous and synchonous implementations. Also it keeps the flexibility to e.g. change to a Wasm-based implementation that requires async.

tuloski commented 1 year ago

Yeah 16.04 old but I don't think it's the source of 1000ms signing time. Node is 17.0.1

tuloski commented 1 year ago

As additional info: I tried a minimal script to test only signing. The first sign always takes a lot (1000 to 2000 ms), the following less (300 to 800 ms). I need to check the code, but maybe the first time it computes something that is later saved in memory.

The sign itself (crypto_1.Secp256k1.createSignature) takes between 15 to 80ms. Most of the time is due to calling twice getKeyPair(hdPath) in DirectSecp256k1HdWallet.

Overall the time it takes for a SigningCosmWasmClient.sign() for the same messages, same signer, same everything is VERY VARIABLE which is weird IMO.

tuloski commented 1 year ago

Additional info: using DirectSecp256k1Wallet instead of DirectSecp256k1HdWallet (No HD wallet) the signing is WAY FASTER, in the order of 3-30 ms.

webmaster128 commented 1 year ago

The first sign always takes a lot (1000 to 2000 ms), the following less (300 to 800 ms). I need to check the code, but maybe the first time it computes something that is later saved in memory.

The other option is that the code is JIT compiled on demand (i.e. code gets faster when executed more often).

Most of the time is due to calling twice getKeyPair(hdPath) in DirectSecp256k1HdWallet. Additional info: using DirectSecp256k1Wallet instead of DirectSecp256k1HdWallet (No HD wallet) the signing is WAY FASTER, in the order of 3-30 ms.

Yeah then this is is because the keypaid is generated on demand and not cached inside of getAccounts(). The keypair could be cached or be computed in the DirectSecp256k1HdWallet constructor to get to the non-HD performance level.

You can try it with a local copy of class DirectSecp256k1HdWallet where you apply those changes.

tuloski commented 1 year ago

Yeah then this is is because the keypaid is generated on demand and not cached inside of getAccounts(). The keypair could be cached or be computed in the DirectSecp256k1HdWallet constructor to get to the non-HD performance level.

You can try it with a local copy of class DirectSecp256k1HdWallet where you apply those changes.

I tried that (cache when generating the wallet) and it has way better performances, but not yet in par with non-HD. PS: you can't compute it in the constructor because it requires an async function to await crypto_1.Secp256k1.makeKeypair and the constructor can't be async. I'm fine for now using non-hd signer but probably the cache thing is something nice to implement.

webmaster128 commented 1 year ago

I tried caching the Secp256k1Keypairs from getKeyPair and the AccountDataWithPrivkeys in getAccountsWithPrivkeys (keypair plus derived address) in a deno 1.36.1 environment. Those are the timings for the .sign call without network access:

Before:
    14ms
    16ms
    14ms
    10ms
    18ms

After 1 (Caching Secp256k1Keypair):
    [might have missed the first timing here]
    8ms
    7ms
    9ms
    5ms
    5ms
    3ms
    6ms

After 2 (Caching AccountDataWithPrivkey):
    14ms
    7ms
    5ms
    4ms
    3ms
    5ms

Looks like some solid performance gains. But both old and new values are nowhere near the 1000ms range in my case.