Skip to content

Conversation

@dcousens
Copy link
Contributor

@dcousens dcousens commented Sep 26, 2017

@afk11 maybe this is what you intended in #909 ?

f4a83f8 is a follow up to #907 which was having issues locally

if (input.value !== undefined && input.value !== witnessValue) throw new Error('Input didn\'t match witnessValue')
typeforce(types.Satoshi, witnessValue)
input.value = witnessValue
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this side steps the call to fixMultisigOrder, which would remove any signatures from the array which aren't valid?

Copy link
Contributor Author

@dcousens dcousens Sep 27, 2017

Choose a reason for hiding this comment

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

@afk11 this isn't related to MS order, it simply ensures that .value is added on if missing, I may have misinterpreted #909, but, this resolves the bug in #901

@afk11
Copy link
Contributor

afk11 commented Sep 27, 2017

@dcousens ah, I see the confusion.. I just found the exact same issue as mine recently: #901 (comment) which is indeed related to the input.value not being present, but the fixMsOrder demonstrates how it can be a problem if you leave it until sign - invalid signatures remain in the array unless fixMultisigOrder is called.

So my solution was #909, which means that signatures fail and aren't extracted IF the transaction, which was passed to TransactionBuilder.fromTransaction as inputs which have the input.value for segwit outputs.

Ping @dakk since we are having identical issues.

Copy link
Contributor

@afk11 afk11 left a comment

Choose a reason for hiding this comment

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

Concept ACK. Not a full fix for #909 but restores fields which may otherwise have been missing

@dcousens dcousens merged commit ace2b55 into master Sep 27, 2017
@dcousens dcousens deleted the txbfix branch September 27, 2017 13:17
@dcousens
Copy link
Contributor Author

As discussed on Slack, we'll need to incorporate the second half of the complete fix for this problem in #909

@dakk
Copy link

dakk commented Sep 27, 2017

I confirm it works now with my test case, excellent work

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