Skip to content

Conversation

@dcousens
Copy link
Contributor

Followup to #373 and #378, where @rubensayshi and I are discussing whether or not we should re-order signatures (and potentially verify more than is necessary) or opt for the fastest solution.

The case can be seen clearly in the following example by @rubensayshi:
If you have the following set of pubkeys and signatures:

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

That is, all but pubKey2 has signed, and therefore their signature is missing.
If we allow re-ordering, the evaluation will be:

  • 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

If we don't:

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

This is only 1 less verification, but it could be substantial if there was more pubKeys and less signatures.

Any other feedback from any other onlookers is welcome :)

@dcousens
Copy link
Contributor Author

@rubensayshi separated from #378 as it is a bit more extensive now in test re-factoring, and I don't want that to interfere.

@dcousens dcousens self-assigned this Mar 11, 2015
@rubensayshi
Copy link
Contributor

var allowed = input.signatures.slice(i) where i is the index of the pubKey is not a good idea for case;

pubKey1  |  pubKey2  |  pubKey3   |  pubKey4
sig2  |  sig3  |  sig4

@dcousens
Copy link
Contributor Author

@rubensayshi it is now an equivalent of what you had in #378 haha. However, I use some over forEach to avoid the extra loops. Not much of a difference otherwise.

@dcousens
Copy link
Contributor Author

Will merge after ACK and release as 1.5.2

@dcousens dcousens force-pushed the 373opti branch 2 times, most recently from a3cd23a to 6c02e16 Compare March 13, 2015 04:42
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 99.16% when pulling 6c02e16 on 373opti into 6800518 on master.

@rubensayshi
Copy link
Contributor

ACK

dcousens added a commit that referenced this pull request Mar 15, 2015
TransactionBuilder.sign signature re-ordering and verification optimization
@dcousens dcousens merged commit 9e631ce into master Mar 15, 2015
@dcousens dcousens deleted the 373opti branch March 15, 2015 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants