Skip to content

isScriptHashInput logic fix #360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 19, 2015
Merged

isScriptHashInput logic fix #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bcd169
tests: actually run invalid tests
dcousens Feb 15, 2015
c982622
scripts: catch error in Script.fromBuffer
dcousens Feb 15, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ Script.fromASM = function(asm) {

Script.fromBuffer = function(buffer) {
var chunks = []

var i = 0

while (i < buffer.length) {
Expand Down
8 changes: 7 additions & 1 deletion src/scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,13 @@ function isScriptHashInput(script, allowIncomplete) {
if (!Buffer.isBuffer(lastChunk)) return false

var scriptSig = Script.fromChunks(script.chunks.slice(0, -1))
var scriptPubKey = Script.fromBuffer(lastChunk)
var scriptPubKey

try {
scriptPubKey = Script.fromBuffer(lastChunk)
} catch (e) {
Copy link
Contributor Author

@dcousens dcousens Feb 15, 2015

Choose a reason for hiding this comment

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

I wonder if we should be checking specifically for a RangeError vs this catch all...

Copy link
Contributor

@weilu weilu Feb 16, 2015

Choose a reason for hiding this comment

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

I was wondering the same. How about catching all for now and console.debug the exception just in case someone may wonder at which stage the classification returned. Hopefully they report back if it's an unexpected case.

Copy link
Contributor Author

@dcousens dcousens Feb 20, 2015

Choose a reason for hiding this comment

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

This is an interesting concept, how would this be any different than returning a console.debug for every other instance where it returned false?

return false
}

return classifyInput(scriptSig, allowIncomplete) === classifyOutput(scriptPubKey)
}
Expand Down
4 changes: 4 additions & 0 deletions test/fixtures/scripts.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@
{
"description": "redeemScript not data",
"scriptSig": "OP_0 304402207515cf147d201f411092e6be5a64a6006f9308fad7b2a8fdaab22cd86ce764c202200974b8aca7bf51dbf54150d3884e1ae04f675637b926ec33bf75939446f6ca2801 3045022100ef253c1faa39e65115872519e5f0a33bbecf430c0f35cf562beabbad4da24d8d02201742be8ee49812a73adea3007c9641ce6725c32cd44ddb8e3a3af460015d140501 OP_RESERVED"
},
{
"description": "signature forms invalid script",
"scriptSig": "OP_0 3045022100e12b17b3a4c80c401a1687487bd2bafee9e5f1f8f1ffc6180ce186672ad7b43a02205e316d1e5e71822f5ef301b694e578fa9c94af4f5f098c952c833f4691307f4e01"
}
],
"isPubKeyInput": [
Expand Down
31 changes: 29 additions & 2 deletions test/scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,11 @@ describe('Scripts', function() {
})

;['PubKey', 'PubKeyHash', 'ScriptHash', 'Multisig', 'NullData'].forEach(function(type) {
var inputFn = scripts['is' + type + 'Input']
var outputFn= scripts['is' + type + 'Output']
var inputFnName = 'is' + type + 'Input'
var outputFnName = 'is' + type + 'Output'

var inputFn = scripts[inputFnName]
var outputFn= scripts[outputFnName]

describe('is' + type + 'Input', function() {
fixtures.valid.forEach(function(f) {
Expand All @@ -76,6 +79,18 @@ describe('Scripts', function() {
}
}
})

if (!(inputFnName in fixtures.invalid)) return

fixtures.invalid[inputFnName].forEach(function(f) {
if (inputFn && f.scriptSig) {
it('returns false for ' + f.scriptSig, function() {
var script = Script.fromASM(f.scriptSig)

assert.equal(inputFn(script), false)
})
}
})
})

describe('is' + type + 'Output', function() {
Expand All @@ -90,6 +105,18 @@ describe('Scripts', function() {
})
}
})

if (!(outputFnName in fixtures.invalid)) return

fixtures.invalid[outputFnName].forEach(function(f) {
if (outputFn && f.scriptPubKey) {
it('returns false for ' + f.scriptPubKey, function() {
var script = Script.fromASM(f.scriptPubKey)

assert.equal(outputFn(script), false)
})
}
})
})
})

Expand Down