Update secp2561 ECDSA dependency to v4.0.0 #228

holgerd77 · 2020-01-29T10:59:52Z

Did some first tests here, as @alcuadrado stated the API changed significantly and there are currently 11 tests failing for functionality in account.ts and signature.ts.

Main thing here is that the API changed from using Buffer values to Uint8Array values, see v4 API docs and v3 API docs for comparison.

Two things to address here:

1. Analyze API Change Implications

My knowledge of the implications of this switch is actually limited. I would need some additional voices here from people knowing better what this comes along with on the performance and compatibility side.

2. Library Fixes

I would assume that (at least on a first round) it would make most sense to wrap the Uint8Array usage and hide this from users of this library, since the whole EthereumJS ecosystem is based on Buffer usage anyhow?

Did a first test wrapping the isValidPrivate function, this didn't improve the test success numbers, not sure why, this would need some further analysis.

/**
 * Checks if the private key satisfies the rules of the curve secp256k1.
 */
export const isValidPrivate = function(privateKey: Buffer): boolean {
  const privateKeyU = new Uint8Array(privateKey)
  console.log(privateKey)
  console.log(privateKeyU)
  return secp256k1.privateKeyVerify(privateKeyU)
}

Generally there would minimally be the need to get all test passing again.

// @michaelsbradleyjr @alcuadrado @iurimatias @evertonfraga @ryanio

Follow-up on #225

holgerd77 · 2020-01-29T11:04:56Z

Have also submitted an issue #229 to split the test cases into separate files, this would actually ease the work here.

holgerd77 · 2020-01-30T10:22:06Z

Rebased this on top of #231

michaelsbradleyjr · 2020-01-30T17:40:00Z

package.json

@@ -92,7 +92,7 @@
    "ethjs-util": "0.1.6",
    "keccak": "^3.0.0",
    "rlp": "^2.2.3",
-    "secp256k1": "^3.0.1"
+    "secp256k1": "^4.0.0"


With this bump and keccak having been bumped to ^3.0.0 should an "engines" field be added to package.json?

"engines": { "node": ">=10.0.0" }

Likewise, should Node 8.x be dropped from the testing matrix?

I have no experience with the engines field but I would assume it would make sense to use this throughout the EthereumJS libraries in general. Does this prevent an installation from happening when defined too strict?

Yes, Node 8 should then also be dropped from the test matrix. We nevertheless need to get these basic decisions outlined above done first.

"engines" behavior varies depending on whether npm is used or yarn is used.

In the case of npm, the restriction is only enforced with respect to your project's package.json. That is, if dev/Deps listed in your project's package.json have an "engines" setting that isn't satisfied, it doesn't matter, they'll get installed anyway. But if your project's package.json has an "engines" setting that isn't satisfied, then running npm install in your project will fail.

In the case of yarn both your project's and its dependencies' "engines" settings are checked and yarn install will fail if there is any mismatch.

Either way, the "engines" setting is helpful metadata for humans, signaling a package manager's intention to support some Node versions and not others.

Ok, thanks, but then I would assume at least to define a max version is somewhat dangerous and should eventually be skipped.

Typically you only want a max version if you know that, for some reason, your package isn't compatible with a newer version of node.

For awhile the embark package had:

"engines": { "node": ">=10.17.0 <12.0.0" }

10.17.0 was the first Node version that had a change to Node's built-in scrypt that we wanted to be sure was in-place in a user's setup; at the same time, we had some transitive dependencies that weren't yet compatible with Node >=12.0.0. We eventually resolved the latter and now we have:

"engines": { "node": ">=10.17.0" }

Yeah, right, likely a case-by-case decision. If there is a dependency clearly not compatible on the upper bound it makes sense to narrow. If one is is potentially - and unnecessarily - preventing users to upgrade to a future Node version, this should be left open.

@cgewecke ah, and can you also add an engines field with "engines": { "node": ">=10.0.0" } as suggested in the first comment here?

holgerd77 · 2020-01-31T12:52:18Z

Update: had another short look, actually it's not the success cases which fail (e.g. isValidPrivate(Buffer.from('fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364140', 'hex')) ("should work otherwise (< N)" test case in test/account.spec.js) still evaluates to true), it's the error cases.

