BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) #31989
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/31989. 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. |
jamesob
changed the title
|
NACK. As Towns just pointed out on the mailing list, this proposal represents an unsolicited modification of Bitcoin’s code and consensus rules. There is no clear user demand, no demonstrated market need, and no compelling application that justifies taking on this risk. The absence of significant business interest or ecosystem momentum further underscores the speculative nature of this change. Bitcoin’s resilience depends on its conservatism. To safeguard against unnecessary complexity and centralized agenda-setting, we should reject rule changes that lack overwhelming, emergent consensus. Until we establish clear standards for what constitutes legitimate consensus-driven changes and market-driven necessity, rather than academic or developer curiosity, proposals of this nature should be treated as research, not candidates for inclusion in Core. |
|
Echoing what @BitcoinErrorLog said—clear user demand must be shown to justify this. Any proposed change should be weighed against the worst-case scenario: civil war or a chain split, which we’ve seen before. The use case needs to be so compelling that it decisively outweighs these risks and gains overwhelming community support. |
This comment was marked as abuse.
This comment was marked as abuse.
|
@jamesob I'd be happy to help you keep this thread focused by moderating off-topic comments and pointing them to the appropriate forums. You've suggested and linked to Delving which seems appropriate for general/conceptual discussion, however there is no thread/post over there to actually point folks to. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@melvincarvalho please review the moderation policy: https://github.com/bitcoin-core/meta/blob/main/MODERATION-GUIDELINES.md Github pull request review comments are restricted exclusively for PULL REQUEST REVIEW COMMENTS |
https://delvingbitcoin.org/t/bip-119-op-checktemplateverify-no-activation/1494 |
@jamesob see my pending email in answer to AJ for the issues with CSFS building on @naumenkogs’s TxWithhold:
Especially if it’s possible to do a TxWithhold on the spend of any coinbase output beyond |
|
@ariard I think your comment is more appropriate on the mailing list or delving. You are even mentioning a specific post from the mailing list. I expect a lot of off-topic comments on this PR and would appreciate your help staying focused. PR comments MUST be entirely focused on THIS PR code. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
The code in this pull request is of good quality and is well tested, but it's not clear to me what's the goal with this pull request nor how it fits in the big picture. As this introduces a new consensus feature, it seems premature to merge this into Bitcoin Core until widespread consensus is reached among Bitcoin users to eventually activate this new feature in a soft fork. To be clear, i am not saying the details of the activation need to be figured out before the implementation of the feature goes in, but there needs to at least be rough consensus among Bitcoin users to activate this at all. |
To state the obvious if you push "conceptual discussion" (a critical part of the Bitcoin Core review process for a PR) to external forums (why not an accompanying issue in this repo?) the consideration of whether to merge this pull request would require monitoring these external forums. Merging this pull request would be a step towards a second declared activation attempt for CTV and a potential chain split if there isn't community consensus. Pushing that conceptual discussion outside of this repo doesn't change that fact. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as abuse.
This comment was marked as abuse.
|
Before proceeding with a full technical review, I’d like to clarify whether this pull request represents a complete rewrite or simply a refactor of the original implementation from three years ago. Off-topic Concept ACK CTV is an objectively well-reviewed, fundamental upgrade to Bitcoin, and it has gained broad consensus among subject matter experts over the past three years. There will only ever be a handful of legitimate, code-complete upgrades to Bitcoin that are truly ready for merge—and this is one of them, an ingenious, well-thought-out enhancement to Bitcoin with no downside. I disagree with the notion that consensus-breaking code should receive special treatment compared to other changes. Every upgrade to Bitcoin carries some risk of unknowns (after all, the bug that caused the chain split in 2010 wasn’t consensus code). Moreover, not every low-level piece of C++ code needs to be judged solely on market demand. In Bitcoin, the only reason some people focus their complaints on consensus-breaking code is because such changes require a broad social activation method—to ensure miners are informed, upgrade their nodes, and keep the network in sync to avoid a chain split. This process isn’t a social vote; it’s simply a way of signaling to the hardware operators that Bitcoin software must be upgraded and indicating when it is safe to activate. Frankly, the only reason this ends up on the non-technical timeline is because social consensus is needed to get the word out to miners. Otherwise, like the hundreds of other low-level Bitcoin Core C++ code merges, this wouldn’t attract such uninformed commentary. While there’s naturally a significant overlap between consensus code and Bitcoin’s core fundamentals, BIP119 provides additional scaling techniques and supports second-layer mechanisms without fundamentally altering Bitcoin itself. Ultimately, mergeable C++ code that has undergone thorough review and reached technical consensus should be merged in a timely manner to avoid staleness—even if activation debates continue elsewhere. Given the limited number of humans capable of writing and reviewing low-level C++ Bitcoin upgrades, I hope this is the last time we code and review the same feature in the same codebase. Unless there are substantive technical issues, I recommend we focus on the code review here and move broader conceptual discussions to the appropriate forums. |
|
simple implementation, helpful comments, good test coverage, no backwards compatibility issues, passes all CI. LGTM! |
|
@ariard thanks - probably the place for that is the meta issue or Delving Bitcoin. I don't see anything about your attack that's specific to CTV; e.g. your test script doesn't use the template hash or OP_NOP4 (OP_CTV). Rather it seems like a limitation of legacy script in general. But let's move this discussion off this PR please. |
@jamesob It’s correct it’s not specific for CTV, though it’s easy for CTV to fix it. The test case doesn’t use the template hash, but the “effect” should be the same when OP_NOP4. Yeah, let’s discuss the fix on your core GH repository better. |
|
Opened a pull request on Jamesob repository with the suggested changes: due to current block sigs exhaustions exposure: Changes motivated by current exposure to block signature overflow attacks and it’s quite simple to fix. |
and associated SignatureChecker method.
Also modifies script_tests.json to enable OP_NOP4 as OP_CHECKTEMPLATEVERIFY.
Co-authored-by: James O'Beirne <[email protected]>
Co-authored-by: James O'Beirne <[email protected]>
Co-authored-by: James O'Beirne <[email protected]> Co-authored-by: Gary Krause <[email protected]>
Co-authored-by: James O'Beirne <[email protected]>
|
I believe the ability to commit to the transaction to spend an output, combined with BIP340 signature verification of arbitrary messages, is a reasonable bundle of capabilities to consider for a Bitcoin soft fork. However there is clearly no consensus about this among the Bitcoin development community (see for instance here and here for recent examples). On top of disagreement regarding the capability itself, objections were raised about CTV's approach to implement it. Bitcoin Core should implement but not dictate Bitcoin's consensus rules. As such, a Bitcoin soft fork should only be implemented in Bitcoin Core after there is (rough) consensus among the Bitcoin developer community, and preferably after there is also demonstrated support from Bitcoin users. Outstanding disagreements to CTV on both conceptual and specifications ground make it absolutely premature to consider it for inclusion into Bitcoin Core. The repeated attempts by the author of this PR to try to pressure his way to getting this change merged also leads me to believe he's trying to take advantage of Bitcoin Core's position as the dominant Bitcoin full node implementation to circumvent the broader consensus process. I think such an attempt would ultimately fail, but should be opposed rather than facilitated by the Bitcoin Core project on the sole ground that it would set a terrible precedent for how we approach changes to Bitcoin's consensus rules. For all these reasons, Concept NACK on merging this pull request. To be clear, i am not arguing this PR needs necessarily be closed. I understand a pull request to Bitcoin Core has uses beyond requesting being pulled into master, for instance to keep an up-to-date implementation of the proposal with already configured CI to point people to. But in my opinion marking it as draft would better indicate its status. |
| add_witness_commitment(block) | ||
| block.hashMerkleRoot = block.calc_merkle_root() | ||
| block.solve() | ||
| return block.serialize(True).hex(), block.hash |
There was a problem hiding this comment.
AttributeError: 'CBlock' object has no attribute 'hash'
This is not sufficient for inclusion into the most popular implementation. Developers are not a governance system for Bitcoin. Higher standards must be defined, required, and culturally acceptable. |
| SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE = (1U << 20), | ||
|
|
||
| // CHECKTEMPLATEVERIFY validation (BIP-119) | ||
| SCRIPT_VERIFY_CHECKTEMPLATEVERIFY = (1U << 21), |
There was a problem hiding this comment.
fyi this is the same bit used for csfs PR #32247
|
-----BEGIN PGP SIGNED MESSAGE----- Concept ACK. I'm not going to enter into the polemic about what should be or not the broader consensus process. While, I did discontinue this effort to spend more time investigating security bugs (there is only I have no opinion if bitcoin core is or not the dominant full node implementaiton in the bitcoin Given the champion of this proposed consensus change, CHECKTEMPLATEVERIFY, have opened the code here With all those elements in mind, I'm not the Pythia of Delphi, so in the lack of a more formalized On the design of CHECKTEMPLATEVERIFY, of which, describing in my own words allows to build "hash-chain" Historically, my concern on CTV have been of 2 kind:
On the lack of flexible enough capabilities offered for off-chain protocol designers, I'm not On the use-cases presented by CTV that have always been a bit of free technical claims (e.g congestion Now, since 2022, we have more "toy" implementations of CTV uses-cases with vaults and the Ark thing Of course, there are trade-off with CTV-based vault approach versus signature-based vault approaches In my humble opinion, it's okay to offer different bitcoin scripts ways to design and develop About the unknown knowns of CHECKTEMPLATEVERIFY, I can think of 3 of them, 1) is all the denial-of-service I'm deliberately not going to prononunce myself on the comparative design of CHECKTEMPLATEVERIFY, More flexible designs come with more surface of unforeseen interactions, and sadly I only realize I would support a change of CTV to be Taproot only, but I also understand the concern of the champions That's all -- I think I forgot nothing, but if so I'll correct myself. I do make mine all the concerns of the usual Bitcoin core hackers, that's it's better to very Disclaimer: I'm not paid and I'm not interested to receive any compensation for the time spent on All my professional commitments are under bulletproofs legal provisions to let me be free of whatever In Q4 2021, around the Thursday 2 December to be more exact, we were approached with Gleb Naumenko I'm an investor in bitcoin, the self-custodied asset. iQIzBAEBCAAdFiEEpaaGjXqpHdAKwaZ/gX/6Ao72HJQFAmirlRgACgkQgX/6Ao72 |
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
|
🐙 This pull request conflicts with the target branch and needs rebase. |
24 participants
This implements BIP-119 (
OP_CHECKTEMPLATEVERIFY), but only specifies a regtest deployment. There is no effective policy change, since theSCRIPT_VERIFY_*flags (as used) result in the same NOP-like behavior.This change can be composed with other opcode specifications (e.g.
CSFS,CAT) 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: