Skip to content

Conversation

@rubensayshi
Copy link
Contributor

when I did my first PR to deal with the out of order signatures etc this was I think in there but we removed it because there was no use/test case that required it.

unfortunatly I had a client who was still using a old version of our SDK that used bitcoinjs-lib@1.4.4 and it produces only OP_0s for the precending pubkeys, so if you sign key with 2 - out of 3 - then you get OP_0 SIG_2 for signatures (and not OP_0 SIG_2 OP_0).

this fixes the signing breaking if this occurs.

testcase is hardcoded instead of fixtures because I had to start out with a partially signed half broken raw hex

you can cherry-pick blocktrail@442835b for 1.5.x version of this

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.41% when pulling d2bee4d on blocktrail:sign-incorrect-op0s-master into f64df79 on bitcoinjs:master.

@dcousens
Copy link
Contributor

Thanks for this @rubensayshi, I'll review this asap :)

@dcousens dcousens self-assigned this May 30, 2015
@dcousens dcousens added this to the 1.5.8 milestone May 30, 2015
@weilu
Copy link
Contributor

weilu commented May 30, 2015

@rubensayshi Would you consider move the signature check to ecdsa's verify function instead? It'd make the verify function more robust, and easier to test.

@dcousens
Copy link
Contributor

@weilu hmmm, I can't decide if that is a good design or not? Right now I'd prefer to see an Error thrown (as it does) if the signature is missing?

@weilu
Copy link
Contributor

weilu commented May 30, 2015

@dcousens the rest of verify doesn't tell you why it's returning false, does it? :P And you know you hate "can't call foo on undefined" as much as I do. One solution I can think of is to pass an optional debug argument to verify which will populate the reason key when verify returns false.

@dcousens
Copy link
Contributor

can't call foo on undefined

But, at least then I know the problem. I'll have a think on this one. I'm not overwhelmingly against it.

@dcousens
Copy link
Contributor

dcousens commented Jun 1, 2015

ACK. Thanks @rubensayshi,

dcousens added a commit that referenced this pull request Jun 1, 2015
fix txb.sign having issues when an incomplete TX contains OP_0s, but not enough
@dcousens dcousens merged commit eb55085 into bitcoinjs:master Jun 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants