Util: remove `ecsign` method (instead directly use `secp256k1.sign` from external `ethereum-cryptography` package #3948

jochem-brouwer · 2025-03-28T21:54:25Z

Part of the breaking series to remove methods from util which are almost directly supported from other dedicated external packages (ethereum-cryptography)

TODO:

Remove all imports from 'ethereum-cryptography/secp256k1-compat.js'. These are legacy imports and are deprecated. We should use secp256k1 directly instead of this compat module. Partially done, other imports are part of this issue now Remove imports from secp256k1-compat.js #3966
CustomCrypto.ecsign type should point to the output of secp256k1.sign with ONLY the fields r, s, recovery
~~- [ ] Change tx type v to number (thus also yParity!)~~ (Not workable because chainId could be very large)
Fix WASM test in client
Dedupe ecsign/ecdsaSign in customCrypto interface

…um-cryptography external package)

codecov · 2025-03-28T21:57:50Z

Codecov Report

Attention: Patch coverage is 84.74576% with 9 lines in your changes missing coverage. Please review.

Project coverage is 79.70%. Comparing base (a5b0dc3) to head (de30338).
Report is 1 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
block	`83.34% <100.00%> (+0.04%)`	⬆️
blockchain	`89.33% <ø> (ø)`
client	`68.41% <55.00%> (?)`
common	`97.54% <ø> (ø)`
devp2p	`86.78% <100.00%> (+0.05%)`	⬆️
evm	`72.98% <ø> (ø)`
genesis	`99.98% <ø> (ø)`
mpt	`89.76% <ø> (ø)`
rlp	`91.43% <ø> (ø)`
statemanager	`69.16% <ø> (ø)`
tx	`90.59% <100.00%> (+0.01%)`	⬆️
util	`82.02% <100.00%> (+0.05%)`	⬆️
vm	`57.20% <ø> (ø)`
wallet	`88.55% <ø> (ø)`

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

packages/tx/src/util/authorization.ts

jochem-brouwer · 2025-04-07T10:42:44Z

Our original ecsign method was a wrapper method. If we want to keep the same signature, we have to use ecdsaSign from ethereum-cryptography/secp256k1-compat.js. Need to do a tiny investigation on this.

jochem-brouwer · 2025-04-07T10:45:28Z

There's a warning in ethereum-cryptography for this compat:

"Warning: use secp256k1 instead. This module is only for users who upgraded from ethereum-cryptography v0.1. It could be removed in the future."

https://github.com/ethereum/js-ethereum-cryptography/tree/main?tab=readme-ov-file#secp256k1-compat-compatibility-layer-with-other-libraries

Will directly do that here, removing all these compat imports.

jochem-brouwer · 2025-04-08T15:53:46Z

I just did a fresh client run with datadir deleted and pkgs freshly installed to ensure that devp2p works, just imported blocks, so this looks fine :)

packages/tx/src/util/authorization.ts

packages/devp2p/src/rlpx/ecies.ts

packages/tx/src/capabilities/legacy.ts

acolytec3

LGTM. A couple of small nits but could be disregarded.

This reverts commit 99859e5.

jochem-brouwer · 2025-04-09T16:51:58Z

We should indeed remove ecdsaSign. This is in our current client code (this has changed this PR)

    cryptoFunctions.ecsign = (msg: Uint8Array, pk: Uint8Array) => {
      if (msg.length < 32) {
        // WASM errors with `unreachable` if we try to pass in less than 32 bytes in the message
        throw EthereumJSErrorWithoutCode('message length must be 32 bytes or greater')
      }
      const buf = secp256k1Sign(msg, pk)
      const r = bytesToBigInt(buf.slice(0, 32))
      const s = bytesToBigInt(buf.slice(32, 64))
      const recovery = buf[64]

      return { r, s, recovery }
    }
    cryptoFunctions.ecdsaSign = (hash: Uint8Array, pk: Uint8Array) => {
      const sig = secp256k1Sign(hash, pk)
      const r = bytesToBigInt(sig.slice(0, 32))
      const s = bytesToBigInt(sig.slice(32, 64))
      const recovery = Number(sig[64])

      return {
        r,
        s,
        recovery,
      }
    }

(They are equivalent)

Note: it was thus previously this wrapper which threw, not the method it seems? But in the new ecdsaSign the length check of hash is not there, which is why the

    assert.throws(
      () => wasmSign(randomBytes(31), randomBytes(32)),
      'message length must be 32 bytes or greater',
    )

Failed

acolytec3 · 2025-04-09T18:18:31Z

We should indeed remove ecdsaSign. This is in our current client code (this has changed this PR)

    cryptoFunctions.ecsign = (msg: Uint8Array, pk: Uint8Array) => {
      if (msg.length < 32) {
        // WASM errors with `unreachable` if we try to pass in less than 32 bytes in the message
        throw EthereumJSErrorWithoutCode('message length must be 32 bytes or greater')
      }
      const buf = secp256k1Sign(msg, pk)
      const r = bytesToBigInt(buf.slice(0, 32))
      const s = bytesToBigInt(buf.slice(32, 64))
      const recovery = buf[64]

      return { r, s, recovery }
    }
    cryptoFunctions.ecdsaSign = (hash: Uint8Array, pk: Uint8Array) => {
      const sig = secp256k1Sign(hash, pk)
      const r = bytesToBigInt(sig.slice(0, 32))
      const s = bytesToBigInt(sig.slice(32, 64))
      const recovery = Number(sig[64])

      return {
        r,
        s,
        recovery,
      }
    }

(They are equivalent)

Note: it was thus previously this wrapper which threw, not the method it seems? But in the new ecdsaSign the length check of hash is not there, which is why the

    assert.throws(
      () => wasmSign(randomBytes(31), randomBytes(32)),
      'message length must be 32 bytes or greater',
    )

Failed

To be clear, at one point, the wasm version of ecsign did throw when you passed in a message that was less than 32 bytes, but it was an obscure WASM error that didn't tell you what was wrong. This seems to be no longer the case, which is why we can remove that check. I added that check to clarify what the error was.

holgerd77

Not so deep into the topic, so only some loose side-review without any approval decision.

holgerd77 · 2025-04-10T08:59:23Z

packages/client/bin/utils.ts

@@ -38,7 +38,8 @@ import {
  sha256 as wasmSha256,
 } from '@polkadot/wasm-crypto'
 import { keccak256 } from 'ethereum-cryptography/keccak.js'
-import { ecdsaRecover, ecdsaSign } from 'ethereum-cryptography/secp256k1-compat.js'
+import { ecdsaRecover } from 'ethereum-cryptography/secp256k1-compat.js'


So here it is still necessary that we use this deprecated compat module https://github.com/ethereum/js-ethereum-cryptography/blob/main/src/secp256k1-compat.ts ?

I did not want to tackle this here because I thought it would bloat the PR too much, but yes we can directly do here. It is also part of this issue #3966

holgerd77 · 2025-04-10T09:02:11Z

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

@@ -18,9 +17,11 @@ import {
  waitReady,
  sha256 as wasmSha256,
 } from '@polkadot/wasm-crypto'
+import { secp256k1 } from 'ethereum-cryptography/secp256k1'
 import { ecdsaRecover, ecdsaSign } from 'ethereum-cryptography/secp256k1-compat.js'


Also here, still this compat thing in.

holgerd77 · 2025-04-10T09:04:00Z

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

@@ -18,9 +17,11 @@ import {
  waitReady,
  sha256 as wasmSha256,
 } from '@polkadot/wasm-crypto'
+import { secp256k1 } from 'ethereum-cryptography/secp256k1'


Not totally a blocker, but it's a bit semi-beautiful that we have a mixture of these .js and non-.js imports for the ethereum-cryptography imports.

holgerd77 · 2025-04-10T09:05:28Z

packages/tx/src/types.ts

