Skip to content

Conversation

@dcousens
Copy link
Contributor

@dcousens dcousens commented Sep 26, 2017

Resolves #862

witnessScriptHash was a bit of a cluster... returned redeemScriptSig, a script, not witness data.
Could be a breaking change, but I'd consider it a bug tbh.

@dcousens dcousens added the bug label Sep 26, 2017
@dcousens dcousens requested a review from afk11 September 26, 2017 22:32
@dcousens dcousens added this to the 3.2.1 milestone Sep 26, 2017
@dcousens
Copy link
Contributor Author

dcousens commented Sep 26, 2017

We don't even use witnessScriptHash.input in TransactionBuilder...
We build it manually:

        witness = buildStack(input.witnessScriptType, input.signatures, input.pubKeys, allowIncomplete)
        witness.push(input.witnessScript)

We do the same for scriptHash.input...

  // append redeemScript if necessary
  if (p2sh) {
    sig.push(input.redeemScript)
  }

  ...

  script: bscript.compile(sig),

if (f.type !== 'pubkeyhash' && f.type !== 'witnesspubkeyhash') return
if (!f.inputStack) return

var pubKey = Buffer.from(f.pubKey, 'hex')
Copy link
Member

Choose a reason for hiding this comment

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

use fromHex here?

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.

@junderw I was only using fromHex in the .map(function (x) { return Buffer.from(x, 'hex') }) case, if we had ES6 lambdas, I wouldn't have bothered.

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.

utACK

@dcousens dcousens merged commit 1157856 into master Sep 27, 2017
@dcousens dcousens deleted the segwittests branch September 27, 2017 14:03
@dcousens
Copy link
Contributor Author

Published v3.2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants