Skip to content
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.

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

SIGHASH_ANYONECANPAY should be 0x80 in ScriptInterpreter #424

Closed
ethers opened this issue Jul 15, 2014 · 4 comments · Fixed by #431
Closed

SIGHASH_ANYONECANPAY should be 0x80 in ScriptInterpreter #424

ethers opened this issue Jul 15, 2014 · 4 comments · Fixed by #431

Comments

@ethers
Copy link
Contributor

ethers commented Jul 15, 2014

It should be 0x80 to match https://github.com/bitcoin/bitcoin/blob/master/src/script.h#L183 as well as what's in lib/Transaction.js. (unifying these constants in ScriptInterpreter and Transaction can be done in another issue)

Also, SIGHASH_ANYONECANPAY is untested. eg set SIGHASH_ANYONECANPAY to 0 and all the tests still pass.

@ryanxcharles
Copy link
Contributor

Good catch. Someone mis-typed 80 for 0x80 in ScriptInterpreter.

@maraoz
Copy link
Contributor

maraoz commented Jul 15, 2014

I remember changing it in Transaction.js because it was wrong and I never guessed it could be repeated in other place. We should definitely unify the constants. Maybe it makes more sense to have a separate protocol constants file (constants.js)?

On Tue, Jul 15, 2014 at 3:53 PM, Ryan X. Charles notifications@github.com
wrote:

Good catch. Someone mis-typed 80 for 0x80 in ScriptInterpreter. We also have the same constant in Transaction.js, but it is defined correctly there.

Reply to this email directly or view it on GitHub:
#424 (comment)

@ryanxcharles
Copy link
Contributor

Yeah I thought I had remembered seeing this issue before. I'm open to putting all the constants in a single file. However, that does mean that if you need just one constant, you would have to include the entire list, which is undesirable for the browser.

@ethers
Copy link
Contributor Author

ethers commented Jul 16, 2014

IMO ScriptInterpreter is fine for these constants.
PR looks good.

[I should have made these changes since I missed that SIGHASH_ANYONECANPAY is indeed tested by Transaction tests (instead of ScriptInterpreter tests)].

micahriggan pushed a commit to micahriggan/bitcore that referenced this issue Dec 27, 2018
Use getTotalAmount() instead of amount field on input selection
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 a pull request may close this issue.

3 participants