Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32247. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK |
| // Making unknown public key versions (in BIP 342 scripts) non-standard | ||
| SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE = (1U << 20), | ||
|
|
||
| // Validating OP_CHECKSIGFROMSTACK(VERIFY) |
There was a problem hiding this comment.
should maybe use bits 24 and 25
| } | ||
|
|
||
| bool XOnlyPubKey::VerifySchnorr(const uint256& msg, std::span<const unsigned char> sigbytes) const | ||
| bool XOnlyPubKey::VerifySchnorr( |
There was a problem hiding this comment.
i think it might be better to introduce a new function, VerifySchnorrArbitrary here, that way any existing use is identical API. Or use multi dispatch.
| } | ||
| } | ||
| if (pubkey_in.size() == 0) { | ||
| return set_error(serror, SCRIPT_ERR_PUBKEYTYPE); |
There was a problem hiding this comment.
should we just update the BIP for this to just be the internalkey? At the very least, there isn't a hard reason I can think of to burn this keytype (other than checksig keytype parity)
we'd still be bound to the bytes per sigop restrictions, and it seems harmless to allow?
There was a problem hiding this comment.
https://gnusha.org/pi/bitcoindev/CAD5xwhi6DYVm3sONub0x4s=Ef0TupA4j4KxY616RnacXr1GsLA@mail.gmail.com/
I think I agree with 2022-you; separate opcode for key is cleaner and strictly better. Leaving it undefined might be fine?
There was a problem hiding this comment.
I'm fine with that -- if we want INTERNALKEY it can be it's own opcode later on.
follow up would be: does symmetry with CHECKSIG mean we should make OP_0 permanently a non-key? v.s. preserving the upgrade path?
shahsb
left a comment
shahsb
left a comment
There was a problem hiding this comment.
I agree with @JeremyRubin comments..!
This comment was marked as low quality.
This comment was marked as low quality.
|
cr ACK cb0c9f6 This matches the BIP's semantics, and the implementation is reasonable. Minor nits above to reduce possibility of behavior changes for other users of Schnorr APIs. |
|
I spent a bit of time writing tests (reading directly from the BIP) for feature_taproot.py: https://github.com/instagibbs/bitcoin/tree/2025-04-bip348_tests This should hopefully give some nice coverage of border conditions, including sigops budget and maybe inspired people to add more if they want to contribute in a positive way. https://github.com/instagibbs/bitcoin/tree/2025-03-bip348-inq-28 this was an alternative implementation of the core logic, but I think it matches here |
|
Thanks @instagibbs; I've compressed your test changes into a single commit and added that here. |
Some code and ideas from Elements by stevenroose, and sanket1729 Porting help from moonsettler. Tests added to the transaction tests framework. Co-authored-by: James O'Beirne <github@au92.org>
|
Concept ACK |
1 similar comment
|
Concept ACK |
|
In an effort to keep pull requests focused on technical discussion, I invite all contributors of conceptual review to post here: Please try to keep pull request comments focused on the code changes, and move all other comments including "concept N/ACK" to the discussion page. |
| return 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) |
There was a problem hiding this comment.
nit: this is between EvalChecksigPreTapscript and EvalChecksigTapscript which can be a little confusing/annoying
|
🐙 This pull request conflicts with the target branch and needs rebase. |
652424a test: additional test coverage for script_verify_flags (Anthony Towns) 417437e script/verify_flags: extend script_verify_flags to 64 bits (Anthony Towns) 3cbbcb6 script/interpreter: make script_verify_flag_name an ordinary enum (Anthony Towns) bddcade script/verify_flags: make script_verify_flags type safe (Anthony Towns) a5ead12 script/interpreter: introduce script_verify_flags typename (Anthony Towns) 4577fb2 rpc: have getdeploymentinfo report script verify flags (Anthony Towns) a398693 validation: export GetBlockScriptFlags() (Anthony Towns) 5db8cd2 Move mapFlagNames and FormatScriptFlags logic to script/interpreter.h (Anthony Towns) Pull request description: We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](https://github.com/benthecarman/bitcoin/blob/d4a86277ed8a0712e03fbbce290e9209165e049c/src/script/interpreter.h#L175-L195). Therefore, bump this to 64 bits. In order to make it easier to update this logic in future, this PR also introduces a dedicated type for the script flags, and disables implicit conversion between that type and the underlying integer type. To make verifying that this change doesn't cause flags to disappear, this PR also resurrects the changes from #28806 so that the script flags that are consensus enforced on each block can be queried via getdeploymentinfo. ACKs for top commit: instagibbs: reACK 652424a achow101: ACK 652424a darosior: ACK 652424a theStack: Code-review ACK 652424a 🎏 Tree-SHA512: 7b30152196cdfdef8b9700b571b7d7d4e94d28fbc5c26ea7532788037efc02e4b1d8de392b0b20507badfdc26f5c125f8356a479604a9149b8aae23a7cf5549f
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
2 similar comments
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
13 participants
This implements BIP-348 (
OP_CHECKSIGFROMSTACK), but only specifies a regtest deployment. There is no effective policy change, since the SCRIPT_VERIFY_* flags (as used) result in the same OP_SUCCESS-like behavior.This change can be composed with other opcode specifications (e.g. CTV, see #31989) and bundled into the same deployment (yet to be specified).
I encourage more general, conceptual discussion to happen on Delving Bitcoin and not on this pull request.
Some related discussion on Delving Bitcoin here:
See also: