BIP348 OP_CHECKSIGFROMSTACK #72
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -343,6 +343,72 @@ static bool EvalChecksigPreTapscript(const valtype& vchSig, const valtype& vchPu | |
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Returns false when script execution must immediately fail. | ||
| * `success_out` must be set when returning true. | ||
| */ | ||
| static bool EvalChecksigFromStack(const valtype& sig, const valtype& msg, const valtype& pubkey_in, ScriptExecutionData& execdata, unsigned int flags, SigVersion sigversion, ScriptError* serror, bool& success_out) | ||
|
There was a problem hiding this comment. nit: this is between |
||
| { | ||
| assert(sigversion == SigVersion::TAPSCRIPT); | ||
|
|
||
| /* | ||
| * Implementation follows the BIP348 spec as closely as possible for correctness. | ||
| */ | ||
|
|
||
| // If the public key size is zero, the script MUST fail and terminate immediately. | ||
| if (pubkey_in.size() == 0) { | ||
| return set_error(serror, SCRIPT_ERR_PUBKEYTYPE); | ||
| } | ||
|
|
||
| // If the public key size is 32 bytes, it is considered to be a public key as described in BIP 340: | ||
| // If the signature is not the empty vector, the signature is validated against the public key and message according to BIP 340. Validation failure in this case immediately terminates script execution with failure. | ||
| if (pubkey_in.size() == 32) { | ||
| if (!sig.empty()) { | ||
| if (sig.size() != 64) { | ||
| return set_error(serror, SCRIPT_ERR_SCHNORR_SIG_SIZE); | ||
| } | ||
|
|
||
| XOnlyPubKey pubkey{pubkey_in}; | ||
| if (!pubkey.VerifySchnorr(msg, sig)) { | ||
| return set_error(serror, SCRIPT_ERR_SCHNORR_SIG); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If the public key size is not zero and not 32 bytes; the public key is of an unknown public key type. | ||
| // Signature verification for unknown public key types succeeds as if signature verification for a known | ||
| // public key type had succeeded. | ||
| if (pubkey_in.size() != 0 && pubkey_in.size() != 32) { | ||
| /* | ||
| * New public key version softforks should be defined before this `if` block. | ||
| * Generally, the new code should not do anything but terminating the script execution. To avoid | ||
| * consensus bugs, it should not modify any existing values (including `success_out`). | ||
| */ | ||
| if ((flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE) != 0) { | ||
| return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_PUBKEYTYPE); | ||
| } | ||
| } | ||
|
|
||
| // If the script did not fail and terminate before this step, regardless of the public key type: | ||
|
|
||
| if (sig.empty()) { | ||
| // If the signature is the empty vector: An empty vector is pushed onto the stack, and execution continues with the next opcode. | ||
| success_out = false; | ||
| } else { | ||
| // If the signature is not the empty vector: | ||
| // - The opcode is counted towards the sigops budget as described in BIP 342. | ||
| // - A 1-byte value 0x01 is pushed onto the stack. | ||
| assert(execdata.m_validation_weight_left_init); | ||
| execdata.m_validation_weight_left -= VALIDATION_WEIGHT_PER_SIGOP_PASSED; | ||
| if (execdata.m_validation_weight_left < 0) { | ||
| return set_error(serror, SCRIPT_ERR_TAPSCRIPT_VALIDATION_WEIGHT); | ||
| } | ||
|
There was a problem hiding this comment. doesn't sound right to validate the signature before checking this
Author
There was a problem hiding this comment. This PR follows the BIP more closely than random implementation details of taproot. I'm willing to change it but I'd need general agreement on it. There was a problem hiding this comment. The BIP says "These opcodes are treated identically to other signature checking opcodes and count against the sigops and budget." which to me implies it should closely follow the random implementation details of the other signature checking opcodes... (Checking the budget before incurring the expense doesn't seem like a random implementation detail, though...) There was a problem hiding this comment. I think this is strictly equivalent? There is no execution path where we would perform the verification but not end up either accounting for it here or immediately stopping Script execution. There was a problem hiding this comment. you'd do one extra signature validation, not the end of the world jut calling it out that its different from how checksig implemented
Author
There was a problem hiding this comment. @benthecarman I assume he meant consensus semantically, not implementation-wise. If it's a blocker for moving forward it can be changed... There was a problem hiding this comment. Not a huge deal, just calling out the difference There was a problem hiding this comment.
You get a different error message (failing signature vs exceeded the budget); and you spend more execution time reporting the failed signature. Still fails in either case, so not the end of the world, but seems strictly worse, and not particularly justified by any of the BIP text. Matching the |
||
| success_out = true; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| static bool EvalChecksigTapscript(const valtype& sig, const valtype& pubkey, ScriptExecutionData& execdata, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* serror, bool& success) | ||
| { | ||
| assert(sigversion == SigVersion::TAPSCRIPT); | ||
|
|
@@ -1283,6 +1349,40 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& | |
| } | ||
| break; | ||
|
|
||
| case OP_CHECKSIGFROMSTACK: { | ||
|
|
||
| // DISCOURAGE for OP_CHECKSIGFROMSTACK is handled in OP_SUCCESS handling | ||
| // OP_CHECKSIGFROMSTACK is only available in Tapscript | ||
| if (sigversion == SigVersion::BASE || sigversion == SigVersion::WITNESS_V0) { | ||
| return set_error(serror, SCRIPT_ERR_BAD_OPCODE); | ||
| } | ||
|
|
||
| // If fewer than 3 elements are on the stack, the script MUST fail and terminate immediately | ||
| if (stack.size() < 3) { | ||
| return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION); | ||
| } | ||
|
|
||
| // The public key (top element) | ||
| // message (second to top element), | ||
| // and signature (third from top element) are read from the stack. | ||
| const valtype& pubkey = stacktop(-1); | ||
| const valtype& msg = stacktop(-2); | ||
| const valtype& sig = stacktop(-3); | ||
|
|
||
| bool push_success = true; | ||
| if (!EvalChecksigFromStack(sig, msg, pubkey, execdata, flags, sigversion, serror, push_success)) { | ||
| return false; // serror set by EvalChecksigFromStack | ||
| } | ||
|
|
||
| popstack(stack); | ||
| popstack(stack); | ||
| popstack(stack); | ||
|
|
||
| stack.push_back(push_success ? vchTrue : vchFalse); | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| default: | ||
| return set_error(serror, SCRIPT_ERR_BAD_OPCODE); | ||
| } | ||
|
|
@@ -1988,6 +2088,12 @@ std::optional<bool> CheckTapscriptOpSuccess(const CScript& exec_script, unsigned | |
| } else if (!(flags & SCRIPT_VERIFY_OP_CAT)) { | ||
| return set_success(serror); | ||
| } | ||
| } else if (opcode == OP_CHECKSIGFROMSTACK) { | ||
| if (flags & SCRIPT_VERIFY_DISCOURAGE_CHECKSIGFROMSTACK) { | ||
| return set_error(serror, SCRIPT_ERR_DISCOURAGE_OP_CHECKSIGFROMSTACK); | ||
| } else if (!(flags & SCRIPT_VERIFY_CHECKSIGFROMSTACK)) { | ||
| return set_success(serror); | ||
| } | ||
| } else { | ||
| // OP_SUCCESS behaviour | ||
| if (flags & SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want different values from the OP_CAT deployment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to need an explainer to figure out deployments in inquisition; can fix it along with potential signet one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajtowns could you provide guidance here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #82; though you'll need a binana number. Maybe figure out a nice way to make a binana doc just reference a particular revision of a bip? (Have BIN-2025-xxx.md just have a header and a table that maps binana revision numbers to bips repo commit ids maybe?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitcoin-inquisition/binana#1 turns out IK is already a thing in BINANA (and compatible), CSFS needs modification. Can the BINANA be modified with author's consent or do you suggest a new one? I believe the *VERIFY variant is considered abandoned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifications should be with author's agreement, and bumping the revision number and date. If it's just changing to reference an updated spec from the same author in the bips repo, I'd be happy to assume the author's agreement. Just getting a new number assigned is fine too (though I guess I'd want to add the author to the table in the README to better distinguish them in that case?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd want the literal spec in BIP348 (obviously), seems easiest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reardencode Are you ok with this idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that seems reasonable. Definitely better than trying to keep the two docs in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might like to reference BIN16-118 for how to simplify BINANA assignments for BIPs.