Implement BIP-119 Validation (CheckTemplateVerify) #21702

JeremyRubin · 2021-04-15T17:36:54Z

This is an implementation of BIP-119 OP_CHECKTEMPLATEVERIFY. It has been rebased on top of Taproot / ST. The specification has been otherwise unrevised since publication nearly a year and half ago.

Minus tests and deployment, it's about 180 LOC.

This branch activates against regtest so you can test on your local node.

This PR includes standardness rules for a bare single CTV Hash script, which has applications for congestion control. Other than that there is limited support for bare scripts using CTV, although the patches to #16766 lay the groundwork for better wallet support with proper IsTrusted parents scanning, which can be amended later to handle chains of irrefutable transactions.

You can learn more about the design of BIP-119 at https://utxos.org/. In particular I recommend the workshop transcript and slides https://utxos.org/workshops/ for a thorough review of how CTV works.

You can stress test CTV for complex examples by trying out the Sapio compiler: learn.sapio-lang.org. There is also some support for CTV in miniscript via the Sapio Rust Miniscript Fork.

Discussion welcome at ##ctv-bip-review in Libera.

If you would like to connect to a signet, I operate one you can connect to with the parameters below.

[signet]
signetchallenge=512102946e8ba8eca597194e7ed90377d9bbebc5d17a9609ab3e35e706612ee882759351ae 
addnode=50.18.75.225

You may also wish to have the below parameters for things to work nicely:

server = 1
txindex=1
rpcport=18332
rpcworkqueue=1000
fallbackfee=0.0002
minrelaytxfee=0

DrahtBot · 2021-04-15T19:24:17Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	michaelfolkson, ghost, AmadeusK525, BitcoinErrorLog
Concept ACK	RobinLinus, jonatack, jaybny, ProofOfKeags, vincenzopalazzo, 1440000bytes, vicariousdrama, cryptoquick
Stale ACK	benthecarman, josediegorobles, Rspigler

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:

#24897 ([Draft / POC] Silent Payments by w0xlt)
#24737 (Remove taproot chain param by MarcoFalke)
#24595 (deploymentstatus: move g_versionbitscache global to ChainstateManager by ajtowns)
#24149 (Signing support for Miniscript Descriptors by darosior)
#22954 ([TESTS] Allow tx_invalid.json tests to include flag rules for if_unset: [A,B,C] then_unset: [D] by JeremyRubin)
#22793 (Simplify BaseSignatureChecker virtual functions and GenericTransactionSignatureChecker constructors by achow101)
#21283 (Implement BIP 370 PSBTv2 by achow101)
#19426 (refactor: Change * to & in MutableTransactionSignatureCreator by MarcoFalke)

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.

Coverage

Coverage	Change (pull 21702, `a4b9a55`)	Reference (master, `e2c4ac7`)
Lines	+0.0319 %	83.9750 %
Functions	+0.0211 %	75.8508 %
Branches	+0.0242 %	52.5419 %

^{Updated at: 2021-07-15T07:47:36.255429.}

src/script/interpreter.cpp

pyskell · 2021-04-20T12:15:19Z

src/script/interpreter.cpp

+        } else {
+            // otherwise we still have *most* of the hash cached,
+            // so just re-compute the correct one and compare
+            assert(txTo != nullptr);


Can this assert just be moved to the start of the function?

These asserts can technically just be deleted looking at the surrounded code, txTo will already be able to deref nullptr from many other functions.

I put them close to where the deref's actually occur as I think that's best practice, but yes they could go at the start.

I'd actually be more excited about changing the representation internally to a reference as follow up work so we get rid of all these unchecked derefs...

src/script/script.cpp

test/functional/feature_checktemplateverify.py

michaelfolkson · 2021-04-22T21:49:59Z

I think CTV is interesting and will likely be a candidate for a future soft fork whether that be a standalone CTV soft fork or a soft fork with a bundle of features. But at least for me I feel it is premature to even look at this with Taproot not yet activated and zero discussion (mere speculation) on the form of the next soft fork after Taproot has (hopefully) activated. I guess there is no harm in opening this but personally I feel it is extremely early. I look forward to those discussions on the next soft fork and digging into this at some point in the future though.

JeremyRubin · 2021-04-23T01:06:32Z

You're free to review or not review any PR you like whenever you like. No one is "in charge" of the roadmap for Bitcoin development. All changes -- soft forks or not -- proceed when they have rough consensus and the code is ready.

This PR is for code review of BIP-119 OP_CTV's implementation. If you wish to discuss Bitcoin Project management please take it to an appropriate venue such as the mailing list.

michaelfolkson · 2021-04-23T10:17:05Z

@JeremyRubin:

All changes -- soft forks or not -- proceed when they have rough consensus and the code is ready.

On this we are definitely in strong disagreement. A soft fork (or hard fork) requires overwhelming consensus not only in this repo but in the wider community. Taproot reached that bar (there was one NACK from a long term contributor due to quantum concerns I believe in this repo and overwhelming community support) and I expect the next soft fork, whatever it contains, to also reach that bar. To the extent that it appears you disagree with that worries me immensely. But indeed when Taproot has (hopefully) activated I will continue on the mailing list.

This has a hard Concept NACK from me until then. All long term contributors to this repository should have the chance to review this and there should be overwhelming consensus within this repo and in the wider community that this should be included in the next soft fork as is. In my opinion we are a long, long way away from that. And this is from someone who finds CTV extremely interesting and is looking forward (once Taproot has activated) to examining in greater detail the use cases of it (ie CTV and ANYPREVOUT's use in vaults).

JeremyRubin · 2021-04-23T12:17:17Z

Please, again, take the metaphysics to the mailing list. This is a PR for people who want to review BIP-119 CTV.

JeremyRubin · 2021-04-23T17:48:51Z

Apologies for the line noise -- I hit an issue with GH tracking the tip of my branch and had to use isaacs/github#361 (comment) workaround.

DrahtBot · 2021-05-03T09:32:08Z

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

RobinLinus · 2021-05-05T02:52:23Z

concept ACK. Covenants would open up lots of exciting new use cases.
What are the next steps?

src/script/interpreter.cpp

benthecarman · 2021-05-06T21:19:28Z

CR-ACK 491804c

besides the nit on spacing

Still need to look through tests

…n and disabling the exhaustive failure test

…t: [A,B,C] then_unset: [D]

…eep patchset diff for CTV minimized for 0.23

[TESTS] modify script_tests.json to enable OP_NOP4 as OP_CHECKTEMPLATEVERIFY

…ase of bisecting should downstream software expect to parse NOP4 this now fails)

…ly init BIP-119 hash data

…ency injecting sync primitives for laziness This is required given that the script interpreter translation unit cannot link pthread.

…tdeploymentinfo test

JeremyRubin · 2022-12-17T04:06:25Z

Rebased against v24.0 tag so that this branch remains mergeable/testable against a recent release, closing with up for grabs label if someone is motivated to work on keeping this compatible with future changes to the software.

DrahtBot added Consensus RPC/REST/ZMQ TX fees and policy Validation Wallet labels Apr 15, 2021

JeremyRubin force-pushed the checktemplateverify-rebase-4-15-21 branch from f52318d to 3b07e2f Compare April 15, 2021 21:41

This was referenced Apr 16, 2021

[tests] Reduced number of validations in tx_validationcache_tests #13533

Merged

Introduce deploymentstatus #19438

Merged

pyskell reviewed Apr 20, 2021

View reviewed changes

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

pyskell reviewed Apr 20, 2021

View reviewed changes

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

maflcko reviewed Apr 22, 2021

View reviewed changes

test/functional/feature_checktemplateverify.py Outdated Show resolved Hide resolved

maflcko reviewed Apr 22, 2021

View reviewed changes

test/functional/feature_checktemplateverify.py Outdated Show resolved Hide resolved

JeremyRubin closed this Apr 23, 2021

JeremyRubin reopened this Apr 23, 2021

JeremyRubin force-pushed the checktemplateverify-rebase-4-15-21 branch 3 times, most recently from aa00f64 to 39b0a50 Compare April 23, 2021 17:47

rn-g approved these changes May 5, 2021

View reviewed changes

benthecarman reviewed May 6, 2021

View reviewed changes

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

JeremyRubin mentioned this pull request May 17, 2021

[Doc] New Opcodes for tapscript ElementsProject/elements#949

Merged

JeremyRubin added 23 commits December 16, 2022 22:52

[TESTS] Update Transaction Tests to permit setting a flag as always o…

98f5313

…n and disabling the exhaustive failure test

[TESTS] Allow tx_invalid.json tests to include flag rules for if_unse…

e490756

…t: [A,B,C] then_unset: [D]

[Backport Compatibility] Shim for block_index to be named pindex to k…

484c8f6

…eep patchset diff for CTV minimized for 0.23

Add StandardTemplateHash definition

42c760c

Add SignatureChecker method for checking DefaultCheckTemplateVerifyHash.

39f25fe

Add OP_CHECKTEMPLATEVERIFY Opcode as OP_NOP4

198cab2

[TESTS] modify script_tests.json to enable OP_NOP4 as OP_CHECKTEMPLATEVERIFY

Fixup: Make CTV Opcode use Span

1c9921b

Change printing of NOP4 to CheckTemplateVerify (separate commit for e…

13db26d

…ase of bisecting should downstream software expect to parse NOP4 this now fails)

[Anti-DoS Caching] Precompute the DefaultCheckTemplateVerifyHash

1ef57bc

[Refactor] Document and pack the fields in the PrecomputedData

9e5007e

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

a819cc8

[Anti-DoS Caching] Make PrecomputedTransactionData non-const and lazi…

06255ad

…ly init BIP-119 hash data

[Anti-DoS Caching] Refactor BIP-119 LazyInit to use lambda for depend…

e958000

…ency injecting sync primitives for laziness This is required given that the script interpreter translation unit cannot link pthread.

Make Bare OP_CHECKTEMPLATEVERIFY basic transactions standard

eda7015

OP_CHECKTEMPLATEVERIFY Deployment parameters

24b4283

[TESTS] Add OP_CHECKTEMPLATEVERIFY functional tests

203fc0e

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

afd26aa

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

78d1d72

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

57027d3

[TESTS] Fixup: CTV Hash Computation Tests patch lazy init

708e9e1

[TESTS] Fixup lazy injection

f186c21

FIXUP: [TEST] Rename status-next to status_next for rpc_blockchain ge…

02df478

…tdeploymentinfo test

Fixup: Always set cache synchronizer to f if not already set

ee7dda5

JeremyRubin force-pushed the checktemplateverify-rebase-4-15-21 branch from d4af88d to ee7dda5 Compare December 17, 2022 04:03

JeremyRubin added the Up for grabs label Dec 17, 2022

JeremyRubin closed this Dec 17, 2022

ajtowns mentioned this pull request Jan 16, 2023

Implement BIP 119 validation (OP_CHECKTEMPLATEVERIFY) bitcoin-inquisition/bitcoin#17

Merged

bitcoin locked and limited conversation to collaborators Dec 17, 2023

maflcko removed the Up for grabs label Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement BIP-119 Validation (CheckTemplateVerify) #21702

Implement BIP-119 Validation (CheckTemplateVerify) #21702

JeremyRubin commented Apr 15, 2021 •

edited

DrahtBot commented Apr 15, 2021 •

edited

pyskell Apr 20, 2021

JeremyRubin Apr 20, 2021

michaelfolkson commented Apr 22, 2021

JeremyRubin commented Apr 23, 2021

michaelfolkson commented Apr 23, 2021

JeremyRubin commented Apr 23, 2021

JeremyRubin commented Apr 23, 2021

DrahtBot commented May 3, 2021

RobinLinus commented May 5, 2021 •

edited

benthecarman commented May 6, 2021

JeremyRubin commented Dec 17, 2022

Implement BIP-119 Validation (CheckTemplateVerify) #21702

Implement BIP-119 Validation (CheckTemplateVerify) #21702

Conversation

JeremyRubin commented Apr 15, 2021 • edited

DrahtBot commented Apr 15, 2021 • edited

Reviews

Conflicts

Coverage

pyskell Apr 20, 2021

Choose a reason for hiding this comment

JeremyRubin Apr 20, 2021

Choose a reason for hiding this comment

michaelfolkson commented Apr 22, 2021

JeremyRubin commented Apr 23, 2021

michaelfolkson commented Apr 23, 2021

JeremyRubin commented Apr 23, 2021

JeremyRubin commented Apr 23, 2021

DrahtBot commented May 3, 2021

RobinLinus commented May 5, 2021 • edited

benthecarman commented May 6, 2021

JeremyRubin commented Dec 17, 2022

JeremyRubin commented Apr 15, 2021 •

edited

DrahtBot commented Apr 15, 2021 •

edited

RobinLinus commented May 5, 2021 •

edited