Repository: bitcoinjs/bitcoinjs-lib
Pull request #481 (2015-09-29 09:03:33)

isStandard compliance [for pubKeyHash / scriptHash outputs]
As pointed out [here](https://github.com/bitcoinjs/bitcoinjs-lib/issues/480),  this is an interesting issue.

`script.decompile` will allow for non-minimal `OP_PUSHDATA`s (that is,  you can specify a larger `OP_PUSHDATAx` than needed),  meaning extra data can be added into the script.

In some cases (as pointed out in https://github.com/bitcoinjs/bitcoinjs-lib/issues/480),  this can be confusing because [bitcoin/bitcoin](https://github.com/bitcoin/bitcoin/blob/2b0567b00291205731cbb910fb3cf33359a046ee/src/script/standard.cpp#L61) doesn't consider these transactions standard.

So far, bitcoinjs-lib hasn't cared either way, and has allowed the construction of non-standard transactions.

Perhaps an `IsStandard` flag that is enabled by default on these scripts?
Thoughts?

**Or,  better yet,  perhaps our `is*` functions are assumed to be adhering to `IsStandard`, so naturally all we have to do is amend the functions as per this PR?**

@fanatid curious on your opinion here.

dcousens: But then again,  that would mean `isScriptHashInput` is inconsistent, because right now it doesn't care about standard rules and checks generically.
fanatid: 
Not sure about this, but I think better way will be add `isStandard` (or `strict`) argument to `is*` functions and to `docompile` (when `isStandard` is `true` `decompile` following to minimal OP_PUSHDATA compliance).
Example for `isScriptHashOutput`:
```js
function isScriptHashOutput (script, isStandard) {
  var chunks = decompile(script, isStandard)
  return chunks.length === 3 &&
    chunks[0] === OPS.OP_HASH160 &&
    Buffer.isBuffer(chunks[1]) &&
    chunks[1].length === 20 &&
    chunks[2] === OPS.OP_EQUAL
}
```

In this case it's allowed to check scripts in strict mode and have backward compatibility with old code (`isStandard` should be `false` by default).
dcousens: Thought about this for a few hours,  and,  it makes sense to make the `is*` functions mostly compliant with `IsStandard` rules simply because the entire concept of a `pay-to-pubkeyhash` and `pay-to-scripthash` is about following the community set standards for those functions.
dcousens: **TL;DR**:
Considering the `is*` functions are used to match scripts against community set script templates,  adhering to the bitcoin core `IsStandard` rules makes sense IMHO.

To match otherwise, you should probably use a script interpreter or write your own function.

fanatid: I think that `isCanonicalPubKey` also must need changes... I write some closely code in [script2addresses](https://github.com/fanatid/script2addresses/blob/master/src/publickey.js) 
dcousens: @fanatid what do you mean?  The code for `isCanonicalPubKey` mirrors the code from https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L67-L87
fanatid: hm, may be I am wrong.
but in different places you can see different rules:
[size is 33 <= x <= 65 (first byte doesn't important)](https://github.com/bitcoin/bitcoin/blob/0fbfc5106cd9866325b4a1ab3b9db8e91e54f070/src/script/standard.cpp#L129)
[size is 33 (first byte 2 or 3) and 65 (first byte is 4, 6 or 7)](https://github.com/bitcoin/bitcoin/blob/0fbfc5106cd9866325b4a1ab3b9db8e91e54f070/src/pubkey.h#L48) ([called from TransactionSignatureChecker::CheckSig](https://github.com/bitcoin/bitcoin/blob/0fbfc5106cd9866325b4a1ab3b9db8e91e54f070/src/script/interpreter.cpp#L1111))
[and finally in secp256k1 with additional checks](https://github.com/bitcoin/bitcoin/blob/0fbfc5106cd9866325b4a1ab3b9db8e91e54f070/src/secp256k1/src/eckey_impl.h#L17)
I don't think, that is right to take example from `interpreter.cpp` for `isCanonicalPubKey`.
dcousens: @fanatid remember this isn't a script interpreter, but just a parser.
If you want to verify the public key is a valid ECDSA public key, then you will need to do that yourself (and naturally handle each error case that could consist of).

Patch:
diff --git a/src/address.js b/src/address.js
index d6924b32..e6314017 100644
--- a/src/address.js
+++ b/src/address.js
@@ -18,11 +18,10 @@ function fromBase58Check (address) {
 function fromOutputScript (scriptPubKey, network) {
   network = network || networks.bitcoin
 
-  var chunks = bscript.decompile(scriptPubKey)
-  if (bscript.isPubKeyHashOutput(chunks)) return toBase58Check(chunks[2], network.pubKeyHash)
-  if (bscript.isScriptHashOutput(chunks)) return toBase58Check(chunks[1], network.scriptHash)
+  if (bscript.isPubKeyHashOutput(scriptPubKey)) return toBase58Check(scriptPubKey.slice(3, 23), network.pubKeyHash)
+  if (bscript.isScriptHashOutput(scriptPubKey)) return toBase58Check(scriptPubKey.slice(2, 22), network.scriptHash)
 
-  throw new Error(bscript.toASM(chunks) + ' has no matching Address')
+  throw new Error(bscript.toASM(scriptPubKey) + ' has no matching Address')
 }
 
 function toBase58Check (hash, version) {
diff --git a/src/script.js b/src/script.js
index 3239e37e..5be3c688 100644
--- a/src/script.js
+++ b/src/script.js
@@ -156,15 +156,14 @@ function isPubKeyHashInput (script) {
 }
 
 function isPubKeyHashOutput (script) {
-  var chunks = decompile(script)
-
-  return chunks.length === 5 &&
-    chunks[0] === OPS.OP_DUP &&
-    chunks[1] === OPS.OP_HASH160 &&
-    Buffer.isBuffer(chunks[2]) &&
-    chunks[2].length === 20 &&
-    chunks[3] === OPS.OP_EQUALVERIFY &&
-    chunks[4] === OPS.OP_CHECKSIG
+  var buffer = compile(script)
+
+  return buffer.length === 25 &&
+    buffer[0] === OPS.OP_DUP &&
+    buffer[1] === OPS.OP_HASH160 &&
+    buffer[2] === 0x14 &&
+    buffer[23] === OPS.OP_EQUALVERIFY &&
+    buffer[24] === OPS.OP_CHECKSIG
 }
 
 function isPubKeyInput (script) {
@@ -199,13 +198,12 @@ function isScriptHashInput (script, allowIncomplete) {
 }
 
 function isScriptHashOutput (script) {
-  var chunks = decompile(script)
+  var buffer = compile(script)
 
-  return chunks.length === 3 &&
-    chunks[0] === OPS.OP_HASH160 &&
-    Buffer.isBuffer(chunks[1]) &&
-    chunks[1].length === 20 &&
-    chunks[2] === OPS.OP_EQUAL
+  return buffer.length === 23 &&
+    buffer[0] === OPS.OP_HASH160 &&
+    buffer[1] === 0x14 &&
+    buffer[22] === OPS.OP_EQUAL
 }
 
 // allowIncomplete is to account for combining signatures


Test patch:
diff --git a/test/fixtures/script.json b/test/fixtures/script.json
index 8abdcd17..797186f6 100644
--- a/test/fixtures/script.json
+++ b/test/fixtures/script.json
@@ -200,6 +200,18 @@
         "scriptPubKey": "02359c6e3f04cefbf089cf1d6670dc47c3fb4df68e2bad1fa5a369f9ce4b42bbd1ffffff OP_CHECKSIG"
       }
     ],
+    "isPubKeyHashOutput": [
+      {
+        "description": "non-minimal encoded isPubKeyHashOutput (non BIP62 compliant)",
+        "scriptPubKeyHex": "76a94c14aa4d7985c57e011a8b3dd8e0e5a73aaef41629c588ac"
+      }
+    ],
+    "isScriptHashOutput": [
+      {
+        "description": "non-minimal encoded isScriptHashOutput (non BIP62 compliant)",
+        "scriptPubKeyHex": "a94c14c286a1af0947f58d1ad787385b1c2c4a976f9e7187"
+      }
+    ],
     "isMultisigOutput": [
       {
         "description": "OP_CHECKMULTISIG not found",
diff --git a/test/script.js b/test/script.js
index b8e43ebe..ccb009ff 100644
--- a/test/script.js
+++ b/test/script.js
@@ -149,19 +149,17 @@ describe('script', function () {
       if (!(inputFnName in fixtures.invalid)) return
 
       fixtures.invalid[inputFnName].forEach(function (f) {
-        if (inputFn && (f.scriptSig || f.scriptSigHex)) {
-          it('returns false for ' + f.description + ' (' + (f.scriptSig || f.scriptSigHex) + ')', function () {
-            var scriptSig
+        it('returns false for ' + f.description + ' (' + (f.scriptSig || f.scriptSigHex) + ')', function () {
+          var scriptSig
 
-            if (f.scriptSig) {
-              scriptSig = bscript.fromASM(f.scriptSig)
-            } else {
-              scriptSig = bscript.fromHex(f.scriptSigHex)
-            }
+          if (f.scriptSig) {
+            scriptSig = bscript.fromASM(f.scriptSig)
+          } else {
+            scriptSig = new Buffer(f.scriptSigHex, 'hex')
+          }
 
-            assert.strictEqual(inputFn(scriptSig), false)
-          })
-        }
+          assert.strictEqual(inputFn(scriptSig), false)
+        })
       })
     })
 
@@ -181,13 +179,17 @@ describe('script', function () {
       if (!(outputFnName in fixtures.invalid)) return
 
       fixtures.invalid[outputFnName].forEach(function (f) {
-        if (outputFn && f.scriptPubKey) {
-          it('returns false for ' + f.description + ' (' + f.scriptPubKey + ')', function () {
-            var scriptPubKey = bscript.fromASM(f.scriptPubKey)
+        it('returns false for ' + f.description + ' (' + (f.scriptPubKey || f.scriptPubKeyHex) + ')', function () {
+          var scriptPubKey
 
-            assert.strictEqual(outputFn(scriptPubKey), false)
-          })
-        }
+          if (f.scriptPubKey) {
+            scriptPubKey = bscript.fromASM(f.scriptPubKey)
+          } else {
+            scriptPubKey = new Buffer(f.scriptPubKeyHex, 'hex')
+          }
+
+          assert.strictEqual(outputFn(scriptPubKey), false)
+        })
       })
     })
   })