Skip to content

Conversation

@dcousens
Copy link
Contributor

This pull request fixes the classification functions in scripts to only accept canonical signatures and public keys, as well as puts them under more strict testing.
There isn't much testing for false positives, which is important to ensure does not occur, but we can add those cases as/if we get them.

A major point of annoyance was a triangular (circular) dependency issue formed by scripts, Address and ECPubKey.
The problem has been added as an inline comment in daa2cb7.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.34%) when pulling 8b1e2c5 on dcousens:classify into d93623e on bitcoinjs:master.

@dcousens dcousens changed the title Classify Script classification fixes Jun 24, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.16%) when pulling 924ecfb on dcousens:classify into d93623e on bitcoinjs:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.15%) when pulling 5c53178 on dcousens:classify into d93623e on bitcoinjs:master.

@dcousens
Copy link
Contributor Author

For easier eye-balling of the script test data, I've added to/fromASM.

@kyledrake
Copy link
Contributor

+1

@dcousens
Copy link
Contributor Author

Added some amendments to isMultisigOutput. Should now be good to go. @weilu @kyledrake

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.16%) when pulling 759bba5 on dcousens:classify into d93623e on bitcoinjs:master.

@kyledrake
Copy link
Contributor

I want to get one more +1 before pulling this. @abrkn @weilu @jprichardson

src/scripts.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

2nd argument false isn't used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cheers, will remove. It was an artifact of previous recursion protection.
On Jun 29, 2014 9:29 AM, "Wei Lu" notifications@github.com wrote:

In src/scripts.js:

 this.chunks[1] === opcodes.OP_CHECKSIG

}

-function isScripthash() {

  • return this.chunks[this.chunks.length - 1] == opcodes.OP_EQUAL &&
  • this.chunks[0] == opcodes.OP_HASH160 &&
    +function isScriptHashInput() {
  • if (this.chunks.length < 2) return false
  • var lastChunk = this.chunks[this.chunks.length - 1]
  • if (!Buffer.isBuffer(lastChunk)) return false
  • var scriptSig = Script.fromChunks(this.chunks.slice(0, -1))
  • var scriptPubKey = Script.fromBuffer(lastChunk)
  • return classifyInput(scriptSig, false) === classifyOutput(scriptPubKey)

2nd argument false isn't used.


Reply to this email directly or view it on GitHub
https://github.com/bitcoinjs/bitcoinjs-lib/pull/224/files#r14326810.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.18%) when pulling 886bdee on dcousens:classify into d93623e on bitcoinjs:master.

weilu added a commit that referenced this pull request Jun 29, 2014
Script classification fixes
@weilu weilu merged commit d9e240b into bitcoinjs:master Jun 29, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants