From 788325059fec8c532b81a475308ceec8d521fdbf Mon Sep 17 00:00:00 2001 From: Tyler J Date: Tue, 5 Jan 2016 11:11:16 -0800 Subject: [PATCH 1/5] remove classification from Script.isScriptHashIn() - The bitcore-lib Script is primarily concerned with serializing, deserializing, building common bitcoin scripts, and extracting commonly needed information - Script.isScriptHashIn() requires the Script object to conclude whether a P2SH redeem script is valid, which in many cases, it cannot (nor should it) - The Script object should remain unopinionated, especially in light of a P2SH redeemscript, where the benefits of classification are dwarfed by the costs of it being incorrect - In the case of BIP 65's OP_CHECKLOCKTIMEVERIFY, this error causes consumers of bitcore-lib to incorrectly conclude that CLTV transactions are not P2SH spends - Over-zealous classification in this case is fragile and breaks forward compatibility - it should be removed until a more stable P2SH check is implemented --- lib/script/script.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/script/script.js b/lib/script/script.js index 176efdb35..208879796 100644 --- a/lib/script/script.js +++ b/lib/script/script.js @@ -411,8 +411,7 @@ Script.prototype.isScriptHashIn = function() { } throw e; } - var type = redeemScript.classify(); - return type !== Script.types.UNKNOWN; + return true }; /** From 5295c268c89089831af205a9d923f1eb144e0d5a Mon Sep 17 00:00:00 2001 From: Tyler J Date: Tue, 5 Jan 2016 11:42:20 -0800 Subject: [PATCH 2/5] check for OP_RETURN in Script.isScriptHashIn() - Data transactions consist of OP_RETURN followed by a push of bytes to the stack - P2SH redeem scripts (generally) consist of signatures and relevant information followed by a push of bytes to the stack - To differentiate the two, we check that the first OP code is not OP_RETURN in Script.isScriptHashIn() --- lib/script/script.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/script/script.js b/lib/script/script.js index 208879796..964067d4d 100644 --- a/lib/script/script.js +++ b/lib/script/script.js @@ -393,7 +393,7 @@ Script.prototype.isScriptHashOut = function() { * Note that these are frequently indistinguishable from pubkeyhashin */ Script.prototype.isScriptHashIn = function() { - if (this.chunks.length <= 1) { + if (this.chunks.length <= 1 || this.chunks[0].opcodenum === Opcode.OP_RETURN) { return false; } var redeemChunk = this.chunks[this.chunks.length - 1]; From 410221f05c38d68377bef124222e6ea077e35265 Mon Sep 17 00:00:00 2001 From: Tyler J Date: Wed, 6 Jan 2016 00:37:23 -0800 Subject: [PATCH 3/5] fix script tests to use an invalid scriptsig - Since we've relaxed our definition of what a valid redeemscript comprises of, we need to update the test to check for that. In this case, it's a script that simply pushes bytes to the stack. --- test/script/script.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/script/script.js b/test/script/script.js index 2434b7b63..956258c6d 100644 --- a/test/script/script.js +++ b/test/script/script.js @@ -372,8 +372,7 @@ describe('Script', function() { it('should identify this problematic non-scripthashin scripts', function() { var s = new Script('71 0x3044022017053dad84aa06213749df50a03330cfd24d6' + 'b8e7ddbb6de66c03697b78a752a022053bc0faca8b4049fb3944a05fcf7c93b2861' + - '734d39a89b73108f605f70f5ed3401 33 0x0225386e988b84248dc9c30f784b06e' + - '02fdec57bbdbd443768eb5744a75ce44a4c'); + '734d39a89b73108f605f70f5ed3401'); var s2 = new Script('OP_RETURN 32 0x19fdb20634911b6459e6086658b3a6ad2dc6576bd6826c73ee86a5f9aec14ed9'); s.isScriptHashIn().should.equal(false); s2.isScriptHashIn().should.equal(false); From 2a54b18a62496fd6196dbfe0fb774a5aad060c61 Mon Sep 17 00:00:00 2001 From: Tyler J Date: Mon, 11 Jan 2016 17:43:38 -0800 Subject: [PATCH 4/5] add test to ensure that invalid redeem scripts are caught --- test/script/script.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/script/script.js b/test/script/script.js index 956258c6d..61087372a 100644 --- a/test/script/script.js +++ b/test/script/script.js @@ -10,6 +10,7 @@ var Networks = bitcore.Networks; var Opcode = bitcore.Opcode; var PublicKey = bitcore.PublicKey; var Address = bitcore.Address; +var errors = bitcore.errors; describe('Script', function() { @@ -381,6 +382,10 @@ describe('Script', function() { var s = Script.fromString('73 0x3046022100dc7a0a812de14acc479d98ae209402cc9b5e0692bc74b9fe0a2f083e2f9964b002210087caf04a711bebe5339fd7554c4f7940dc37be216a3ae082424a5e164faf549401'); s.isScriptHashIn().should.equal(false); }); + it('should return false for a script whose redeemscript cannot be deserialized', function() { + var s = Script.fromBuffer(new Buffer('00024d33', 'hex')); + s.isScriptHashIn().should.equal(false); + }); }); describe('#isScripthashOut', function() { From aa48196f6655bf31e5e9b8294d0e84509cc34f08 Mon Sep 17 00:00:00 2001 From: Tyler J Date: Mon, 11 Jan 2016 18:00:10 -0800 Subject: [PATCH 5/5] add test to ensure that errors unrelated to script validation propagate --- test/script/script.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/script/script.js b/test/script/script.js index 61087372a..34ec0baf2 100644 --- a/test/script/script.js +++ b/test/script/script.js @@ -386,6 +386,14 @@ describe('Script', function() { var s = Script.fromBuffer(new Buffer('00024d33', 'hex')); s.isScriptHashIn().should.equal(false); }); + it('should propagate errors that are unrelated to script validation', function() { + var fails = function() { + var s = Script.fromBuffer(new Buffer('00024d33', 'hex')); + s.chunks[1] = {buf: 1}; + return s.isScriptHashIn(); + }; + fails.should.throw(TypeError); + }); }); describe('#isScripthashOut', function() {