Client/Monorepo: Use WASM Crypto (keccak256) for Hashing / Consistent Hash Function Overwrite #3192

holgerd77 · 2023-12-14T10:07:43Z

This was actually the original triggger for #3187, hehe. 😁

So this PR tests the @polkadot/wasm-crypto library as a replacement for the trie key hashing for the VM state manager in the client. Had this long on my plate to analyse this a bit better, since theoretically (re-)using a more native hash function should have a significant performance imact.

So I tested with the following early mainnet block (now starting directly with the block to avoid state caches):

npm run client:start -- --vmProfileBlocks --executeBlocks=227715

Block has 79 simple value transfer txs, so not much else is happening and mainly the sender and receiver state changes are written and come into play here.

Following results (with just the first two commits, so only exchanging for trie in execution):

Before (Entire Block): 280ms 277ms 276ms 283ms. 285ms 283ms. 280ms. 279ms
After. (Entire Block): 264ms. 270ms. 265ms. 268ms. 266ms. 262ms. 263ms. 265ms

Results are significant enough that I would continue here, especially since these are results for only exchanging things for the execution state trie, I expect this to get even substantially further if we do this holistically and exchange for all other places where keccak256 hashing is applied (tx trie validations, keccak256 precompile + opcode (there are these two, right?), guess some other things as well).

I will work a bit on this since I would like to get this done in a clean way. If I am not mistaken our whole "replace the hash function" functionality/API is generally not complete (so pretty sure e.g. for VM this is neither possible in the precompile or anywhere + things (the changed hash function) would at the end also needed to passed down to the lower level libraries.

codecov · 2023-12-14T10:10:49Z

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (a8f326a) 87.93% compared to head (1d90668) 87.94%.

Additional details and impacted files

Flag	Coverage Δ
block	`87.55% <100.00%> (+0.03%)`	⬆️
blockchain	`91.60% <ø> (ø)`
client	`84.68% <80.95%> (+0.05%)`	⬆️
common	`98.26% <100.00%> (+<0.01%)`	⬆️
devp2p	`82.05% <92.06%> (-0.11%)`	⬇️
ethash	`∅ <ø> (∅)`
evm	`76.92% <38.46%> (-0.04%)`	⬇️
genesis	`99.98% <ø> (ø)`
rlp	`?`
statemanager	`86.57% <86.48%> (+0.03%)`	⬆️
trie	`89.71% <100.00%> (+0.03%)`	⬆️
tx	`95.74% <100.00%> (+0.01%)`	⬆️
util	`89.13% <100.00%> (ø)`
vm	`80.81% <90.90%> (-0.02%)`	⬇️
wallet	`91.00% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

…manager for VM

acolytec3 · 2023-12-18T11:58:04Z

I'm liking this new approach. I wonder if we should do the same with KZG and move away from the global singleton

packages/client/bin/cli.ts

holgerd77 · 2023-12-29T14:22:02Z

packages/trie/src/trie.ts

@@ -93,6 +96,8 @@ export class Trie {
        throw new Error('`valueEncoding` can only be set if a `db` is provided')
      }
      this._opts = { ...this._opts, ...opts }
+      this._opts.useKeyHashingFunction =
+        opts.common?.customCrypto.keccak256 ?? opts.useKeyHashingFunction ?? keccak256


Just remember that recently in an exchange with @jochem-brouwer we stumbled upon a problem (unexpected evaluation) when using this kind of operator chaining, can't completely recall the constallation specifics though. 🤔

Did some basic tests and these went as expected:

So just dropping this here for awareness.

(maybe though it was something with the ? operator from TypeScript? So a problem might occur when - in the case above - common is not defined? Maybe things would then not evaluate properly? Not sure if Jochem can give more insight here (If I recall correctly a solution for our problem were adding some brackets)

I can't recall the specifics, do you maybe have some more context when we stumbled upon this? I do slightly remember a "gotcha" with these operators though, but I don't recall in which context 😅

holgerd77 · 2023-12-29T14:25:49Z

packages/tx/src/capabilities/legacy.ts

  if (Object.isFrozen(tx)) {
    if (!tx.cache.hash) {
-      tx.cache.hash = keccak256(tx.serialize())
+      tx.cache.hash = keccackFunction(tx.serialize())


Really happy here that our capability consolidation in tx is already paying off. 🤩

//cc @gabrocheleau

holgerd77 · 2023-12-29T14:50:28Z

…-wasm-lib-hashing

* Devp2p: add keccak function and Common passing structure to RLPx, DPT, DNS * Client: pass in common along devp2p DPT instantiation * Devp2p: expand to ECIES, first replacement test * Devp2p: First full-round replacement (including concatBytes occurrences) for ECIES * Devp2p: integrate into DPT server and message * Devp2p: Add to DNS ENR * Devp2p: remove util keccak256 helper function

…-wasm-lib-hashing

holgerd77 · 2024-01-17T12:28:42Z

Updated this via UI.

@acolytec3 feel free to merge this in whenever you think it’s ready!

holgerd77 · 2024-01-17T12:31:04Z

(from my side additional commits look good)

packages/client/bin/cli.ts

jochem-brouwer · 2024-01-18T01:06:46Z

packages/client/bin/cli.ts

@@ -828,6 +890,8 @@ async function run() {
      chain: chainName,
      mergeForkIdPostMerge: args.mergeForkIdPostMerge,
    })
+    //@ts-ignore
+    common.customCrypto = cryptoFunctions


Nit: should likely add this customCrypto to the fromGethGenesis constructor also?

Good point. Maybe the next PR.

packages/client/test/util/wasmCrypto.spec.ts

jochem-brouwer · 2024-01-18T01:16:21Z

packages/devp2p/src/rlpx/rlpx.ts

@@ -50,6 +51,8 @@ export class RLPx {
  protected _refillIntervalId: NodeJS.Timeout
  protected _refillIntervalSelectionCounter: number = 0

+  protected _keccakFunction: (msg: Uint8Array) => Uint8Array


Not sure, why is this added? It's used nowhere?

Not sure, but I'll sure use it in future devp2p PRs.

packages/evm/test/customCrypto.spec.ts

jochem-brouwer · 2024-01-18T01:21:20Z

packages/trie/src/trie.ts

    let key = ROOT_DB_KEY

    const encoding =
      opts?.valueEncoding === ValueEncoding.Bytes ? ValueEncoding.Bytes : ValueEncoding.String

    if (opts?.useKeyHashing === true) {
-      key = (opts?.useKeyHashingFunction ?? keccak256)(ROOT_DB_KEY) as Uint8Array
+      key = keccakFunction.call(undefined, ROOT_DB_KEY) as Uint8Array


Not sure, why do we have to .call here?

🤷 I think this was left over from when @holgerd77 was trying to introduce a global crypto thing or something.

jochem-brouwer

Left a few nits, ready to merge once CI passes

acolytec3

Approving this. @jochem-brouwer or @holgerd77, this can be merged if everyone is satisfied. Plenty of work to add in future PRs but I think this one is fine as is.

jochem-brouwer · 2024-01-18T03:01:58Z

I'm fine to merge this when CI passes :)

holgerd77 · 2024-01-18T08:57:12Z

👍

holgerd77 added type: feature PR state: WIP type: refactor type: performance type: enhancement package: client labels Dec 14, 2023

holgerd77 added 5 commits December 18, 2023 09:53

Add @polkadot/wasm crypto dependency

9124038

Use @polkadot/wasm keccak256 for trie key hashing in execution state …

954f603

…manager for VM

Tighten calling for useKeyHashingFunction

4cc394b

Add customCrypto options dict to Common, first keccak256 usage and test

3c3c85f

Add custom keccak256 usage to EVM keccak256 opcode

159abd1

holgerd77 force-pushed the client-use-polkadot-wasm-lib-hashing branch from b9a8f0b to 159abd1 Compare December 18, 2023 11:44

acolytec3 and others added 7 commits December 22, 2023 12:25

Add wasm based ecrecover option

691e27c

export calcSigRecovery

91a12b0

use customcrypto if available

b7852a1

add test for custom ecrecover

de7ac90

Add optional common to trie interface

c8d5052

Add cli option

1d1bb45

Rebuild package-lock.json

1c8e3fb

holgerd77 commented Dec 29, 2023

View reviewed changes

packages/client/bin/cli.ts Show resolved Hide resolved

holgerd77 added 3 commits December 29, 2023 15:05

Add @polkadot/util as a dev dependency to the tx package for testing

aa36a91

Merge branch 'master' into client-use-polkadot-wasm-lib-hashing

7190802

Rebuild package-lock.json

508ab47

holgerd77 commented Dec 29, 2023

View reviewed changes

acolytec3 and others added 2 commits January 2, 2024 11:05

Merge remote-tracking branch 'origin/master' into client-use-polkadot…

1448e7c

…-wasm-lib-hashing

Pass common into trie instantiations in snap sync fetchers

44069df

holgerd77 added package: vm package: common package: mpt package: evm labels Jan 12, 2024

holgerd77 and others added 8 commits January 12, 2024 10:32

Mock polkadot util module

995e48c

try installing peers

8b1c24f

Merge remote-tracking branch 'origin/master' into client-use-polkadot…

9e24d67

…-wasm-lib-hashing

Add custom sha256 support and tests

528ec4a

Add wasm ecSign

4739240

Merge remote-tracking branch 'origin/master' into client-use-polkadot…

c107103

…-wasm-lib-hashing

Merge branch 'master' into client-use-polkadot-wasm-lib-hashing

7e174ef

acolytec3 added 2 commits January 17, 2024 13:18

Passing common in various places

00cd74d

Fix import

99f412b

acolytec3 added PR state: needs review PR state: merge ready and removed PR state: WIP labels Jan 17, 2024

block/client: minor changes

0f9cc6c

jochem-brouwer reviewed Jan 18, 2024

View reviewed changes

jochem-brouwer previously approved these changes Jan 18, 2024

View reviewed changes

Add ecrecover function

1d90668

acolytec3 dismissed jochem-brouwer’s stale review via 1d90668 January 18, 2024 02:48

acolytec3 approved these changes Jan 18, 2024

View reviewed changes

jochem-brouwer merged commit 54446b1 into master Jan 18, 2024
46 checks passed

holgerd77 deleted the client-use-polkadot-wasm-lib-hashing branch January 18, 2024 08:57

acolytec3 mentioned this pull request Jan 19, 2024

Additional Cryptography Primitives Usage / Devp2p #3246

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client/Monorepo: Use WASM Crypto (keccak256) for Hashing / Consistent Hash Function Overwrite #3192

Client/Monorepo: Use WASM Crypto (keccak256) for Hashing / Consistent Hash Function Overwrite #3192

holgerd77 commented Dec 14, 2023 •

edited

Loading

codecov bot commented Dec 14, 2023 •

edited

Loading

acolytec3 commented Dec 18, 2023

holgerd77 Dec 29, 2023

jochem-brouwer Dec 30, 2023

holgerd77 Dec 29, 2023

holgerd77 commented Dec 29, 2023 •

edited by acolytec3

Loading

holgerd77 commented Jan 17, 2024

holgerd77 commented Jan 17, 2024

jochem-brouwer Jan 18, 2024

acolytec3 Jan 18, 2024

jochem-brouwer Jan 18, 2024

acolytec3 Jan 18, 2024

jochem-brouwer Jan 18, 2024

acolytec3 Jan 18, 2024

jochem-brouwer left a comment

acolytec3 left a comment

jochem-brouwer commented Jan 18, 2024

holgerd77 commented Jan 18, 2024

Client/Monorepo: Use WASM Crypto (keccak256) for Hashing / Consistent Hash Function Overwrite #3192

Client/Monorepo: Use WASM Crypto (keccak256) for Hashing / Consistent Hash Function Overwrite #3192

Conversation

holgerd77 commented Dec 14, 2023 • edited Loading

codecov bot commented Dec 14, 2023 • edited Loading

Codecov Report

acolytec3 commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Dec 29, 2023 • edited by acolytec3 Loading

holgerd77 commented Jan 17, 2024

holgerd77 commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

acolytec3 left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Jan 18, 2024

holgerd77 commented Jan 18, 2024

holgerd77 commented Dec 14, 2023 •

edited

Loading

codecov bot commented Dec 14, 2023 •

edited

Loading

holgerd77 commented Dec 29, 2023 •

edited by acolytec3

Loading