OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE) #29198

reardencode · 2024-01-07T18:35:13Z

This pull request contains a the same implementation of OP_CHECKTEMPLATEVERIFY (BIP119) as @jamesob's Covenant Tools, an implementation of OP_CHECKSIGFROMSTACK(VERIFY) and of OP_INTERNALKEY.

There are no testnet or mainnet activation parameters proposed in this pull request. I am deeply uninterested in the details of activation semantics.

This combination of changes allows for the implementation of a variety of layer two proposals and improvements. Including, but not limited to, Lightning Symmetry, Point-Time-Locked-Contracts, Timeout Trees, and unidirectional non-interactive channels.

For anyone interested, I also have a branch with these same changes based on the latest release.

DrahtBot · 2024-01-07T18:35:16Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	michaelfolkson
Concept ACK	jonatack, moonsettler, hsjoberg, niteshbalusu11, 1440000bytes, chrisguida

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29547 (kernel: chainparams updates for 27.x by fanquake)
#29280 (Implement OP_CHECKTEMPLATEVERIFY by reardencode)
#29270 (Implement OP_CHECKSIGFROMSTACK(VERIFY) by reardencode)
#29269 (Add OP_INTERNALKEY for Tapscript by reardencode)
#29247 (Reenable OP_CAT by 0xBEEFCAF3)
#29221 (Implement 64 bit arithmetic op codes in the Script interpreter by Christewart)
#29134 (Compressed Bitcoin Transactions by TomBriar)
#29050 (Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes by stevenroose)
#29039 (versionbits refactoring by ajtowns)
#28806 (rpc: Add script verification flags to getdeploymentinfo by ajtowns)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#28334 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 by ajtowns)
#27433 (getblocktemplate improvements for segwit and sigops by Sjors)
#26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
#26201 (Remove Taproot activation height by Sjors)

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.

michaelfolkson · 2024-01-07T18:47:57Z

This should be opened to bitcoin-inquisition rather than this repo at this stage? I thought that was the whole point of bitcoin-inquisition.

I'm interested in why you think LN-Symmetry would be better implemented not using APO but perhaps that discussion can be had elsewhere.

reardencode · 2024-01-07T19:06:45Z

This should be opened to bitcoin-inquisition rather than this repo at this stage? I thought that was the whole point of bitcoin-inquisition.

Let's focus on code review. There is no strict process suggesting that code first flow through inquisition.

I'm interested in why you think LN-Symmetry would be better implemented not using APO but perhaps that discussion can be had elsewhere.

Happy to discuss on delving

jonatack

Concept ACK

src/script/interpreter.cpp

moonsettler

Concept ACK

callebtc · 2024-01-07T20:48:42Z

src/script/interpreter.cpp

@@ -591,7 +652,42 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
                    break;
                }

-                case OP_NOP1: case OP_NOP4: case OP_NOP5:


case OP_NOP1: was removed (by accident?). Should it be added back to L691?
Edit: sorry, misread, already done.

Git diff. Making life unnecessarily hard sometimes.

1440000bytes

Concept ACK

CHECKSIGFROMSTACK complements CHECKTEMPLATEVERIFY and helps in ticking more boxes. INTERNALKEY saves 32 bytes in some cases.

jonatack · 2024-01-07T21:29:41Z

In commit 0ac5cf9

$ ./src/test/test_bitcoin -t transaction_tests
Running 7 test cases...
test/transaction_tests.cpp:192: error: in "transaction_tests/tx_valid": mapFlagNames is missing a script verification flag

Edit: 4371ffe two commits later appears to contain the missing change. Can wait until next need to push for something else.

hsjoberg · 2024-01-07T21:35:18Z

Concept ACK; this would make pretty much all sides happy.

reardencode · 2024-01-08T19:27:44Z

Force pushed to fix clean commits.

niteshbalusu11 · 2024-01-09T04:36:42Z

Concept ACK.

Sjors · 2024-01-09T09:05:39Z

It seems too early in the process to open a pull request like this. It's also possible to open a pull request against your own repo-fork of Bitcoin Core, or Inquisition as suggested above.

For comparison, the first Taproot pull request to this repo #17977 was made in early 2020, which was after about year of in person workshops studying the proposal. The BIP's themselves were first proposed in 2018. The first work-in-progress taproot branch, not a pull request, is also from around 2018.

Then again, being able to use the full Bitcoin Core CI can help in getting the code in good shape, which in turn is useful in improving the proposal. You can run that locally too in Docker. That said, a few extra (draft) PR's for soft-fork proposals is not a significant drain on (paid for by someone) CI capacity.

Typically if a pull request depends on another pull request we mark it as draft. That's not the case here, but it does depend on two BIP proposals that were made only two days ago. We could also use some other tag to indicate that the underlying proposal is still in the concept stage.

1440000bytes · 2024-01-09T10:06:16Z

A few days back Ava Chow had mentioned on twitter that there is no pull request open for CHECKTEMPLATEVERIFY:

https://x.com/achow101/status/1742427508488729044
https://x.com/achow101/status/1742555598003048753
There were some misunderstandings about unaddressed issues related to previous pull request:

https://x.com/JeremyRubin/status/1742582215354019956
Since @reardencode was already working on CHECKSIGFROMSTACK and INTERNALKEY to be used in combination with CHECKTEMPLATEVERIFY, I think it makes sense to open this pull request in bitcoin core repository for code review.
There have been several meetings and workshops for CHECKTEMPLATEVERIFY: https://utxos.org/workshops/. I am not sure if CHECKSIGFROMSTACK and INTERNALKEY need workshops or LNHANCE however they could be done with the pull request open as well.
For comparison, Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes #29050 was opened recently and still looks in conceptual stage with unaddressed comments on BIP, incomplete implementation and still open for code review in bitcoin core repository instead of inquisition.

Then again, being able to use the full Bitcoin Core CI can help in getting the code in good shape, which in turn is useful in improving the proposal. You can run that locally too in Docker. That said, a few extra (draft) PR's for soft-fork proposals is not a significant drain on (paid for by someone) CI capacity.

Agree

michaelfolkson · 2024-01-09T10:34:05Z

A few days back Ava Chow had mentioned on twitter that there is no pull request open for CHECKTEMPLATEVERIFY:

There is a pull request open (in draft) that contains CTV in addition to other opcodes and sighash flags (ie proposed consensus changes).

You don't have to be an expert in combinatorics to realize that continuing down this path results in multiple pull requests to this repo from different authors with different combinations of opcodes and sighash flags depending on the PR author's individual preference. That was one of the major motivations for bitcoin-inquisition where multiple opcodes and sighash flags can be activated on the default signet not creating noise in this repo and allowing both review and experimentation with new opcodes and sighash flags that may or may not ever get activated on mainnet. If for some reason bitcoin-inquisition isn't to the PR author's tastes (I"m not aware of any problems so far) the same can be done on a custom signet with a different fork of this repo.

I totally agree with Sjors. The approach taken here of avoiding steps other soft fork proposals have gone through and just assuming shortcuts can be taken is a massive backwards step and I suspect won't achieve anything except creating more unnecessary noise in this repo.

Co-authored-by: James O'Beirne <github@au92.org>

reardencode · 2024-01-14T15:01:28Z

Force pushed to deconflict opcodes with VAULT, add deployment info output, and slightly improve tx_invalid tests.

benthecarman · 2024-01-14T19:06:34Z

src/script/interpreter.cpp

+    } else if (pubkey_in.size() == 32) {
+        if (!success) return true;
+        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);


does this mean you can do schnorr sigs with segwit v0 and p2sh? is that intended?

msg size is also not verifiied here

Yes, intended to support both ECDSA and BIP340 sigs in segwitv0 and legacy.

No need for message size check here because BIP340 supports arbitrary length message.

benthecarman · 2024-01-14T19:06:46Z

src/script/interpreter.cpp

+        // Pubkeys of length 33 are only constrained in legacy and segwitv0. In these script types only those beginning
+        // with 0x02 or 0x03 are constrained. Others are unknown pubkey types.


if schnorr is allowed in non-taproot then why forbid ecdsa for taproot?

Decision was somewhat arbitrary on my part. ECDSA doesn't really add anything in Tapscrpit where Schnorr adds ability to use well specified MPC protocols like MuSig2 and FROST in legacy, so I settled on adding Schnorr for legacy but not adding ECDSA for Tapscript.

In Tapscript, I wonder if we'll eventually want to fork in 33-byte 02/03 keys as x-y Schnorr, so I was a bit hesitant to consume those prefixes now.

benthecarman · 2024-01-14T19:07:06Z

src/script/interpreter.cpp

+            return false;
+        }
+
+        if (msg.size() != 32) return set_error(serror, SCRIPT_ERR_INVALID_DATA_LENGTH);


should different message sizes be left for future upgrades

ECDSA requires 32-byte message. OP_SHA256 ahead would enable scripts to handle other data sizes. Making that explicit seems like the better option for ECDSA to enable shorter scripts when pre-hashing is not needed, but also allow pre-hashed data when needed.

reardencode · 2024-01-14T19:22:41Z

Thanks for the comments @benthecarman !

Testing is minimal, but so is the code.

Some code and ideas from Elements by stevenroose, and sanket1729 Porting help from moonsettler Tests added to the transaction tests framework.

1440000bytes · 2024-01-15T13:29:11Z

I think consensus label should be added for this pull request

reardencode · 2024-01-16T06:34:07Z

Converted this to a draft while I work through AJ's review on my similar PR to inquisition. Will port those changes over here and undraft when the time comes.

DrahtBot · 2024-01-31T03:59:25Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20473421681}

DrahtBot · 2024-03-05T17:07:24Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the CI failed label Jan 7, 2024

jonatack reviewed Jan 7, 2024

View reviewed changes

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

moonsettler reviewed Jan 7, 2024

View reviewed changes

reardencode force-pushed the lnhance branch 2 times, most recently from 3f01b52 to ec96ee5 Compare January 7, 2024 20:41

callebtc reviewed Jan 7, 2024

View reviewed changes

1440000bytes reviewed Jan 7, 2024

View reviewed changes

reardencode force-pushed the lnhance branch from ec96ee5 to 3262e68 Compare January 7, 2024 21:43

DrahtBot mentioned this pull request Jan 7, 2024

Compressed Bitcoin Transactions #29134

Closed

DrahtBot removed the CI failed label Jan 7, 2024

reardencode force-pushed the lnhance branch from 3262e68 to 2ec4e45 Compare January 8, 2024 19:27

Davidson-Souza mentioned this pull request Jan 8, 2024

Topicos Janeiro saopaulobitdevs/saopaulobitdevs.org#17

Closed

JeremyRubin and others added 7 commits January 14, 2024 06:59

[Refactor] Document and pack the fields in the PrecomputedData

7174699

[Anti-DoS Caching] No longer Cache CTV Hash at input index 0

3af8cc7

Make Bare OP_CHECKTEMPLATEVERIFY basic transactions standard

1c27aac

[TESTS] Add OP_CHECKTEMPLATEVERIFY functional tests

b85149d

[TESTS] add tx_valid.json tests for BIP-119 CheckTemplateVerify

9da43b9

[TESTS] Add tx_invalid.json examples for BIP-119 CheckTemplateVerify

46f0aed

[TESTS] Add CTV Hash Computation Unit Test & Mutation Tester

05d96e3

Co-authored-by: James O'Beirne <github@au92.org>

reardencode force-pushed the lnhance branch from 2ec4e45 to 2fa22d7 Compare January 14, 2024 15:00

DrahtBot removed the CI failed label Jan 14, 2024

reardencode mentioned this pull request Jan 14, 2024

LNHANCE mutinynet (CHECKSIGFROMSTACK and INTERNALKEY) benthecarman/bitcoin#2

Closed

benthecarman reviewed Jan 14, 2024

View reviewed changes

reardencode added 2 commits January 14, 2024 13:47

Add OP_INTERNALKEY for Tapscript

2edbf80

Testing is minimal, but so is the code.

Implement OP_CHECKSIGFROMSTACK(VERIFY)

e7670ab

Some code and ideas from Elements by stevenroose, and sanket1729 Porting help from moonsettler Tests added to the transaction tests framework.

reardencode force-pushed the lnhance branch from 2fa22d7 to e7670ab Compare January 14, 2024 22:03

DrahtBot mentioned this pull request Jan 15, 2024

Reenable OP_CAT #29247

Open

reardencode marked this pull request as draft January 16, 2024 06:33

bitcoin deleted a comment from BossMannKali Jan 16, 2024

This was referenced Jan 17, 2024

Add SigVersion parameter to IsOpSuccess() #29265

Closed

Implement OP_CHECKSIGFROMSTACK(VERIFY) #29270

Draft

Add OP_INTERNALKEY for Tapscript #29269

Draft

reardencode mentioned this pull request Jan 19, 2024

Implement OP_CHECKTEMPLATEVERIFY #29280

Draft

DrahtBot added the CI failed label Jan 31, 2024

DrahtBot mentioned this pull request Mar 4, 2024

kernel: chainparams updates for 27.x #29547

Merged

DrahtBot added the Needs rebase label Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE) #29198

OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE) #29198

reardencode commented Jan 7, 2024 •

edited

DrahtBot commented Jan 7, 2024 •

edited

michaelfolkson commented Jan 7, 2024

reardencode commented Jan 7, 2024

jonatack left a comment

moonsettler left a comment

callebtc Jan 7, 2024 •

edited

reardencode Jan 7, 2024

1440000bytes left a comment

jonatack commented Jan 7, 2024 •

edited

hsjoberg commented Jan 7, 2024

reardencode commented Jan 8, 2024

niteshbalusu11 commented Jan 9, 2024

Sjors commented Jan 9, 2024

1440000bytes commented Jan 9, 2024

michaelfolkson commented Jan 9, 2024

reardencode commented Jan 14, 2024

benthecarman Jan 14, 2024

benthecarman Jan 14, 2024

reardencode Jan 14, 2024

benthecarman Jan 14, 2024

reardencode Jan 14, 2024

benthecarman Jan 14, 2024

reardencode Jan 14, 2024

reardencode commented Jan 14, 2024

1440000bytes commented Jan 15, 2024

reardencode commented Jan 16, 2024

DrahtBot commented Jan 31, 2024

DrahtBot commented Mar 5, 2024

		// Pubkeys of length 33 are only constrained in legacy and segwitv0. In these script types only those beginning
		// with 0x02 or 0x03 are constrained. Others are unknown pubkey types.

OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE) #29198

Are you sure you want to change the base?

OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE) #29198

Conversation

reardencode commented Jan 7, 2024 • edited

DrahtBot commented Jan 7, 2024 • edited

Code Coverage

Reviews

Conflicts

michaelfolkson commented Jan 7, 2024

reardencode commented Jan 7, 2024

jonatack left a comment

Choose a reason for hiding this comment

moonsettler left a comment

Choose a reason for hiding this comment

callebtc Jan 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1440000bytes left a comment

Choose a reason for hiding this comment

jonatack commented Jan 7, 2024 • edited

hsjoberg commented Jan 7, 2024

reardencode commented Jan 8, 2024

niteshbalusu11 commented Jan 9, 2024

Sjors commented Jan 9, 2024

1440000bytes commented Jan 9, 2024

michaelfolkson commented Jan 9, 2024

reardencode commented Jan 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reardencode commented Jan 14, 2024

1440000bytes commented Jan 15, 2024

reardencode commented Jan 16, 2024

DrahtBot commented Jan 31, 2024

DrahtBot commented Mar 5, 2024

reardencode commented Jan 7, 2024 •

edited

DrahtBot commented Jan 7, 2024 •

edited

callebtc Jan 7, 2024 •

edited

jonatack commented Jan 7, 2024 •

edited