Repository: bitcoinjs/bitcoinjs-lib
Pull request #857 (2017-08-22 00:37:04)

Reject uncompressed keys for witness scripts
From https://github.com/bitcoinjs/bitcoinjs-lib/issues/641

- [x] Prevent `TransactionBuilder` creating these at all

Compliant with our policy in https://github.com/bitcoinjs/bitcoinjs-lib/issues/640

> `bitcoinjs.template.*` functions should follow the script "templates" typically used in the community (and consequently, the bitcoin blockchain).
This is a matter of community consensus, but is probably going to always be closely related to what bitcoin core does by default.
They are quite plainly just pattern matching functions, and should be treated as such.

`bitcoin.template.witnessPubKeyHash.decode` should fail if the pattern matched doesn't community policy [of rejected uncompressed pubkeys], these are templates afterall.
You can easily `bitcoin.script.decompile` yourself if you want to DIY.

As per [BIP143](https://github.com/bitcoin/bips/pull/459/files#diff-d6a595a37a5d316054e79e53e9c2382fR128)

afk11: Looks good! 

It'd be cool if there an approach to making scripts script that was easier than calling `script.X.encode(the hashXXX())` primitives directly.. 

maybe a factory that takes a script, the user calls p2sh(), and/or p2wsh(), before a final method is called that looks at what was done and applies contextual checks? (like conditionally forbidding uncompressed public keys)
dcousens: > script.X.encode(the hashXXX())

I concur,  this is lame.
But,  if with powerful primitives,  then we can build powerful convenience wrappers :+1: .

We should always be open to ideas as to how we can improve UX,  with the fallback of a consistent, strict and safe API :smiley: .

Patch:
diff --git a/src/templates/pubkey/input.js b/src/templates/pubkey/input.js
index 0e8ed8db..1d34cb42 100644
--- a/src/templates/pubkey/input.js
+++ b/src/templates/pubkey/input.js
@@ -1,7 +1,6 @@
 // {signature}
 
 var bscript = require('../../script')
-var types = require('../../types')
 var typeforce = require('typeforce')
 
 function check (script) {
@@ -13,7 +12,7 @@ function check (script) {
 check.toJSON = function () { return 'pubKey input' }
 
 function encodeStack (signature) {
-  typeforce(types.Buffer, signature)
+  typeforce(bscript.isCanonicalSignature, signature)
   return [signature]
 }
 
diff --git a/src/templates/pubkeyhash/input.js b/src/templates/pubkeyhash/input.js
index 82b89f96..0a1fe1e5 100644
--- a/src/templates/pubkeyhash/input.js
+++ b/src/templates/pubkeyhash/input.js
@@ -1,7 +1,6 @@
 // {signature} {pubKey}
 
 var bscript = require('../../script')
-var types = require('../../types')
 var typeforce = require('typeforce')
 
 function check (script) {
@@ -15,9 +14,11 @@ check.toJSON = function () { return 'pubKeyHash input' }
 
 function encodeStack (signature, pubKey) {
   typeforce({
-    signature: types.Buffer, pubKey: types.Buffer
+    signature: bscript.isCanonicalSignature,
+    pubKey: bscript.isCanonicalPubKey
   }, {
-    signature: signature, pubKey: pubKey
+    signature: signature,
+    pubKey: pubKey
   })
 
   return [signature, pubKey]
diff --git a/src/templates/witnesspubkeyhash/input.js b/src/templates/witnesspubkeyhash/input.js
index d9d678bf..eacdab95 100644
--- a/src/templates/witnesspubkeyhash/input.js
+++ b/src/templates/witnesspubkeyhash/input.js
@@ -1,9 +1,44 @@
 // {signature} {pubKey}
 
-var pkh = require('../pubkeyhash/input')
+var bscript = require('../../script')
+var typeforce = require('typeforce')
+
+function isCompressedCanonicalPubKey (pubKey) {
+  return bscript.isCanonicalPubKey(pubKey) && pubKey.length === 33
+}
+
+function check (script) {
+  var chunks = bscript.decompile(script)
+
+  return chunks.length === 2 &&
+    bscript.isCanonicalSignature(chunks[0]) &&
+    isCompressedCanonicalPubKey(chunks[1])
+}
+check.toJSON = function () { return 'witnessPubKeyHash input' }
+
+function encodeStack (signature, pubKey) {
+  typeforce({
+    signature: bscript.isCanonicalSignature,
+    pubKey: isCompressedCanonicalPubKey
+  }, {
+    signature: signature,
+    pubKey: pubKey
+  })
+
+  return [signature, pubKey]
+}
+
+function decodeStack (stack) {
+  typeforce(check, stack)
+
+  return {
+    signature: stack[0],
+    pubKey: stack[1]
+  }
+}
 
 module.exports = {
-  check: pkh.check,
-  decodeStack: pkh.decodeStack,
-  encodeStack: pkh.encodeStack
+  check: check,
+  decodeStack: decodeStack,
+  encodeStack: encodeStack
 }
diff --git a/src/transaction_builder.js b/src/transaction_builder.js
index 45d80226..8eee5d0a 100644
--- a/src/transaction_builder.js
+++ b/src/transaction_builder.js
@@ -14,6 +14,14 @@ var ECPair = require('./ecpair')
 var ECSignature = require('./ecsignature')
 var Transaction = require('./transaction')
 
+function supportedType (type) {
+  return SIGNABLE.indexOf(type) !== -1
+}
+
+function supportedP2SHType (type) {
+  return P2SH.indexOf(type) !== -1
+}
+
 function extractChunks (type, chunks, script) {
   var pubKeys = []
   var signatures = []
@@ -74,7 +82,7 @@ function expandInput (scriptSig, witnessStack) {
     script = redeemScript
   }
 
-  var classifyWitness = bscript.classifyWitness(witnessStack)
+  var classifyWitness = bscript.classifyWitness(witnessStack, true)
   if (classifyWitness === scriptTypes.P2WSH) {
     witnessScript = witnessStack[witnessStack.length - 1]
     witnessScriptType = bscript.classifyOutput(witnessScript)
@@ -82,7 +90,7 @@ function expandInput (scriptSig, witnessStack) {
     if (scriptSig.length === 0) {
       prevOutScript = bscript.witnessScriptHash.output.encode(bcrypto.sha256(witnessScript))
       prevOutType = scriptTypes.P2WSH
-      if (typeof redeemScript !== 'undefined') {
+      if (redeemScript !== undefined) {
         throw new Error('Redeem script given when unnecessary')
       }
       // bare witness
@@ -96,9 +104,10 @@ function expandInput (scriptSig, witnessStack) {
       }
     }
 
-    if (SIGNABLE.indexOf(bscript.classifyOutput(witnessScript)) === -1) {
+    if (!supportedType(bscript.classifyOutput(witnessScript))) {
       throw new Error('unsupported witness script')
     }
+
     script = witnessScript
     scriptType = witnessScriptType
     chunks = witnessStack.slice(0, -1)
@@ -124,7 +133,7 @@ function expandInput (scriptSig, witnessStack) {
     scriptType = scriptTypes.P2PKH
     chunks = witnessStack
   } else if (redeemScript) {
-    if (P2SH.indexOf(redeemScriptType) === -1) {
+    if (!supportedP2SHType(redeemScriptType)) {
       throw new Error('Bad redeemscript!')
     }
 
@@ -390,7 +399,6 @@ function buildStack (type, signatures, pubKeys, allowIncomplete) {
   }
 
   if (!allowIncomplete) throw new Error('Not enough signatures provided')
-
   return []
 }
 
@@ -398,7 +406,8 @@ function buildInput (input, allowIncomplete) {
   var scriptType = input.prevOutType
   var sig = []
   var witness = []
-  if (SIGNABLE.indexOf(scriptType) !== -1) {
+
+  if (supportedType(scriptType)) {
     sig = buildStack(scriptType, input.signatures, input.pubKeys, allowIncomplete)
   }
 
@@ -406,13 +415,14 @@ function buildInput (input, allowIncomplete) {
   if (scriptType === bscript.types.P2SH) {
     // We can remove this error later when we have a guarantee prepareInput
     // rejects unsignable scripts - it MUST be signable at this point.
-    if (P2SH.indexOf(input.redeemScriptType) === -1 && !allowIncomplete) {
+    if (!allowIncomplete && !supportedP2SHType(input.redeemScriptType)) {
       throw new Error('Impossible to sign this type')
     }
 
-    if (SIGNABLE.indexOf(input.redeemScriptType) !== -1) {
+    if (supportedType(input.redeemScriptType)) {
       sig = buildStack(input.redeemScriptType, input.signatures, input.pubKeys, allowIncomplete)
     }
+
     // If it wasn't SIGNABLE, it's witness, defer to that
     if (input.redeemScriptType) {
       p2sh = true
@@ -420,18 +430,25 @@ function buildInput (input, allowIncomplete) {
     }
   }
 
-  if (scriptType === bscript.types.P2WPKH) {
+  switch (scriptType) {
     // P2WPKH is a special case of P2PKH
-    witness = buildStack(bscript.types.P2PKH, input.signatures, input.pubKeys, allowIncomplete)
-  } else if (scriptType === bscript.types.P2WSH) {
-    // We can remove this check later
-    if (SIGNABLE.indexOf(input.witnessScriptType) === -1 && !allowIncomplete) {
-      throw new Error('Impossible to sign this type')
-    } else if (SIGNABLE.indexOf(input.witnessScriptType) !== -1) {
-      witness = buildStack(input.witnessScriptType, input.signatures, input.pubKeys, allowIncomplete)
-      witness.push(input.witnessScript)
-      scriptType = input.witnessScriptType
-    }
+    case bscript.types.P2WPKH:
+      witness = buildStack(bscript.types.P2PKH, input.signatures, input.pubKeys, allowIncomplete)
+      break
+
+    case bscript.types.P2WSH:
+      // We can remove this check later
+      if (!allowIncomplete && !supportedType(input.witnessScriptType)) {
+        throw new Error('Impossible to sign this type')
+      }
+
+      if (supportedType(input.witnessScriptType)) {
+        witness = buildStack(input.witnessScriptType, input.signatures, input.pubKeys, allowIncomplete)
+        witness.push(input.witnessScript)
+        scriptType = input.witnessScriptType
+      }
+
+      break
   }
 
   // append redeemScript if necessary
@@ -442,7 +459,7 @@ function buildInput (input, allowIncomplete) {
   return {
     type: scriptType,
     script: bscript.compile(sig),
-    witness: witness
+    witness: bscript.toStack(witness)
   }
 }
 
@@ -616,7 +633,7 @@ TransactionBuilder.prototype.__build = function (allowIncomplete) {
 
     // skip if no result
     if (!allowIncomplete) {
-      if (SIGNABLE.indexOf(result.type) === -1 && result.type !== bscript.types.P2WPKH) {
+      if (!supportedType(result.type) && result.type !== bscript.types.P2WPKH) {
         throw new Error(result.type + ' not supported')
       }
     }
@@ -676,6 +693,8 @@ TransactionBuilder.prototype.sign = function (vin, keyPair, redeemScript, hashTy
   var signed = input.pubKeys.some(function (pubKey, i) {
     if (!kpPubKey.equals(pubKey)) return false
     if (input.signatures[i]) throw new Error('Signature already exists')
+    if (!keyPair.compressed &&
+      input.signType === scriptTypes.P2WPKH) throw new Error('BIP143 rejects uncompressed public keys in P2WPKH or P2WSH')
 
     input.signatures[i] = keyPair.sign(signatureHash).toScriptSignature(hashType)
     return true


Test patch:
diff --git a/test/fixtures/transaction_builder.json b/test/fixtures/transaction_builder.json
index 282e7f0b..4f059853 100644
--- a/test/fixtures/transaction_builder.json
+++ b/test/fixtures/transaction_builder.json
@@ -401,7 +401,6 @@
           }
         ]
       },
-
       {
         "description": "Transaction w/ P2WSH|P2PK -> P2PKH",
         "txHex": "010000000001014533a3bc1e039bd787656068e135aaee10aee95a64776bfc047ee6a7c1ebdd2f0000000000ffffffff0160ea0000000000001976a914851a33a5ef0d4279bd5854949174e2c65b1d450088ac02473044022039725bb7291a14dd182dafdeaf3ea0d5c05c34f4617ccbaa46522ca913995c4e02203b170d072ed2e489e7424ad96d8fa888deb530be2d4c5d9aaddf111a7efdb2d3012321038de63cf582d058a399a176825c045672d5ff8ea25b64d28d4375dcdb14c02b2bac00000000",
@@ -1327,11 +1326,68 @@
           }
         ],
         "locktime": 0
+      },
+      {
+        "description": "P2SH(P2WSH(multisig 2-of-2)), incomplete",
+        "network": "testnet",
+        "txHex": "010000000001012915794541ffa77ca795ec7c23ee989a63ccd1a71fab73e1c27ed20c4b6c69a4010000002322002024376a0a9abab599d0e028248d48ebe817bc899efcffa1cd2984d67289daf5afffffffff01b8820100000000001976a914aa4d7985c57e011a8b3dd8e0e5a73aaef41629c588ac040047304402203b334650f1f13574a1c2edc76421867f7252950968bf0293c8b3ed086ab89e3d0220565cafab0a5044617e94756b948241525b2483a52504e1064d29f641fb18129e010047522102bbbd6eb01efcbe4bd9664b886f26f69de5afcb2e479d72596c8bf21929e352e22102d9c3f7180ef13ec5267723c9c2ffab56a4215241f837502ea8977c8532b9ea1952ae00000000",
+        "incomplete": true,
+        "inputs": [
+          {
+            "txId": "a4696c4b0cd27ec2e173ab1fa7d1cc639a98ee237cec95a77ca7ff4145791529",
+            "vout": 1,
+            "signs": [
+              {
+                "keyPair": "cRAwuVuVSBZMPu7hdrYvMCZ8eevzmkExjFbaBLhqnDdrezxN3nTS",
+                "witnessScript": "OP_2 02bbbd6eb01efcbe4bd9664b886f26f69de5afcb2e479d72596c8bf21929e352e2 02d9c3f7180ef13ec5267723c9c2ffab56a4215241f837502ea8977c8532b9ea19 OP_2 OP_CHECKMULTISIG",
+                "redeemScript": "OP_0 24376a0a9abab599d0e028248d48ebe817bc899efcffa1cd2984d67289daf5af",
+                "value": 100000
+              }
+            ],
+            "prevTxScript": "OP_HASH160 b64f1a3eacc1c8515592a6f10457e8ff90e4db6a OP_EQUAL"
+          }
+        ],
+        "outputs": [
+          {
+            "script": "OP_DUP OP_HASH160 aa4d7985c57e011a8b3dd8e0e5a73aaef41629c5 OP_EQUALVERIFY OP_CHECKSIG",
+            "value": 99000
+          }
+        ]
+      },
+      {
+        "description": "P2SH(P2WSH(multisig 2-of-2))",
+        "network": "testnet",
+        "txHex": "010000000001012915794541ffa77ca795ec7c23ee989a63ccd1a71fab73e1c27ed20c4b6c69a4010000002322002024376a0a9abab599d0e028248d48ebe817bc899efcffa1cd2984d67289daf5afffffffff01b8820100000000001976a914aa4d7985c57e011a8b3dd8e0e5a73aaef41629c588ac040047304402203b334650f1f13574a1c2edc76421867f7252950968bf0293c8b3ed086ab89e3d0220565cafab0a5044617e94756b948241525b2483a52504e1064d29f641fb18129e0148304502210096e859827fb629b6547658c613f7c8298de151513d74b224560aa8608d521d600220736fb5564322237716ec940de44c67c428198adf5dedfda183c17aa77cd28d640147522102bbbd6eb01efcbe4bd9664b886f26f69de5afcb2e479d72596c8bf21929e352e22102d9c3f7180ef13ec5267723c9c2ffab56a4215241f837502ea8977c8532b9ea1952ae00000000",
+        "incomplete": true,
+        "inputs": [
+          {
+            "txId": "a4696c4b0cd27ec2e173ab1fa7d1cc639a98ee237cec95a77ca7ff4145791529",
+            "vout": 1,
+            "signs": [
+              {
+                "keyPair": "cRAwuVuVSBZMPu7hdrYvMCZ8eevzmkExjFbaBLhqnDdrezxN3nTS",
+                "witnessScript": "OP_2 02bbbd6eb01efcbe4bd9664b886f26f69de5afcb2e479d72596c8bf21929e352e2 02d9c3f7180ef13ec5267723c9c2ffab56a4215241f837502ea8977c8532b9ea19 OP_2 OP_CHECKMULTISIG",
+                "redeemScript": "OP_0 24376a0a9abab599d0e028248d48ebe817bc899efcffa1cd2984d67289daf5af",
+                "value": 100000
+              },
+              {
+                "keyPair": "cTUFsNeVd8TKU4yREN8nMdViNnHyNvCCYVRmRUmkMLgomiMWTiii",
+                "witnessScript": "OP_2 02bbbd6eb01efcbe4bd9664b886f26f69de5afcb2e479d72596c8bf21929e352e2 02d9c3f7180ef13ec5267723c9c2ffab56a4215241f837502ea8977c8532b9ea19 OP_2 OP_CHECKMULTISIG",
+                "redeemScript": "OP_0 24376a0a9abab599d0e028248d48ebe817bc899efcffa1cd2984d67289daf5af",
+                "value": 100000
+              }
+            ],
+            "prevTxScript": "OP_HASH160 b64f1a3eacc1c8515592a6f10457e8ff90e4db6a OP_EQUAL"
+          }
+        ],
+        "outputs": [
+          {
+            "script": "OP_DUP OP_HASH160 aa4d7985c57e011a8b3dd8e0e5a73aaef41629c5 OP_EQUALVERIFY OP_CHECKSIG",
+            "value": 99000
+          }
+        ]
       }
-
-
-    ]
-    ,
+    ],
     "fromTransaction": [
       {
         "description": "Transaction w/ scriptHash(multisig 2-of-2) -> OP_RETURN | 1 OP_0, no signatures",
@@ -1781,6 +1837,29 @@
           }
         ]
       },
+      {
+        "exception": "BIP143 rejects uncompressed public keys in P2WPKH or P2WSH",
+        "inputs": [
+          {
+            "txId": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
+            "vout": 0,
+            "prevTxScript": "OP_0 15a71ffa7b5bb70cddefcf364494071022efe390",
+            "signs": [
+              {
+                "keyPair": "5JiHJJjdufSiMxbvnyNcKtQNLYH6SvUpQnRv9yZENFDWTQKQkzC",
+                "value": 10000,
+                "throws": true
+              }
+            ]
+          }
+        ],
+        "outputs": [
+          {
+            "script": "OP_0 aa4d7985c57e011a8b3dd8e0e5a73aaef41629c5",
+            "value": 10000
+          }
+        ]
+      },
       {
         "exception": "RedeemScript not supported \"OP_RETURN 06deadbeef03f895a2ad89fb6d696497af486cb7c644a27aa568c7a18dd06113401115185474\"",
         "inputs": [
diff --git a/test/transaction_builder.js b/test/transaction_builder.js
index b09a2feb..6606e386 100644
--- a/test/transaction_builder.js
+++ b/test/transaction_builder.js
@@ -91,7 +91,9 @@ describe('TransactionBuilder', function () {
 
         var tx = Transaction.fromHex(f.txHex)
         var txb = TransactionBuilder.fromTransaction(tx, network)
-        assert.strictEqual(txb.build().toHex(), f.txHex)
+        var txAfter = f.incomplete ? txb.buildIncomplete() : txb.build()
+
+        assert.strictEqual(txAfter.toHex(), f.txHex)
         assert.strictEqual(txb.network, network)
       })
     })
@@ -305,7 +307,8 @@ describe('TransactionBuilder', function () {
     fixtures.valid.build.forEach(function (f) {
       it('builds "' + f.description + '"', function () {
         var txb = construct(f)
-        var tx = txb.build()
+        var tx = f.incomplete ? txb.buildIncomplete() : txb.build()
+
         assert.strictEqual(tx.toHex(), f.txHex)
       })
     })