Re enable `OP_CAT` #39

0xBEEFCAF3 · 2023-10-24T12:03:49Z

Draft BIP
Basing implementation off Elements PR

Things left todo

Use opcode OP_SUCCESS126
test: get deployment info functional test
test: add to tx_valid.json
test: functional test ctv example

ajtowns · 2023-10-27T18:26:34Z

Oh, I didn't see this PR. Here's what I would expect this to look like: https://github.com/ajtowns/bitcoin/commits/202310-inq25-cat

0xBEEFCAF3 · 2023-10-27T20:59:44Z

Oh, I didn't see this PR. Here's what I would expect this to look like: https://github.com/ajtowns/bitcoin/commits/202310-inq25-cat

@ajtowns thanks for the reference. I'll get the deployment changes included in this branch.

src/script/interpreter.cpp

0xBEEFCAF3 · 2023-12-07T22:57:04Z

@ajtowns I'm currently working on getting functional tests for CAT. Modeling it after CTV functional tests. Are there any other major pieces missing before this PR can get merged and activated?

src/script/interpreter.cpp

src/test/data/script_tests.json

src/script/script.cpp

ajtowns · 2023-12-08T05:30:43Z

@ajtowns I'm currently working on getting functional tests for CAT. Modeling it after CTV functional tests. Are there any other major pieces missing before this PR can get merged and activated?

Would be ideal to get some unit tests as well, if possible.

Seems like the BIP should get PRed to the bips repo; if it can get assigned a number we can use that in the signet activation path.

ajtowns · 2023-12-18T03:59:06Z

Lint is failing again

0xBEEFCAF3 · 2023-12-18T15:25:02Z

Lint is failing again

Thanks! Fixed

0xBEEFCAF3 · 2023-12-18T15:30:48Z

Note: Bip has been added to the bips repo bitcoin/bips#1525

0xBEEFCAF3 · 2024-01-16T18:09:35Z

@ajtowns Doesn't seem like we're getting a number from Luke anytime soon. How about we use Bip390 for the activation params?

ajtowns · 2024-01-17T00:44:37Z

@ajtowns Doesn't seem like we're getting a number from Luke anytime soon. How about we use Bip390 for the activation params?

Nah, picking your own bip numbers is just asking for confusion, and there's a bunch of other proposals similarly stalled. Let's just make our own numbering scheme instead; yours is BIN-2024-0001, so the code snippet will become:

    ... = SetupDeployment{
        .year = 2024, .number = 1, .revision = 0,
        .start = ..., .timeout = ...,
    };

I think I've translated your doc to markdown accurately, but if I missed something or there's other tweaks needed, make a PR on that repo?

ajtowns · 2024-01-18T01:28:49Z

https://github.com/ajtowns/bitcoin/tree/202401-inq25-opcat is a rough suggestion for updating this PR fwiw

test/functional/test_framework/script.py

src/script/interpreter.cpp

ajtowns

It would be good to squash these commits so that each commit just has "this is the way we decided to do things" rather than "we tried it this way, then decided to undo that and do it a different way instead" (eg, 81e31f7 introduces opcat_tests.cpp then 81edc01 removes it)

I guess one option would be to merge this for 25.x now so we can get it activated, and do the squash when rebasing to 27.x?

ajtowns · 2024-04-10T12:23:36Z

test/functional/feature_opcat.py

+            MAX_PUSH_ERROR,
+            self.nodes[0].sendrawtransaction,
+            taproot_op_cat_stack_limit_spend.serialize().hex(),
+        )


Would probably be good to check 520 bytes works and 521 bytes doesn't, rather than just checking 1024 bytes?

Something like "12345678" DUP DUP CAT DUP CAT DUP CAT DUP CAT DUP CAT CAT for 520 bytes and an additional 1 CAT for 521 bytes?

(These cases are already checked in test/script_tests.cpp though)

Yeah im considering removing that testcase

halseth · 2024-04-19T07:32:57Z

It would be good to squash these commits so that each commit just has "this is the way we decided to do things" rather than "we tried it this way, then decided to undo that and do it a different way instead" (eg, 81e31f7 introduces opcat_tests.cpp then 81edc01 removes it)

I guess one option would be to merge this for 25.x now so we can get it activated, and do the squash when rebasing to 27.x?

Strongly agree on squasing before merge.

ajtowns

Some commit contents/ordering comments. Would probably be better to order as:

Re-enable OP_CAT
Add OP_CAT script_tests unittests
op_cat deployment
functional tests for OP_CAT
clean up discourage flags in validation.cpp

(ie, add the feature without activating; add the tests for the feature; activate it; add the functional tests to check it's activated; cleanup)

Otherwise, lgtm.

ajtowns · 2024-04-23T03:57:17Z

src/validation.cpp

@@ -1009,9 +1009,12 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws)

    const bool ctv_active = DeploymentActiveAfter(m_active_chainstate.m_chain.Tip(), m_active_chainstate.m_chainman, Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY);
    const bool apo_active = DeploymentActiveAfter(m_active_chainstate.m_chain.Tip(), m_active_chainstate.m_chainman, Consensus::DEPLOYMENT_ANYPREVOUT);
+    const bool opcat_active = DeploymentActiveAfter(m_active_chainstate.m_chain.Tip(), m_active_chainstate.m_chainman, Consensus::DEPLOYMENT_OP_CAT);


This should be part of the "op_cat deployment" commit

src/script/interpreter.cpp

test/functional/test_framework/script.py

0xBEEFCAF3 · 2024-04-24T02:16:45Z

@ajtowns
re-order done
Ended up also squashing "clean up discourage flags " into the deployment commit

ajtowns · 2024-04-24T05:21:53Z

LGTM, ACK c7f053c . Anyone want to find some last minute bugs and make me look like a clown?

weikengchen · 2024-04-24T06:05:05Z

One very small concern is whether we REALLY need SCRIPT_VERIFY_DISCOURAGE_OP_CAT, or whether DISCOURAGE is needed.

This is because flags is uint_32. If SCRIPT_VERIFY_OP_CAT takes the spot 26 and SCRIPT_VERIFY_DISCOURAGE_OP_CAT takes the spot 27, then we only have 28, 29, 30, 31 left.

Or, are we planning to make flags uint64_t when we used up all the spots?

(This concern should not be a stopper of this PR)

weikengchen · 2024-04-24T06:09:10Z

Actually the question above has been discussed in the club.
https://bitcoincore.reviews/bitcoin-inquisition-39

^ please ignore that concern, it was for transition.

0xBEEFCAF3 · 2024-04-24T16:32:04Z

Updated 3257a41 to fix this comment #39 (comment)

kallewoof

LGTM

ajtowns · 2024-04-26T03:23:41Z

reACK e51aa64

0xBEEFCAF3 changed the title ~~[WIP] Re enable op cat~~ [WIP] Re enable OP_CAT Oct 24, 2023

0xBEEFCAF3 marked this pull request as draft October 24, 2023 12:04

ajtowns reviewed Nov 1, 2023

View reviewed changes

src/script/interpreter.cpp Outdated Show resolved Hide resolved

0xBEEFCAF3 force-pushed the armin/re-enable-op-cat branch from 86ab1b1 to 6a7c8cd Compare November 2, 2023 23:32

ajtowns added this to the 25.x milestone Nov 4, 2023

ajtowns reviewed Nov 6, 2023

View reviewed changes

src/script/interpreter.cpp Outdated Show resolved Hide resolved

0xBEEFCAF3 force-pushed the armin/re-enable-op-cat branch from 6a7c8cd to 54a8c52 Compare November 10, 2023 14:08

0xBEEFCAF3 changed the title ~~[WIP] Re enable OP_CAT~~ Re enable OP_CAT Nov 17, 2023

moonsettler reviewed Nov 23, 2023

View reviewed changes

src/script/interpreter.cpp Show resolved Hide resolved

0xBEEFCAF3 marked this pull request as ready for review December 3, 2023 18:19

ajtowns reviewed Dec 8, 2023

View reviewed changes

src/script/interpreter.cpp Show resolved Hide resolved

ajtowns reviewed Dec 8, 2023

View reviewed changes

src/test/data/script_tests.json Outdated Show resolved Hide resolved

ajtowns reviewed Dec 8, 2023

View reviewed changes

src/script/script.cpp Outdated Show resolved Hide resolved

EthanHeilman mentioned this pull request Dec 11, 2023

BIP 347: OP_CAT in Tapscript bitcoin/bips#1525

Merged

0xBEEFCAF3 force-pushed the armin/re-enable-op-cat branch 2 times, most recently from ce04f20 to b0f3e1f Compare December 16, 2023 18:03

0xBEEFCAF3 force-pushed the armin/re-enable-op-cat branch from b0f3e1f to bdadf05 Compare December 18, 2023 15:10

0xBEEFCAF3 mentioned this pull request Jan 14, 2024

Reenable OP_CAT bitcoin/bitcoin#29247

Open

DrahtBot added the Needs rebase label Jan 17, 2024

0xBEEFCAF3 force-pushed the armin/re-enable-op-cat branch from 7de6410 to b78dd18 Compare January 18, 2024 01:30

ajtowns reviewed Apr 10, 2024

View reviewed changes

test/functional/test_framework/script.py Outdated Show resolved Hide resolved

