Repository: bitcoinjs/bitcoinjs-lib (js)

Issue:
s/hashType/hashTypes in `TransactionBuilder.inputs`
Reference https://github.com/bitcoinjs/bitcoinjs-lib/issues/417.

We currently assume that signatures are signing with the same hash type, but as described by @dthorpe,  that assumption can break in some scenarios:

(paraphrased and simplified)
```
* Alice and Bob send funds to a 2-of-2 multisig
* Bob builds a transaction spending the funds, and sends it to Alice
* Alice signs the relevant inputs with SIGHASH_ALL | SIGHASH_ANYONECANPAY and returns the transaction
* Carol adds some (signed) inputs to the transaction
* Bob signs the transaction with SIGHASH_ALL
* Transaction is broadcasted
```

Pull request:
TransactionBuilder: further cleanup and refactoring
Just part of the effort of making this more review-able. 

Cleaned up a few assumptions that weren't correct too in `fixMultisigOrder`,  it simply won't ever work unless a `redeemScript` was provided (and hence, `pubKeys` defined)

Also resolves #642 

Patch:
diff --git a/.travis.yml b/.travis.yml
index 037e1f8b..463c24bc 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -4,7 +4,6 @@ before_install:
   - npm install npm -g
 node_js:
   - "0.12"
-  - "io.js"
   - "4"
   - "5"
   - "6"
diff --git a/src/transaction_builder.js b/src/transaction_builder.js
index add817e8..c34ec178 100644
--- a/src/transaction_builder.js
+++ b/src/transaction_builder.js
@@ -11,15 +11,14 @@ var ECPair = require('./ecpair')
 var ECSignature = require('./ecsignature')
 var Transaction = require('./transaction')
 
-// inspects a scriptSig w/ optional provided redeemScript and derives
-// all necessary input information as required by TransactionBuilder
+// inspects a scriptSig w/ optional redeemScript and
+// derives any input information required
 function expandInput (scriptSig, redeemScript) {
   var scriptSigChunks = bscript.decompile(scriptSig)
-  var scriptSigType = bscript.classifyInput(scriptSigChunks, true)
+  var prevOutType = bscript.classifyInput(scriptSigChunks, true)
+  var pubKeys, signatures, prevOutScript
 
-  var hashType, pubKeys, signatures, prevOutScript
-
-  switch (scriptSigType) {
+  switch (prevOutType) {
     case 'scripthash':
       // FIXME: maybe depth limit instead, how possible is this anyway?
       if (redeemScript) throw new Error('Recursive P2SH script')
@@ -36,13 +35,10 @@ function expandInput (scriptSig, redeemScript) {
 
     case 'pubkeyhash':
       // if (redeemScript) throw new Error('Nonstandard... P2SH(P2PKH)')
-      var s = ECSignature.parseScriptSignature(scriptSigChunks[0])
-      hashType = s.hashType
       pubKeys = scriptSigChunks.slice(1)
-      signatures = [s.signature]
+      signatures = scriptSigChunks.slice(0, 1)
 
       if (redeemScript) break
-
       prevOutScript = bscript.pubKeyHashOutput(bcrypto.hash160(pubKeys[0]))
       break
 
@@ -51,9 +47,7 @@ function expandInput (scriptSig, redeemScript) {
         pubKeys = bscript.decompile(redeemScript).slice(0, 1)
       }
 
-      var ss = ECSignature.parseScriptSignature(scriptSigChunks[0])
-      hashType = ss.hashType
-      signatures = [ss.signature]
+      signatures = scriptSigChunks.slice(0, 1)
       break
 
     case 'multisig':
@@ -62,31 +56,53 @@ function expandInput (scriptSig, redeemScript) {
       }
 
       signatures = scriptSigChunks.slice(1).map(function (chunk) {
-        if (chunk === ops.OP_0) return undefined
-
-        var sss = ECSignature.parseScriptSignature(chunk)
-
-        if (hashType !== undefined) {
-          if (sss.hashType !== hashType) throw new Error('Inconsistent hashType')
-        } else {
-          hashType = sss.hashType
-        }
-
-        return sss.signature
+        return chunk === ops.OP_0 ? undefined : chunk
       })
-
       break
   }
 
   return {
-    hashType: hashType,
     pubKeys: pubKeys,
     signatures: signatures,
     prevOutScript: prevOutScript,
-    prevOutType: scriptSigType
+    prevOutType: prevOutType
   }
 }
 
+// could be done in expandInput, but requires the original Transaction for hashForSignature
+function fixMultisigOrder (input, transaction, vin) {
+  if (input.redeemScriptType !== 'multisig' || !input.redeemScript) return
+  if (input.pubKeys.length === input.signatures.length) return
+
+  var unmatched = input.signatures.concat()
+
+  input.signatures = input.pubKeys.map(function (pubKey, y) {
+    var keyPair = ECPair.fromPublicKeyBuffer(pubKey)
+    var match
+
+    // check for a signature
+    unmatched.some(function (signature, i) {
+      // skip if undefined || OP_0
+      if (!signature) return false
+
+      // TODO: avoid O(n) hashForSignature
+      var parsed = ECSignature.parseScriptSignature(signature)
+      var hash = transaction.hashForSignature(vin, input.redeemScript, parsed.hashType)
+
+      // skip if signature does not match pubKey
+      if (!keyPair.verify(hash, parsed.signature)) return false
+
+      // remove matched signature from unmatched
+      unmatched[i] = undefined
+      match = signature
+
+      return true
+    })
+
+    return match
+  })
+}
+
 function expandOutput (script, scriptType, ourPubKey) {
   typeforce(types.Buffer, script)
 
@@ -125,62 +141,7 @@ function expandOutput (script, scriptType, ourPubKey) {
   }
 }
 
-function buildInput (input, scriptType, allowIncomplete) {
-  var signatures = input.signatures
-  var scriptSig
-
-  switch (scriptType) {
-    case 'pubkeyhash':
-      // remove blank signatures
-      signatures = signatures.filter(function (x) { return x })
-
-      if (signatures.length < 1) throw new Error('Not enough signatures provided')
-      if (signatures.length > 1) throw new Error('Too many signatures provided')
-
-      var pkhSignature = signatures[0].toScriptSignature(input.hashType)
-      scriptSig = bscript.pubKeyHashInput(pkhSignature, input.pubKeys[0])
-      break
-
-    case 'pubkey':
-      // remove blank signatures
-      signatures = signatures.filter(function (x) { return x })
-
-      if (signatures.length < 1) throw new Error('Not enough signatures provided')
-      if (signatures.length > 1) throw new Error('Too many signatures provided')
-
-      var pkSignature = signatures[0].toScriptSignature(input.hashType)
-      scriptSig = bscript.pubKeyInput(pkSignature)
-      break
-
-    // ref https://github.com/bitcoin/bitcoin/blob/d612837814020ae832499d18e6ee5eb919a87907/src/script/sign.cpp#L232
-    case 'multisig':
-      signatures = signatures.map(function (signature) {
-        return signature && signature.toScriptSignature(input.hashType)
-      })
-
-      if (allowIncomplete) {
-        // fill in blanks with OP_0
-        for (var i = 0; i < signatures.length; ++i) {
-          signatures[i] = signatures[i] || ops.OP_0
-        }
-      } else {
-        // remove blank signatures
-        signatures = signatures.filter(function (x) { return x })
-      }
-
-      scriptSig = bscript.multisigInput(signatures, allowIncomplete ? undefined : input.redeemScript)
-      break
-  }
-
-  // wrap as scriptHash if necessary
-  if (input.prevOutType === 'scripthash') {
-    scriptSig = bscript.scriptHashInput(scriptSig, input.redeemScript)
-  }
-
-  return scriptSig
-}
-
-function prepareInput (input, kpPubKey, redeemScript, hashType) {
+function prepareInput (input, kpPubKey, redeemScript) {
   if (redeemScript) {
     var redeemScriptHash = bcrypto.hash160(redeemScript)
 
@@ -196,24 +157,19 @@ function prepareInput (input, kpPubKey, redeemScript, hashType) {
     var expanded = expandOutput(redeemScript, undefined, kpPubKey)
     if (!expanded.pubKeys) throw new Error('RedeemScript not supported "' + bscript.toASM(redeemScript) + '"')
 
-    // if we don't have a prevOutScript, generate a P2SH script
-    if (!input.prevOutType) {
-      input.prevOutScript = bscript.scriptHashOutput(redeemScriptHash)
-      input.prevOutType = 'scripthash'
-    }
-
     input.pubKeys = expanded.pubKeys
+    input.signatures = expanded.signatures
     input.redeemScript = redeemScript
     input.redeemScriptType = expanded.scriptType
-    input.signatures = expanded.signatures
+    input.prevOutScript = input.prevOutScript || bscript.scriptHashOutput(redeemScriptHash)
+    input.prevOutType = 'scripthash'
 
-  // maybe we have some prior knowledge
+  // maybe we have some prevOut knowledge
   } else if (input.prevOutType) {
     // pay-to-scriptHash is not possible without a redeemScript
     if (input.prevOutType === 'scripthash') throw new Error('PrevOutScript is P2SH, missing redeemScript')
 
-    // try to derive the missing information about the script now that we
-    // have a kpPubKey
+    // try to derive missing information using our kpPubKey
     expanded = expandOutput(input.prevOutScript, input.prevOutType, kpPubKey)
     if (!expanded.pubKeys) return
 
@@ -224,46 +180,51 @@ function prepareInput (input, kpPubKey, redeemScript, hashType) {
   } else {
     input.prevOutScript = bscript.pubKeyHashOutput(bcrypto.hash160(kpPubKey))
     input.prevOutType = 'pubkeyhash'
-
     input.pubKeys = [kpPubKey]
     input.signatures = [undefined]
   }
-
-  input.hashType = hashType
 }
 
-function fixMultisigOrder (input, transaction, vin) {
-  var hashScriptType = input.redeemScriptType || input.prevOutType
-  if (hashScriptType !== 'multisig') throw new TypeError('Expected multisig input')
+function buildInput (input, allowIncomplete) {
+  var signatures = input.signatures
+  var scriptType = input.redeemScriptType || input.prevOutType
+  var scriptSig
 
-  var hashType = input.hashType || Transaction.SIGHASH_ALL
-  var hashScript = input.redeemScript || input.prevOutScript
+  switch (scriptType) {
+    case 'pubkeyhash':
+    case 'pubkey':
+      if (signatures.length < 1 || !signatures[0]) throw new Error('Not enough signatures provided')
+      if (scriptType === 'pubkeyhash') {
+        scriptSig = bscript.pubKeyHashInput(signatures[0], input.pubKeys[0])
+      } else {
+        scriptSig = bscript.pubKeyInput(signatures[0])
+      }
 
-  // maintain a local copy of unmatched signatures
-  var unmatched = input.signatures.concat()
-  var signatureHash = transaction.hashForSignature(vin, hashScript, hashType)
+      break
 
-  input.signatures = input.pubKeys.map(function (pubKey, y) {
-    var keyPair = ECPair.fromPublicKeyBuffer(pubKey)
-    var match
+    // ref https://github.com/bitcoin/bitcoin/blob/d612837814020ae832499d18e6ee5eb919a87907/src/script/sign.cpp#L232
+    case 'multisig':
+      signatures = signatures.map(function (signature) {
+        return signature || ops.OP_0
+      })
 
-    // check for a signature
-    unmatched.some(function (signature, i) {
-      // skip if undefined || OP_0
-      if (!signature) return false
+      if (!allowIncomplete) {
+        // remove blank signatures
+        signatures = signatures.filter(function (x) { return x !== ops.OP_0 })
+      }
 
-      // skip if signature does not match pubKey
-      if (!keyPair.verify(signatureHash, signature)) return false
+      scriptSig = bscript.multisigInput(signatures, allowIncomplete ? undefined : input.redeemScript)
+      break
 
-      // remove matched signature from unmatched
-      unmatched[i] = undefined
-      match = signature
+    default: return
+  }
 
-      return true
-    })
+  // wrap as scriptHash if necessary
+  if (input.prevOutType === 'scripthash') {
+    scriptSig = bscript.scriptHashInput(scriptSig, input.redeemScript)
+  }
 
-    return match || undefined
-  })
+  return scriptSig
 }
 
 function TransactionBuilder (network) {
@@ -315,14 +276,7 @@ TransactionBuilder.fromTransaction = function (transaction, network) {
 
   // fix some things not possible through the public API
   txb.inputs.forEach(function (input, i) {
-    // attempt to fix any multisig inputs if they exist
-    if ((input.redeemScriptType || input.prevOutType) === 'multisig') {
-      // pubKeys will only exist for 'multisig' if a redeemScript was found
-      if (!input.pubKeys || !input.signatures) return
-      if (input.pubKeys.length === input.signatures.length) return
-
-      fixMultisigOrder(input, transaction, i)
-    }
+    fixMultisigOrder(input, transaction, i)
   })
 
   return txb
@@ -408,12 +362,6 @@ TransactionBuilder.prototype.buildIncomplete = function () {
   return this.__build(true)
 }
 
-var canBuildTypes = {
-  'multisig': true,
-  'pubkey': true,
-  'pubkeyhash': true
-}
-
 TransactionBuilder.prototype.__build = function (allowIncomplete) {
   if (!allowIncomplete) {
     if (!this.tx.ins.length) throw new Error('Transaction has no inputs')
@@ -425,21 +373,17 @@ TransactionBuilder.prototype.__build = function (allowIncomplete) {
   // Create script signatures from inputs
   this.inputs.forEach(function (input, i) {
     var scriptType = input.redeemScriptType || input.prevOutType
+    if (!scriptType && !allowIncomplete) throw new Error('Transaction is not complete')
 
-    if (!allowIncomplete) {
-      if (!scriptType) throw new Error('Transaction is not complete')
-      if (!canBuildTypes[scriptType]) throw new Error(scriptType + ' not supported')
-
-      // FIXME: only relevant to types that need signatures
-      if (!input.signatures) throw new Error('Transaction is missing signatures')
-    }
+    // build a scriptSig
+    var scriptSig = buildInput(input, allowIncomplete)
 
-    // FIXME: only relevant to types that need signatures
     // skip if no scriptSig exists
-    if (!input.signatures) return
+    if (!scriptSig) {
+      if (!allowIncomplete) throw new Error(scriptType + ' not supported')
+      return
+    }
 
-    // build a scriptSig
-    var scriptSig = buildInput(input, scriptType, allowIncomplete)
     tx.setInputScript(i, scriptSig)
   })
 
@@ -447,8 +391,7 @@ TransactionBuilder.prototype.__build = function (allowIncomplete) {
 }
 
 function canSign (input) {
-  return input.hashType !== undefined &&
-    input.prevOutScript !== undefined &&
+  return input.prevOutScript !== undefined &&
     input.pubKeys !== undefined &&
     input.signatures !== undefined &&
     input.signatures.length === input.pubKeys.length &&
@@ -462,18 +405,16 @@ TransactionBuilder.prototype.sign = function (vin, keyPair, redeemScript, hashTy
 
   var input = this.inputs[vin]
 
-  // if redeemScript was provided, enforce consistency
-  if (input.redeemScript !== undefined && redeemScript) {
-    if (!input.redeemScript.equals(redeemScript)) throw new Error('Inconsistent redeemScript')
-  }
-
-  if (input.hashType !== undefined) {
-    if (input.hashType !== hashType) throw new Error('Inconsistent hashType')
+  // if redeemScript was previously provided, enforce consistency
+  if (input.redeemScript !== undefined &&
+      redeemScript &&
+      !input.redeemScript.equals(redeemScript)) {
+    throw new Error('Inconsistent redeemScript')
   }
 
   var kpPubKey = keyPair.getPublicKeyBuffer()
   if (!canSign(input)) {
-    prepareInput(input, kpPubKey, redeemScript, hashType)
+    prepareInput(input, kpPubKey, redeemScript)
 
     if (!canSign(input)) throw Error(input.prevOutType + ' not supported')
   }
@@ -483,25 +424,34 @@ TransactionBuilder.prototype.sign = function (vin, keyPair, redeemScript, hashTy
   var signatureHash = this.tx.hashForSignature(vin, hashScript, hashType)
 
   // enforce in order signing of public keys
-  var valid = input.pubKeys.some(function (pubKey, i) {
+  var signed = input.pubKeys.some(function (pubKey, i) {
     if (!kpPubKey.equals(pubKey)) return false
     if (input.signatures[i]) throw new Error('Signature already exists')
 
-    input.signatures[i] = keyPair.sign(signatureHash)
+    input.signatures[i] = keyPair.sign(signatureHash).toScriptSignature(hashType)
     return true
   })
 
-  if (!valid) throw new Error('Key pair cannot sign for this input')
+  if (!signed) throw new Error('Key pair cannot sign for this input')
+}
+
+function signatureHashType (buffer) {
+  return buffer.readUInt8(buffer.length - 1)
 }
 
 TransactionBuilder.prototype.__canModifyInputs = function () {
-  return this.inputs.every(function (otherInput) {
-    // no signature
-    if (otherInput.hashType === undefined) return true
+  return this.inputs.every(function (input) {
+    // any signatures?
+    if (input.signatures === undefined) return true
+
+    return input.signatures.every(function (signature) {
+      if (!signature) return true
+      var hashType = signatureHashType(signature)
 
-    // if SIGHASH_ANYONECANPAY is set, signatures would not
-    // be invalidated by more inputs
-    return otherInput.hashType & Transaction.SIGHASH_ANYONECANPAY
+      // if SIGHASH_ANYONECANPAY is set, signatures would not
+      // be invalidated by more inputs
+      return hashType & Transaction.SIGHASH_ANYONECANPAY
+    })
   })
 }
 
@@ -509,20 +459,22 @@ TransactionBuilder.prototype.__canModifyOutputs = function () {
   var nInputs = this.tx.ins.length
   var nOutputs = this.tx.outs.length
 
-  return this.inputs.every(function (input, i) {
-    // any signatures?
-    if (input.hashType === undefined) return true
-
-    var hashTypeMod = input.hashType & 0x1f
-    if (hashTypeMod === Transaction.SIGHASH_NONE) return true
-    if (hashTypeMod === Transaction.SIGHASH_SINGLE) {
-      // if SIGHASH_SINGLE is set, and nInputs > nOutputs
-      // some signatures would be invalidated by the addition
-      // of more outputs
-      return nInputs <= nOutputs
-    }
+  return this.inputs.every(function (input) {
+    if (input.signatures === undefined) return true
+
+    return input.signatures.every(function (signature) {
+      if (!signature) return true
+      var hashType = signatureHashType(signature)
 
-    return false
+      var hashTypeMod = hashType & 0x1f
+      if (hashTypeMod === Transaction.SIGHASH_NONE) return true
+      if (hashTypeMod === Transaction.SIGHASH_SINGLE) {
+        // if SIGHASH_SINGLE is set, and nInputs > nOutputs
+        // some signatures would be invalidated by the addition
+        // of more outputs
+        return nInputs <= nOutputs
+      }
+    })
   })
 }
 


Test patch:
diff --git a/test/fixtures/transaction_builder.json b/test/fixtures/transaction_builder.json
index 24f039b4..25ba79f7 100644
--- a/test/fixtures/transaction_builder.json
+++ b/test/fixtures/transaction_builder.json
@@ -196,6 +196,34 @@
           }
         ]
       },
+      {
+        "description": "Transaction w/ scriptHash(multisig 2-of-3), different hash types",
+        "network": "testnet",
+        "txHex": "0100000001ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00000000fd5e0100483045022100eec19e061cad41610f9b42d2b06638b6b0fec3da0de9c6858e7f8c06053979900220622936dd47e202b2ad17639cda680e52334d407149252959936bb1f38e4acc5201483045022100d404fb6de6cf42efb9d7948d2e8fb6618f8eba55ecd25907d18d576d9aa6f39d02205ec2e7fa7c5f8a9793732ca9d2f9aba3b2bb04ca6d467ba36940e0f695e48de5024cc952410479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b84104c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee51ae168fea63dc339a3c58419466ceaeef7f632653266d0e1236431a950cfe52a4104f9308a019258c31049344f85f89d5229b531c845836f99b08601f113bce036f9388f7b0f632de8140fe337e62a37f3566500a99934c2231b6cb9fd7584b8e67253aeffffffff01e8030000000000001976a914aa4d7985c57e011a8b3dd8e0e5a73aaef41629c588ac00000000",
+        "inputs": [
+          {
+            "txId": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
+            "vout": 0,
+            "signs": [
+              {
+                "keyPair": "91avARGdfge8E4tZfYLoxeJ5sGBdNJQH4kvjJoQFacbgwmaKkrx",
+                "redeemScript": "OP_2 0479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 04c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee51ae168fea63dc339a3c58419466ceaeef7f632653266d0e1236431a950cfe52a 04f9308a019258c31049344f85f89d5229b531c845836f99b08601f113bce036f9388f7b0f632de8140fe337e62a37f3566500a99934c2231b6cb9fd7584b8e672 OP_3 OP_CHECKMULTISIG",
+                "hashType": 1
+              },
+              {
+                "keyPair": "91avARGdfge8E4tZfYLoxeJ5sGBdNJQH4kvjJoQFacbgx3cTMqe",
+                "hashType": 2
+              }
+            ]
+          }
+        ],
+        "outputs": [
+          {
+            "script": "OP_DUP OP_HASH160 aa4d7985c57e011a8b3dd8e0e5a73aaef41629c5 OP_EQUALVERIFY OP_CHECKSIG",
+            "value": 1000
+          }
+        ]
+      },
       {
         "description": "Transaction w/ scriptHash(pubKey) -> pubKeyHash",
         "txHex": "0100000001ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff000000006c47304402201115644b134932c8a7a8e925769d130a801288d477130e2bf6fadda20b33754d02202ecefbf63844d7cb2d5868539c39f973fe019f72e5c31a707836c0d61ef317db012321033e29aea1168a835d5e386c292082db7b7807172a10ec634ad34226f36d79e70facffffffff0100f90295000000001976a914aa4d7985c57e011a8b3dd8e0e5a73aaef41629c588ac00000000",
@@ -806,34 +834,6 @@
           }
         ]
       },
-      {
-        "exception": "Inconsistent hashType",
-        "network": "testnet",
-        "inputs": [
-          {
-            "txId": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
-            "vout": 0,
-            "signs": [
-              {
-                "redeemScript": "OP_2 0479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 04c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee51ae168fea63dc339a3c58419466ceaeef7f632653266d0e1236431a950cfe52a OP_2 OP_CHECKMULTISIG",
-                "keyPair": "91avARGdfge8E4tZfYLoxeJ5sGBdNJQH4kvjJoQFacbgwmaKkrx",
-                "hashType": 4
-              },
-              {
-                "keyPair": "91avARGdfge8E4tZfYLoxeJ5sGBdNJQH4kvjJoQFacbgww7vXtT",
-                "hashType": 2,
-                "throws": true
-              }
-            ]
-          }
-        ],
-        "outputs": [
-          {
-            "script": "OP_DUP OP_HASH160 aa4d7985c57e011a8b3dd8e0e5a73aaef41629c5 OP_EQUALVERIFY OP_CHECKSIG",
-            "value": 1000
-          }
-        ]
-      },
       {
         "exception": "RedeemScript not supported \"OP_HASH160 7f67f0521934a57d3039f77f9f32cf313f3ac74b OP_EQUAL\"",
         "inputs": [
diff --git a/test/transaction_builder.js b/test/transaction_builder.js
index 71e3f5bd..de4dc27f 100644
--- a/test/transaction_builder.js
+++ b/test/transaction_builder.js
@@ -34,20 +34,20 @@ function construct (f, sign) {
     txb.addOutput(bscript.fromASM(output.script), output.value)
   })
 
-  if (sign === undefined || sign) {
-    f.inputs.forEach(function (input, index) {
-      input.signs.forEach(function (sign) {
-        var keyPair = ECPair.fromWIF(sign.keyPair, network)
-        var redeemScript
-
-        if (sign.redeemScript) {
-          redeemScript = bscript.fromASM(sign.redeemScript)
-        }
+  if (sign === false) return txb
 
-        txb.sign(index, keyPair, redeemScript, sign.hashType)
-      })
+  f.inputs.forEach(function (input, index) {
+    input.signs.forEach(function (sign) {
+      var keyPair = ECPair.fromWIF(sign.keyPair, network)
+      var redeemScript
+
+      if (sign.redeemScript) {
+        redeemScript = bscript.fromASM(sign.redeemScript)
+      }
+
+      txb.sign(index, keyPair, redeemScript, sign.hashType)
     })
-  }
+  })
 
   return txb
 }
@@ -65,7 +65,7 @@ describe('TransactionBuilder', function () {
 
   describe('fromTransaction', function () {
     fixtures.valid.build.forEach(function (f) {
-      it('builds TransactionBuilder, with ' + f.description, function () {
+      it('returns TransactionBuilder, with ' + f.description, function () {
         var network = NETWORKS[f.network || 'bitcoin']
         var tx = Transaction.fromHex(f.txHex)
         var txb = TransactionBuilder.fromTransaction(tx, network)
@@ -76,7 +76,7 @@ describe('TransactionBuilder', function () {
     })
 
     fixtures.valid.fromTransaction.forEach(function (f) {
-      it('builds TransactionBuilder, with ' + f.description, function () {
+      it('returns TransactionBuilder, with ' + f.description, function () {
         var tx = new Transaction()
 
         f.inputs.forEach(function (input) {