Skip to content

Conversation

@rubensayshi
Copy link
Contributor

this PR is based off https://github.com/bitcoinjs/bitcoinjs-lib/tree/op0fix (#372) and I also merged in #370 because both of those are needed for me to continue upon in this PR.

  • fix the order of the signature in txb.sign if there's not enough OP_0s (or forceFixMultisigSigOrder is specified when the user knows they're getting a messed up TX).
  • changed tests to use fixtures.

Like I said in #369 when doing txb = new TransactionBuilder(); txb.addInput(); txb.sign() the sign is the first time we actually learn about the redeemScript so it's not possible to do this stuff in fromTransaction but it has to go into sign.

@rubensayshi rubensayshi force-pushed the fix-msig-sigs-order branch from eb5469c to f12de10 Compare March 3, 2015 12:17
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 99.24% when pulling f12de10 on blocktrail:fix-msig-sigs-order into b132714 on bitcoinjs:master.

rubensayshi referenced this pull request in blocktrail/bitcoinjs-lib Mar 4, 2015
@rubensayshi
Copy link
Contributor Author

hey @dcousens if you use the 'Files Changed' tab of the PR and do comments there instead of going commit for commit than it's a bit easier imo to reply to the stuff than going to the commits.

also because you went commit for commit, you basicly commented a lot of a commit I actually took the code out for in a later commit ;-)

also after a rebase you're comments are sort of lost :( afaik if you comment from 'Files Changed' tab then it will list it in the PR as 'comment on outdated comment' so they're not completely lost

anyway to sum up the stuff from your comments in the commits;

  • I replaced the tests with fixtures and took out the old test that was there (fixtures also have a better description, no more gapped)
  • I took out the fixMultisigSigOrder option, you're right that there's no real use case for it and I don't like adding it to fromTransaction
  • You're right that these tests are testing not a single method of the TransactionBuilder but a chain of actions, so in the purest sence they're not unittests anymore.
    If you want I will move them to test/integration/multisig.js instead, though I personally consider tests only to be 'integration tests' if they are really large or required to interact with external systems (such as an API, DB or faucet)

@rubensayshi rubensayshi force-pushed the fix-msig-sigs-order branch 3 times, most recently from d495d5b to a46e63c Compare March 4, 2015 10:00
@dcousens
Copy link
Contributor

dcousens commented Mar 4, 2015

@rubensayshi no worries, not sure why I commented on the commits. My bad.

@dcousens
Copy link
Contributor

dcousens commented Mar 4, 2015

Will review tonight.

@rubensayshi rubensayshi force-pushed the fix-msig-sigs-order branch from a46e63c to 1e752d9 Compare March 4, 2015 10:42
@dcousens
Copy link
Contributor

dcousens commented Mar 5, 2015

Sorry, was a bit delayed on reviewing this. If you could please rebase on master :). Will begin review anyway.

@dcousens dcousens modified the milestone: 2.0.0 Mar 5, 2015
…n txb.sign when it's not already prefilled with OP_0s
redid P2SH multisig tests to use fixtures
@rubensayshi rubensayshi force-pushed the fix-msig-sigs-order branch from 1e752d9 to 745eace Compare March 5, 2015 09:21
@rubensayshi rubensayshi mentioned this pull request Mar 5, 2015
Closed
@rubensayshi
Copy link
Contributor Author

rebased

dcousens added a commit that referenced this pull request Mar 10, 2015
TransactionBuilder P2SH Multisig order of signatures
@dcousens dcousens merged commit e06f7f6 into bitcoinjs:master Mar 10, 2015
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only qualm was that I don't think this is necessary. However I know it is necessary given the current solution of pre-emptively always putting in the OP_0's. I might change that and submit a PR.

@dcousens
Copy link
Contributor

Awesome work @rubensayshi . Thanks so much for this.

Sorry it took me a while to get around to it.

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.

3 participants