@@ -225,7 +225,7 @@ export interface TransactionInterface<T extends TransactionType = TransactionTyp
  errorStr(): string

  addSignature(
-    v: bigint,
+    v: bigint, // TODO: change this to number?


This TODO should be removed.

holgerd77 · 2025-04-10T09:08:59Z

packages/devp2p/src/rlpx/ecies.ts

-    signature: Uint8Array
-    recid: number
-  }
+  protected _ecdsaSign: Required<CustomCrypto>['ecsign']


Is this ok from a TypeScript/Node.js compatibility perspective?

(if not AND this was not caught by any unit tests: we should really add this to CI in some form, so that similar code adoptions do not slip through in the future)

@gabrocheleau could you take a look here? How would we test this?

Same as the other place, there really isn't any issue from a Nodejs compatibility perspective because none of these field declarations are in the compiled code. Is that what you're asking about?

holgerd77 · 2025-04-10T09:09:41Z

packages/common/test/customCrypto.spec.ts

+  const customEcSign = (
+    _msg: Uint8Array,
+    _pk: Uint8Array,
+  ): Pick<ReturnType<typeof secp256k1.sign>, 'recovery' | 'r' | 's'> => {


Maybe also here: ok from TypeScript/Node.js compatibility perspective?

The interface gets compiled away so should have zero impact on Nodejs compatibility. So, in this case, this Pick<ReturnType>... is duplicated from here. When you look in the compiled code, there's nothing in dist/esm/types.js

have also confirmed that these fancy types do not cause an issue in Node 23 with node --conditions=typescript --experimental-strip-types...

packages/block/src/consensus/clique.ts

acolytec3

LGTM

…rom external `ethereum-cryptography` package (ethereumjs#3948) * util/monorepo: remove ecsign (use secp256k1.sign directly from ethereum-cryptography external package) * client: read wasm from util * tx: update auth sign to read from common crypto * devp2p: convert deprecated ecsign to secp256k1.sign * update customCrypto.ecsign return type * fix wasmCrypto test * switch v to number * Revert "switch v to number" This reverts commit 99859e5. * remove old comment * dedupe ecdsaSign * fix test * remove compat version of ecdsaRecover * tx: remove TODO * block/clique: ensure customCrypto and left padding is adhered --------- Co-authored-by: acolytec3 <[email protected]> Co-authored-by: Holger Drewes <[email protected]>

jochem-brouwer added PR state: WIP package: monorepo package: util breaking labels Mar 28, 2025

util/monorepo: remove ecsign (use secp256k1.sign directly from ethere…

ccea943

…um-cryptography external package)

jochem-brouwer force-pushed the remove-ecsign branch from c665e4e to ccea943 Compare March 28, 2025 21:56

acolytec3 reviewed Mar 31, 2025

View reviewed changes

packages/tx/src/util/authorization.ts Outdated Show resolved Hide resolved

jochem-brouwer added 3 commits April 6, 2025 20:35

Merge branch 'master' into remove-ecsign

d202a6a

client: read wasm from util

0fa9458

tx: update auth sign to read from common crypto

f17680d

jochem-brouwer force-pushed the remove-ecsign branch from 612d252 to f17680d Compare April 6, 2025 18:46

jochem-brouwer added 2 commits April 8, 2025 17:45

devp2p: convert deprecated ecsign to secp256k1.sign

204004f

Merge branch 'master' into remove-ecsign

e6e2844

acolytec3 reviewed Apr 8, 2025

View reviewed changes

packages/tx/src/util/authorization.ts Outdated Show resolved Hide resolved

acolytec3 reviewed Apr 8, 2025

View reviewed changes

packages/devp2p/src/rlpx/ecies.ts Show resolved Hide resolved

acolytec3 reviewed Apr 8, 2025

View reviewed changes

packages/tx/src/capabilities/legacy.ts Outdated Show resolved Hide resolved

acolytec3 previously approved these changes Apr 8, 2025

View reviewed changes

jochem-brouwer and others added 2 commits April 8, 2025 19:29

Merge branch 'master' into remove-ecsign

332a3ed

update customCrypto.ecsign return type

bec05a2

acolytec3 dismissed their stale review via bec05a2 April 8, 2025 18:38

acolytec3 added 3 commits April 8, 2025 16:21

fix wasmCrypto test

aadc685

switch v to number

99859e5

Revert "switch v to number"

6a18dc9

This reverts commit 99859e5.

acolytec3 added 2 commits April 9, 2025 14:16

Merge remote-tracking branch 'origin/master' into remove-ecsign

100b71e

remove old comment

8887101

dedupe ecdsaSign

5611c06

acolytec3 marked this pull request as ready for review April 9, 2025 18:31

acolytec3 added package: block package: tx package: vm package: common package: devp2p package: client and removed PR state: WIP labels Apr 9, 2025

acolytec3 and others added 2 commits April 9, 2025 14:52

fix test

2758cd9

Merge branch 'master' into remove-ecsign

32dea48

holgerd77 reviewed Apr 10, 2025

View reviewed changes

remove compat version of ecdsaRecover

bbd2791

jochem-brouwer commented Apr 10, 2025

View reviewed changes

packages/block/src/consensus/clique.ts Outdated Show resolved Hide resolved

packages/block/src/consensus/clique.ts Outdated Show resolved Hide resolved

jochem-brouwer added 2 commits April 10, 2025 18:23

tx: remove TODO

8f835bd

block/clique: ensure customCrypto and left padding is adhered

de30338

acolytec3 approved these changes Apr 10, 2025

View reviewed changes

acolytec3 merged commit b843695 into master Apr 10, 2025
40 of 41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Util: remove `ecsign` method (instead directly use `secp256k1.sign` from external `ethereum-cryptography` package #3948

Util: remove `ecsign` method (instead directly use `secp256k1.sign` from external `ethereum-cryptography` package #3948

jochem-brouwer commented Mar 28, 2025 •

edited by acolytec3

Loading

codecov bot commented Mar 28, 2025 •

edited

Loading

jochem-brouwer commented Apr 7, 2025

jochem-brouwer commented Apr 7, 2025

jochem-brouwer commented Apr 8, 2025

acolytec3 left a comment

jochem-brouwer commented Apr 9, 2025 •

edited

Loading

acolytec3 commented Apr 9, 2025

holgerd77 left a comment

holgerd77 Apr 10, 2025

jochem-brouwer Apr 10, 2025

holgerd77 Apr 10, 2025

holgerd77 Apr 10, 2025

holgerd77 Apr 10, 2025

holgerd77 Apr 10, 2025

jochem-brouwer Apr 10, 2025

acolytec3 Apr 10, 2025

holgerd77 Apr 10, 2025

acolytec3 Apr 10, 2025

acolytec3 Apr 10, 2025

acolytec3 left a comment

Util: remove ecsign method (instead directly use secp256k1.sign from external ethereum-cryptography package #3948

Util: remove ecsign method (instead directly use secp256k1.sign from external ethereum-cryptography package #3948

Conversation

jochem-brouwer commented Mar 28, 2025 • edited by acolytec3 Loading

codecov bot commented Mar 28, 2025 • edited Loading

Codecov Report

jochem-brouwer commented Apr 7, 2025

jochem-brouwer commented Apr 7, 2025

jochem-brouwer commented Apr 8, 2025

acolytec3 left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Apr 9, 2025 • edited Loading

acolytec3 commented Apr 9, 2025

holgerd77 left a 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

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

Choose a reason for hiding this comment

acolytec3 left a comment

Choose a reason for hiding this comment

Util: remove `ecsign` method (instead directly use `secp256k1.sign` from external `ethereum-cryptography` package #3948

Util: remove `ecsign` method (instead directly use `secp256k1.sign` from external `ethereum-cryptography` package #3948

jochem-brouwer commented Mar 28, 2025 •

edited by acolytec3

Loading

codecov bot commented Mar 28, 2025 •

edited

Loading

jochem-brouwer commented Apr 9, 2025 •

edited

Loading