chore: upgrade noble curves

[gabrocheleau]

Fixes #4178

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.55%. Comparing base (a5e725c) to head (f00c827).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
block	88.43% <ø> (ø)
blockchain	88.85% <ø> (ø)
common	93.38% <ø> (ø)
evm	62.01% <80.00%> (-0.03%)	⬇️
mpt	90.11% <ø> (ø)
statemanager	78.10% <ø> (ø)
static	91.35% <ø> (ø)
tx	88.07% <ø> (ø)
util	84.94% <ø> (ø)
vm	65.08% <ø> (ø)

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.

[ScottyPoi]

I have this worked out so that the tests pass, i'm just doing some extra double-checking to make sure I didn't change anything unintentionally.

• p256 is now imported from @noble/curves/nist.js
• ProjectivePoint was renamed to Point
• Need to call toAffine() when we want the .x and .y coordinates

[gabrocheleau]

I have this worked out so that the tests pass, i'm just doing some extra double-checking to make sure I didn't change anything unintentionally.

• p256 is now imported from @noble/curves/nist.js
• ProjectivePoint was renamed to Point
• Need to call toAffine() when we want the .x and .y coordinates

Seems like there is a few vm-state tests still failing. Haven't yet been able to nail down what's wrong,

[paulmillr]

lowS: true is in v2 active for p256. To bring back previous behavior, pass lowS: false to sign / verify.

Although lowS: false makes signatures trivially malleable. Ethereum has fixed it in EIP-2 for secp256k1, but p256 has been left unattented for now.

[paulmillr]

@holgerd77 I suggest increasing priority of merging this to get it into the next minor / patch release, in order to not duplicate user code between old/new noble in the future.

I'm available for a help.

[holgerd77]

@paulmillr Ok, thanks for your help offer, yes, aggree that it makes sense to prioritize a bit, also regarding the imminent Fusaka hardfork with some emphasis on p256.

Added the lowS: false to sign/verify in 23504fb, tests still not fully passing though, we'll stay on it.

[holgerd77]

To re-iterate here, the most lightweight to continue is likely to use the Osaka tests - currently not integrated into the full blockchain/state test run (which is actually an interesting insight, that this none-integration is actually useful, we might want to consider this when seeing how/if to integrate 🤔 (side thought)), so this would be:

npm run test:osaka:state

Current result of this, with various "errors thrown in file":

1..1113
# tests 1113
# pass  895
# fail  218

A dedicated test run would be (so this long thing from --test='' is the test name by convention from execution-spec-tests):

npm run test:osaka:state -- --test='tests/osaka/eip7951_p256verify_precompiles/test_p256verify.py::test_valid[fork_Osaka-state_test--wycheproof/ecdsa_webcrypto_test.json EcdsaP1363Verify SHA-256 #134: s == 1]'

Will give this a quick run through the VSCode debugger:

Ok, this is giving false for isValid when running p256.verify() when the test name indicates that this should be a valid case.

@paulmillr let me know if you can already spot anything, otherwise I will try to extract the values used to copy over (I'll convert to hex for easier copy-over):

const signature = ''0x555555550000000055555555555555553ef7a8e48d07df81a693439654210c700000000000000000000000000000000000000000000000000000000000000001'
const msgHash = '0x532eaabd9574880dbf76b9b8cc00832c20a6ec113d682299550d7a6e0f345e25'
// bytesToHex(publicKey.toBytes(false))
const pubKey = '0x04e84d9b232e971a43382630f99725e423ec1ecb41e55172e9c69748a03f0d5988618b15b427ad83363bd041ff75fac98ef2ee923714e7d1dfe31753793c7588d4'

// and the we have the `{ lowS: false }` in so the above values would feed (as bytes) the below line in our code:
const isValid = p256.verify(signature, msgHash, publicKey.toBytes(false), { lowS: false })

Maybe you can test this out on your side?

[paulmillr]

@holgerd77 { lowS: false, prehash: false }

import { p256 } from '@noble/curves/nist.js';
import { hexToBytes } from '@noble/curves/utils.js';

const signature = '555555550000000055555555555555553ef7a8e48d07df81a693439654210c700000000000000000000000000000000000000000000000000000000000000001'
const msgHash = '532eaabd9574880dbf76b9b8cc00832c20a6ec113d682299550d7a6e0f345e25'
const pubKey = '04e84d9b232e971a43382630f99725e423ec1ecb41e55172e9c69748a03f0d5988618b15b427ad83363bd041ff75fac98ef2ee923714e7d1dfe31753793c7588d4'

const isValid = p256.verify(
  hexToBytes(signature), hexToBytes(msgHash),
  hexToBytes(pubKey),
  { lowS: false, prehash: false }
)
console.log(isValid);

[holgerd77]

Ah ok. Still not all. But the Osaka tests are fixed now.

[paulmillr]

Any other specific vector which fails?

[holgerd77]

@paulmillr yes, now the other tests that fail are the Prague state/blockchain tests for bn254 multiplication, likely related to the changes in this file https://github.com/ethereumjs/ethereumjs-monorepo/pull/4179/files#diff-d8e4e56024eab6bc6446b026499696dcfcc4cbb9b6bdfe6c9fd35e983052d026 respectively not sufficient adoptions.

Can you spot anything?

[holgerd77]

( @paulmillr and if you have time to look over #4192 would also be appreciated (might also point to a bug in Noble), blocking me a bit)

[paulmillr]

Just validated this -- the bn254 changes are 100% correct.

If you could provide a specific test vector, that would be very helpful. Not sure it's related to bn itself. I can take a detailed glance, and debug, but only next week.

[holgerd77]

Ok, I tested with this test:

npm run test:state -- --fork=Prague --test='ecmul_1-2_5617_21000_128'

The values/calculation should be valid (as per test definition) but Noble is returning "invalid field element: outside of range 0..ORDER".

I followed the call chain, the validation (this.isValid(scalar), modular.js in Noble) in bn254.fields.Fr.fromBytes(input) (EVM) is giving false. Seems that the scalar === ORDER case is tested, see debugger screenshot:

It seems that Noble has a validation check with num < this.ORDER, is this correct? 🤔

At least this is then not matching the error message, which says "'invalid field element: outside of range 0..ORDER" (so ORDER is by error message included in the valid range).

[paulmillr]

That's new security feature; try bn254.fields.Fr.fromBytes(input, true) to skip validation.

Scalars >= ORDER are invalid, because finite field (Fr) max value is ORDER. Everything else larger than ORDER should either be mod-divided (what you're doing), or rejected. You're automatically wrapping scalars larger than ORDER here, so validation can be skipped:

ethereumjs-monorepo/packages/evm/src/precompiles/bn254/noble.ts

Lines 60 to 64 in c2bf887

	const Fr = bn254.fields.Fr.fromBytes(input)
	if (Fr >= bn254.fields.Fr.ORDER) {
	return Fr % bn254.fields.Fr.ORDER
	}
	return Fr

[holgerd77]

@paulmillr This works, cool 🙏 (have now used the create() method from Noble directly, since this matched well in this context). Then I guess we can take this in!

[holgerd77]

LGTM