These now throw with the new library (behavior before: evaluate to false), e.g. isValidPrivate(Buffer.from('0011223344', 'hex')) ("should fail on short input" test case):

PATH/ethereumjs-util/node_modules/secp256k1/lib/index.js:18
  if (!cond) throw new Error(msg)
             ^

Error: Expected private key to be an Uint8Array with length 32

So seems we have to decide here if we want to adopt here along the new API behavior (throw as well) or wrap this up in a try/catch and continue to just return false.

Likely also a cc for @alcuadrado 😄

Maybe a two-step approach makes sense: on the first round wrap this up to be able to do a minor release here but keep this in mind to change the behavior on the next major release?

holgerd77 · 2020-02-03T12:48:23Z

Ok, I have now added an exemplary backwards-compatible fix for the isValidPrivate() functionality, preserving the old "wrong length input" -> return false behavior and re-throwing on other cases.

If no objection I would continue to do fixes like this towards a backwards-compatible release with an updated secp2561 library. Further down the line I think the API should be changed here.

alcuadrado · 2020-02-03T20:42:49Z

My knowledge of the implications of this switch is actually limited. I would need some additional voices here from people knowing better what this comes along with on the performance and compatibility side.

I think there are no performance implications, Uint8Array is also just a byte array. I wouldn't be surprised if v8 better optimizes those.

Regarding compatibility, Uint8Array has been standard since 2015, so it's supported everywhere now. In an ideal world, everything would use Uint8Array instead of Buffer, as Buffer has to be polyfill in the browser.

I would assume that (at least on a first round) it would make most sense to wrap the Uint8Array usage and hide this from users of this library, since the whole EthereumJS ecosystem is based on Buffer usage anyhow?

I agree. I'd do this to get a minor release out. I'd also do things like the try/catch you introduced to keep compatibility.

What worries me about this change, is that this library re-exports secp256k1 here: https://github.com/ethereumjs/ethereumjs-util/blob/master/src/index.ts#L20 -- This makes upgrading a breaking change.

holgerd77 · 2020-02-04T11:08:17Z

What worries me about this change, is that this library re-exports secp256k1 here: https://github.com/ethereumjs/ethereumjs-util/blob/master/src/index.ts#L20 -- This makes upgrading a breaking change.

Thanks a lot for catching this, wouldn't have thought about this along the way! Thought about this a bit longer: I think we then just can't do the release in the intended way - so as a minor release - no way to get around that here without risking to break stuff for people.

Since we had this minor vs major release discussion already I would suggest that we will then finally target a major release. From my perspective it would then also make a lot of sense to just give this another - let's say four weeks - and use the occasion and to...

Improve error handling on the library (e.g. in the cases already touched here)
Do the breaking API changes discussed here (this list is likely not up-to-date and needs some modifications) and take the chance and make the API more strict and well-defined regarding the input values.

@alcuadrado Does this make sense? I know people are eager to get this released but I think this would be very much worth it the short additional waiting period.

Will also include @s1na here to take notice (and eventually also leave a comment or suggestion).

alcuadrado · 2020-02-08T15:37:52Z

Makes sense @holgerd77.

I think I'd also remove the re-export of secp256k1 if we are introducing a breaking change around it. This way we'll avoid repeating this situation (i.e. being forced to a breaking change by it) in the future.

TBH, I'm not sure what's the advantage of that export anyway.

github-actions · 2020-04-23T04:15:34Z

Coverage increased (+0.004%) to 99.649% when pulling 4dd6580 on update-secp2561-dependency into 3bbc948 on master.

cgewecke · 2020-04-23T04:37:25Z

There's more to do here...just leaving a couple intermediate notes so things don't get forgotten.

the Node 8 job passes but it's no longer supported per secp256k1 160. Could be removed?
There is a behavioral change with validation of invalid SEC1 public keys in the browser (only).. This is shown by:
- Failing karma test in cc300dd
- Tests passing with 4dd6580

It looks like invalid SEC1 keys will validate in the browser if sanitization is turned on. There
is a recent change in secp's browser fallback elliptic library...not certain what to make of this
tbh...

I would assume that (at least on a first round) it would make most sense to wrap the Uint8Array usage and hide this from users of this library, since the whole EthereumJS ecosystem is based on Buffer usage anyhow?

@holger What is the consensus about this? The secp256k1 package allows Buffer inputs and the changes in this PR just make sure Buffer comes out as well. Should the API reflect the new interface more closely?

Cross links

Remove secp256k1 export, cf: Review of behaviour: exporting imports #193
UintArray vs. Buffer: cf No implicit Buffer Conversion #172

cgewecke · 2020-04-23T17:20:09Z

RE: Invalid SEC1 tests.

Secpk256k1's tests for publicKeyVerify are here and look like this:

    t.test('validate invalid public keys', (t) => {
      const privateKey = util.getPrivateKey()
      const publicKey = util.getPublicKey(privateKey)

      const invalidVersion = Buffer.from(publicKey.compressed)
      invalidVersion[0] = 0x00
      t.false(secp256k1.publicKeyVerify(invalidVersion), 'invalid version byte')

      const invalidY = Buffer.from(publicKey.uncompressed)
      invalidY[64] ^= 0x01
      t.false(secp256k1.publicKeyVerify(invalidY), 'invalid Y')

      t.end()
    })

They prefix the array with 0x00 or corrupt the last byte. The invalid SEC1 key in the ejs-utils suite looks like this:

ethereumjs-util/test/account.spec.ts

Line 148 in 3bbc948

    
           '023a443d8381a6798a70c6ff9304bdc8cb0163c23211d11628fae52ef9e0dca11a001cf066d56a8156fc201cd5df8a36ef694eecd258903fca7086c1fae7441e1d',

Changing our test case to so that it resembles secp256k1's causes the tests to pass as expected (e.g without making the options change).

But this doesn't explain why the result for the existing case is different in the browser vs. node...think we may need to open an issue asking for advice at secp256k1 unless someone already knows what's going on here.

holgerd77 · 2020-04-24T16:23:39Z

@cgewecke Can you do a branch update here?

…secp256k1 v4.0.0 API

holgerd77 · 2020-04-24T16:29:21Z

the Node 8 job passes but it's no longer supported per secp256k1 160. Could be removed?

Yeah, Node 8 can be dropped from the test matrix

But this doesn't explain why the result for the existing case is different in the browser vs. node...think we may need to open an issue asking for advice at secp256k1 unless someone already knows what's going on here.

Yes, might be really worth to open an issue over there, can you do that? Maybe we are lucky and get a short-term answer/investigation.

cgewecke · 2020-04-24T16:33:15Z

@holgerd77

Yeah, Node 8 can be dropped from the test matrix
👍

Yes, might be really worth to open an issue over there, can you do that? Maybe we are lucky and get a short-term answer/investigation.

Yes I will make a minimal reproduction case for that today and report at secp256k1. Have looked through the issues at elliptic as well and haven't seen anything that looks exactly like this problem...

holgerd77 · 2020-04-24T19:15:17Z

Just reread some issues and stumbled over the "WASM modules for Crypto" issue https://github.com/ethereumjs/ethereumjs-util/issues/198 opened by @s1na.

If this here causes some problems I wonder if it might be more promising (and - apart from that - generally more future-proof eventually) to play around with https://github.com/bitauth/bitcoin-ts mentioned in the thread. @cgewecke what do you think?

cgewecke · 2020-04-27T06:51:26Z

@holgerd77 One thing about bitauth/bitcoin-ts is that it uses BigInt. @ricmoo has identified some drawbacks with this while reviewing a similar proposal in ethers 727 - they're not supported by Safari and therefore can't be used on iOS. He has decided to defer BigInt support until Ethers 6.

New things in this PR since last look...

rebased, removed Node 8, and did a little cleanup.
opened secp256k1 166 asking for advice about the invalid SEC1 tests.

holgerd77 · 2020-04-27T14:23:41Z

bitauth/bitcoin-ts: ah ok, wasn't aware this is using BigInt, than this is also likely something being 1-2 more major releases away on our side.

