#373 clean up

[dcousens]

#373 introduced some changes to TransactionBuilder that solved a few multisig bugs.

Thanks to @rubensayshi :)

However, there a few things that just need to be cleaned up before publish :)

edit: WIP PR, ready for merge by tomorrow hopefully

[dcousens]

@rubensayshi on further testing, I was optimizing the sign from O(n*m) to make use of the guaranteed order of the public keys, and I've noticed you are still re-ordering the public keys if they are shuffled.

Was there a reason for this? I thought we agreed to not include that since that would mean a client is incorrectly signing transactions.
It also means the current solution cannot handle the case (AFAIK) where a multi-signature address is made up of the same public key more than once.
I'll have to add a test for that to find out :)

[rubensayshi]

it actually does make use of the order of pubKeys and signatures a bit, just not for the pubKeys for which there's no signature yet.

whenever a signature is matched it's spliced and the next pubKey won't be checking it, so if we have

pubKey1  |  pubKey2  |  pubKey3   |  pubKey4
sig1  |  sig3  |  sig4

then

• pubKey1 will match sig1 and break,
• pubKey2 will try to match sig3 and sig4 and fail
• pubKey3 will match sig3 and break
• pubKey4 will match sig4 and we're done

I guess you want pubKey2 to break if it doesn't match sig3 and not try sig4.

[rubensayshi]

ah sorry for leaving that in

[dcousens]

I'll have to read through it again. Thanks for the explanation.
On 10 Mar 2015 8:41 pm, "Ruben de Vries" notifications@github.com wrote:

it actually does make use of the order of pubKeys and signatures a bit,
just not for the pubKeys for which there's no signature yet.

whenever a signature is matched it's spliced and the next pubKey won't be
checking it, so if we have

pubKey1 | pubKey2 | pubKey3 | pubKey4
sig1 | sig3 | sig4

then

• pubKey1 will match sig1 and break,
  • pubKey2 will try to match sig3 and sig4 and fail
• pubKey3 will match sig3 and break
• pubKey4 will match sig4 and we're done

I guess you want pubKey2 to break if it doesn't match sig3 and not try
sig4.

—
Reply to this email directly or view it on GitHub
#378 (comment)
.

[rubensayshi]

the tests are failing because with this change if a 2of3 is signed key1 and key2 (or a 2of2 with just key1) you won't have OP_0s for key3, imo you should stick to input.signatures = input.pubKeys.map to make sure that input.signatures.length == input.pubKeys.length.

[dcousens]

Stupid battery died, charger at work. Will follow up tomorrow.
On 10 Mar 2015 8:46 pm, "Ruben de Vries" notifications@github.com wrote:

In src/transaction_builder.js
#378 (comment)
:

•  return signature || ops.OP_0
  

•  allowed.some(function (signature) {
  

•    if (!pubKey.verify(signatureHash, signature)) return
  

•    signatures[i] = signature
  

the tests are failing because with this change if a 2of3 is signed key1
and key2 (or a 2of2 with just key1) you won't have OP_0s for key3, imo
you should stick to input.signatures = input.pubKeys.map to make sure
that input.signatures.length == input.pubKeys.length.

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoinjs/bitcoinjs-lib/pull/378/files#r26108461.

[dcousens]

I guess you want pubKey2 to break if it doesn't match sig3 and not try sig4.

IMHO, yes. It overall means less verifies (faster), and should never be a valid case unless someone is using buggy software.

[dcousens]

@rubensayshi (ref 7cd60aa), please ACK

If you feel these tests are beneficial, could you outline what they are testing, maybe we could add them to the scripts fixtures?

In the end, anything really odd would be picked up by the final txHex comparison anyway.

[dcousens]

@rubensayshi I cleaned up the tests a lot, and there were a few comments I made along the way that may not make sense anymore, please verify by checking this thread (not your email haha).

I have opted for 69eb58c instead of txHexIncomplete, it is more obvious what is happening and very easy to debug.

[dcousens]

Perhaps for easy test transparency, instead of (or at least, in addition to) the final txHex, we could instead just iterate over each input and have a final ASM scriptSig with which to verify against?

[dcousens]

@rubensayshi probably easiest to review commit-by-commit.

[coveralls]

Coverage remained the same at 99.16% when pulling a29761c on 373clean into 3e44c41 on master.

[dcousens]

Tests are only failing because helloblock is experiencing technical difficulties.

[rubensayshi]

@dcousens we have a different taste for unittesting, but that's fine and the changes look okay