The entire point of this module is to be in coherence (with side-by-side readability) with BIP66... I'd rather not.

@dcousens BIP66 says nothing about encode, why not include auto-padding in this case?
What you think about check as try-catch?
I checked BIP66 more precisely, it's strange that IsValidSignatureEncoding has no conditions that lenR or lenS more than 33 bytes...

Indeed, it really should have been a 64-byte fixed format, maybe with segwit...

@dcousens BIP66 says nothing about encode, why not include auto-padding in this case?

Can you outline where it would be useful?

not a 64-bytes, 66-bytes (2 x 33 bytes)
right now I see that lenR + lenS should be less than 72 - 6, but in this case if lenR is 35 and lenS is 31, IsValidSignatureEncoding will still return true, although it is invalid secp256k1 DER signature

@dcousens BIP66 says nothing about encode, why not include auto-padding in this case?

Can you outline where it would be useful?

https://github.com/cryptocoinjs/secp256k1-node/blob/880a9ebb9e972f24803c1d86eaf851701c60ebd9/lib/index.js#L322

not a 64-bytes, 66-bytes (2 x 33 bytes)

No, I meant 64 bytes. The 33 byte case is basically just a padding byte (for the twos complement) anyway.

right now I see that lenR + lenS should be less than 72 - 6, but in this case if lenR is 35 and lenS is 31, IsValidSignatureEncoding will still return true, although it is invalid secp256k1 DER signature

Can you write a test case for that?

@fanatid PR it! 👍
Ping @sipa, I guess it was just assumed that this level of error checking would be handled by the underlying ecdsa library? (namely, libsecp256k1?)

@dcousens what you final thoughts about auto-padding in bip66 instead secp256k1 (and other libraries)?

If you pass a DER-valid encoding of an invalid signature to libsecp256k1, its parsing will succeed, but validation will fail.

So typically the steps are:

• Validate according to BIP66; if that fails, the script is invalid
• Parse with libsecp256k1. This should always succeed for BIP66 transactions. There is a piece of C code in the libsecp256k1 contrib directory to do parsing more lax, which will accept at least all valid signatures in the blockchain. You could translate it to JS if it's necessary for parsing old signatures.
• Call secp256k1_ecdsa_signature_normalize if you want to support highS signatures (this is not part of parsing, as it's technically changing the contents of the signature).
• Call secp256k1_ecdsa_signature_verify. If this fails, the signature is invalid, but the script itself may still be valid (e.g. in a CHECKMULTISIG, or more obscurely, when you would have the nonsensical script CHECKSIG NOT).

On the reason why BIP66 does not enforce a maximum length on R and S: it didn't need to.

All BIP66 try to do was to give valid signatures a unique and easy-to-determine encoding. Having an overlength R or S automatically make the transaction invalid, so it was not within scope.

Another way of looking at it is, if you're trying to duplicate part of the validation rules in BIP66 (which is about its encoding, not validity), why stop there?

• You could check whether R and S are within the range [1...secp256k1_order-1], as other signatures are always invalid.
• You could even go check whether R is a valid X coordinate on the curve modulo the secp256k1 field size.

In short, there was no need to add a rule limiting the size of R or S, so we didn't. There is a rule that checks the total length, because if that was over 255 bytes, you'd need a multi-byte-length decoder to check whether the signature is DER.

If we'd want to go a step further, I think we should add a rule that a signature either has to be valid (as in, completely valid) or empty. That would remove malleability of invalid signatures. Unfortunately that isn't possible with how CHECKMULTISIG works now.

Regarding 64-byte signatures: yes, absolutely. Once segwit is in (which introduces script versioning, making such changes much easier), I do plan on proposing a Schnorr-based scheme with 64-byte signatures. @gmaxwell recently came up with a scheme that would allow reducing all signatures in a transaction into a single one even.

@dcousens please reopen, if you decide inline auto-padding to encode:

var r = Buffer.concat([new Buffer([0]), sigObj.r])
for (var lenR = 33, posR = 0; lenR > 1 && r[posR] === 0x00 && !(r[posR + 1] & 0x80); --lenR, ++posR);

var s = Buffer.concat([new Buffer([0]), sigObj.s])
for (var lenS = 33, posS = 0; lenS > 1 && s[posS] === 0x00 && !(s[posS + 1] & 0x80); --lenS, ++posS);

@fanatid would all existing test fixtures still be valid?
What wouldn't be valid?
When does this 'auto'-padding kick in?

@dcousens not all existed fixtures will be valid (I fixed them in new commit)
since BIP66 not check that r/s can be longer than 32 bytes, I changed PR to adding/removing leading zero for DER (i.e. new commit not expand r/s to 32 bytes)

you can pass 0 to 32 bytes r/s buffer to encode
decode return 1 to 63(?) bytes r/s buffer

Hmmm, I understand why this is the best place to put this... but that doesn't make this feel a little out of place... but seeing as only encode is having a change in behaviour, not decode, I'm not against this as much as I was.

you do not want remove excessive leading zero byte on decode?

you do not want remove excessive leading zero byte on decode?

If the r value encoded is 0x01, I want 0x01 returned from decode. Not 0x0001.
If this change is merged, indeed, I'd want to remove the added zero byte.

you do not want remove excessive leading zero byte on decode?

If the r value encoded is 0x01, I want 0x01 returned from decode. Not 0x0001.
If this change is merged, indeed, I'd want to remove the added zero byte.

ok, first 0x01 can be encoded only as 0x01, but
0x80 encoded as 0x0080 and current master return 0x0080
with this PR result will be 0x80

PR:

bip66.encode(new Buffer([1]), new Buffer([128]))
<Buffer 30 07 02 01 01 02 02 00 80>
bip66.decode(bip66.encode(new Buffer([1]), new Buffer([128])))
{ r: <Buffer 01>, s: <Buffer 80> }

master:

bip66.decode(bip66.encode(new Buffer([1]), new Buffer([128])))
Error: S value is negative
bip66.encode(new Buffer([1]), new Buffer([0, 128]))
<Buffer 30 07 02 01 01 02 02 00 80>
bip66.decode(bip66.encode(new Buffer([1]), new Buffer([0, 128])))
{ r: <Buffer 01>, s: <Buffer 00 80> }

Right. Then this was all my fault. Thanks @fanatid

I'll see if I can't clean up a few things, PR it, merge then release patch.

Thanks, I'll create PR with tape/nyc in nearest time.

@fanatid so turns out this will probably break everything that depends on this package.
It appears everything was expecting the encoding/decoding of DER integers.

If we revert, we should really make that clear.

Thoughts on a way forward?

@fanatid basically we've "optimised" the package to return positive only integers, makes sense, but, the question is do we make this 2.0.0? Or do we revert back to DER two's complement integers which is currently the standard (ugh).

Yes, this PR require major version bump :(
with PR decode return positive/negative, before PR it could be only positive
IDK, maybe will better revert changes and let developer pass r/s as two's complement? (but we should write about this in readme!)

but we should write about this in readme!

Indeed.