TransactionBuilder P2SH Multisig order of signatures

[rubensayshi]

When building a transaction the order of the signatures of a P2SH multisig should be the same as the order of their respective pubKey.

This PR fixes that by looping over the pubKeys in the __build and finding the signature that belongs to the pubKey (and 2 changes to make sure that the input.pubKeys is actually the correct list of pubKeys).

A problem however that I'm using pubKey.verify calls to figure out which signature belongs to which pubKey, and the pubKey.verify calls are reletively heavy on performance, making the __build take ~0.12 instead of the old ~0.03 (testing with the 2of2 from the test case).

Alternatively I could move the resposibility to put the signatures in the right order to txb.sign and TransactionBuilder.fromTransaction, prefilling the input.signatures with NULL values for missing signatures, in which case fromTransaction would be doing the pubKey.verify calls instead of __build.
To compare I did another PR with that version of the code; <gimme a sec to make the PR ;-)>

The 2nd solution is probably better since it involves less pubKey.verify calls and they're only in txb.sign, which is where you would expect the 'heavy lifting'

[coveralls]

Coverage increased (+0.02%) to 98.27% when pulling bbbc8a2 on blocktrail:multisig-signature-order into abf870f on bitcoinjs:master.

[rubensayshi]

test failure seems to be a failure of the faucet?

[dcousens]

This is already implemented (among other things) in #329, infact, it looks to have been done in almost the same way too.

The verify step implemented here is also O(n^2) vs worst-case O(n) as implemented here.

[dcousens]

@rubensayshi granted the PR size of #329 is larger, it also does serve a slightly larger purpose in ensuring less information is thrown away in fromTransaction.

For the best comparison, I'd read through each of the commits individually.

[rubensayshi]

@dcousens cool, I'll close this PR and await the 2.0.0 release (and in the meantime use my own patch)

[dcousens]

@rubensayshi cheers mate, I'd appreciate another pair of eyes for review if you have the time.

These PRs were great to see another approach to problem (even if it was very similar).

[rubensayshi]

@dcousens I'll take a close look at it.

PS. how stable is the 2.0.0 branch? maybe I could already run on the 2.0.0 branch to test it a bit?

[dcousens]

The 2.0.0 branch it self is unstable, infact I don't even know if it would pass a compilation test. It needs to be rebased on all the other changes (and fixes). Once #329 is merged, I'll be doing that and then soon after releasing it.

[rubensayshi]

okay, I'll keep an eye out :)

[rubensayshi]

@dcousens in my alternative implementation (which isn't O(n^2)) #349 I actually moved the pubKey.verify calls into the txb.sign instead of TransactionBuilder.fromTransaction.

doing it in TransactionBuilder.fromTransaction shouldn't be neccesary unless you're going to add a signature with txb.sign, I think it's safe to asume that the signatures should already be in the correct order in cases where you're not going to sign it and that way it's a little bit better for performance (and imo it's a bit weird that TransactionBuilder.fromTransaction would have such a performance hit).
this also applies for example when you're only going to sign 1 input in a TX with more (already signed) inputs.

if you agree, I can do a PR to change your code to do the same?