-
Notifications
You must be signed in to change notification settings - Fork 36.5k
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
CAT in Tapscript (BIP-347) #29247
base: master
Are you sure you want to change the base?
CAT in Tapscript (BIP-347) #29247
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/29247. ReviewsSee the guideline for information on the review process. 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. |
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
Drafting until I get the inquisition PR approved and I can get the builds passing. |
Some of us have been playing with the idea of neutered CAT: instead of the MAX_SCRIPT_ELEMENT_SIZE (520 bytes) the maximum output size would be 80 bytes. Rationale: Combined with CSFS it can be used as a signed datacarrier, for which the 'standard' limit is 80 bytes, it also has to be smaller than 84 bytes which is required for building CTV templates on the stack, and larger than 64/65/72 bytes which are respectively needed for:
Could be a livable compromise between the conservatives that want to preserve certain characteristics of bitcoin and the prometheans who want to give the developers more practical and useful tools to build with. |
Script is already expressive enough to (awkwardly, expensively) compute arbitrary SHA256 hashes on the stack, except that the result would be broken in smaller pieces of at most 4 bytes. With CAT, those can be concatenated to get a single 32-byte result. Therefore, neutering CATs does not achieve the desired result of preventing the CHECKSIG tricks, unless you limit the length of the result to less than 32 bytes - which would also neuter most of the utility of the opcode. |
b9c67da
to
f1fd2b6
Compare
f1fd2b6
to
6b259bd
Compare
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
6b259bd
to
a04c22b
Compare
if (flags & SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS) { | ||
return set_error(serror, SCRIPT_ERR_DISCOURAGE_OP_SUCCESS); | ||
if (opcode == OP_CAT) { | ||
if (flags & SCRIPT_VERIFY_DISCOURAGE_OP_CAT) { |
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.
SCRIPT_VERIFY_DISCOURAGE_OP_CAT
is acting as a CAT-specific SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS
that can be turned off while still not relaying transasctions with other OP_SUCCESS opcodes. Since this is the first time we're carving out an OP_SUCCESS and doing this, it would be great to have a test that exercises the combinations of SCRIPT_VERIFY_DISCOURAGE_OP_CAT and SCRIPT_VERIFY_OP_CAT
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.
Great suggestion! Added some additional unit tests to script_tests.json testing different combinations of scripts with and without (SCRIPT_VERIFY_DISCOURAGE_OP_CAT, SCRIPT_VERIFY_OP_CAT and SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS)
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.
Added to the inquisition PR in bitcoin-inquisition#68
f5807a5
to
d6a668a
Compare
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
d6a668a
to
6e05acf
Compare
Converting to draft while waiting for consensus on this. |
@achow101 What does draft vs non-draft mean in the bitcoin-core github? My understanding was moving from draft to non-draft means code is ready for review. Similar to moving out of WIP. Is draft/non-draft is being used to signal community consensus or bitcoin-core consensus? If so, I agree this should be a draft. |
In general, draft means that a PR is not in a state where it could be merged. It indicates to reviewers that they may not want to do any in depth code review of the PR yet. In the context of this PR, there does not appear to be consensus for the concept of OP_CAT in TapScript yet, so this PR cannot be merged even if the code is okay. Hence marking it as a draft. Note that this is a result of a Kill/Shill/Merge session that occurred at the most recent CoreDev. |
@achow101 Thanks, that's helpful for understanding the context. If consensus forms around OP_CAT should I or @0xBEEFCAF3 mark this as no longer as draft? I can't speak for @0xBEEFCAF3 but I would feel slightly uncomfortable making that decision as it would open me up to criticism that I was attempting to manufacture consensus, even if there was consensus. Then again if that is the expectation I suppose we'd have to do that and take the licks. What is the process here? |
Yes. A maintainer may also do so as well. |
6e05acf
to
e24086b
Compare
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
e24086b
to
94fcae9
Compare
94fcae9
to
4e337f7
Compare
4e337f7
to
01945a5
Compare
Implement OP_CAT as a new Tapscript op code by redefining the opcode OP_SUCCESS126 (126 in decimal and 0x7e in hexadecimal). This is the same opcode value used by the original OP_CAT. When evaluated, the OP_CAT instruction: Pops the top two values off the stack, concatenates the popped values together in stack order, and then pushes the concatenated value on the top of the stack. OP_CAT fails if there are fewer than two values on the stack or if a concatenated value would have a combined size greater than the maximum script element size of 520 bytes. See BIP 347 for a deeper description.
Co-authored-by: Ethan Heilman <[email protected]> This commit introduces unit tests for opcat in script_tests.json. Additionally, it creates new test utilities for future Taproot script tests within script_tests.json. The key features of this commit are the addition of three new tags: `#SCRIPT#`, `#CONTROLBLOCK#`, and `#TAPROOTOUTPUT#`. These tags streamline the test creation process by eliminating the need to manually generate these components outside the test suite. * `#SCRIPT#`: Parses Tapscript and outputs a byte string of opcodes. * `#CONTROLBLOCK#`: Automatically generates the control block for a given Taproot output. * `#TAPROOTOUTPUT#`: Generates the final Taproot scriptPubKey.
The goal of this functional test is to ensure OP_CAT spends are still disabled by default in segwitv0 and legacy spends. Spending such inputs should result in `mandatory-script-verify-flag-failed (Attempted to use a disabled opcode)`. While spending OP_CAT inputs in tapscript should be discouraged under the default `STANDARD_SCRIPT_VERIFY_FLAGS`.
01945a5
to
7377841
Compare
CAT in Tapscript
This PR provides the necessary code to enable the opcode OP_CAT in Tapscript as specified in BIP-347: OP_CAT in Tapscript and BIN-2024-0001,
Important: This PR does not include miner activation functionality. This means that merging this PR into bitcoin-core will not make OP_CAT functional in Bitcoin.
If this PR is merged it is not a signal of community consensus around activating CAT nor be read as a portent about the activation process or timeline.
The PR is not a stand-in for consensus around such decisions.
This PR includes:
SCRIPT_VERIFY_OP_CAT
andSCRIPT_VERIFY_DISCOURAGE_OP_CAT
.src/test/script_tests.cpp
JSON script format enabling the rapid creation of new Tapscript unit tests.Activation on Bitcoin-inquisition (signet)
Bitcoin-inquisition PR 37 which contained our implementation of OP_CAT was merged into bitcoin-inquisition (signet) on Apr 25 2024, released as bitcoin-inquisition release 25.2 on Apr 26 2024. and activated in bitcoin-inquisition Apr 30 2024. The bitcoin-inquisition PR was reviewed in the PR review club (transcript of discussion). Since Apr 30 2024 there have been many OP_CAT transactions created and spent on signet.
The code merged into bitcoin-inquisition differs from this PR, as we have removed the consensus logic which was used to activate it on signet.
OP_CAT Tapscript Implementation
We implement OP_CAT as a new Tapscript op code by redefining the opcode OP_SUCCESS126 (126 in decimal and 0x7e in hexadecimal). This is the same opcode value used by the original OP_CAT.
When evaluated, the OP_CAT instruction:
OP_CAT fails if there are fewer than two values on the stack or if a concatenated value would have a combined size greater than the maximum script element size of 520 bytes.
See BIP 347 for a deeper description.
Errors thrown
If evaluated OP_CAT can throw the following errors:
SCRIPT_ERR_INVALID_STACK_OPERATION
MAX_SCRIPT_ELEMENT_SIZE
(520 bytes) we throw the error:SCRIPT_ERR_PUSH_SIZE
.Script verification flags
While this PR does not contain any miner signaling and activation logic and can not activate OP_CAT, it does contain two flags which future activation logic could set to control activation of OP_CAT.
SCRIPT_VERIFY_OP_CAT
IF a bitcoin node has this set to true, then it treat OP_CAT enabled for Tapscript. That is, OP_SUCCESS126 will be redefining to OP_CAT in Tapscript. If this was set to true at the consensus level this would cause a soft fork.SCRIPT_VERIFY_DISCOURAGE_OP_CAT
When set to true, a node receiving any Tapscript transaction containing the opcode OP_CAT or OP_SUCCESS126 will reject the transaction throwing the errorSCRIPT_ERR_DISCOURAGE_OP_CAT
but not banning the node which relayed the transaction. This prevents nodes from relaying transactions with OP_CAT. This is equivalent to the behavior ofSCRIPT_ERR_DISCOURAGE_OP_CAT = true
whenSCRIPT_VERIFY_OP_CAT = false
as OP_CAT is an OP_SUCCESS (OP_SUCCESS126).This is how these two flags are intended to be used to ensure a smooth soft fork.
SCRIPT_VERIFY OP_CAT
SCRIPT_VERIFY DISCOURAGE_OP_CAT
The flag
SCRIPT_ERR_DISCOURAGE_OP_CAT
provides a window of time for the network to fully activate, before nodes will relay or accept transactions containing OP_CAT in their mem pools.Tests
This PR contains a suite of script tests to ensure that OP_CAT functions as expected. In the JSON script tests (
script_tests.json
), we test:All of these tests are designed to cover the happy path of OP_CAT, the various errors which OP_CAT can throw and all the corner cases between those two outcomes.
Additionally we include three tests outside of the JSON script tests.
cat_simple
andcat_empty_stack
are designed to test OP_CAT outside of the JSON serialization regime. Ensure that we catch bugs that we might miss in the JSON script tests due to a bug introduced at the JSON serialization layer.cat_dup_test
enumerates all stack element sizes from 1 to 522 bytes and then enumerates up to 10 repetitions ofOP\_DUP OP\_CAT
. It then tests if the stack element would exceed 520 bytes and if so did OP_CAT throw the errorSCRIPT_ERR_PUSH_SIZE
. This allows us to be certain that OP_CAT will not introduce anyOP\_DUP OP\_CAT
memory exhaustion attacks.Better Tapscript tests in JSON script tests
While writing these JSON script tests (
script_tests.json
) we ran into the following problem. The JSON script tests are simple and easy to write for pre-Tapscript scripts, but adding or changing a Tapscript test requires substantial work per test.Consider the following pre-tapscript test:
whereas a Tapscript test for the same script (annotated with comments for better readability) would look like:
Computing the Tapscript output, such as
0x51 0x20 0x15048ed3a65748549c27b671936987093cf73a4c9cb18522a74fb9553060ca99
, requires writing custom code and running it for each test. The same is true for the Tapscript control block, such asc0d6889cb081036e0faefa3a35157ad71086b123b2b144b649798b494c300a961d
. If a test is changed or updated new outputs and control blocks must be computed. The complexity of doing this is likely the reason that no one has added any Tapscript tests to JSON script tests until this PR.In this PR we address this issue by adding the following improvements to JSON script tests:
"#SCRIPT#
and#CONTROLBLOCK#
) that allow the script test parser to automatically generate and inject a valid Tapscript output and control block to be computed automatically from the JSON script.#SCRIPT#
. This transforms the unreadable script7e4c02aabb87
into#SCRIPT# CAT 0x4c 0x02 0xaabb EQUAL
.This results in the following JSON script test which is far easier to write and easier to read.