Thanks for opening the issue over on the secp2561-node library, @fanatid has already reacted and released v4.0.1 (so great, thanks for that! 🎉 🙂), @cgewecke than you can directly update the dependency and we should be ready to go here?

Have removed the Node 8 tests from the checks being required, so this is now passing CI. Let me know once this is ready for review.

cgewecke · 2020-04-27T17:18:32Z

@holgerd77 040cb54 updates to 4.0.1 and reverts the SEC1 test case changes. Looks like everything's working ok now.

holgerd77

Looks good, will approve/merge by admin merge since this is technically not my PR and therefore uncritical.

holgerd77 · 2020-04-27T20:07:13Z

src/account.ts

@@ -182,7 +183,7 @@ export const privateToPublic = function(privateKey: Buffer): Buffer {
 export const importPublic = function(publicKey: Buffer): Buffer {
  assertIsBuffer(publicKey)
  if (publicKey.length !== 64) {
-    publicKey = secp256k1.publicKeyConvert(publicKey, false).slice(1)
+    publicKey = toBuffer(secp256k1.publicKeyConvert(publicKey, false).slice(1))
  }
  return publicKey
 }


Ok, have checked for all return value conversions, looks complete.

holgerd77 · 2020-04-27T20:14:29Z

src/externals.ts

-/**
- * [`secp256k1`](https://github.com/cryptocoinjs/secp256k1-node/)
- */
-export { secp256k1 }


For reference: removal of the secp2561 re-export

holgerd77 · 2020-04-27T20:19:55Z

src/signature.ts

-  const senderPubKey = secp256k1.recover(msgHash, signature, recovery)
-  return secp256k1.publicKeyConvert(senderPubKey, false).slice(1)
+  const senderPubKey = secp256k1.ecdsaRecover(signature, recovery, msgHash)
+  return toBuffer(secp256k1.publicKeyConvert(senderPubKey, false).slice(1))
 }

 /**


Ok, also checked for completeness here.

holgerd77 · 2020-04-30T21:22:38Z

This has now been published in the v7.0.0 release on npm.

holgerd77 mentioned this pull request Jan 29, 2020

Split tests from index.js into separate files #229

Closed

holgerd77 force-pushed the update-secp2561-dependency branch from 3684eba to d8434f8 Compare January 30, 2020 10:21

holgerd77 added effort: E2 days effort: E3 weeks eval: needs context eval: needs discussion PR state: open discussion PR state: WIP prio: P3 important labels Jan 30, 2020

michaelsbradleyjr reviewed Jan 30, 2020

View reviewed changes

This was referenced Feb 4, 2020

Add basic API tests for re-exports #234

Closed

fix(package): update secp256k1 to 3.8.0 #237

Closed

roman-vanesyan mentioned this pull request Feb 16, 2020

Fix broken tests #239

Closed

This was referenced Apr 17, 2020

VM v5 Release Planning ethereumjs/ethereumjs-monorepo#681

Closed

Release - v4.0.0 ethereumjs/merkle-patricia-tree#111

Merged

cgewecke force-pushed the update-secp2561-dependency branch from 4d40475 to 65a459d Compare April 23, 2020 00:59

This was referenced Apr 24, 2020

v1.0.0 Release planning ethereumjs/ethereumjs-wallet#113

Closed

v3.0.0 Release Planning ethereumjs/ethereumjs-devp2p#63

Closed

Updated secp2561 ECDSA dependency to v4.0.0

86b19f6

holgerd77 and others added 9 commits April 24, 2020 09:26

Updated isValidPrivate() (backwards-compatible) to work with changed …

705cbb2

…secp256k1 v4.0.0 API

Additional test for isValidPrivate() to throw on unrelated exceptions

5b72d72

Fix "wrong input type" test TS error

ba90749

Upgrade @types/secp256k1 to ^4.0.1

7f2de0c

Update secp256k1 method calls for 4.0.0 interface

326431e

Remove secp256k1 as external export

6a45752

Set santize flag to false for invalid SEC1 tests

1065dad

Make isValidPrivate throw on wrong key length

a994011

Remove reference to secp256k1 in externals.ts comment

98bf0dc

cgewecke force-pushed the update-secp2561-dependency branch from 10e2607 to 98bf0dc Compare April 24, 2020 16:29

cgewecke added 4 commits April 26, 2020 22:40

Merge branch 'master' into update-secp2561-dependency

ff00f8e

Remove Node 8 from CI

697b161

Enforce buffer input for isValidPublic

36fe94c

Revert docs changes

550c207

cgewecke mentioned this pull request Apr 27, 2020

Different behavior between Node and Browser for invalid SEC1 key cryptocoinjs/secp256k1-node#166

Closed

Upgrade secp256k1 to ^4.0.1 and revert SEC1 test changes

040cb54

Merge branch 'master' into update-secp2561-dependency

1085f6c

holgerd77 commented Apr 27, 2020

View reviewed changes

holgerd77 merged commit 795ae41 into master Apr 27, 2020

holgerd77 deleted the update-secp2561-dependency branch April 27, 2020 20:22

holgerd77 removed the PR state: WIP label Apr 27, 2020

holgerd77 mentioned this pull request Apr 30, 2020

New release v7.0.0 #249

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update secp2561 ECDSA dependency to v4.0.0 #228

Update secp2561 ECDSA dependency to v4.0.0 #228

holgerd77 commented Jan 29, 2020 •

edited

Loading

holgerd77 commented Jan 29, 2020

holgerd77 commented Jan 30, 2020

michaelsbradleyjr Jan 30, 2020

holgerd77 Jan 30, 2020

michaelsbradleyjr Jan 30, 2020 •

edited

Loading

holgerd77 Jan 30, 2020

michaelsbradleyjr Jan 30, 2020

holgerd77 Jan 31, 2020

holgerd77 Apr 27, 2020

holgerd77 commented Jan 31, 2020

holgerd77 commented Feb 3, 2020

alcuadrado commented Feb 3, 2020

holgerd77 commented Feb 4, 2020

alcuadrado commented Feb 8, 2020

github-actions bot commented Apr 23, 2020

cgewecke commented Apr 23, 2020 •

edited

Loading

cgewecke commented Apr 23, 2020 •

edited

Loading

holgerd77 commented Apr 24, 2020

holgerd77 commented Apr 24, 2020

cgewecke commented Apr 24, 2020

holgerd77 commented Apr 24, 2020

cgewecke commented Apr 27, 2020

holgerd77 commented Apr 27, 2020

cgewecke commented Apr 27, 2020

holgerd77 left a comment

holgerd77 Apr 27, 2020

holgerd77 Apr 27, 2020

holgerd77 Apr 27, 2020

holgerd77 commented Apr 30, 2020

Update secp2561 ECDSA dependency to v4.0.0 #228

Update secp2561 ECDSA dependency to v4.0.0 #228

Conversation

holgerd77 commented Jan 29, 2020 • edited Loading

1. Analyze API Change Implications

2. Library Fixes

holgerd77 commented Jan 29, 2020

holgerd77 commented Jan 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsbradleyjr Jan 30, 2020 • edited Loading

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

holgerd77 commented Jan 31, 2020

holgerd77 commented Feb 3, 2020

alcuadrado commented Feb 3, 2020

holgerd77 commented Feb 4, 2020

alcuadrado commented Feb 8, 2020

github-actions bot commented Apr 23, 2020

cgewecke commented Apr 23, 2020 • edited Loading

cgewecke commented Apr 23, 2020 • edited Loading

holgerd77 commented Apr 24, 2020

holgerd77 commented Apr 24, 2020

cgewecke commented Apr 24, 2020

holgerd77 commented Apr 24, 2020

cgewecke commented Apr 27, 2020

holgerd77 commented Apr 27, 2020

cgewecke commented Apr 27, 2020

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

holgerd77 commented Apr 30, 2020

holgerd77 commented Jan 29, 2020 •

edited

Loading

michaelsbradleyjr Jan 30, 2020 •

edited

Loading

cgewecke commented Apr 23, 2020 •

edited

Loading

cgewecke commented Apr 23, 2020 •

edited

Loading