0xBEEFCAF3 force-pushed the armin/re-enable-op-cat branch 2 times, most recently from 96cbdcf to c9984ad Compare April 10, 2024 14:01

0xBEEFCAF3 requested a review from ajtowns April 10, 2024 14:49

EthanHeilman reviewed Apr 11, 2024

View reviewed changes

src/script/interpreter.cpp Outdated Show resolved Hide resolved

0xBEEFCAF3 force-pushed the armin/re-enable-op-cat branch from fb53c52 to 5390a5e Compare April 17, 2024 13:55

ajtowns reviewed Apr 19, 2024

View reviewed changes

0xBEEFCAF3 force-pushed the armin/re-enable-op-cat branch 5 times, most recently from efa0219 to 734f342 Compare April 22, 2024 14:25

ajtowns reviewed Apr 23, 2024

View reviewed changes

Re-enable OP_CAT

534ee54

0xBEEFCAF3 force-pushed the armin/re-enable-op-cat branch from 734f342 to c7f053c Compare April 24, 2024 02:05

EthanHeilman and others added 3 commits April 24, 2024 12:29

Adds OP_CAT script_tests unittests

3257a41

OP_CAT deployment

7c3ca0d

functional tests for OP_CAT

e51aa64

0xBEEFCAF3 force-pushed the armin/re-enable-op-cat branch from c7f053c to e51aa64 Compare April 24, 2024 16:30

kallewoof approved these changes Apr 25, 2024

View reviewed changes

ajtowns merged commit 873573e into bitcoin-inquisition:25.x Apr 26, 2024
1 check passed

ajtowns mentioned this pull request May 2, 2024

Implement BIN-24-1 validation (BIP 347, OP_CAT) #57

Merged

0xBEEFCAF3 mentioned this pull request Jul 31, 2024

[feat] Disabled opcodes (CVE-2010-5137) keep-starknet-strange/shinigami#56

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re enable `OP_CAT` #39

Re enable `OP_CAT` #39

0xBEEFCAF3 commented Oct 24, 2023 •

edited

Loading

ajtowns commented Oct 27, 2023

0xBEEFCAF3 commented Oct 27, 2023

0xBEEFCAF3 commented Dec 7, 2023

ajtowns commented Dec 8, 2023

ajtowns commented Dec 18, 2023

0xBEEFCAF3 commented Dec 18, 2023

0xBEEFCAF3 commented Dec 18, 2023 •

edited

Loading

0xBEEFCAF3 commented Jan 16, 2024

ajtowns commented Jan 17, 2024

ajtowns commented Jan 18, 2024

ajtowns left a comment

ajtowns Apr 10, 2024

0xBEEFCAF3 Apr 22, 2024

halseth commented Apr 19, 2024

ajtowns left a comment

ajtowns Apr 23, 2024

0xBEEFCAF3 commented Apr 24, 2024

ajtowns commented Apr 24, 2024

weikengchen commented Apr 24, 2024

weikengchen commented Apr 24, 2024

0xBEEFCAF3 commented Apr 24, 2024

kallewoof left a comment

ajtowns commented Apr 26, 2024

Re enable OP_CAT #39

Re enable OP_CAT #39

Conversation

0xBEEFCAF3 commented Oct 24, 2023 • edited Loading

ajtowns commented Oct 27, 2023

0xBEEFCAF3 commented Oct 27, 2023

0xBEEFCAF3 commented Dec 7, 2023

ajtowns commented Dec 8, 2023

ajtowns commented Dec 18, 2023

0xBEEFCAF3 commented Dec 18, 2023

0xBEEFCAF3 commented Dec 18, 2023 • edited Loading

0xBEEFCAF3 commented Jan 16, 2024

ajtowns commented Jan 17, 2024

ajtowns commented Jan 18, 2024

ajtowns left a comment

Choose a reason for hiding this comment

ajtowns Apr 10, 2024

Choose a reason for hiding this comment

0xBEEFCAF3 Apr 22, 2024

Choose a reason for hiding this comment

halseth commented Apr 19, 2024

ajtowns left a comment

Choose a reason for hiding this comment

ajtowns Apr 23, 2024

Choose a reason for hiding this comment

0xBEEFCAF3 commented Apr 24, 2024

ajtowns commented Apr 24, 2024

weikengchen commented Apr 24, 2024

weikengchen commented Apr 24, 2024

0xBEEFCAF3 commented Apr 24, 2024

kallewoof left a comment

Choose a reason for hiding this comment

ajtowns commented Apr 26, 2024

Re enable `OP_CAT` #39

Re enable `OP_CAT` #39

0xBEEFCAF3 commented Oct 24, 2023 •

edited

Loading

0xBEEFCAF3 commented Dec 18, 2023 •

edited

Loading