-
Notifications
You must be signed in to change notification settings - Fork 835
Util: remove ecsign method (instead directly use secp256k1.sign from external ethereum-cryptography package
#3948
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
ccea943
d202a6a
0fa9458
f17680d
204004f
e6e2844
332a3ed
bec05a2
aadc685
99859e5
6a18dc9
100b71e
8887101
5611c06
2758cd9
32dea48
bbd2791
8f835bd
de30338
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,13 +16,13 @@ import { | |
| } from '@ethereumjs/common' | ||
| import { | ||
| EthereumJSErrorWithoutCode, | ||
| bytesToBigInt, | ||
| bytesToHex, | ||
| calculateSigRecovery, | ||
| concatBytes, | ||
| createAddressFromPrivateKey, | ||
| createAddressFromString, | ||
| ecrecover, | ||
| ecsign, | ||
| hexToBytes, | ||
| parseGethGenesisState, | ||
| randomBytes, | ||
|
|
@@ -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' | ||
|
||
| import { secp256k1 } from 'ethereum-cryptography/secp256k1.js' | ||
| import { sha256 } from 'ethereum-cryptography/sha256.js' | ||
| import { KZG as microEthKZG } from 'micro-eth-signer/kzg' | ||
| import * as verkle from 'micro-eth-signer/verkle' | ||
|
|
@@ -626,16 +627,12 @@ function generateAccount(): Account { | |
| return [address, privKey] | ||
| } | ||
|
|
||
| export async function generateClientConfig(args: ClientOpts) { | ||
| // Give chainId priority over networkId | ||
| // Give networkId precedence over network name | ||
| const chainName = args.chainId ?? args.networkId ?? args.network ?? Chain.Mainnet | ||
| const chain = getPresetChainConfig(chainName) | ||
| export async function getCryptoFunctions(useJsCrypto: boolean): Promise<CustomCrypto> { | ||
| const cryptoFunctions: CustomCrypto = {} | ||
|
|
||
| const kzg = new microEthKZG(trustedSetup) | ||
| // Initialize WASM crypto if JS crypto is not specified | ||
| if (args.useJsCrypto === false) { | ||
| if (useJsCrypto === false) { | ||
| await waitReadyPolkadotSha256() | ||
| cryptoFunctions.keccak256 = keccak256WASM | ||
| cryptoFunctions.ecrecover = ( | ||
|
|
@@ -659,17 +656,22 @@ export async function generateClientConfig(args: ClientOpts) { | |
| throw EthereumJSErrorWithoutCode('message length must be 32 bytes or greater') | ||
| } | ||
| const buf = secp256k1Sign(msg, pk) | ||
| const r = buf.slice(0, 32) | ||
| const s = buf.slice(32, 64) | ||
| const v = BigInt(buf[64]) | ||
| const r = bytesToBigInt(buf.slice(0, 32)) | ||
| const s = bytesToBigInt(buf.slice(32, 64)) | ||
| const recovery = buf[64] | ||
|
|
||
| return { r, s, v } | ||
| 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 { | ||
| signature: sig.slice(0, 64), | ||
| recid: sig[64], | ||
| r, | ||
| s, | ||
| recovery, | ||
| } | ||
| } | ||
| cryptoFunctions.ecdsaRecover = (sig: Uint8Array, recId: number, hash: Uint8Array) => { | ||
|
|
@@ -679,12 +681,24 @@ export async function generateClientConfig(args: ClientOpts) { | |
| cryptoFunctions.keccak256 = keccak256 | ||
| cryptoFunctions.ecrecover = ecrecover | ||
| cryptoFunctions.sha256 = sha256 | ||
| cryptoFunctions.ecsign = ecsign | ||
| cryptoFunctions.ecdsaSign = ecdsaSign | ||
| cryptoFunctions.ecsign = secp256k1.sign | ||
| cryptoFunctions.ecdsaSign = secp256k1.sign | ||
| cryptoFunctions.ecdsaRecover = ecdsaRecover | ||
| } | ||
| cryptoFunctions.kzg = kzg | ||
| cryptoFunctions.verkle = verkle | ||
| return cryptoFunctions | ||
| } | ||
|
|
||
| export async function generateClientConfig(args: ClientOpts) { | ||
| // Give chainId priority over networkId | ||
| // Give networkId precedence over network name | ||
| const chainName = args.chainId ?? args.networkId ?? args.network ?? Chain.Mainnet | ||
| const chain = getPresetChainConfig(chainName) | ||
|
|
||
| // `useJsCrypto` defaults to `false` in the CLI defaults | ||
| const cryptoFunctions = await getCryptoFunctions(args.useJsCrypto ?? false) | ||
|
|
||
| // Configure accounts for mining and prefunding in a local devnet | ||
| const accounts: Account[] = [] | ||
| if (typeof args.unlock === 'string') { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,6 @@ | |
| bytesToHex, | ||
| calculateSigRecovery, | ||
| concatBytes, | ||
| ecsign, | ||
| hexToBytes, | ||
| randomBytes, | ||
| setLengthLeft, | ||
|
|
@@ -18,9 +17,11 @@ | |
| waitReady, | ||
| sha256 as wasmSha256, | ||
| } from '@polkadot/wasm-crypto' | ||
| import { secp256k1 } from 'ethereum-cryptography/secp256k1' | ||
|
||
| import { ecdsaRecover, ecdsaSign } from 'ethereum-cryptography/secp256k1-compat.js' | ||
|
||
| import { sha256 as jsSha256 } from 'ethereum-cryptography/sha256.js' | ||
| import { assert, describe, it } from 'vitest' | ||
| import { getCryptoFunctions } from '../../bin/utils.ts' | ||
| describe('WASM crypto tests', () => { | ||
| it('should compute public key and hash correctly using common.customCrypto functions', async () => { | ||
| const wasmecrecover = ( | ||
|
|
@@ -65,26 +66,16 @@ | |
| }) | ||
|
|
||
| it('should compute the same signature whether js or WASM signature used', async () => { | ||
| const wasmSign = (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 new Error('message length must be 32 bytes or greater') | ||
| } | ||
| const buf = secp256k1Sign(msg, pk) | ||
| const r = buf.slice(0, 32) | ||
| const s = buf.slice(32, 64) | ||
| const v = BigInt(buf[64]) | ||
|
|
||
| return { r, s, v } | ||
| } | ||
| const crypto = await getCryptoFunctions(true) | ||
| const wasmSign = crypto.ecdsaSign! | ||
|
|
||
| await waitReady() | ||
| const msg = hexToBytes('0x82ff40c0a986c6a5cfad4ddf4c3aa6996f1a7837f9c398e17e5de5cbd5a12b28') | ||
| const pk = hexToBytes('0x3c9229289a6125f7fdf1885a77bb12c37a8d3b4962d936f7e3084dece32a3ca1') | ||
| const jsSig = ecsign(msg, pk) | ||
| const jsSig = secp256k1.sign(msg, pk) | ||
| const wasmSig = wasmSign(msg, pk) | ||
| assert.deepEqual(wasmSig, jsSig, 'wasm signatures produce same result as js signatures') | ||
| assert.throws( | ||
|
Check failure on line 78 in packages/client/test/util/wasmCrypto.spec.ts
|
||
| () => wasmSign(randomBytes(31), randomBytes(32)), | ||
| 'message length must be 32 bytes or greater', | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,22 +2,24 @@ import * as crypto from 'crypto' | |
| import { RLP } from '@ethereumjs/rlp' | ||
| import { | ||
| EthereumJSErrorWithoutCode, | ||
| bigIntToBytes, | ||
| bytesToInt, | ||
| concatBytes, | ||
| hexToBytes, | ||
| intToBytes, | ||
| setLengthLeft, | ||
| } from '@ethereumjs/util' | ||
| import debugDefault from 'debug' | ||
| import { keccak256 } from 'ethereum-cryptography/keccak.js' | ||
| import { getRandomBytesSync } from 'ethereum-cryptography/random.js' | ||
| import { ecdh, ecdsaRecover, ecdsaSign } from 'ethereum-cryptography/secp256k1-compat.js' | ||
| import { ecdh, ecdsaRecover } from 'ethereum-cryptography/secp256k1-compat.js' | ||
| import { secp256k1 } from 'ethereum-cryptography/secp256k1.js' | ||
|
|
||
| import { assertEq, genPrivateKey, id2pk, pk2id, unstrictDecode, xor, zfill } from '../util.ts' | ||
|
|
||
| import { MAC } from './mac.ts' | ||
|
|
||
| import type { Common } from '@ethereumjs/common' | ||
| import type { Common, CustomCrypto } from '@ethereumjs/common' | ||
| type Decipher = crypto.Decipher | ||
|
|
||
| const debug = debugDefault('devp2p:rlpx:peer') | ||
|
|
@@ -77,13 +79,7 @@ export class ECIES { | |
| protected _bodySize: number | null = null | ||
|
|
||
| protected _keccakFunction: (msg: Uint8Array) => Uint8Array | ||
| protected _ecdsaSign: ( | ||
| msg: Uint8Array, | ||
| pk: Uint8Array, | ||
| ) => { | ||
| signature: Uint8Array | ||
| recid: number | ||
| } | ||
| protected _ecdsaSign: Required<CustomCrypto>['ecsign'] | ||
acolytec3 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gabrocheleau could you take a look here? How would we test this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| protected _ecdsaRecover: ( | ||
| sig: Uint8Array, | ||
| recId: number, | ||
|
|
@@ -101,7 +97,7 @@ export class ECIES { | |
| this._ephemeralPublicKey = secp256k1.getPublicKey(this._ephemeralPrivateKey, false) | ||
|
|
||
| this._keccakFunction = common?.customCrypto.keccak256 ?? keccak256 | ||
| this._ecdsaSign = common?.customCrypto.ecdsaSign ?? ecdsaSign | ||
| this._ecdsaSign = common?.customCrypto.ecdsaSign ?? secp256k1.sign | ||
| this._ecdsaRecover = common?.customCrypto.ecdsaRecover ?? ecdsaRecover | ||
| } | ||
|
|
||
|
|
@@ -198,7 +194,11 @@ export class ECIES { | |
| const x = ecdhX(this._remotePublicKey, this._privateKey) | ||
| const sig = this._ecdsaSign(xor(x, this._nonce), this._ephemeralPrivateKey) | ||
| const data = [ | ||
| concatBytes(sig.signature, Uint8Array.from([sig.recid])), | ||
| concatBytes( | ||
| setLengthLeft(bigIntToBytes(sig.r), 32), | ||
| setLengthLeft(bigIntToBytes(sig.s), 32), | ||
| Uint8Array.from([sig.recovery]), | ||
| ), | ||
| // this._keccakFunction(pk2id(this._ephemeralPublicKey)), | ||
| pk2id(this._publicKey), | ||
| this._nonce, | ||
|
|
@@ -221,8 +221,9 @@ export class ECIES { | |
| const x = ecdhX(this._remotePublicKey, this._privateKey) | ||
| const sig = this._ecdsaSign(xor(x, this._nonce), this._ephemeralPrivateKey) | ||
| const data = concatBytes( | ||
| sig.signature, | ||
| Uint8Array.from([sig.recid]), | ||
| bigIntToBytes(sig.r), | ||
| bigIntToBytes(sig.s), | ||
| Uint8Array.from([sig.recovery]), | ||
| this._keccakFunction(pk2id(this._ephemeralPublicKey)), | ||
| pk2id(this._publicKey), | ||
| this._nonce, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,7 +225,7 @@ export interface TransactionInterface<T extends TransactionType = TransactionTyp | |
| errorStr(): string | ||
|
|
||
| addSignature( | ||
| v: bigint, | ||
| v: bigint, // TODO: change this to number? | ||
|
||
| r: Uint8Array | bigint, | ||
| s: Uint8Array | bigint, | ||
| convertV?: boolean, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.