Remove explicit DER/Compact decoding #459

dcousens · 2015-09-08T06:35:21Z

None of ECSignature, except parseScriptSignature, is relevant to bitcoin or this libraries intended use.

Ideally #394 might allow us to expose these functions for general purpose in the underlying ECC library.
Since we don't expose raw ecdsa, that would be the best place for it.

It is basically a pointless abstraction for this library.

IMHO.

Chained on #456

dcousens · 2015-12-13T23:59:01Z

Rebased and addressed #459 (comment)

jprichardson · 2015-12-14T21:20:43Z

Funny, I just updated https://github.com/cryptocoinjs/ecdsa to track bitcoinjs-lib/ecdsa. I suppose these methods can rest in there for the time being.

dcousens · 2016-07-11T09:13:58Z

Rebased, obviously still 3.0.0 though

dcousens · 2016-07-11T09:16:23Z

src/transaction_builder.js

      scriptSig = bscript.pubKeyInput(pkSignature)
      break

    case 'multisig':
      var msSignatures = input.signatures.map(function (signature) {
-        return signature && signature.toScriptSignature(input.hashType)
+        return signature && bscript.signature.encode(signature, input.hashType)
      })

      // fill in blanks with OP_0


This file outlines the real differences that users will feel.

IMHO it is an improvement, but @jprichardson I'd be up to hear your opinion.
Less magic, clear parameters and a functional style.

The only thing I don't like is that .decode(signature).signature is a thing.
Perhaps

{ r: "...", s: "...", hashType: 0 }

Would be better as the decode result

jprichardson · 2016-07-11T13:53:39Z

Given that signatures need to be serialized, parsed, and validated, I'm not so sure I'd be quick to call the abstraction useless - yes, maybe this implementation, but an implementation, probably not. This seems like would be very low-priority. I'd be in favor of ditching it for 3.0 - but I think 3.0 should just focus on the move to secp2561k and other pending breaking changes.

dcousens · 2016-07-11T15:01:58Z

This seems like would be very low-priority.

Indeed my thoughts. However, this could make the movement to secp256k1 easier due to less specialised / non-generic interfaces?

dcousens · 2016-10-13T12:52:52Z

This is ultimately part of #407.
With #456 merged, and bip66 being a separate module... there is literally no reason to have toDER and toCompact.

This finalises that move.
This is hopefully the last non-trivial (aka documentation) PR for 3.0.0.

Ping @jprichardson @rubensayshi @afk11

https://www.npmjs.com/package/bip66 is very explicit in its purpose... and I think that explicit nature is a bonus to everyone, provided we document that pathway for people to easily understand.

dcousens · 2016-10-13T13:05:01Z

Known breaks for code in the wild (based on a GitHub search):

https://github.com/sabakaio/mnemoauth-client/blob/a46202351a976e4346277ddf9827661a29ca30be/index.js#L19

It would need to become

var signature = authKey.sign(messageHash)
var encoded = bip66.encode(signature.r, signature.s)

Manual unpacking... yay? 😕

https://github.com/darkwallet/darkwallet/blob/4c51dacacb8541305f1442ef7fa1628eb7c19a79/src/js/dwutil/multisig.js#L202
Very weird usage there?
Not sure why, but, again, should be a straight bip66 drop in.

https://github.com/BitGo/BitGoJS/blob/5a06433c4c424e5f7bebe1ef2895b857622751b6/src/bitcoin.js#L114

They are using secp256k1 for performance, and then double serialising? Interesting... would be a bip66.decode(...)

https://github.com/Colu-platform/colu-access/blob/b03bac5b8d23110b48383a1b319939ef817246ea/src/coluaccess.js#L317

Straight up replacement with bip66 module.

dcousens · 2016-10-13T13:10:35Z

If no consensus is reached within 24 hours, I'm going to bump this to 4.0.0 and start the 3.0.0 release process.

rubensayshi · 2016-10-13T13:17:32Z

🚀

the BitGo case would actually just become return secp256k1.signatureExport(sig) right? they overload the ECPair.sign and then go from Buffer -> Signature, but now they can just return that buffer.

the others are odd use cases for which the bip66 package is a perfect solution.

dcousens · 2016-10-13T13:37:14Z

@rubensayshi for PR to be complete in that respect, just realised it needs to serialize r in ecdsa.sign and deserialize in ecdsa.verify

Maybe we should just return a bip66 signature there instead?
Save everyone of that hassle?

script.signature.encode would then just work on the DER signature... aka just adding the extra byte

dcousens · 2016-11-02T03:01:48Z

Bumped to 4.0.0...

dcousens · 2018-04-13T05:31:15Z

Rebased

dcousens · 2018-04-13T05:58:39Z

src/script_signature.js

+module.exports = {
+  fromRSBuffer,
+  decode: decode,
+  encode: encode


For 4.0.0, the signature parameter will probably become a 64-byte RS Buffer, for direct compatability with secp256k1 libraries.

This implementation accepts the { r, s } tuple for now as it is a simpler transition path, even if it never gets published.

The intention is that ECPair.sign should pass directly to these functions without issue.
So when ECPair.sign changes, this will change.

dcousens · 2018-05-14T16:20:56Z

Rebased (and tests passing locally, incl. integration tests).

Will merge on green tick if no ACKs appear 👍

Part of #1049

(viewing Travis log, there was a standard issue, that will be promptly fixed, no point waiting further)

dcousens added the breaking change label Sep 8, 2015

dcousens modified the milestones: 2.0.0, 3.0.0 Sep 8, 2015

dcousens force-pushed the nocompact branch from 5850d3c to 6075c9f Compare September 14, 2015 07:47

dcousens mentioned this pull request Nov 30, 2015

API for 4.0.0 #508

Closed

dcousens force-pushed the nocompact branch from 6075c9f to 3204ae9 Compare December 13, 2015 23:58

fanatid mentioned this pull request Mar 23, 2016

Extract ECC operations to 3rd party module #394

Closed

dcousens force-pushed the nocompact branch from 3204ae9 to 5c17485 Compare July 11, 2016 09:13

dcousens reviewed Jul 11, 2016
View reviewed changes

dcousens assigned jprichardson, dcousens and fanatid Jul 11, 2016

dcousens closed this Oct 7, 2016

dcousens reopened this Oct 7, 2016

dcousens force-pushed the nocompact branch from 5c17485 to bfc2597 Compare October 13, 2016 12:51

dcousens modified the milestones: 3.0.0, 4.0.0 Oct 13, 2016

dcousens added the how to / question / docs label Oct 13, 2016

dcousens modified the milestones: 4.0.0, 3.0.0 Nov 2, 2016

dcousens mentioned this pull request Dec 17, 2016

High performance ECDH #737

Closed

dcousens mentioned this pull request Oct 19, 2017

BIP32/secp256k1/keyPair preparation #915

Merged

1 task

dcousens force-pushed the nocompact branch from bfc2597 to 5350eb5 Compare April 13, 2018 05:31

dcousens force-pushed the nocompact branch from 5350eb5 to 883d211 Compare April 13, 2018 05:32

dcousens commented Apr 13, 2018

View reviewed changes

dcousens force-pushed the nocompact branch from 1aef989 to e9ec196 Compare April 16, 2018 01:36

dcousens added 3 commits May 15, 2018 02:18

rm ECSignature, add script.signature instead

c58ada3

add toRSBuffer for less breaking changes

c168474

use safe-buffer

90a73e1

dcousens force-pushed the nocompact branch from e9ec196 to 90a73e1 Compare May 14, 2018 16:19

dcousens merged commit 087b49e into master May 14, 2018

dcousens deleted the nocompact branch May 14, 2018 23:58

Ashtonulliac mentioned this pull request Aug 1, 2019

Send my bitcoin $to ulliactrevor267@gmail.com Ashtonulliac/bitcoinjs-lib#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove explicit DER/Compact decoding #459

Remove explicit DER/Compact decoding #459

dcousens commented Sep 8, 2015

dcousens commented Dec 13, 2015

jprichardson commented Dec 14, 2015

dcousens commented Jul 11, 2016

dcousens Jul 11, 2016 •

edited

Loading

jprichardson commented Jul 11, 2016

dcousens commented Jul 11, 2016

dcousens commented Oct 13, 2016 •

edited

Loading

dcousens commented Oct 13, 2016 •

edited

Loading

dcousens commented Oct 13, 2016

rubensayshi commented Oct 13, 2016 •

edited

Loading

dcousens commented Oct 13, 2016 •

edited

Loading

dcousens commented Nov 2, 2016

dcousens commented Apr 13, 2018

dcousens Apr 13, 2018 •

edited

Loading

dcousens commented May 14, 2018 •

edited

Loading

Remove explicit DER/Compact decoding #459

Remove explicit DER/Compact decoding #459

Conversation

dcousens commented Sep 8, 2015

dcousens commented Dec 13, 2015

jprichardson commented Dec 14, 2015

dcousens commented Jul 11, 2016

dcousens Jul 11, 2016 • edited Loading

Choose a reason for hiding this comment

jprichardson commented Jul 11, 2016

dcousens commented Jul 11, 2016

dcousens commented Oct 13, 2016 • edited Loading

dcousens commented Oct 13, 2016 • edited Loading

dcousens commented Oct 13, 2016

rubensayshi commented Oct 13, 2016 • edited Loading

dcousens commented Oct 13, 2016 • edited Loading

dcousens commented Nov 2, 2016

dcousens commented Apr 13, 2018

dcousens Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

dcousens commented May 14, 2018 • edited Loading

dcousens Jul 11, 2016 •

edited

Loading

dcousens commented Oct 13, 2016 •

edited

Loading

dcousens commented Oct 13, 2016 •

edited

Loading

rubensayshi commented Oct 13, 2016 •

edited

Loading

dcousens commented Oct 13, 2016 •

edited

Loading

dcousens Apr 13, 2018 •

edited

Loading

dcousens commented May 14, 2018 •

edited

Loading