Implement BIP-119 Validation (CheckTemplateVerify)

JeremyRubin commented at 5:36 pm on April 15, 2021: contributor

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.

0[signet]
1signetchallenge=512102946e8ba8eca597194e7ed90377d9bbebc5d17a9609ab3e35e706612ee882759351ae 
2addnode=50.18.75.225

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

0server = 1
1txindex=1
2rpcport=18332
3rpcworkqueue=1000
4fallbackfee=0.0002
5minrelaytxfee=0

DrahtBot added the label Consensus on Apr 15, 2021

DrahtBot added the label RPC/REST/ZMQ on Apr 15, 2021

DrahtBot added the label TX fees and policy on Apr 15, 2021

DrahtBot added the label Validation on Apr 15, 2021

DrahtBot added the label Wallet on Apr 15, 2021

DrahtBot commented at 7:24 pm on April 15, 2021: contributor

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, a4b9a55240a0ab0c58f75e99398e56ceed8a3d35)	Reference (master, e2c4ac7cfb58e741)
Lines	+0.0319 %	83.9750 %
Functions	+0.0211 %	75.8508 %
Branches	+0.0242 %	52.5419 %

Updated at: 2021-07-15T07:47:36.255429.

JeremyRubin force-pushed on Apr 15, 2021

in src/script/interpreter.cpp:1520 in 3b07e2ff83 outdated

1512@@ -1433,6 +1513,9 @@ void PrecomputedTransactionData::Init(const T& txTo, std::vector<CTxOut>&& spent
1513     // Determine which precomputation-impacting features this transaction uses.
1514     bool uses_bip143_segwit = false;
1515     bool uses_bip341_taproot = false;
1516+    // TODO: Improve this heuristic
1517+    bool uses_bip119_ctv = true;

pyskell commented at 12:15 pm on April 20, 2021:

As the TODO says, should be improved

JeremyRubin commented at 4:34 pm on April 20, 2021:

yes; this became a TODO post taproot because we started doing much more aggressive/expensive heuristics, scanning to see if it’s a taproot spend. We used to just IIRC always compute all caches.

It’s not clear to me the performance benefit of scanning v.s. always computing the hashes @sipa. 50% of transactions are SegWit these days (https://charts.woobull.com/bitcoin-segwit-adoption/), and CTV only does extra non O(1) hashing in the event that it’s a transaction that uses non segwit inputs.

It’d be also possible to just never cache CTV, if the extra work seems not worth it. Or to only cache for single input txns, only cache for txns with a bare script standard CTV, only cache for segwit or has bare CTV, etc. It’s kinda hard to say what the optimal caching policy is and it’s bikesheddable + non consensus, so I chose to just leave it for now as the extra work is not terrible, and more advanced policies open up the potential for quadratic hashing bugs (e.g., txn has a scriptSig on every input except the CTV one and the CTV one is CTV CTV… and we don’t cache).

JeremyRubin commented at 3:24 am on April 21, 2021:

One last thing on caching: CTV was designed so that if you’re expecting to operate the same CTV at diff input indexes, you can cache the sha256 midstate because index is the final field. However, I think that it’s relatively rare you’d want CTV scripts at indexes other than 0.

mzumsande commented at 1:00 am on January 2, 2022:

It’d be also possible to just never cache CTV, if the extra work seems not worth it.

Not sure if I understand this correctly, but in the way it is done, it seems that the extra hashes that only CTV needs are precomputed for each transaction, but only used if the tx makes use of OP_CTV. On the other hand, if it is used, the caching saves us computing some hashes. Do you have an estimate for the global percentage of transactions making use of CTV needed for caching to be an efficiency net gain instead of a efficiency loss?

I’d guess that the percentage that would be observed in the wild would be a rather small number even in optimistic scenarios, because OP_CTV is something only needed for specialised use cases as outlined in the BIP?!

JeremyRubin commented at 1:08 am on January 3, 2022:

the main reason to cache the hashing it to prevent scripts like <H> CTV CTV CTV.... CTV from being able to cause N^2 hashing to occur. Fixed hashing (like if the input is at input one, not the ‘usual’ 0), of e.g. 80 bytes (idk offhand how long ctv digest is) is not an issue as it’s like script: <v> HASH256 HASH256 HASH256 HASH256..., but it is a problem if you have to hash an entire (big) tx.

hence the mention of quadratic hashing above. Caching is required for “safety”, not performance.

jamesob commented at 4:12 pm on January 21, 2022:

the main reason to cache the hashing it to prevent scripts like CTV CTV CTV…. CTV from being able to cause N^2 hashing to occur. @JeremyRubin what’s the reason that we can’t populate a cache “inline” while we’re doing script execution? That way we could both avoid having to precompute for transactions that don’t use CTV and avoid N^2 hashing. I assume there’s some limitation that prevented you from doing this in the first place.

JeremyRubin commented at 3:26 am on January 26, 2022:

This is the approach now taken in this PR!

in src/script/interpreter.cpp:1922 in 3b07e2ff83 outdated

1915+        if (nIn == 0) {
1916+            return std::equal(txdata->m_standard_template_single_hash.begin(), txdata->m_standard_template_single_hash.end(), hash.data());
1917+        } else {
1918+            // otherwise we still have *most* of the hash cached,
1919+            // so just re-compute the correct one and compare
1920+            assert(txTo != nullptr);

pyskell commented at 12:15 pm on April 20, 2021:

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

JeremyRubin commented at 4:40 pm on April 20, 2021:

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…

in src/script/script.cpp:201 in 3b07e2ff83 outdated

197@@ -198,6 +198,13 @@ unsigned int CScript::GetSigOpCount(const CScript& scriptSig) const
198     return subscript.GetSigOpCount(true);
199 }
200 
201+bool CScript::IsPayToBasicStandardTemplate() const

pyskell commented at 12:17 pm on April 20, 2021:

Minor bike shedding but “Basic Standard” and “Standard Template” are somewhat confusing terms. Are there non-standard templates?

JeremyRubin commented at 4:45 pm on April 20, 2021:

Yes… Perhaps “standard” and “basic” should be flipped terminology wise, but as is:

Standard is the 32 byte argument form interpreted as the StandardTemplateHash defined in this PR
Basic is the ScriptType that is just a script with a single <h:32> CTV.

CTV as a NOP is extensible in the future to support different templates via having a different length arg + whatever rules around that you want.

BasicStandardTemplate is just for enabling relaying of a single special cased barescript type (for congestion control uses). TBH it could be dropped from this PR and handled separately, although it makes testing more painful.

JeremyRubin commented at 5:59 pm on May 7, 2021:

discussion of this moved here https://github.com/bitcoin/bips/pull/1118

in test/functional/feature_checktemplateverify.py:74 in 3b07e2ff83 outdated

69+    return tx
70+
71+class CheckTemplateVerifyTest(BitcoinTestFramework):
72+    def set_test_params(self):
73+        self.num_nodes = 1
74+        self.extra_args = [['-whitelist=127.0.0.1', '-par=1']]  # Use only one script thread to get the exact reject reason for testing

maflcko commented at 6:31 pm on April 22, 2021:

please enumerate the exact permission flags needed. Whitelist is deprecated

JeremyRubin commented at 7:04 pm on April 22, 2021:

What’s the replacement – tbh don’t know if this is even needed anymore?

maflcko commented at 7:17 pm on April 22, 2021:

For example -whitelist=noban@ip

JeremyRubin commented at 2:02 am on April 23, 2021:

I removed it and it still runs fine… presumably this is helpful during testing if we’re trying to connect some sort of malicious peer, but I think I don’t need it here.

in test/functional/feature_checktemplateverify.py:126 in 3b07e2ff83 outdated

121+        self.coinbase_txids = [self.nodes[0].getblock(b)['tx'][0] for b in self.nodes[0].generate(BLOCKS)]
122+
123+        self.log.info("Creating setup transactions")
124+        outputs, script = random_real_outputs_and_script(10)
125+        # Add some fee satoshis
126+        amount = (sum(out.nValue for out in outputs)+200*500) /100e6

maflcko commented at 6:32 pm on April 22, 2021:

I know this is just the tests, but it would be nice to not use floating point

maflcko commented at 6:40 pm on April 22, 2021:

100e6 refers to the COIN constant?

JeremyRubin commented at 7:06 pm on April 22, 2021:

yeah I think it kinda has to be floating point because the RPCs use BTC and we have sats… if I used a non float I would truncate?

But I can refactor it to be COIN? Or maybe preserve the values as sats for longer and only convert to BTC in one place?

maflcko commented at 7:18 pm on April 22, 2021:

There is decimal.Decimal

michaelfolkson commented at 9:49 pm on April 22, 2021: contributor

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 commented at 1:06 am on April 23, 2021: contributor

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 commented at 10:17 am on April 23, 2021: contributor

@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 commented at 12:17 pm on April 23, 2021: contributor

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

JeremyRubin closed this on Apr 23, 2021

JeremyRubin reopened this on Apr 23, 2021

JeremyRubin force-pushed on Apr 23, 2021

JeremyRubin commented at 5:48 pm on April 23, 2021: contributor

Apologies for the line noise – I hit an issue with GH tracking the tip of my branch and had to use https://github.com/isaacs/github/issues/361#issuecomment-114300645 workaround.

DrahtBot commented at 9:32 am on May 3, 2021: contributor

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

rn-g approved

rn-g commented at 2:26 am on May 5, 2021: none

RobinLinus commented at 2:52 am on May 5, 2021: none

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

in src/script/interpreter.cpp:1690 in 39b0a505d5 outdated

1682@@ -1585,8 +1683,11 @@ bool SignatureHashSchnorr(uint256& hash_out, const ScriptExecutionData& execdata
1683 
1684     hash_out = ss.GetSHA256();
1685     return true;
1686+
1687 }
1688 
1689+
1690+

benthecarman commented at 9:07 pm on May 6, 2021:

nit: unneeded spacing

benthecarman commented at 9:19 pm on May 6, 2021: contributor

CR-ACK 491804c589c5f3e68dc17516377930e18986cb19

besides the nit on spacing

Still need to look through tests

in src/script/interpreter.cpp:1460 in 39b0a505d5 outdated

1452@@ -1416,9 +1453,52 @@ uint256 GetSpentScriptsSHA256(const std::vector<CTxOut>& outputs_spent)
1453     return ss.GetSHA256();
1454 }
1455 
1456+/* Not Exported, just convenience */
1457+template<typename TxType>
1458+uint256 GetStandardTemplateHashWithScript(const TxType& tx, const uint256& outputs_hash, const uint256& sequences_hash,
1459+                                const uint256& scriptSig_hash, const uint32_t input_index) {
1460+    auto h =  CHashWriter(SER_GETHASH, 0)

benthecarman commented at 3:12 am on May 18, 2021:

piconit: 2 spaces after the = here

in src/script/interpreter.cpp:1475 in 39b0a505d5 outdated

1470+}
1471+
1472+template<typename TxType>
1473+uint256 GetStandardTemplateHashEmptyScript(const TxType& tx, const uint256& outputs_hash, const uint256& sequences_hash,
1474+                                const uint32_t input_index) {
1475+    auto h =  CHashWriter(SER_GETHASH, 0)

benthecarman commented at 3:12 am on May 18, 2021:

same here

in src/script/interpreter.cpp:1548 in 39b0a505d5 outdated

1550+            m_scriptSigs_single_hash = uint256{};
1551+            // TODO: Cache midstate?
1552+            m_standard_template_single_hash = GetStandardTemplateHashEmptyScript(txTo, m_outputs_single_hash, m_sequences_single_hash, 0);
1553+        } else {
1554+            m_scriptSigs_single_hash = GetScriptSigsSHA256(txTo);
1555+            m_standard_template_single_hash = GetStandardTemplateHashWithScript(txTo, m_outputs_single_hash, m_sequences_single_hash, m_scriptSigs_single_hash, 0);

benthecarman commented at 3:23 am on May 18, 2021:

Would it be safer to set input_index here to uint32_t’s max value? That way if there is less of a chance that we miss in a test the handling of a changing input_index since most likely the tested input_index will be 0

benthecarman commented at 3:27 am on May 18, 2021:

We do lose the effiency of always having the 0th input cached however

JeremyRubin commented at 2:00 pm on May 18, 2021:

yeah the whole point is caching the most common case – we could also cache the midstate without index and just finalize with an index in consensus.

rxgrant commented at 3:24 am on May 27, 2021: none

@RobinLinus

What are the next steps?

I think the taproot process is an exemplar to build off of, for all future upgrades.

Documentation, review workshops, tutorial media, and gradually building press exposure all attract devs who want to understand and review it well. Without review workshops, it’s too easy to fall into a bad habit of gauging consensus by noticing bellwether reviewers. Tutorial media make it easier to see the consequences of new technology and get excited enough to put effort in.

There are several things that we want to know from reviewers of new features:

How can this concept hurt those in Bitcoin who do not upgrade at all?
How can this concept hurt those in Bitcoin who validate blocks with the feature, but who do not use it for their transactions?
Does this look like clean and correct code?
Do the tests prove that the code is correct?
Do people who willingly experiment with this have clear boundaries to keep their other funds safe if the feature has a bug?
How is this feature important to keep Bitcoin relevant?
Why can’t this need be completely served in other layers? (eg. L2/L3)
Are there other approaches to solve this need in Bitcoin’s consensus layer?
What makes this pull request the simplest and safest way to do this feature in Bitcoin’s consensus layer?

For all upgrades, I’d eventually like to see a semi-standardized table of consequences for ecosystem participants, similar to the hazmat fire diamond.

https://en.wikipedia.org/wiki/NFPA_704

For OP_CTV, best-practice reviews might work through a toy problem on either regtest or the mainnet (using the available signing servers).

Here are my answers on the questions above, as they relate to this pull request:

How can this concept hurt those in Bitcoin who do not upgrade at all?

If the code has bugs, it could introduce a security flaw. However it does not even introduce a new address type, so it is very safe.
How can this concept hurt those in Bitcoin who validate blocks with the feature, but who do not use it for their transactions?

Again, if the code has bugs, it could introduce a security flaw.
Does this look like clean and correct code?

I’d like to test the code more before giving my answer here. It looks like clean code.
Do the tests prove that the code is correct?

The external compiler with the Sapio language is generating a very wide variety of tests. I think this offers high confidence.
Do people who willingly experiment with this have clear boundaries to keep their other funds safe if the feature has a bug?

Two answers here: yes and no.

Yes, in that it’s easy to not lock UTXOs into OP_CTV trees.

No, in that someone could pay your address in an OP_CTV tree of transactions, without your consent. Your software might not be able to recognize longer chains of CPFP dependencies, or might consider unconfirmed transactions below a confirmed root as weaker than they really are, depending on the branching structure of the OP_CTV tree. I would consider both of these failures as small disruptions, since the transactions are a few confirmations away from appearing normal in any wallet, but we should test whether any wallets don’t show them at all until the leaf transactions are confirmed.
How is this feature important to keep Bitcoin relevant?

It enables a broad class of smart contracts, including: unchangeable efficient batching of exchange payouts; better custody control using vaults; and even decentralization improvements to mining pool payouts.
Why can’t this need be completely served in other layers? (eg. L2/L3)

Using the L2 Lightning Network as a solution require a counterparty who is willing to enter into an HTLC. Using the L2 Liquid Network requires trusting the federated sidechain. “L3” solutions such as RGB require a counterparty who is consensually running the same client version, to define the asset tip.

OP_CTV works with script that you can apply to create your own addresses, adding no additional counterparty risk over Bitcoin’s proof-of-work model. This makes it the only way to do batched payouts and advanced custody vaults for L1 on-chain UTXOs, without requiring fancy trust-pushing tricks like deleting keys or using external signing oracles.
Are there other approaches to solve this need in Bitcoin’s consensus layer?

Simplicity is one, but there is no current plan to use it on Bitcoin’s consensus layer.
What makes this pull request the simplest and safest way to do this feature in Bitcoin’s consensus layer?

It has remained remarkably stable throughout the entire Taproot rollout, which indicates it’s a mature concept. It is only a few lines of consensus code. The testing infrastructure (which includes a compiler outside of bitcoind) already includes very different styles of contracts.

(edit: wrapping)

RobinLinus commented at 10:15 pm on May 27, 2021: none

How can this concept hurt those in Bitcoin who do not upgrade at all?

Is it relevant to mention here that it’s possible to activate OP_CTV with a softfork because it could overwrite some OP_NOOP?

How is this feature important to keep Bitcoin relevant? It enables a broad class of smart contracts, including: unchangeable efficient batching of exchange payouts; better custody control using vaults; and even decentralization improvements to mining pool payouts.

Maybe it’s worth mentioning that it enables permissionless sidechain consensus mechanisms such as Spacechains and Stakechains.

JeremyRubin commented at 5:51 pm on May 30, 2021: contributor

I really don’t have the answers for what the pipeline of accepting soft forks to Bitcoin should look like. I think these sorts of conversations are better suited for the mailing list or for an IRC chat room (although given recent IRC system disruptions IDK how logging is functioning), and GitHub is better suited to discuss the technical merits of the code.

that said, short of a perfect venue for this discussion, and to respond to some of the above:

It seems to me that there’s a bit of an Abilene Paradox where people (whether CTV, anyprevout, sigagg, or something else) all don’t know what the process could/should be or what would be ok and we settle on something that (most everyone?) doesn’t want, which is arbitrary delay because it seems that’s what other people want. It’s not that delay is not appropriate at all, but the arbitrariness that is confusing. A further complaint I’ve seen raised is around the notion of “nextness”. That “CTV should be next”, “anyprevout next”, “sigagg next”, etc, implying that Bitcoin is inherently a serial development process. I think that it’s natural to have multiple development fronts that can proceed independently (to the extent they are independent) and do things as they are “ready” rather than some sort of order enforced by “ has to be next”. We should have our fingers in more pies when it comes to securing the future of the bitcoin network.

I’ll reiterate: I don’t have the answers on what the pipeline should look like, so I can only do a best effort job at being a sherpa through the review process for changes I think convey a large benefit to Bitcoin users. If anyone feels that I’ve misjudged or made an error in advocacy, I’m happy to accept the criticism and improve.

The main action item I’m seeing from @rxgrant is some easy to understand risk/reward section for upgrades (I think that’s a great idea!) and to get some more reviewers to try solving some problems with CTV/Sapio. I think that’s doable. I’m working on more resources for builders, but the sapio book is a decent place to start if you’re motivated.

rxgrant commented at 0:11 am on May 31, 2021: none

Are there other approaches to solve this need in Bitcoin’s consensus layer?

Answers here should also reference SIGHASH_ANYPREVOUTANYSCRIPT and OP_CAT as discussed in bip119. SIGHASH_ANYPREVOUT helps LN state updates require less stored state, and OP_CTV does complex contracts in fewer bytes (including settling comlpex LN HTLCs using many UTXOs in fewer bytes onchain), so both of them are good ideas.

in src/chainparams.cpp:96 in 39b0a505d5 outdated

90@@ -91,6 +91,13 @@ class CMainParams : public CChainParams {
91         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = 1628640000; // August 11th, 2021
92         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 709632; // Approximately November 12th, 2021
93 
94+        // Deployment of CTV (BIP 119)
95+        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].bit = 5;
96+        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].nStartTime =Consensus::BIP9Deployment::NEVER_ACTIVE;

rxgrant commented at 0:18 am on May 31, 2021:

0        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].nStartTime = Consensus::BIP9Deployment::NEVER_ACTIVE;

in src/chainparams.cpp:222 in 39b0a505d5 outdated

216@@ -210,6 +217,12 @@ class CTestNetParams : public CChainParams {
217         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = 1628640000; // August 11th, 2021
218         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay
219 
220+        // Deployment of CTV (BIP 119)
221+        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].bit = 5;
222+        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].nStartTime =Consensus::BIP9Deployment::NEVER_ACTIVE;

rxgrant commented at 0:18 am on May 31, 2021:

0        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].nStartTime = Consensus::BIP9Deployment::NEVER_ACTIVE;

in src/chainparams.cpp:365 in 39b0a505d5 outdated

359@@ -347,6 +360,12 @@ class SigNetParams : public CChainParams {
360         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
361         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay
362 
363+        // Deployment of CTV (BIP 119)
364+        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].bit = 5;
365+        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].nStartTime =Consensus::BIP9Deployment::NEVER_ACTIVE;

rxgrant commented at 0:18 am on May 31, 2021:

0        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].nStartTime = Consensus::BIP9Deployment::NEVER_ACTIVE;

in src/chainparams.cpp:439 in 39b0a505d5 outdated

433@@ -415,6 +434,12 @@ class CRegTestParams : public CChainParams {
434         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
435         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay
436 
437+        // Deployment of CTV (BIP 119)
438+        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].bit = 5;
439+        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].nStartTime =Consensus::BIP9Deployment::ALWAYS_ACTIVE;

rxgrant commented at 0:19 am on May 31, 2021:

0        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].nStartTime = Consensus::BIP9Deployment::ALWAYS_ACTIVE;

DrahtBot added the label Needs rebase on Jun 17, 2021

Rspigler commented at 4:30 am on June 21, 2021: contributor

Strong Concept ACK

Edit: I should note that the implementation details are above my paygrade, and I am ACK’ing the use-case improvements, with the condition that it passes review and gains consensus

JeremyRubin force-pushed on Jun 26, 2021

DrahtBot removed the label Needs rebase on Jun 26, 2021

JeremyRubin commented at 5:03 am on June 26, 2021: contributor

Rebased to fix conflicts, whitespace issues fixed, and squashed the fixups to the tests.

DrahtBot added the label Needs rebase on Jul 1, 2021

JeremyRubin force-pushed on Jul 6, 2021

DrahtBot removed the label Needs rebase on Jul 6, 2021

JeremyRubin force-pushed on Jul 6, 2021

JeremyRubin force-pushed on Jul 7, 2021

JeremyRubin commented at 6:30 am on July 7, 2021: contributor

rebased! @ajtowns might you be able to check commit 0141eb4d7d8c35088287353b92813575ce9b79a7 since I had some non obvious rebasing to do following https://github.com/bitcoin/bitcoin/pull/19438

in src/script/interpreter.cpp:604 in b04c35c850 outdated

621+                {
622+                    // if flags not enabled; treat as a NOP4
623+                    if (!(flags & SCRIPT_VERIFY_STANDARD_TEMPLATE)) break;
624+
625+                    if (stack.size() < 1)
626+                        return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION);

benthecarman commented at 7:43 pm on July 12, 2021:

If I comment out these 2 lines feature_checktemplateverify.py still passes

JeremyRubin commented at 8:30 pm on July 12, 2021:

good catch, I should test empty stack.

JeremyRubin commented at 6:09 am on July 13, 2021:

now covered :)

in src/script/interpreter.cpp:621 in b04c35c850 outdated

635+                        default:
636+                            // future upgrade can add semantics for this opcode with different length args
637+                            // so discourage use when applicable
638+                            if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS) {
639+                                return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS);
640+                            }

benthecarman commented at 7:45 pm on July 12, 2021:

replacing this with a break has feature_checktemplateverify.py still passing

JeremyRubin commented at 8:29 pm on July 12, 2021:

good catch, I should pass in some non-32 byte values to check that the txns are rejected.

JeremyRubin commented at 6:08 am on July 13, 2021:

Now covered :)

in src/script/interpreter.cpp:1520 in b04c35c850 outdated

1509@@ -1430,6 +1510,8 @@ void PrecomputedTransactionData::Init(const T& txTo, std::vector<CTxOut>&& spent
1510         m_spent_outputs_ready = true;
1511     }
1512 
1513+    // TODO: Improve this heuristic
1514+    bool uses_bip119_ctv = true;

benthecarman commented at 7:47 pm on July 12, 2021:

setting this to false still has feature_checktemplateverify.py passing

JeremyRubin commented at 8:28 pm on July 12, 2021:

This is to be expected, if uses_bip119_ctv is false, then the checker will just compute the values without the cache (unless taproot/143 flags passed – i suppose we could tighten when the cache gets made). If it is true, then the cache is always available. The cache being present or not shouldn’t break anything!

in src/script/interpreter.cpp:1548 in b04c35c850 outdated

1549+            m_scriptSigs_single_hash = uint256{};
1550+            // TODO: Cache midstate?
1551+            m_standard_template_single_hash = GetStandardTemplateHashEmptyScript(txTo, m_outputs_single_hash, m_sequences_single_hash, 0);
1552+        } else {
1553+            m_scriptSigs_single_hash = GetScriptSigsSHA256(txTo);
1554+            m_standard_template_single_hash = GetStandardTemplateHashWithScript(txTo, m_outputs_single_hash, m_sequences_single_hash, m_scriptSigs_single_hash, 0);

benthecarman commented at 7:51 pm on July 12, 2021:

changing the input_index param to 100 in either of these causes the tests to fail which seems concerncing to me as this is just a cache

JeremyRubin commented at 8:25 pm on July 12, 2021:

That’s to be expected – the cached precompute expects input_index == 0, https://github.com/bitcoin/bitcoin/blob/b04c35c8506160e95a965382956fa5b68d139b29/src/script/interpreter.cpp#L1910, so if you change what is cached to input_index == 100 the checker will use the wrong value.

in src/script/interpreter.cpp:1561 in b04c35c850 outdated

1551+            m_standard_template_single_hash = GetStandardTemplateHashEmptyScript(txTo, m_outputs_single_hash, m_sequences_single_hash, 0);
1552+        } else {
1553+            m_scriptSigs_single_hash = GetScriptSigsSHA256(txTo);
1554+            m_standard_template_single_hash = GetStandardTemplateHashWithScript(txTo, m_outputs_single_hash, m_sequences_single_hash, m_scriptSigs_single_hash, 0);
1555+        }
1556+        m_bip119_ctv_ready = true;

benthecarman commented at 7:52 pm on July 12, 2021:

changing this to false has feature_checktemplateverify still pass

JeremyRubin commented at 8:22 pm on July 12, 2021:

Yep, that’s normal.

If the cache is disabled (which making ready = false does), then the cache isn’t used.

mzumsande commented at 11:01 pm on January 2, 2022:

If the cache is disabled (which making ready = false does), then the cache isn’t used.

Just observing that this does not apply anymore in the current version, because Commit 4c823859fc2e46606d13eef8d21ce7aa1fe3edea (“Use HandleMissingData for unexpected missing precomputed data” ) introduces a strict dependence on the cached data, resulting in an assert if the data wasn’t cached.

JeremyRubin commented at 1:03 am on January 3, 2022:

great point and solid review note, was a part of rebasing this onto the new HandleMissingData check, which changed this stuff to errors to be missing across the codebase.

Note that it’s still possible the CTV hash is not cached, but has to be computed from the other cached parts.

JeremyRubin commented at 1:14 am on January 3, 2022:

btw if curious, #21330

it’s mostly for signing analysis, not consensus, that missing data paths were every supported. the referenced pr makes it a bit more clean.

benthecarman commented at 7:53 pm on July 12, 2021: contributor

Did some testing on how exhuastive tests are

JeremyRubin force-pushed on Jul 13, 2021

in src/script/interpreter.cpp:1924 in 9ec56bb911 outdated

1919+                GetStandardTemplateHashWithScript(*txTo, txdata->m_outputs_single_hash, txdata->m_sequences_single_hash,
1920+                        txdata->m_scriptSigs_single_hash, nIn);
1921+            return std::equal(hash_tmpl.begin(), hash_tmpl.end(), hash.data());
1922+        }
1923+    }
1924+    assert(txTo != nullptr);

maflcko commented at 8:38 am on July 13, 2021:

This is untested code: https://drahtbot.space/host_reports/DrahtBot/reports/coverage/bitcoin/bitcoin/21702/total.coverage/src/script/interpreter.cpp.gcov.html#1924

JeremyRubin commented at 3:05 pm on July 13, 2021:

Good point – trying to think about how to trigger no precomputed data available, other than from unit tests. Are their code paths where we do not precompute?

JeremyRubin commented at 3:06 pm on July 13, 2021:

(e.g., unit tests I can just call the function, but i’d love to test it on a full code path)

maflcko commented at 3:27 pm on July 13, 2021:

Not sure if there is a code path that can hit this in production. Have you seen https://github.com/bitcoin/bitcoin/pull/21330/files ?

JeremyRubin commented at 4:35 pm on July 13, 2021:

Yeah I noticed that, it’s great. According to the PR, Validation code passes ASSERT_FAIL (as at validation time all data should always be passed, and anything else is a serious bug in the code), while signing code uses FAIL..

However, for CTV were we ever to relax the heuristic (which is right now, for every transaction), we would want to be able to handle the case where the data is not available. But since that code isn’t written (and likely won’t be) it’s fine to have that case being a missing data. I’ll do it as a separate commit in case there’s a relaxed heuristic in the future.

JeremyRubin commented at 2:59 am on July 15, 2021:

btw I’m not sure if the patch that i made actually improve coverage much, given that I think the missingdata branch is still never hit…

maflcko commented at 8:39 am on July 13, 2021: member

Ran a coverage report in #21702 (comment) and noticed an untested line of code.

JeremyRubin force-pushed on Jul 13, 2021

DrahtBot added the label Needs rebase on Jul 20, 2021

JeremyRubin force-pushed on Jul 20, 2021

DrahtBot removed the label Needs rebase on Jul 20, 2021

DrahtBot added the label Needs rebase on Jul 24, 2021

bitcoin deleted a comment on Jul 24, 2021

JeremyRubin force-pushed on Aug 3, 2021

DrahtBot removed the label Needs rebase on Aug 3, 2021

JeremyRubin force-pushed on Aug 4, 2021

michaelfolkson commented at 1:02 pm on August 4, 2021: contributor

Edit: I should note that the implementation details are above my paygrade, and I am ACK’ing the use-case improvements, with the condition that it passes review and gains consensus @Rspigler: The implementation details are the most critical thing with this proposed consensus change. Everyone will ACK more use cases, that is the easy part. The hard part is reviewers (ideally more than a handful of people) coming to a view on whether the opcode’s current design is optimal as once it goes live there are no do overs. I get the inferiority complex (and I often share it :) ) but as reviewers we do need to try to do better.

For example, a good starting point seems to be understanding how the design of this opcode has evolved over the years and the current trade-offs of its design (real world implementation of its declared use cases probably answers this best). I like StackExchange but a Q&A anywhere with detailed technical answers would be good. I’d be happy to transfer answers to StackExchange.

e.g. https://bitcoin.stackexchange.com/questions/107891/how-has-the-design-of-the-opcode-op-checktemplateverify-evolved-over-its-various

I’ll repeat my view that the discussion on the form of the next soft fork (what is in it and when) seems to have barely started (for good reason imo with Taproot not yet activated and tonnes of work to do to leverage the benefits of Taproot) but I agree that can be left to the mailing list and this PR can focus on opcode design and code.

Rspigler commented at 7:00 pm on August 4, 2021: contributor

I agree (that the implementation details are the most critical). I disagree that everyone will ACK more use cases - there are actually a few soft fork ideas in the works right now that are controversial. I did plenty of research before Concept ACKing this, and I feel confident in my Concept ACK, but that’s all I can do, and I can’t review the code.

That being said, I agree strongly that implementation details are critical, and so is consensus; hence my conditional:

I am ACK’ing the use-case improvements, with the condition that it passes review and gains consensus

I agree that there is still plenty of discussion to be had

in src/chainparams.cpp:99 in b560a59b1d outdated

90@@ -91,6 +91,13 @@ class CMainParams : public CChainParams {
91         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = 1628640000; // August 11th, 2021
92         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 709632; // Approximately November 12th, 2021
93 
94+        // Deployment of CTV (BIP 119)

glozow commented at 10:51 am on August 6, 2021:

in b560a59b1db7ca8fcdca2ea7722cf7cdbb004320 “OP_CHECKTEMPLATEVERIFY Deployment parameters”

If you’re going to add deployment parameters, you’ll also want to update getblockchaininfo() RPC’s softforks result.

JeremyRubin commented at 9:30 pm on August 7, 2021:

will do – that might also be a new thing.

in src/script/interpreter.h:180 in 4c68aab5ee outdated

159@@ -160,9 +160,14 @@ struct PrecomputedTransactionData
160     uint256 m_outputs_single_hash;
161     uint256 m_spent_amounts_single_hash;
162     uint256 m_spent_scripts_single_hash;
163+    uint256 m_scriptSigs_single_hash;
164+    uint256 m_standard_template_single_hash;
165     //! Whether the 5 fields above are initialized.
166     bool m_bip341_taproot_ready = false;
167

glozow commented at 10:57 am on August 6, 2021:

You may want to move this down so as not to invalidate the comment on m_bip341_taproot_ready

0    //! Whether the 5 fields above are initialized.
1    bool m_bip341_taproot_ready = false;
2    uint256 m_scriptSigs_single_hash;
3    uint256 m_standard_template_single_hash;

JeremyRubin commented at 9:29 pm on August 7, 2021:

nack this wastes memory with packing, will update comment though

glozow commented at 6:20 am on August 9, 2021:

yeah that works too. could also just move the comment up

JeremyRubin commented at 2:37 am on September 2, 2021:

i’m adding a new commit which fully packs, reorders, and improves the docs here.

in test/functional/feature_checktemplateverify.py:83 in 92c34e2fa7 outdated

78+        self.extra_args = [['-par=1']]  # Use only one script thread to get the exact reject reason for testing
79+        self.setup_clean_chain = True
80+        self.rpc_timeout = 120
81+
82+    def skip_test_if_missing_module(self):
83+        self.skip_if_no_wallet()

glozow commented at 11:05 am on August 6, 2021:

This test shouldn’t depend on the wallet. Perhaps look into using MiniWallet? I’m sure create_transaction_to_script() could be modified to not require signrawtransactionwithwallet

glozow commented at 12:36 pm on August 6, 2021:

Also, since you’re doing some random script creation and anyonecanspends - you might find some helpful helpers in script_util.py

JeremyRubin commented at 9:28 pm on August 7, 2021:

will check; wrote tests ~2 years ago so might be some new stuff to use

glozow commented at 6:21 am on August 9, 2021:

yeah there’s definitely lots of new stuff to use :)

in test/functional/feature_checktemplateverify.py:352 in 04af84316a outdated

347+
348+        try:
349+            self.nodes[0].sendrawtransaction(check_template_verify_tx_empty_stack.serialize().hex(), 0)
350+            raise Exception("Expected to fail sendrawtransaction")
351+        except JSONRPCException as e:
352+            assert_equal(str(e), STACK_TOO_SHORT_ERROR + " (-26)")

glozow commented at 12:24 pm on August 6, 2021:

Note on all the try / excepts in 92c34e2fa753d1c0a6076446f62ab77c8d08c75a tests: We have utils for asserting RPC errors - this is cleaner and will print the log on a failure:

0        assert_raises_rpc_error(-26, STACK_TOO_SHORT_ERROR, self.nodes[0].sendrawtransaction, check_template_verify_tx_empty_stack.serialize().hex())

JeremyRubin commented at 9:28 pm on August 7, 2021:

hah i guess new since i wrote the tests 2 years ago?

in test/functional/feature_checktemplateverify.py:411 in 04af84316a outdated

406+        p2sh_check_template_verify_tx.nVersion = 2
407+        p2sh_check_template_verify_tx.vin = [CTxIn(p2sh_ctv_outpoint, CScript([script]))]
408+        p2sh_check_template_verify_tx.vout = outputs
409+
410+        try:
411+            self.nodes[0].sendrawtransaction(p2sh_check_template_verify_tx.serialize().hex(), 0)

glozow commented at 1:08 pm on August 6, 2021:

Note on all the sendrawtransactions in 92c34e2fa753d1c0a6076446f62ab77c8d08c75a tests:

Trying to efforts to isolate the OP_CTV logic as much as possible, setting maxfeerate=0 is an unnecessary override. You’re adding fees anyway (I tried removing them and it doesn’t seem to make a difference).

JeremyRubin commented at 9:28 pm on August 7, 2021:

will remove

in src/script/interpreter.cpp:1426 in 04af84316a outdated

1439+    CHashWriter ss(SER_GETHASH, 0);
1440+    for (const auto& in : txTo.vin) {
1441+        ss << in.scriptSig;
1442     }
1443     return ss.GetSHA256();
1444 }

glozow commented at 1:12 pm on August 6, 2021:

In 1d93783f712592d14a6fd792d0ab60f9a0f8e3ce Add StandardTemplateHash definition:

I expected to see some unit tests for this? e.g. in sighash_tests.cpp?

JeremyRubin commented at 6:04 am on September 12, 2021:

done

in src/test/transaction_tests.cpp:66 in 04af84316a outdated

62@@ -63,6 +63,7 @@ static std::map<std::string, unsigned int> mapFlagNames = {
63     {std::string("DISCOURAGE_UPGRADABLE_PUBKEYTYPE"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE},
64     {std::string("DISCOURAGE_OP_SUCCESS"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS},
65     {std::string("DISCOURAGE_UPGRADABLE_TAPROOT_VERSION"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION},
66+    {std::string("CHECKTEMPLATEVERIFY"), (unsigned int) SCRIPT_VERIFY_STANDARD_TEMPLATE},

glozow commented at 1:29 pm on August 6, 2021:

In 614d76f0d61c7ec85d223eb83b40b78ff07c5b23 Add OP_CHECKTEMPLATEVERIFY Opcode as OP_NOP4:

Similar reasoning, “CHECKTEMPLATEVERIFY” should correspond to SCRIPT_VERIFY_CHECKTEMPLATEVERIFY

Will you be adding OP_CTV tests here (transaction_tests, tx_invalid.json, tx_valid.json) btw?

JeremyRubin commented at 2:38 am on September 12, 2021:

added tx_invalid.json, tx_valid.json based on the python tests. @Christewart too since he asked for this a while back.

in src/test/data/script_tests.json:878 in 04af84316a outdated

874@@ -875,7 +875,6 @@
875 
876 ["Ensure 100% coverage of discouraged NOPS"],
877 ["1", "NOP1",  "P2SH,DISCOURAGE_UPGRADABLE_NOPS", "DISCOURAGE_UPGRADABLE_NOPS"],
878-["1", "NOP4",  "P2SH,DISCOURAGE_UPGRADABLE_NOPS", "DISCOURAGE_UPGRADABLE_NOPS"],

glozow commented at 2:05 pm on August 6, 2021:

614d76f0d61c7ec85d223eb83b40b78ff07c5b23 fails script_tests because NOP4 is still discouraged at that point, so you may want to squash it with 04af84316aa43a8fdc58b36811c07ee796ce0e39

JeremyRubin commented at 9:27 pm on August 7, 2021:

prefer squash or put before?

glozow commented at 6:21 am on August 9, 2021:

squash

glozow commented at 2:11 pm on August 6, 2021: member

I’ve left some comments, mostly on testing.

This is perhaps more feedback for BIP119, but it seems like there should be more explicit test vectors there to help verify that an implementation is correct. Otherwise it’s very difficult to implement OP_CTV for another client and verify that it matches this.

in src/script/interpreter.cpp:622 in 04af84316a outdated

615@@ -616,7 +616,32 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
616                     break;
617                 }
618 
619-                case OP_NOP1: case OP_NOP4: case OP_NOP5:
620+                case OP_CHECKTEMPLATEVERIFY:
621+                {
622+                    // if flags not enabled; treat as a NOP4
623+                    if (!(flags & SCRIPT_VERIFY_STANDARD_TEMPLATE)) break;

JeremyRubin commented at 5:56 pm on September 2, 2021:

If the flag is not enabled, we currently do not treat it as a NOP4 (e.g., discouraged). Will patch this shortly.

JeremyRubin commented at 6:00 pm on September 2, 2021:

referencing https://github.com/bitcoin/bitcoin/pull/6124/files it looks like it would be proper to discourage historically.

JeremyRubin force-pushed on Sep 2, 2021

JeremyRubin force-pushed on Sep 3, 2021

in src/script/interpreter.cpp:598 in 7b225f4b84 outdated

615@@ -616,7 +616,36 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
616                     break;
617                 }
618 
619-                case OP_NOP1: case OP_NOP4: case OP_NOP5:
620+                case OP_CHECKTEMPLATEVERIFY:
621+                {
622+                    // if flags not enabled; treat as a NOP4
623+                    if (!(flags & SCRIPT_VERIFY_DEFAULT_CHECK_TEMPLATE_VERIFY_HASH)) {
624+                        if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS)

glozow commented at 11:10 am on September 3, 2021:

Oh, I think I see what’s causing your error in #22865. You don’t need to fail on SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS now that you’ve repurposed this NOP.

JeremyRubin commented at 3:56 pm on September 3, 2021:

This is actually a new line I added – it’s required for correctness if this is to be merged/released before setting release params. #21702 (review)

However, it is not the reason there is a problem. It is also a problem later on where !=32 byte arguments are upgradable too.

JeremyRubin commented at 7:13 pm on September 10, 2021:

resolving as per #22865 (comment).

this approach, although mildly (temporarily) problematic, yields safer results until CTV is active.

JeremyRubin force-pushed on Sep 3, 2021

JeremyRubin force-pushed on Sep 4, 2021

JeremyRubin force-pushed on Sep 12, 2021

jonatack commented at 10:31 am on September 13, 2021: contributor

Concept ACK, debug build/tests clean/green, the code looks pretty good on first overlook, and the last push (git diff 4a457d4 118ff53) fixed the CI integer sanitizer failure.

jaybny commented at 7:15 pm on October 7, 2021: none

Concept ACK based on this being part of a bigger picture , and as a solid stand alone. will review details and code shortly.

ACK: reviewed implementation code, and deployment code, as a sanity check against objective and text of bip119. reviewed test code, compiled fork, ran tests, tested with regtest.

in src/chainparams.cpp:100 in 118ff53bab outdated

90@@ -91,6 +91,13 @@ class CMainParams : public CChainParams {
91         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].nTimeout = 1628640000; // August 11th, 2021
92         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 709632; // Approximately November 12th, 2021
93 
94+        // Deployment of CTV (BIP 119)
95+        consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY].bit = 5;

jaybny commented at 1:01 pm on October 11, 2021:

is there a reason why we use bit 5 and not 3?

JeremyRubin commented at 5:17 pm on October 11, 2021:

no reason, i just picked a bit that i didn’t think would conflict with anything else.

michaelfolkson commented at 1:28 pm on October 11, 2021: contributor

Posted this to the mailing list with my thoughts on how soft fork PRs should be treated and broader considerations before merging.

I am a long term Concept ACK on enabling covenant functionality for the vault use case when there is community consensus that it should be enabled. My personal view currently is I don’t yet feel comfortable that that functionality should definitely be enabled by OP_CTV at the expense of or in addition to alternative proposals although OP_CTV is clearly a strong candidate. Until there is community consensus that this should definitely be included in the next soft fork I’d rather this PR was marked as draft let alone marked as high priority. But I understand that this PR is a high priority for the PR author.

ProofOfKeags commented at 11:26 pm on November 8, 2021: none

Concept ACK.

This represents the least controversial change that could give us useful covenents in Bitcoin. The properties of non-recursion and strict enumerability should be enough to gain larger support and still give us the ability to create useful output constraints. While I generally favor something more general than what is presented here, they are not mutually exclusive and any risks incurred by users of OP_CTV are fairly easy to understand which should make it easier to come to consensus on. Further, this will broadly demonstrate the utility of covenants and will help illuminate the desirable properties in a more general solution.

I have not yet reviewed the specific code implementing it.

in src/script/interpreter.cpp:1412 in 544b117caa outdated

1401@@ -1402,6 +1402,18 @@ uint256 GetSpentAmountsSHA256(const std::vector<CTxOut>& outputs_spent)
1402     CHashWriter ss(SER_GETHASH, 0);
1403     for (const auto& txout : outputs_spent) {
1404         ss << txout.nValue;
1405+

mjdietzx commented at 2:41 pm on November 17, 2021:

nit: accidental newline added?

in src/script/interpreter.cpp:1438 in 544b117caa outdated

1427@@ -1416,9 +1428,52 @@ uint256 GetSpentScriptsSHA256(const std::vector<CTxOut>& outputs_spent)
1428     return ss.GetSHA256();
1429 }
1430 
1431+/* Not Exported, just convenience */

mjdietzx commented at 2:49 pm on November 17, 2021:

can be static instead of this comment?

in src/script/interpreter.cpp:1689 in 544b117caa outdated

1638@@ -1584,8 +1639,10 @@ bool SignatureHashSchnorr(uint256& hash_out, const ScriptExecutionData& execdata
1639 
1640     hash_out = ss.GetSHA256();
1641     return true;
1642+

mjdietzx commented at 2:55 pm on November 17, 2021:

nit: accidental newline added?

in src/script/interpreter.cpp:1692 in 544b117caa outdated

1638@@ -1584,8 +1639,10 @@ bool SignatureHashSchnorr(uint256& hash_out, const ScriptExecutionData& execdata
1639 
1640     hash_out = ss.GetSHA256();
1641     return true;
1642+
1643 }
1644 
1645+

mjdietzx commented at 2:55 pm on November 17, 2021:

nit: accidental newline added?

benthecarman commented at 5:32 am on January 3, 2022:

in commit 51f0bdd36328f1c9334ad455a56f7cb47d76abda

nit unneeded spacing

JeremyRubin commented at 5:55 am on January 3, 2022:

i think 1645 is needed, 1642 could probaby go

JeremyRubin commented at 4:36 am on January 26, 2022:

removing both; not sure why they were there to begin with.

in src/script/interpreter.cpp:1455 in 544b117caa outdated

1443+        << input_index;
1444+    return h.GetSHA256();
1445+}
1446+
1447+template<typename TxType>
1448+uint256 GetDefaultCheckTemplateVerifyHashEmptyScript(const TxType& tx, const uint256& outputs_hash, const uint256& sequences_hash,

mjdietzx commented at 2:59 pm on November 17, 2021:

can be static?

jaybny commented at 3:41 am on November 23, 2021: none

as an aside. importance of merging this bip, is that it shows that Bitcoin accepts solid scientific enhancements from legitimate programmers , even when other devs may have different ideas and paths for the same utility ++

DrahtBot added the label Needs rebase on Nov 25, 2021

maflcko commented at 7:13 pm on December 16, 2021: member

This has been needing rebase for several weeks now, so I’ve removed it from high-prio for now. (Can be added back after rebase)

JeremyRubin force-pushed on Dec 17, 2021

DrahtBot removed the label Needs rebase on Dec 17, 2021

JeremyRubin force-pushed on Dec 17, 2021

JeremyRubin commented at 2:38 am on December 18, 2021: contributor

rebased, please re-add. sorry i’ve been juggling a lot of priorities working on the blog series… with the hopes of attracting some more review!

in src/test/transaction_tests.cpp:332 in 0162cdd587 outdated

327+                            if (flag == mapFlagNames.end()) BOOST_ERROR("Unknown Flag: " << then_unset[j].get_str());
328+                            then_set_flags |= flag->second;
329+                        }
330+
331+                    }
332+                } catch (std::runtime_error e) {

mzumsande commented at 8:39 pm on January 1, 2022:

This gives me a compiler warning: catching polymorphic type ‘class std::runtime_error’ by value [-Wcatch-value=]

Suggest to use catch (const std::runtime_error&)

JeremyRubin commented at 0:55 am on January 3, 2022:

will fix in the base PRs for transaction_tests.cpp

ajtowns commented at 4:58 am on October 2, 2022:

This seems to only show up with gcc not clang for me.

in src/script/interpreter.cpp:1536 in b6d2ba82fd outdated

1537@@ -1536,11 +1538,25 @@ void PrecomputedTransactionData::Init(const T& txTo, std::vector<CTxOut>&& spent
1538         if (uses_bip341_taproot && uses_bip143_segwit) break; // No need to scan further if we already need all.
1539     }
1540 
1541-    if (uses_bip143_segwit || uses_bip341_taproot) {
1542+    if (uses_bip143_segwit || uses_bip341_taproot || uses_bip119_ctv) {
1543         // Computations shared between both sighash schemes.

mzumsande commented at 0:43 am on January 2, 2022:

It is no longer clear what “both” in the comment refers to.

JeremyRubin commented at 0:56 am on January 3, 2022:

maybe just nix this comment entirely?

mzumsande commented at 11:41 am on January 3, 2022:

Yeah, changing it to “all” doesn’t seem correct either.

As long as uses_bip119_ctv=true unconditionally, it’s the same behavior - but I think it would be more in the spirit of the existing code to move the calculation of m_scriptSigs_single_hash and m_standard_template_single_hash out of this conditional to a conditional only dependent on uses_bip119_ctv, as is done for segwit-specific or taproot-specific hashes.

JeremyRubin commented at 3:27 am on January 26, 2022:

the approach for this has been changed in follow up patches, so the comment can stay as is :)

in src/script/interpreter.cpp:1548 in b6d2ba82fd outdated

1544         m_prevouts_single_hash = GetPrevoutsSHA256(txTo);
1545         m_sequences_single_hash = GetSequencesSHA256(txTo);
1546         m_outputs_single_hash = GetOutputsSHA256(txTo);
1547+
1548+        bool skip_scriptSigs = std::find_if(txTo.vin.begin(), txTo.vin.end(),
1549+                [](const CTxIn& c) { return c.scriptSig != CScript(); }) == txTo.vin.end();

mzumsande commented at 10:56 pm on January 2, 2022:

Probably a matter of taste, but std::none_of would be an alternative I find easier to read (also applies to GetDefaultCheckTemplateVerifyHash() ).

JeremyRubin commented at 1:00 am on January 3, 2022:

that’s probably fine, yes. If i touch it, will change.

cpp ref says:

0template< class InputIt, class UnaryPredicate >
1constexpr bool none_of(InputIt first, InputIt last, UnaryPredicate p)
2{
3    return std::find_if(first, last, p) == last;
4}

benthecarman commented at 5:38 am on January 3, 2022:

this is also done in 51f0bdd36328f1c9334ad455a56f7cb47d76abda, should probably be made a function

JeremyRubin commented at 5:56 am on January 3, 2022:

hmm yeah. see #21702 (review) this might be movable to a test since HandleMissing gets rid of one of the uses except for testing.

JeremyRubin commented at 4:39 am on January 26, 2022:

opted for all_of and empty.

This should be the same.

JeremyRubin commented at 6:12 am on January 26, 2022:

done

in src/script/interpreter.cpp:1547 in b6d2ba82fd outdated

1543         // Computations shared between both sighash schemes.
1544         m_prevouts_single_hash = GetPrevoutsSHA256(txTo);
1545         m_sequences_single_hash = GetSequencesSHA256(txTo);
1546         m_outputs_single_hash = GetOutputsSHA256(txTo);
1547+
1548+        bool skip_scriptSigs = std::find_if(txTo.vin.begin(), txTo.vin.end(),

mzumsande commented at 11:12 pm on January 2, 2022:

This section duplicates large parts of the logic from GetDefaultCheckTemplateVerifyHash(). Would it be possible to have this logic in one place? Also, after making reliance on the cache obligatory in a later commit, it seems that GetDefaultCheckTemplateVerifyHash is only used in a test anymore (ctvhash_tests), which in my opinion at least deserves a comment.

JeremyRubin commented at 1:04 am on January 3, 2022:

this is a good point. I guess it’s a question of if it’s better to have the functionality available as a part of the API, or if it can be moved to a test.

JeremyRubin commented at 4:24 am on January 11, 2022:

note that using the cache is mandatory, and using the non-cached version of GetDefaultCheckTemplateVerifyHash from e.g. script is insecure.

mzumsande commented at 11:32 pm on January 2, 2022: contributor

No final opinion on the concept yet (still trying to understand the implications better) and I’m also not too experienced with script, but I reviewed how the BIP is implemented - a few minor points below.

JeremyRubin commented at 1:11 am on January 3, 2022: contributor

Thank you Martin for your review and careful eye. I think there are a couple good cleanups that I will take care of either here or in follow up (if there’s no other reason to touch those files perhaps).

if you did review the test modifications #22954 and #22876 are standalone.

in src/script/interpreter.cpp:1948 in 479b0c9c16 outdated

1859@@ -1860,6 +1860,15 @@ bool GenericTransactionSignatureChecker<T>::CheckSequence(const CScriptNum& nSeq
1860     return true;
1861 }
1862 
1863+template <class T>
1864+bool GenericTransactionSignatureChecker<T>::CheckDefaultCheckTemplateVerifyHash(const std::vector<unsigned char>& hash) const

benthecarman commented at 5:36 am on January 3, 2022:

any reason to not use a Span here?

JeremyRubin commented at 5:50 am on January 3, 2022:

i think it didn’t exist when i coded this haha. a span would be fine if i touch it.

JeremyRubin commented at 6:12 am on January 26, 2022:

doing this one – I’m not confident I like it so I’ll leave it as a patch until you ACK it

benthecarman commented at 5:44 am on January 3, 2022: contributor

Couple nits

in test/functional/feature_checktemplateverify.py:38 in 3109df5616 outdated

11+from test_framework.script import CScript, OP_TRUE, OP_DEPTH, OP_ENDIF, OP_IF, OP_CHECKTEMPLATEVERIFY, OP_FALSE, OP_DROP, taproot_construct
12+from test_framework.script_util import script_to_p2sh_script
13+from test_framework.key import ECKey, compute_xonly_pubkey
14+from test_framework.test_framework import BitcoinTestFramework
15+from test_framework.util import assert_equal, assert_raises_rpc_error
16+from test_framework.wallet import MiniWallet, MiniWalletMode

DuncanBetts commented at 2:38 pm on January 3, 2022:

 0from test_framework.blocktools import (
 1    create_coinbase,
 2    create_block,
 3    add_witness_commitment,
 4)
 5from test_framework.messages import (
 6    CTransaction,
 7    CTxOut,
 8    CTxIn,
 9    CTxInWitness,
10    COutPoint,
11    COIN,
12    sha256,
13)
14from test_framework.p2p import P2PInterface
15from test_framework.script import (
16    CScript,
17    OP_TRUE,
18    OP_DEPTH,
19    OP_ENDIF,
20    OP_IF,
21    OP_CHECKTEMPLATEVERIFY,
22    OP_FALSE,
23    OP_DROP,
24    taproot_construct,
25)
26from test_framework.script_util import script_to_p2sh_script
27from test_framework.key import (
28    ECKey,
29    compute_xonly_pubkey,
30)
31from test_framework.test_framework import BitcoinTestFramework
32from test_framework.util import (
33    assert_equal,
34    assert_raises_rpc_error,
35)
36from test_framework.wallet import (
37    MiniWallet,
38    MiniWalletMode,
39)

I prefer this style as it’s harder to later accidentally subtly amend an import later. Sorting alphabetically may also be worthwhile, I haven’t done here to make my suggestion easier to review! Would also match style of feature_taproot.py.

JeremyRubin commented at 4:51 am on January 26, 2022:

#21702 (review)

in test/functional/feature_checktemplateverify.py:42 in 3109df5616 outdated

37+def random_real_outputs_and_script(n, nIn=0, nVin=1, vin_override=None):
38+    outputs = [CTxOut((x+1)*1000, random_p2sh()) for x in range(n)]
39+    script  = CScript([template_hash_for_outputs(outputs, nIn, nVin, vin_override), OP_CHECKTEMPLATEVERIFY])
40+    return outputs, script
41+def random_secure_tree(depth):
42+    leaf_nodes = [CTxOut(100, CScript(bytes([0, 0x14]) + random_bytes(20))) for x in range(2**depth)]

DuncanBetts commented at 4:30 pm on January 3, 2022:

Using a for loop instead of list comprehension here may slow down the test suite slightly, but might be worth it for enhanced comprehension of anyone reading/maintaining these tests later?

0    leaf_nodes = []
1    for x in range(2**depth):
2        nValue = 100
3        scriptPubKey = CScript(bytes([0, 0x14]) + random_bytes(20))
4        node = CTxOut(nValue, scriptPubKey)
5        leaf_nodes.append(node)

Also, my understanding is bytes([0, 0x14]) + random_bytes(20) means OP_FALSE and push 20 random bytes onto the stack?

The wiki specifies OP_FALSE as:

“An empty array of bytes is pushed onto the stack. (This is not a no-op: an item is added to the stack.)`

The 20 bytes seem something other than empty, but my understanding is very basic; I’ve just now looked at the Wiki to understand what this line of code means. Clarification welcome :).

DuncanBetts commented at 4:35 pm on January 3, 2022:

Alternative suggestion, based on style from https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_taproot.py

0    leaf_nodes = [CTxOut(nValue=100, scriptPubKey=CScript(bytes([0, 0x14]) + random_bytes(20))) for x in range(2**depth)]

JeremyRubin commented at 5:04 am on January 26, 2022:

fine

JeremyRubin commented at 5:05 am on January 26, 2022:

if i recall I just wanted something unique and non-spendable… but it’s probably worth documenting better.

in test/functional/feature_checktemplateverify.py:45 in 3109df5616 outdated

40+    return outputs, script
41+def random_secure_tree(depth):
42+    leaf_nodes = [CTxOut(100, CScript(bytes([0, 0x14]) + random_bytes(20))) for x in range(2**depth)]
43+    outputs_tree = [[CTxOut()]*(2**i) for i in range(depth)] + [leaf_nodes]
44+    for d in range(1, depth+2):
45+        idxs =zip(range(0, len(outputs_tree[-d]),2), range(1, len(outputs_tree[-d]), 2))

DuncanBetts commented at 4:36 pm on January 3, 2022:

0        idxs = zip(range(0, len(outputs_tree[-d]),2), range(1, len(outputs_tree[-d]), 2))

Formatting nit.

JeremyRubin commented at 4:50 am on January 26, 2022:

#21702 (review)

DuncanBetts changes_requested

DuncanBetts commented at 4:41 pm on January 3, 2022: none

I’ve barely scratched the surface and intend to do further review of the functional tests, but am submitting now to make my (minor) suggestions visible.

in src/policy/policy.cpp:209 in b08fd3717d outdated

188@@ -189,6 +189,11 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
189             if (subscript.GetSigOpCount(true) > MAX_P2SH_SIGOPS) {
190                 return false;
191             }
192+        } else if (whichType == TxoutType::TX_BARE_DEFAULT_CHECK_TEMPLATE_VERIFY_HASH) {
193+            // after activation, only allow bare with no scriptsig.
194+            // pre-activation disallowing enforced via discouraged logic in the
195+            // interpreter.
196+            if (tx.vin[i].scriptSig.size() != 0) return false;

mzumsande commented at 11:56 am on January 4, 2022:

The standardness of bare basic OP_CTV transactions should probably be covered by a test - whatever I return for TxoutType::TX_BARE_DEFAULT_CHECK_TEMPLATE_VERIFY_HASH, tests don’t fail.

JeremyRubin commented at 5:23 pm on January 4, 2022:

interesting let me look into this one. we very well might be missing a test vector (in tx_invalid.json) for this case.

to be clear, if the template matches on the script, but the scriptSig is nonzero it’d be nonstandard.

meta: does it make sense to just get rid of TxoutType::TX_BARE_DEFAULT_CHECK_TEMPLATE_VERIFY_HASH? We don’t strictly need it in this PR, except to ensure that we are able to test CTV in bare script. That should still be testable to a degree even without a standard type…

aureleoules commented at 7:05 am on September 23, 2022:

0            if (!tx.vin[i].scriptSig.empty()) return false;

aureleoules commented at 2:55 pm on October 3, 2022:

This can be tested with this unit test.

 0diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
 1index 3f9b54075..9b9f2f443 100644
 2--- a/src/test/transaction_tests.cpp
 3+++ b/src/test/transaction_tests.cpp
 4@@ -1018,6 +1018,27 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
 5         t.vout[0].nValue = 239;
 6         CheckIsNotStandard(t, "dust");
 7     }
 8+
 9+    // Check Bare CTV standardness
10+    CMutableTransaction txFromCTV;
11+    txFromCTV.vout.resize(1);
12+    auto dummyHash = GetRandHash();
13+    txFromCTV.vout[0].scriptPubKey = CScript() << ToByteVector(dummyHash) << OP_CHECKTEMPLATEVERIFY;
14+    txFromCTV.vout[0].nValue = 1000;
15+    AddCoins(coins, CTransaction(txFromCTV), 0);
16+
17+    CMutableTransaction txToCTV;
18+    txToCTV.vout.resize(1);
19+    txToCTV.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey()));
20+    txToCTV.vout[0].nValue = 1000;
21+    txToCTV.vin.resize(1);
22+    txToCTV.vin[0].prevout.n = 0;
23+    txToCTV.vin[0].prevout.hash = txFromCTV.GetHash();
24+    BOOST_CHECK(::AreInputsStandard(CTransaction(txToCTV), coins));
25+
26+    // scriptSig must be empty
27+    txToCTV.vin[0].scriptSig = CScript() << OP_0 << ToByteVector(dummyHash);
28+    BOOST_CHECK(!::AreInputsStandard(CTransaction(txToCTV), coins));
29 }
30 
31 BOOST_AUTO_TEST_SUITE_END()

in test/functional/feature_checktemplateverify.py:149 in 3109df5616 outdated

144+        b = bytes(b"Deterministic Privkey")
145+        while not tapkey.is_valid:
146+            b = sha256(b)
147+            tapkey.set(b, False)
148+        xonly, negated = compute_xonly_pubkey(tapkey.get_bytes())
149+        taproot = taproot_construct(xonly, [("only-path", script, 0xc0)])

DuncanBetts commented at 3:02 pm on January 4, 2022:

Nit: readability / variable naming only:

0        priv_key = ECKey()
1        while not priv_key.is_valid:
2            # use simple deterministic private key (k=1)
3            priv_key.set((1).to_bytes(32, 'big'), False)
4        pubkey, _ = compute_xonly_pubkey(priv_key.get_bytes())
5        taproot = taproot_construct(pubkey, [("only-path", script, 0xc0)])

Rename tapkey to priv_key in-line with test/functional/test_framework/wallet.py#L87-L89 and re-use method of generating deterministic private key with aim of easing refactoring/maintenance of the codebase later.
Rename xonly, negated to pubkey, _ in-line with test/functional/test_framework/key.py#L519, test/functional/test_framework/script.py#L874

JeremyRubin commented at 5:12 am on January 26, 2022:

donezo

josediegorobles approved

josediegorobles commented at 9:09 pm on January 4, 2022: none

ACK 782a1b6, I built, ran tests, tested manually by doing basic operations with compiled version and reviewed the code and as far as i can tell it’s OK, I agree it can be merged.

in test/functional/feature_checktemplateverify.py:209 in 3109df5616 outdated

204+
205+        self.log.info("Creating funding txn for spend at position 2 with 2 non-null scriptsigs")
206+        bare_ctv_specific_scriptSigs_position_2_funding_tx = create_transaction_to_script(self.nodes[0], wallet, get_coinbase(),
207+                script_specific_scriptSigs_position_2, amount_sats=amount_specific_scriptSigs_position_2)
208+
209+        self.log.info("Creating funding txns for some anyone can sepnd outputs")

DuncanBetts commented at 11:52 am on January 5, 2022:

Nit: Minor log message text typo.

0        self.log.info("Creating funding txns for some anyone can spend outputs")

JeremyRubin commented at 5:13 am on January 26, 2022:

hodl! done

in test/functional/feature_checktemplateverify.py:176 in 3109df5616 outdated

171+        self.log.info("Creating script for tree size depth 4")
172+        # Small Tree for test speed, can be set to a large value like 16 (i.e., 65K txns)
173+        TREE_SIZE = 4
174+        congestion_tree_txo = random_secure_tree(TREE_SIZE)
175+
176+        self.log.info("Creating funding txn for tree size depth 4")

DuncanBetts commented at 1:47 pm on January 5, 2022:

Nit: This to me feels easier to read and maintain.

0        # Small tree size 4 for test speed, can be set to a large value like 16 (i.e., 65K txns)
1        TREE_SIZE = 4
2        self.log.info("Creating script for tree size depth %d", TREE_SIZE)
3        congestion_tree_txo = random_secure_tree(TREE_SIZE)
4
5        self.log.info("Creating funding txn for tree size depth %d", TREE SIZE)

Further, TREE_SIZE could be exposed as configurable through the test framework config file mentioned in test/functional/test_framework/test_framework.py#L181-L182, though I’m not sure there’s any precedent.

JeremyRubin commented at 5:16 am on January 26, 2022:

done – not going to bother with configurable thing, i dont think it helps with this particular param

in test/functional/feature_checktemplateverify.py:157 in 3109df5616 outdated

152+
153+        self.log.info("Creating funding txn for 10 random outputs as a segwit script")
154+        segwit_ctv_funding_tx = create_transaction_to_script(self.nodes[0], wallet, get_coinbase(),
155+                CScript([0, sha256(script)]), amount_sats=amount_sats)
156+
157+        self.log.info("Creating a CTV with an non-32 byte stack segwit script")

DuncanBetts commented at 2:12 pm on January 5, 2022:

Nit.

0        self.log.info("Creating a CTV with a non 32 byte stack segwit script")

JeremyRubin commented at 5:17 am on January 26, 2022:

done

in test/functional/feature_checktemplateverify.py:226 in 3109df5616 outdated

221+                       , anyone_can_spend_funding_tx
222+                       , bare_ctv_tree_funding_tx
223+                       , bare_ctv_position_2_funding_tx
224+                       , bare_anyone_can_spend_funding_tx
225+                       , bare_ctv_specific_scriptSigs_funding_tx
226+                       , bare_ctv_specific_scriptSigs_position_2_funding_tx]

DuncanBetts commented at 3:23 pm on January 5, 2022:

Nit: Re-order to match the order funding txs were created in, to improve readability/simplify later refactor.

 0        funding_txs = [  taproot_ctv_funding_tx
 1                       , segwit_ctv_funding_tx
 2                       , segwit_ctv_funding_tx_wrongsize_stack
 3                       , segwit_ctv_funding_tx_empty_stack
 4                       , p2sh_ctv_funding_tx
 5                       , bare_ctv_tree_funding_tx
 6                       , bare_ctv_position_2_funding_tx
 7                       , bare_ctv_specific_scriptSigs_funding_tx
 8                       , bare_ctv_specific_scriptSigs_position_2_funding_tx
 9                       , anyone_can_spend_funding_tx
10                       , bare_anyone_can_spend_funding_tx]

JeremyRubin commented at 5:22 am on January 26, 2022:

im not sure i get what this (and the other variable order request) are accomplishing? what is the problem?

in test/functional/feature_checktemplateverify.py:236 in 3109df5616 outdated

231+        segwit_ctv_wrongsize_stack_outpoint,\
232+        segwit_ctv_empty_stack_outpoint,\
233+        p2sh_ctv_outpoint,\
234+        anyone_can_spend_outpoint,\
235+        bare_ctv_tree_outpoint,\
236+        bare_ctv_position_2,\

DuncanBetts commented at 3:26 pm on January 5, 2022:

Nit: Rename for consistency.

0        bare_ctv_position_2_outpoint,\

The order these variables are declared could also be amended to match the order they are created in.

bare_ctv_position_2_outpoint used in test:

0        self.log.info("Testing bare OP_CHECKTEMPLATEVERIFY with CTV at position 2")
1        check_template_verify_tx_pos_2 = CTransaction()
2        check_template_verify_tx_pos_2.nVersion = 2
3        check_template_verify_tx_pos_2.vin = [CTxIn(bare_ctv_position_2_outpoint)]
4        check_template_verify_tx_pos_2.vout = outputs_position_2

JeremyRubin commented at 5:20 am on January 26, 2022:

renamed!

in test/functional/feature_checktemplateverify.py:61 in 3109df5616 outdated

56+        output that is being spent, and the node must not be running
57+        multiple wallets.
58+    """
59+    random_address = script_to_p2sh(CScript())
60+    output = wallet.get_utxo(txid=txid)
61+    rawtx = node.createrawtransaction(inputs=[{"txid": output["txid"], "vout": output["vout"]}], outputs={random_address: Decimal(amount_sats) / COIN})

DuncanBetts commented at 3:30 pm on January 5, 2022:

0    rawtx = node.createrawtransaction(
1        inputs=[{"txid": output["txid"], "vout": output["vout"]}],
2        outputs={random_address: Decimal(amount_sats) / COIN}
3    )

Improve readability of long line.

JeremyRubin commented at 4:49 am on January 26, 2022:

im just going to use black to autoformat for all these.

DuncanBetts commented at 11:38 am on January 7, 2022: none

A few more nits for feature_checktemplateverify.py.

michaelfolkson commented at 3:36 am on January 18, 2022: contributor

Still a hard Concept NACK from me. Others have also voiced their opposition to attempting a soft fork activation for OP_CTV in the near term on the bitcoin-dev mailing list and elsewhere.

On a number of the bars (community consensus, building out of use cases, comparison of OP_CTV to alternative proposals, longer term analysis on the impact of enabling covenants on Bitcoin with OP_CTV) I and others would like to see met before considering a soft fork activation attempt OP_CTV currently falls well short.

Merging this would move us onto a discussion of how a contentious soft fork could be activated which I think the vast majority of us would want to avoid.

ghost commented at 4:20 am on January 18, 2022: none

I had experimented with Sapio Studio in last few days. Have been reading most of the things related to OP_CTV and covenants in general.

Strong Concept ACK and good to see lot of people supporting improvements in Bitcoin. Will review code, test etc. in next few weeks.

@michaelfolkson Your Concept NACK with your rationale has been registered. If you have further questions or discussion points please take them elsewhere (mailing list etc). You have to let other reviewers review the PR and make their own mind up.

in src/script/interpreter.cpp:1518 in b6d2ba82fd outdated

1513@@ -1514,6 +1514,8 @@ void PrecomputedTransactionData::Init(const T& txTo, std::vector<CTxOut>&& spent
1514         m_spent_outputs_ready = true;
1515     }
1516 
1517+    // TODO: Improve this heuristic
1518+    bool uses_bip119_ctv = true;

jamesob commented at 3:53 pm on January 21, 2022:

https://github.com/bitcoin/bitcoin/pull/21702/commits/b6d2ba82fd55ce83aac1f8d910781a3166abb9b9

I’ve benchmarked a fork of this branch that buries activation (https://github.com/jamesob/bitcoin/commit/9dad64880e19e2e785355c4e28388f89cce6c621) and am seeing a ~3% slowdown, presumably because of precomputing the hashes here for all transactions. In the past we’ve preferred to bury prior softforks for a variety of reasons, so it should be noted that were CTV to be deployed, burying the deployment may not be as palatable.

As others (including Jeremy) have noted, it’s possible that we could use heuristics to scan redeem scripts (or equivalent) for OP_CTV (OP_4) in order to avoid unnecessary hashing for transactions that don’t use CTV, but I’m curious as to whether such a scan would save much time relative to just precomputing the hashes as done here. It probably merits testing.

I’d like to hear someone explicate on why we can’t “just-in-time” these hashes in the same way that we do for, e.g., segwit: https://github.com/jamesob/bitcoin/blob/9dad64880e19e2e785355c4e28388f89cce6c621/src/script/interpreter.cpp#L1708-L1710. I.e. why is N^2 hashing not a problem for the code linked here, but is a problem for CTV? I assume CTV’s preimage is larger (the whole tx) and that has something to do with it.

JeremyRubin commented at 5:11 pm on January 21, 2022:

it might be non-obvious but the cache i actually required for validation, and optional for transaction signing purposes.

IMO it would make sense to eventually have the logic more clear that caches aren’t merely a convenience.

jamesob commented at 5:30 pm on January 21, 2022:

But my question is: why is the cache apparently not required for the segwit code I link to there? And when you say, “required,” you just mean to prevent quadratic hashing, right?

JeremyRubin commented at 1:54 am on January 24, 2022:

it is required. E.g., we could (and maybe should) add an assert for something like:

0if (!evaulting_for_transaction_signing) assert(cacheReady)

But it’s not “required” strictly speaking because the code is designed to be usable from both tx signing logic and verification. This is perhaps bad API design and these could be separated into a consensus and non consensus version. honestly… it might even be an issue that we expose the signing logic to DoS (but you just… DoS yourself trying to sign a PSBT?) if we don’t cache this stuff.

in src/wallet/scriptpubkeyman.cpp:203 in 3109df5616 outdated

200@@ -201,6 +201,11 @@ IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& s
201         }
202         break;
203     }
204+    case TxoutType::TX_BARE_DEFAULT_CHECK_TEMPLATE_VERIFY_HASH:
205+    {
206+        ret = IsMineResult::NO;

jamesob commented at 4:22 pm on January 21, 2022:

Is this a placeholder? Maybe this merits commentary about why we’d never consider a bare CTV hash as owned by a wallet (i.e. do we expect that only transactions using this output might be IsMine?).

JeremyRubin commented at 5:17 pm on January 21, 2022:

This is simply the no-behavior-change from without TX_BARE_DEFAULT_CHECK_TEMPLATE_VERIFY_HASH, since before TX_NONSTANDARD was set to IsMine::No, overridable from the watchonly.

The future logic will not be to mark things as IsMine, but to use the IsTrusted recrusive scanning #16766 with a patch to treat bare CTV as trusted (since the TXID must be stable)

jamesob commented at 4:40 pm on January 21, 2022: member

I’ve benchmarked a version of this branch (https://github.com/jamesob/bitcoin/commit/9dad64880e19e2e785355c4e28388f89cce6c621) that buries the CTV deployment, and have seen 2-3% slowdowns for IBD. @JeremyRubin makes the good point that this region of the chain doesn’t see many segwit spends, and so maybe the performance impact in a more contemporary region of the chain may be less (since hash precomputations may be triggered anyway for segwit spends). In any case, I think this is interesting information.

As noted in some inline comments, there may be ways to mitigate this slowdown. Even in the worst case I don’t necessarily look at 2-3% as a dealbreaker if the utility of CTV is hefty, since we have changes under consideration that could more than compensate for this (https://github.com/bitcoin/bitcoin/pull/22702#issuecomment-899545273), but it’s something to be aware of.

Follow-ups may include benchmarking the CTV hash precomputations in isolation (i.e. separating the prehash conditional away from segwit/taproot), and looking at a more relevant region of the chain. But I wanted a quick indication of worst-case behavior to start with.

bench name	command
ibd.local.range.500000.555000	`bitcoind -dbcache=8000 -debug=coindb -debug=bench -listen=0 -connect=0 -addnode=127.0.0.1:8888 -prune=9999999 -printtoconsole=0 -assumevalid=0`

bench/ctv vs. $mergebase (absolute)

bench name	x	bench/ctv	$mergebase
ibd.local.range.500000.555000.total_secs	3	3849.4126 (± 5.7962)	3772.9628 (± 4.3862)
ibd.local.range.500000.555000.peak_rss_KiB	3	6838850.6667 (± 1846.5401)	6852993.3333 (± 6070.4278)
ibd.local.range.500000.555000.cpu_kernel_secs	3	405.0433 (± 0.6728)	436.4533 (± 1.5544)
ibd.local.range.500000.555000.cpu_user_secs	3	39332.0667 (± 11.5050)	38876.8367 (± 20.8530)

bench/ctv vs. $mergebase (relative)

bench name	x	bench/ctv	$mergebase
ibd.local.range.500000.555000.total_secs	3	1.020	1.000
ibd.local.range.500000.555000.peak_rss_KiB	3	1.000	1.002
ibd.local.range.500000.555000.cpu_kernel_secs	3	1.000	1.078
ibd.local.range.500000.555000.cpu_user_secs	3	1.012	1.000

JeremyRubin commented at 5:36 pm on January 21, 2022: contributor

Thank you @jamesob for the detailed review and testing! I’ve prepared two patches and pushed them here (That could be squashed or left separate from the original caching patches) that:

Do not pre-compute or cache the 1st input CTV hash
Cache-on-first-use the scriptSigs hash/scan using std::call_once

1 is safe to do because the amount of work caused is the same as if the CTV input were to always be at the 2nd output. The negative impact is that if CTV becomes very popular, this approach will be potentially a little slower, but when CTV is not yet used such behavior motivated optimizations are premature. Scripts with repeated CTV could have a separate optimization to cache the CTV hash locally, but because it is also less work than a e.g. repeated OP_SHA256 might cause to be done, such optimizations do not really confer a benefit on the actual worst case scripts.

2 is safe because because we shift the CTV computations to still only be once per transaction, but lazily so. The downside of this approach is that it introduces the atomic std::call_once into the interpreter (which means that parallel CScripts with CTV for the same txn might contend). However, std::call_once is heavily optimized and the number of waiters on CScripts is bounded by the number of CheckQueue threads. Subsequent (after first) call_onces are able to cheaply read the modified state with full synchronicity. The downside of this approach is that less is precomputed, making an average CTV txn slower. If CTV is particularly popular in the future we can add back in the precomputation. A further downside is that future changes will have to be careful to rearchitect the call_once function should they want to on the fly cache state as multiple different modifiers might be unsafe (making PrecomputedTransactionData non-const is also a minor downside).

I believe this is the simplest approach to addressing your feedback and makes the overhead for non-ctv transactions essentially zero, while still permitting caching so that the worst case validation costs of many CTVs are still total O(N) (this N to O(N) mapping is used and not CTV is O(1) because the first call will be O(N) for hashing).

JeremyRubin force-pushed on Jan 21, 2022

JeremyRubin commented at 1:16 am on January 22, 2022: contributor

I have added patches to make the cache on first use strategy managed through a lambda “dependency injection” so as to permit the interpreter to be build for environments without access to threading. This makes thread safety the responsibility of the caller.

Lambda for caching, which can use call_once (used from CheckInputsScripts) requires pthread
Force option precomputes it always (threadsafe and usable without pthread, but sometimes you cache more than you’ll need)
No lambda + no force gives you a default cache on first use (not thread safe, does not require any sync primitves).
You can make a PrecomputedData with a lambda, or pass to init, but if you do both we assert since overriding is a logic error

Under no circumstance (unless you provide a lambda that does not invoke the proper call) is this DoS unsafe, however.

It might be possible to e.g. spinlock on an atomic as the default, and possible that atomic might be available even where call_once is not, but that strategy might be better handled if desired in other ways (e.g., platform dependent call_once shim).

This is a bit more complex than I’d like it to be, but the approach seems technically sound. I will squash the patches down after conceptual review on this approach by some of @jamesob / @sipa / @laanwj as followup on our conversation in IRC today.

DrahtBot added the label Needs rebase on Jan 25, 2022

JeremyRubin force-pushed on Jan 26, 2022

DrahtBot removed the label Needs rebase on Jan 26, 2022

JeremyRubin force-pushed on Jan 26, 2022

JeremyRubin commented at 9:02 am on January 26, 2022: contributor

I think I’ve addressed all outstanding feedback!

I’ve kept a copy of the prior revision here https://github.com/JeremyRubin/bitcoin/tree/checktemplateverify-rebase-1-25-22-pre.

DrahtBot added the label Needs rebase on Jan 28, 2022

in src/test/ctvhash_tests.cpp:38 in 489172c353 outdated

33+        UniValue test = tests[idx];
34+        std::string strTest = test.write();
35+        // comment
36+        if (test.isStr())
37+            continue;
38+        else if (test.isObject()) {

PastaPastaPasta commented at 1:57 pm on February 1, 2022:

please use brackets here (maybe just run clang-format)

JeremyRubin commented at 5:45 am on February 3, 2022:

I ran clang format it doesn’t modify this line?

in src/test/transaction_tests.cpp:68 in 489172c353 outdated

63@@ -64,6 +64,7 @@ static std::map<std::string, unsigned int> mapFlagNames = {
64     {std::string("DISCOURAGE_UPGRADABLE_PUBKEYTYPE"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE},
65     {std::string("DISCOURAGE_OP_SUCCESS"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS},
66     {std::string("DISCOURAGE_UPGRADABLE_TAPROOT_VERSION"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION},
67+    {std::string("DEFAULT_CHECK_TEMPLATE_VERIFY_HASH"), (unsigned int) SCRIPT_VERIFY_DEFAULT_CHECK_TEMPLATE_VERIFY_HASH},

PastaPastaPasta commented at 2:00 pm on February 1, 2022:

please remove this c-style cast, use a functional cast instead (maybe could remove the other c-style casts above this as well)

JeremyRubin commented at 5:46 am on February 3, 2022:

as per the other pr, this can be handled as separate work.

in src/test/transaction_tests.cpp:273 in 489172c353 outdated

274                     BOOST_ERROR("Tx unexpectedly failed with flag " << name << " unset: " << strTest);
275                 }
276                 // Removing random combinations of flags
277-                flags = TrimFlags(~(verify_flags | (unsigned int)InsecureRandBits(mapFlagNames.size())));
278-                if (!CheckTxScripts(tx, mapprevOutScriptPubKeys, mapprevOutValues, flags, txdata, strTest, /*expect_valid=*/true)) {
279+                flags = TrimFlags(extra_verify_flags | ~(verify_flags | (unsigned int)InsecureRandBits(mapFlagNames.size())));

PastaPastaPasta commented at 2:02 pm on February 1, 2022:

please remove this c-style cast

JeremyRubin commented at 5:46 am on February 3, 2022:

as per the other pr, this can be handled as separate work.

jamesob commented at 2:48 pm on February 1, 2022: member

I’ve rebenched the bench/ctv branch on a more contemporary region of the chain (https://github.com/chaincodelabs/bitcoinperf/pull/65) and the slowdown is now negligible.

bench name	command
ibd.local.range.667200.700000	`bitcoind -dbcache=8000 -debug=coindb -debug=bench -listen=0 -connect=0 -addnode=127.0.0.1:8888 -prune=9999999 -printtoconsole=0 -assumevalid=0`

bench/ctv vs. $mergebase (absolute)

bench name	x	bench/ctv	$mergebase
ibd.local.range.667200.700000.total_secs	3	3059.7889 (± 4.3977)	3052.3830 (± 10.1913)
ibd.local.range.667200.700000.peak_rss_KiB	3	4956666.6667 (± 795.2012)	4955257.3333 (± 2029.4717)
ibd.local.range.667200.700000.cpu_kernel_secs	3	311.7000 (± 2.8379)	311.5333 (± 0.6437)
ibd.local.range.667200.700000.cpu_user_secs	3	30756.6100 (± 24.4480)	30731.1800 (± 59.6497)

bench/ctv vs. $mergebase (relative)

bench name	x	bench/ctv	$mergebase
ibd.local.range.667200.700000.total_secs	3	1.002	1
ibd.local.range.667200.700000.peak_rss_KiB	3	1.000	1
ibd.local.range.667200.700000.cpu_kernel_secs	3	1.001	1
ibd.local.range.667200.700000.cpu_user_secs	3	1.001	1

JeremyRubin force-pushed on Feb 3, 2022

DrahtBot removed the label Needs rebase on Feb 3, 2022

JeremyRubin force-pushed on Feb 3, 2022

JeremyRubin commented at 7:20 pm on February 3, 2022: contributor

rebased for the getdeploymentinfo changes.

DrahtBot added the label Needs rebase on Feb 14, 2022

JeremyRubin force-pushed on Feb 17, 2022

JeremyRubin commented at 9:29 pm on February 17, 2022: contributor

rebased for the getdeploymentinfo changes again.

i’m also pinning a copy of the code at this point as https://github.com/JeremyRubin/bitcoin/tree/checktemplateverify-signet-23.0-alpha since master hit feature freeze before split off, will start working on having a concrete thing for people to run signet with.

JeremyRubin commented at 9:45 pm on February 17, 2022: contributor

I have verified syncability to this signet using https://github.com/JeremyRubin/bitcoin/tree/checktemplateverify-signet-23.0-alpha

0[signet]
1signetchallenge=512102946e8ba8eca597194e7ed90377d9bbebc5d17a9609ab3e35e706612ee882759351ae
2addnode=50.18.75.225

This should be operational. Let me know if there is any issues you experience (likely with signet itself, but CTV too).

DrahtBot removed the label Needs rebase on Feb 17, 2022

JeremyRubin force-pushed on Feb 28, 2022

JeremyRubin force-pushed on Mar 19, 2022

JeremyRubin commented at 3:32 pm on March 19, 2022: contributor

rebased onto release candidate here: https://github.com/JeremyRubin/bitcoin/tree/checktemplateverify-v23.0rc2, will do a rebase onto release when it is done.

I have rebased this branch onto master, which will now be divergent with the v23 release.

vincenzopalazzo commented at 10:05 pm on April 15, 2022: none

Concept ACK, but I need to drive in the BIP again!

JeremyRubin commented at 2:52 am on April 20, 2022: contributor

diagnosed the build fail as a silent conflict with another PR that was recently merged, will figure out the best strategy to patching (it’s just renaming a variable).

ghost commented at 6:25 pm on April 22, 2022: none

Concept ACK :checkered_flag: 🏁

in src/script/interpreter.cpp:619 in 00f8a091dc outdated

637+                            }
638+                            break;
639+                        default:
640+                            // future upgrade can add semantics for this opcode with different length args
641+                            // so discourage use when applicable
642+                            if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS) {

mjdietzx commented at 6:55 pm on April 22, 2022:

For this condition flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS we know flags is true so isn’t this logically redundant?

JeremyRubin commented at 1:58 am on May 12, 2022:

Nope – it’s checking if flags has discourage_upgradable_nops set, which we don’t know (single and vs double).

Rspigler commented at 7:25 pm on April 22, 2022: contributor

I am removing my concept ACK noted here (https://github.com/bitcoin/bitcoin/pull/21702#issuecomment-864718834).

I have serious concerns with the lack of consensus, and the lack of research around alternatives and how they interact with this proposal. A few posts by this PRs author (although interesting and good reads) does not count as consensus or research.

ghost commented at 8:53 pm on April 22, 2022: none

I have serious concerns with the lack of consensus, and the lack of research around alternatives and how they interact with this proposal.

Can we read these serious concerns? It would be helpful because I was considering you one of the contributors who uses qubes os, know security and privacy and do not blindly follow others. Maybe this might change my opinion and help others as well.

Your strong concept ack had conditions that it should pass review and consensus. CTV has a bug bounty and I expect/respect some bitcoin developers even without bounty to report bugs. Review cannot be better for this PR, consensus is something we cannot control and I can’t even discuss here. Maybe you should participate on mailing list if the moderators allow you.

mjdietzx commented at 10:24 pm on April 22, 2022: contributor

From the bip

If no scriptSigs are set in the transaction, there is no purpose in hashing the data or including it in the DefaultCheckTemplateVerifyHash, so we elide it. It is expected to be common that no scriptSigs will be set as segwit mandates that the scriptSig must be empty

Is there a good reason to not just always hash the scriptSigs, whether empty or not, all the same? The reason I ask is bc to me it seems simpler to have a single “code-path” here where all the inputs’ scriptSigs are hashed, regardless of if they are empty or not. Then you wouldn’t need both GetDefaultCheckTemplateVerifyHashWithScript & GetDefaultCheckTemplateVerifyHashEmptyScript, and the resulting code duplication (and all these light wrappers on top of those two functions - which just seems like extra/unnecessary code and indirection to me). I’d also expect you could then get rid of the NoScriptSigs helper and then get rid of all these conditionals based on that in GetDefaultCheckTemplateVerifyHash, BIP119EagerInit, CheckDefaultCheckTemplateVerifyHash.

There’s already a case where you end up hashing empty scriptSigs anyways, ie Mixed Script Transactions. With just a single legacy input, NoScriptSigs returns false and we end up hashing all the null scriptSigs (ie the transaction also spends one or more segwit input(s))

All that to say, at a conceptual level I don’t see why we don’t always just hash all the same data when forming the template hash. I would imagine this simplifies the code here (we get rid of all these conditionals), but this should also simplify things client side when these hashes in the first place in a similar manner.

Rspigler commented at 3:35 am on April 23, 2022: contributor

Can we read these serious concerns?

Like I stated, the lack of consensus, discussion, and research around alternatives. I don’t want to crowd this PR further, so please contact me elsewhere to discuss (https://robertspigler.com/contact).

Maybe you should participate on mailing list if the moderators allow you.

Reading the ML should demonstrate to you how controversial this is.

JeremyRubin commented at 3:39 am on April 23, 2022: contributor

@mjdietzx the reason for hashing separately is that if we ever add OP_CAT to Taproot via successX, the hashing of the scriptsig would make the scripts more complicated / use more space uneccesarily, so avoiding the commitment lets one write more efficient covenants (potentially) in the future.

mjdietzx commented at 11:39 pm on April 23, 2022: contributor

Thank you for the response @JeremyRubin. Now I have a bunch of questions/concerns regarding the additional caching in this PR, and it circles back to your response (which I don’t fully understand yet and need more time to learn/think about)

In 8b653b2442f634ec4753ee9fd0f6ac14b87c3227 you decide to not cache the CTV hash. Makes sense, it’s a nice simplification. You label the commit as “Experimental” - I take it that you have determined that this was an unnecessary optimization and does not affect DoS safety? If you did determine this, I’d love to see some benchmark on worst-case scripts to verify that this caching really has a material impact on DoS safety in the first place (ie pragmatic, not just big-O theoretical).

In some of the in-line code review comments for commit ffbbe6461dca54707aabf7b2a11851b72f99f4b6 concerning caching, you mention:

the main reason to cache the hashing it to prevent scripts like CTV CTV CTV…. CTV from being able to cause N^2 hashing to occur

Q2) IIUC correctly this is bc, without any caching, for each CTV in the script we’d first need to hash the scriptSigs (if any), and then the ctv hash. So for N CTVs that’s (Edit: 2N) ~N^2~ (worst-case). But this commit was before you removed caching the CTV hash in 8b653b2442f634ec4753ee9fd0f6ac14b87c3227, so IIUC worst-case is now N hashes for this script. So you made a (probably good) judgement call that O(N) is acceptable while O(N^2) is a DoS security risk, but is there any justification/proof you can give?

Q3) What is the justification for these refactor commits cab836ff0b20ab7444a5642c4433e8d2cf227302, e1b453ba6e66bb1f6dbd1d2f6152f2cef800fe30? I mean, at a high-level all you’re doing in the end is caching a single hash of the scriptSigs, why does this need to be so much more complicated than the existing caching for segwit and taproot? ie changing common function declarations all over the place and then needing PrecomputedTransactionData::BIP119LazyInit, PrecomputedTransactionData::BIP119EagerInit, m_bip119_cache_synchronizer (whatever this is?)

To me the way segwit and taproot do caching is much easier to understand and simple than how you do it here. Is it bc you don’t have a good heuristic to determine if a given tx uses_bip119_ctv (ie when caching scriptSigs is actually necessary for a given txTo)? Is there a fundamental reason why this can’t be heuristic based like segwit/taproot?

Related note: I am assuming that this performance degradation @jamesob benchmarked here has been “resolved” in these commits (to justify the additional complexity). Can you confirm what the result/takeaway is here?

Nit: in the PR description you say “Minus tests and deployment, it’s about 180 LOC.”, I’m guessing this was before these commits and needs to be updated?

All that said, this brings me to another conceptual question. In the bip as rationale for Committing to the ScriptSigs Hash:

0We commit to the hash rather than the values themselves as this is already precomputed for each transaction to optimize SIGHASH_ALL signatures.

Obviously this has not already been precomputed or we wouldn’t have to compute and cache this hash in the first place, right? Am I missing something? Because otherwise here is how I see it from a high-level:

You determined N hashes was acceptable from a DoS security perspective here 8b653b2442f634ec4753ee9fd0f6ac14b87c3227
So as long as we have the scriptSigs hash (whether cached or precomputed like the bip says) we do N hashes in the worst-case
And therefore if scriptSigs hash was precomputed, we wouldn’t even need any additional caching added in this PR.

On point 3) Why even hash the scriptSigs then? This is somewhat related to my previous question about having a single code-path for computing this scriptSig hash. Why not simplify it, only serialize non-empty scriptSigs (for the same reasons I mentioned in my first question, but as-related to DoS security and caching there would then be no extra hash to compute), and with no additional caching at all we are at the same worst-case N hashes for the CTVs in script. And I wonder, would that address your reasoning against my initial question:

the reason for hashing separately is that if we ever add OP_CAT to Taproot via successX, the hashing of the scriptsig would make the scripts more complicated / use more space uneccesarily, so avoiding the commitment lets one write more efficient covenants (potentially) in the future.

JeremyRubin commented at 7:30 am on April 24, 2022: contributor

Thank you for the response @JeremyRubin. Now I have a bunch of questions/concerns regarding the additional caching in this PR, and it circles back to your response (which I don’t fully understand yet and need more time to learn/think about)

Thank you very much, these are excellent questions.

In 8b653b2 you decide to not cache the CTV hash. Makes sense, it’s a nice simplification. You label the commit as “Experimental” - I take it that you have determined that this was an unnecessary optimization and does not affect DoS safety? If you did determine this, I’d love to see some benchmark on worst-case scripts to verify that this caching really has a material impact on DoS safety in the first place (ie pragmatic, not just big-O theoretical).

Yes, it doesn’t affect DoS safety. It doesn’t need a benchmark per-se, because CTV hashes are both fixed length (after the non-fixed length portions are cached) so it’s not worse than repeated OP_HASH256. So it’s better than a benchmark, it’s a proof :)

In some of the in-line code review comments for commit ffbbe64 concerning caching, you mention:

the main reason to cache the hashing it to prevent scripts like CTV CTV CTV…. CTV from being able to cause N^2 hashing to occur

Q2) IIUC correctly this is bc, without any caching, for each CTV in the script we’d first need to hash the scriptSigs (if any), and then the ctv hash. So for N CTVs that’s N^2 (worst-case). But this commit was before you removed caching the CTV hash in 8b653b2, so IIUC worst-case is now N hashes for this script. So you made a (probably good) judgement call that O(N) is acceptable while O(N^2) is a DoS security risk, but is there any justification/proof you can give?

This is roughly correct, yes. The issue is that it’s N hashes of O(1) data vs N hashes of O(N) data. N hashes of O(1) data already occurs with OP_HASH256 (which does 2 sha256) so you can write scripts already that do more work than CTV.

Q3) What is the justification for these refactor commits cab836f, e1b453b? I mean, at a high-level all you’re doing in the end is caching a single hash of the scriptSigs, why does this need to be so much more complicated than the existing caching for segwit and taproot? ie changing common function declarations all over the place and then needing PrecomputedTransactionData::BIP119LazyInit, PrecomputedTransactionData::BIP119EagerInit, m_bip119_cache_synchronizer (whatever this is?)

It doesn’t need to be… but @jamesob noted a performance regression caused by the naive caching, so he asked me to fix it, and I did.

Where it got extra complex is because we have a build target for the interpreter which prevents call_once from being used directly, so I had to do a ‘functional dependency injection’ for a synchronizer primitive. Another approach would be to implement a call_once of our own that works in single threaded contexts…

To me the way segwit and taproot do caching is much easier to understand and simple than how you do it here. Is it bc you don’t have a good heuristic to determine if a given tx uses_bip119_ctv (ie when caching scriptSigs is actually necessary for a given txTo)? Is there a fundamental reason why this can’t be heuristic based like segwit/taproot?

Yep, this is pretty much the reason. To do so would require parsing and scanning every input’s scripts entirely for CTV, but it wouldn’t tell you if CTV even gets executed or not. So instead, we cache on first use.

OTOH, it is cheap to use the heuristic of has taproot / has segwit v0 and “should have a signature”… interestingly, should CTV prove popular, some of the caches for segwit v0 and taproot also become over-eager and maybe we’d make them cache on first use too for maginal benefit!

Related note: I am assuming that this performance degradation @jamesob benchmarked here has been “resolved” in these commits (to justify the additional complexity). Can you confirm what the result/takeaway is here? @jamesob?

Nit: in the PR description you say “Minus tests and deployment, it’s about 180 LOC.”, I’m guessing this was before these commits and needs to be updated?

I suppose… do you know the count, now? It still seems to be around the same LoC of consensus code though?

All that said, this brings me to another conceptual question. In the bip as rationale for Committing to the ScriptSigs Hash:
0We commit to the hash rather than the values themselves as this is already precomputed for each transaction to optimize SIGHASH_ALL signatures.
Obviously this has not already been precomputed or we wouldn’t have to compute and cache this hash in the first place, right? Am I missing something? Because otherwise here is how I see it from a high-level:

Yep only CTV uses this.

You determined N hashes was acceptable from a DoS security perspective here 8b653b2 Yep.

So as long as we have the scriptSigs hash (whether cached or precomputed like the bip says) we do N hashes in the worst-case Yep.

And therefore if scriptSigs hash was precomputed, we wouldn’t even need any additional caching added in this PR.

Yep

On point 3) Why even hash the scriptSigs then? This is somewhat related to my previous question about having a single code-path for computing this scriptSig hash. Why not simplify it, only serialize non-empty scriptSigs (for the same reasons I mentioned in my first question, but as-related to DoS security and caching there would then be no extra hash to compute), and with no additional caching at all we are at the same worst-case N hashes for the CTVs in script. And I wonder, would that address your reasoning against my initial question:

It’s not the number of hashes, it’s the lenght of the data being hashed. Just flattening the hash would make it harder to cache and wouldn’t fix the DoS issue.

the reason for hashing separately is that if we ever add OP_CAT to Taproot via successX, the hashing of the scriptsig would make the scripts more complicated / use more space uneccesarily, so avoiding the commitment lets one write more efficient covenants (potentially) in the future.

This is kind of unrelated to DoS, it’s just if your covenant needs to reason of scriptsig or not. For a single input tx with taproot input, it’s always a no.

jamesob commented at 3:02 pm on April 24, 2022: member

Related note: I am assuming that this performance degradation @jamesob benchmarked here has been “resolved” in these commits (to justify the additional complexity). Can you confirm what the result/takeaway is here?

Yes, there is now no marginal performance difference on this branch; that was determined when I updated the benchmark in #21702 (comment).

The original benchmark I chose for this (enabling CTV from genesis and thus doing all caching from genesis) was naive, because it attributed all caching that is done for segwit/taproot spends to CTV as well (since these caches are required for CTV), and doesn’t isolate the marginal overhead of CTV. It was done as an “absolute worst case” bound on performance degradation.

But in hindsight this clearly isn’t a fair benchmark to CTV, since it includes segwit/taproot overhead. In the next week I’d like to remove all fancy caching stuff that Jeremy has done as a result of my (misleading?) benchmark and see if there is actually a difference in performance on regions of the chain where segwit use is common when using Jeremy’s original (simple) approach to caching.

In a nutshell: I came up with a pessimistic benchmark that misrepresented CTV’s performance impact, then asked why we aren’t populating caches lazily (without realizing the practical difficulties as @JeremyRubin describes above), so I inadvertently have caused more complexity in this change than maybe is warranted. I’ll report back with update benchmarks in a few days.

mjdietzx commented at 1:48 am on April 25, 2022: contributor

@JeremyRubin that response is very helpful - I had a few misunderstandings about how caching works here, but it’s all making sense now.

N hashes of O(1) data already occurs with OP_HASH256 (which does 2 sha256) so you can write scripts already that do more work than CTV.

Very good point. I agree w/ this line of reasoning when thinking about DoS safety of CTV. Along these lines, do you happen to know a good worst-case script (ie one of the most computationally expensive scripts) that can be currently run in Bitcoin? Maybe worst-case “consensus valid” and worst-case “is standard / will be relayed” - I wonder if that can inform what the baseline/minimum caching requirement is here.

To do so would require parsing and scanning every input’s scripts entirely for CTV, but it wouldn’t tell you if CTV even gets executed or not

Maybe it’s moot, but I don’t think there is a downside to caching scriptSigs any time an OP_CTV is detected. Even if a CTV is not executed, I don’t see any notable performance hit in the rare event we do an extra hash. I’ll try to explore this a little unless you’ve already tried or came to a conclusion here that it’s not a good route to go down.

My next steps are to do some benchmarking/testing. I want to create one of these worst-case CTV scripts and see how the caching you do in a few of these commits performs. I’ll aim to get numbers for runtimes of worst-case “consensus valid”, and worst-case “is standard / will be relayed” CTV scripts for a handful of this PRs commits. If you have any advice or info you can link to (ie if you’ve already created some of these worst-case scripts) please send my way (feel free to email or IRC)

Also interested in hearing @jamesob report when he revisits this!

cryptoquick commented at 2:49 am on April 25, 2022: none

Concept ACK.

Been following CTV for a little while, have a decent understanding of its implementation, use-cases, and impact. I’m not yet a core dev, but I do a lot of work on wallets and have a decent understanding of Bitcoin (although not as much as I would need to speak with any sense of authority). That said, my thoughts are as follows:

CTV is a very small, conservative change that prevents recursive covenants and the fears surrounding them, while better supporting scalability of more advanced use-cases.

As users are expanding what they use Bitcoin for (DLCs, RGB, TARO, LN channels, none of which need CTV), CTV can be a powerful optimization to keep more of these txs off-chain, yet still provable and can still be settled back on-chain.

It is similar in this regard to MAST, but while (AFAIU) MAST is purely for trees and can’t necessarily backreference, CTV can express DAG-like structures, which can help with de-duplicating redundant tx paths. This could have a dramatic impact on retaining present fee economy (which is quite good) while supporting these expanded use-cases.

Because, like it or not, they will be happening with present tools– and barring any miner-coordinated censorship campaigns against all P2TR txs– ideally we have more tools in our toolbox to reduce on-chain impact. The earlier developers know we can depend on these tools, the earlier we can get ahead of the inevitably more constrained fee economy regime in the decades to come.

I will admit, I’m still a novice when it comes to understanding these things, so I might be wrong in some of these assumptions. Feel free to correct.

JeremyRubin commented at 4:55 pm on April 25, 2022: contributor

@mjdietzx worst case scripts is an ongoing area of research we can chat about sometime out of band of this PR.

i think the thing you misunderstand is that it’s “very expensive” to detect if CTV is used in any script, so it doesn’t make sense to do that instead of just computing the scriptsig hash always or on the first time you use CTV.

The most helpful benchmark you could make for the caching, IMO, would be not to focus on the number of CTVs in the script, but if you do a transaction with N inputs where each has a CTV itself, and test it with parallel CScript validation. That would really hammer the call_once synchronizer which would be, IMO, the most likely to cause slow validation since it causes a single-threadedness for transactions like that. However, call_once is a heavily optimized primitive for exactly this scenario, so it’s probably fine. But i’d be interested to see a benchmark!

in src/script/script.cpp:207 in 2c3f8994c1 outdated

202+{
203+    // Extra-fast test for pay-to-bare-default-check-template-verify-hash CScripts:
204+    return (this->size() == 34 &&
205+            (*this)[0] == 0x20 &&
206+            (*this)[33] == OP_CHECKTEMPLATEVERIFY);
207+}

mjdietzx commented at 5:15 pm on April 25, 2022:

nit: forgot to add newline between functions

in src/script/interpreter.cpp:1471 in 2c3f8994c1 outdated

1491+}
1492 
1493 } // namespace
1494 
1495+template<typename TxType>
1496+uint256 GetDefaultCheckTemplateVerifyHash(const TxType& tx, uint32_t input_index) {

mjdietzx commented at 5:38 pm on April 25, 2022:

Is this used anymore?

mjdietzx commented at 10:04 pm on April 25, 2022:

nit: also input_index should probably be const, but moot if we can just get rid of this

in src/script/interpreter.h:199 in f15ae3beff outdated

204@@ -196,8 +205,6 @@ struct PrecomputedTransactionData
205 
206 /* Standard Template Hash Declarations */
207 template<typename TxType>
208-uint256 GetDefaultCheckTemplateVerifyHash(const TxType& tx, uint32_t input_index);

mjdietzx commented at 5:49 pm on April 25, 2022:

you forgot to remove the corresponding function definition in the cpp file (same as the nit I left there)?

in src/script/interpreter.cpp:1937 in 2c3f8994c1 outdated

1952+{
1953+    // Should already be checked before calling...
1954+    assert(hash.size() == 32);
1955+    if (txdata && txTo) {
1956+        txdata->BIP119LazyInit(*txTo);
1957+        uint256 hash_tmpl = txdata->m_scriptSigs_single_hash.IsNull() ?

mjdietzx commented at 10:04 pm on April 25, 2022:

nit: can be const

mjdietzx commented at 1:05 am on April 26, 2022: contributor

At this point I’ve went through all code in this PR pretty thoroughly (I was really hoping to find something wrong to snatch that bounty 😔). The only thing (implementation wise) I have a problem w/ at this point is the caching.

It’s kinda hard to say what the optimal caching policy is and it’s bikesheddable + non consensus

That’s pretty much where I’m at as well @JeremyRubin - ^ from this comment you made almost exactly a year ago, but since then I think some caching scope creep happened

My opinion at the moment is that this PR is simple, doesn’t do anything fancy, and is very understandable - other than the caching, specifically these commits cab836ff0b20ab7444a5642c4433e8d2cf227302, e1b453ba6e66bb1f6dbd1d2f6152f2cef800fe30.

Correct me if I’m wrong, but the reason we cache is purely for security reasons. We just want to be sure there is not a DoS vector introduced w/ CTV, and I agree that without any cache whatsoever there would be. DoS protection aside, I think the normal cases where the caching added in this PR actually would provide a benefit will be increasingly rare given that legacy transactions should become increasingly rare over time (especially by the time CTV is gaining traction, and used in conjunction w/ ctv). Anyways, I think we agree the performance benefit from caching scriptSigs is negligible, and even if it isn’t we could always improve that in a followup PR when we have some data to inform what’s necessary. For this case all we care about is not introducing a DoS vector (correct me if I’m wrong)

All that said, I want to propose a different approach to caching where we just bound the max hashing. Any transactions over a certain size, we’ll cache the scriptSigs. This way it is very simple, and does caching the same exact way as segwit/taproot. It’s easier to say w/ code (I did it quick so I’m sure there are bugs and room for improvement), so please check out this commit https://github.com/bitcoin/bitcoin/commit/5ca5d300735ba0cdf3211ab7dd13cc1a68640929 on my branch here https://github.com/mjdietzx/bitcoin/tree/poc_simple_cache_heuristic_for_ctv

it basically just replaces cab836ff0b20ab7444a5642c4433e8d2cf227302 and e1b453ba6e66bb1f6dbd1d2f6152f2cef800fe30. The comment there describes the approach in more detail. I picked MAX_TXN_SIZE_PRECOMPUTE_CTV_DATA = 2048 very carelessly, I’m on a slow Mac book air, and estimated w/ > openssl speed sha256 that validating a worst-case CTV script MAX_OPS_PER_SCRIPT = 201 so 201 CTVs in a script with 2048 bytes of scriptSigs would take ~1 second. If the transaction is any bigger than that scriptSigs will get cached and then this runs O(N) as it currently does in this PR

in src/test/ctvhash_tests.cpp:155 in 2c3f8994c1 outdated

150+                            txc.nLockTime = fr.rand32();
151+                        } while (txc.nLockTime == tx.nLockTime);
152+                        hash_will_change = true;
153+                        break;
154+                    }
155+                    case 8: {

vicariousdrama commented at 6:59 pm on April 26, 2022:

should case 8, 9 still exist if doing nothing and breaking out of the switch on these conditions

JeremyRubin commented at 3:52 am on April 30, 2022:

hmm you’re probably right about that! I think it’s an artifact of how I wrote this test, can clean up – interesting it didn’t get a dead code warning.

JeremyRubin force-pushed on Apr 26, 2022

vicariousdrama commented at 9:33 pm on April 26, 2022: none

General concept ACK

brokep commented at 8:34 am on April 28, 2022: none

General thumbs down for this

JeremyRubin commented at 11:37 pm on April 28, 2022: contributor

Thanks for your input – in general, comment like this in Bitcoin Core are given with a Nack, of which there are a few kinds.

Approach NACK: your idea is good, but did it the wrong way
Concept NACK: this is a bad idea
NACK: this shouldn’t be merged

According to the contributors guide, you should also give reason so that your issue might be addressed by the author.

Cheers!

in src/node/psbt.cpp:27 in c24b553d3f outdated

23@@ -24,7 +24,7 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
24 
25     result.inputs.resize(psbtx.tx->vin.size());
26 
27-    const PrecomputedTransactionData txdata = PrecomputePSBTData(psbtx);
28+    PrecomputedTransactionData txdata = PrecomputePSBTData(psbtx);

jaonoctus commented at 3:22 pm on April 30, 2022:

Why did you make PrecomputedTransactionData non-const?

JeremyRubin commented at 4:46 pm on April 30, 2022:

because I put into it a field which mutates internal state during the LazyInit, which would otherwise not be callable unless I made those fields mutable, which is not a preferred pattern in core.

vicariousdrama approved

cryptoquick approved

AmadeusK525 commented at 2:10 pm on May 3, 2022: none

You didn’t squash 8ebe9e into 93f98 and 65961. Lines 609-615 in interpreter.h should be squashed to 93f98 and everything else to 65961

AmadeusK525 commented at 2:24 pm on May 3, 2022: none

Concept NACK on this one. I don’t think batched transactions should be handled on the bedrock that is the Bitcoin blockchain. I also can’t fathom so many people being in favor of reducing Bitcoin’s fungibility like it’s nothing, this is extremely dangerous and should never happen, ever. As the BIP itself says:

As covenants are complex to implement and risk of introducing fungibility discriminants they have not been seriously considered for inclusion in Bitcoin.

. I feel like this will also be, for sure, an entry-point for unwanted BIPs that introduce more and more unnecessary complexity and, as a result, the code will be more prone to introduce loopholes. Again, as the BIP itself says: in the future, as we become aware of more complex but shown to be safe use cases new template types can be added.. Strong concept NACK, but kudos on the research done and implementation

JeremyRubin commented at 3:05 pm on May 3, 2022: contributor

You didn’t squash 8ebe9e into 93f98 and 65961. Lines 609-615 in interpreter.h should be squashed to 93f98 and everything else to 65961

There’s a technical reason to not squash these things pertaining to ease of backporting to other clients where Span is not yet available, since the reason for the ‘Fixup’ was to put it in line with new things available in Core since the code was written. They can be squashed before merge if desired, or left separate.

DrahtBot commented at 8:51 am on May 6, 2022: contributor

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

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on May 6, 2022

ghost commented at 4:59 pm on May 12, 2022: none

I don’t think batched transactions should be handled on the bedrock that is the Bitcoin blockchain.

How do you think batched transactions should be handled?

I also can’t fathom so many people being in favor of reducing Bitcoin’s fungibility like it’s nothing, this is extremely dangerous and should never happen, ever.

How much fungibility is reduced with CTV? Do you think bitcoin is fungible and was this ever considered in any other soft fork?

I would rate bitcoin’s fungibility a score of 1 on a scale of 0 to 9 where 0 is bad and 9 good: https://sethforprivacy.com/posts/fungibility-graveyard/

You might also find payment pools interesting: https://rubin.io/bitcoin/2021/12/10/advent-13/#fnref:greg

Again, as the BIP itself says: in the future, as we become aware of more complex but shown to be safe use cases new template types can be added

This is either shared out of context or I am missing something. The goal of CHECKTEMPLATEVERIFY is to be minimal impact on the existing codebase. BIP 119 adds a simple covenant known as a template that allows for some use cases to be implemented without significant risk. It supports non-recursive covenants with no dynamic state.

AmadeusK525 commented at 5:04 pm on May 13, 2022: none

How do you think batched transactions should be handled?

Second layer solutions, like the Lightning Network

How much fungibility is reduced with CTV? Do you think bitcoin is fungible and was this ever considered in any other soft fork?

Fungibility isn’t really a concrete idea since two different things being exactly the same is borderline impossible, physically speaking. Nothing is ever truly fungible. People say paper money is fungible even though the paper can tear and wear, and, even though people still accept it most of the time, they still internalize that a torn bill is worth less than a fresh one. Having said that, Bitcoin is pretty fungible, if you consider a hyperbitcoinized world. People may buy Bitcoin for more/less fiat value based on tx history, but when Bitcoin IS what determines the value, every UTXO has a value and they’re all worth the same, regardless of where they came from. They can all be spent in the same way, they have the same value, an UTXO with 0.02 BTC has the same amount of BTC as another one with 0.02 BTC in it, they can be used interchangeably. CTV means they CAN’T be used interchangeably, they have to be spent in x way (the only OP_CODE that I consider to slightly reduce BTC’s fungibility is the time lock)

I would rate bitcoin’s fungibility a score of 1 on a scale of 0 to 9 where 0 is bad and 9 good:

Okay

This is either shared out of context or I am missing something. The goal of CHECKTEMPLATEVERIFY is to be minimal impact on the existing codebase. BIP 119 adds a simple covenant known as a template that allows for some use cases to be implemented without significant risk. It supports non-recursive covenants with no dynamic state.

This is definitely NOT taken out of context. It’s literally taken right out of the BIP. First line of the Rationale section. I don’t understand your point.

Edit: apologies if my words are a bit jumbled, I’m in a hurry right now so I can’t go over what I’ve written to make sure that everything makes sense

ProofOfKeags commented at 10:57 pm on May 16, 2022: none

@AmadeusK525

They can all be spent in the same way

This reflects a deep misunderstanding as to how Bitcoin works. At bare minimum you need different signatures to move different utxos that are bound to different scripts. In a substantial number of cases you need fundamentally different requirements (multisig, hashlocks, timelocks, etc.)

CTV means they CAN’T be used interchangeably, they have to be spent in x way (the only OP_CODE that I consider to slightly reduce BTC’s fungibility is the time lock)

This has the same exact impact as having different UTXOs with different scripts, one of which is multisig and requires co-authorization, and the other is single sig that you hold the key. These aren’t “interchangeable” either. The spender’s inability to move the coins without meeting the conditions has been a property of the Bitcoin network since the Satoshi 0.1 client. There is no net fungibility reduction introduced by CTV.

AmadeusK525 commented at 11:05 pm on May 16, 2022: none

@ProofOfKeags Come on, you know what I meant. All UTXOs, once unlocked, can be spent in the same way. This is fundamentally not the case with CTV, as the act of unlocking it itself means that you’re spending the coins in a predefined way, to a predefined output. That UTXO isn’t worth anything to any address except the ones predefined by the scriptPubKey

cryptoquick commented at 11:20 pm on May 16, 2022: none

@AmadeusK525 It just introduces another extrinsic dimension to unlock a spend. To name a few, block time or height for timelocks, preimages for hashlocks, and now output template hashes. Just another tool in the toolbox to take information extrinsic to the blockchain and use it to provide real-world value.

AmadeusK525 commented at 6:53 pm on May 17, 2022: none

@cryptoquick I understand what it does, as I’ve explained above. I’m not in favor of it.

ProofOfKeags commented at 7:59 pm on May 17, 2022: none

Come on, you know what I meant. All UTXOs, once unlocked, can be spent in the same way. This is fundamentally not the case with CTV, as the act of unlocking it itself means that you’re spending the coins in a predefined way, to a predefined output. That UTXO isn’t worth anything to any address except the ones predefined by the scriptPubKey

I wasn’t making some tongue-in-cheek comment. The idea that UTXOs are “unlocked” and then “spent” is entirely wrong. UTXOs move in a single atomic operation. Either the transaction that spends them is valid or it isn’t. CTV introduces a primitive to make certain kinds of spends impossible. This is in no way dissimilar from any other opcode in the script VM. UTXOs are only worth something to the receiver if they can manage and spend them as they see fit. If you can spend your CTV bound UTXO to an unencumbered receiver, it makes no difference to that person whether it came from a CTV bound input or not.

The script that defines the conditions of spending a UTXO is entirely at the discretion of the receiver. This was true before CTV, and it is true if CTV is added as an opcode. The net effect here is that there is zero reduction in the fungibility of these coins. In fact, I’d even go as far as to say that the fungibility between CTV/non-CTV outputs is superior to the fungibility between L1 and L2 funds, which people are overwhelmingly in favor of.

I understand what it does, as I’ve explained above. I’m not in favor of it.

I don’t think you do. As I stated above the way you are thinking about this reflects a misunderstanding of the fundamental method by which transactions make changes to the UTXO set. The concern about fungibility can only arise if the receiver of the coins does not have the discretion to define the spending conditions. The nature of P2SH/P2WSH makes this more or less impossible since the spending conditions are embedded in the address that you present to the sender to pay you. If they don’t have funds that are capable of being spent to an address you define, then it is no different than them not having access to the funds at all.

Consider a vault scenario. If you have to wait 3 days to be able to spend funds after unvaulting them, that isn’t dissimilar from having to wait on a paycheck before paying a vendor.

Alternatively

Consider a fiat world analog. Your fiat money is no less fungible by virtue of other people having set up corporations with treasury spending policies or trusts with administrative bylaws.

I hope this helps dispel the fiction that CTV erodes fungibility.

AmadeusK525 commented at 1:04 am on May 18, 2022: none

The idea that UTXOs are “unlocked” and then “spent” is entirely wrong. UTXOs move in a single atomic operation. Either the transaction that spends them is valid or it isn’t.

That’s just not true. The act of “unlocking” a UTXO is simply providing a valid scriptSig. Up until now, no OP code gets information on the outputs of the transaction, so it makes no judgement on how you spend your coins after you’ve unlocked them. Unlocking just means going forward with the transaction.

CTV introduces a primitive to make certain kinds of spends impossible. This is in no way dissimilar from any other opcode in the script VM.

Again, it is dissimilar in the sense that it restricts the possible output of a transaction. No other OP code does something like this.

The script that defines the conditions of spending a UTXO is entirely at the discretion of the receiver.

That is also not true. If someone hands you a legacy address and you send them coins through a P2PKH script, but add a timelock via OP_CHECKLOCKTIMEVERIFY, does that mean that you haven’t paid them? The one who chose the extra condition of the TimeLock was the sender, not the receiver. Yes, CTV doesn’t change who decides the conditions of the spending of a UTXO, but I never claimed it did.

The concern about fungibility can only arise if the receiver of the coins does not have the discretion to define the spending conditions.

Which is the case with legacy addresses

The nature of P2SH/P2WSH makes this more or less impossible since the spending conditions are embedded in the address that you present to the sender to pay you.

Yeah, that’s true, P2SH and P2WSH addresses are the only ones where the receiver does, in fact, directly define the conditions, since it literally gives you the scriptPubKey’s hash. Other addresses do not do this, so they do not specify conditions. They give the sender variables to use in the scriptPubKey that they create.

If they don’t have funds that are capable of being spent to an address you define, then it is no different than them not having access to the funds at all.

Sorry, I didn’t quite understand this sentence. Isn’t this the point I’m trying to make against CTV?

I hope this helps dispel the fiction that CTV erodes fungibility.

RobinLinus commented at 7:54 am on May 18, 2022: none

@AmadeusK525 You can emulate CTV using deleted-key covenants. Thus, CTV doesn’t introduce any new functionality but only removes the need for a trusted party.

ProofOfKeags commented at 5:30 pm on May 19, 2022: none

That’s just not true. The act of “unlocking” a UTXO is simply providing a valid scriptSig. Up until now, no OP code gets information on the outputs of the transaction, so it makes no judgement on how you spend your coins after you’ve unlocked them. Unlocking just means going forward with the transaction.

The scriptSig commits to the SIGHASH which commits to the outputs, so the outputs are included. If this wasn’t the case, then the outputs would be malleable after the signature was made. This would be the most dangerous malleability vector ever if it were true, however, by design it is not.

That is also not true. If someone hands you a legacy address and you send them coins through a P2PKH script, but add a timelock via OP_CHECKLOCKTIMEVERIFY, does that mean that you haven’t paid them? The one who chose the extra condition of the TimeLock was the sender, not the receiver. Yes, CTV doesn’t change who decides the conditions of the spending of a UTXO, but I never claimed it did.

It does, indeed, mean you haven’t paid them. To make this more obvious, if you took that P2PKH script and added a hashlock to it, you haven’t paid them. The sender cannot both claim that the receiver has been paid and add arbitrary conditions to the receivers script.

Sorry, I didn’t quite understand this sentence. Isn’t this the point I’m trying to make against CTV?

My point here is that this is solely the problem of the sender who had to opt into these conditions in the first place. Therefore it can be used as an argument against any and all forms of restrictions including multisig, or even single sig where you lost the private keys.

ghost commented at 7:25 pm on September 9, 2022: none

@JeremyRubin could you please rebase it and also resolve CI errors?

JeremyRubin commented at 9:07 pm on September 9, 2022: contributor

thanks for the ping.

I need to do some rebasing it looks like, will do so after TAB Conf most likely (working on another project for delivery by then) – it looks like maybe @ajtowns rebased on his branch above, so perhaps I can just use that rebasing?

ajtowns commented at 9:44 pm on September 9, 2022: contributor

Branch above is 23.0 based, so probably not that helpful

in src/script/interpreter.h:183 in c24b553d3f outdated

182+    //! Whether the bip341 fields above are initialized.
183+    bool m_bip341_taproot_ready = false;
184+
185+    //! Whether the bip119 fields above are initialized directly (nullptr)
186+    //! or lazily (using sync primitves)
187+    typedef std::function<void(std::function<void()>)> bip_119_cache_synchronizer_t;

aureleoules commented at 7:14 am on September 23, 2022:

0    using bip_119_cache_synchronizer_t = std::function<void(std::function<void()>)>;

JeremyRubin commented at 6:44 pm on September 30, 2022:

#25695 (comment) was this resolved to a specific preference?

aureleoules commented at 9:28 pm on October 1, 2022:

I generally prefer using because it’s easier to read but yes it’s a preference.

in src/script/interpreter.cpp:1567 in c24b553d3f outdated

1557+        {
1558+            f();
1559+            // disables future calls from recomputing
1560+            m_bip119_cache_synchronizer = nullptr;
1561+        };
1562+        m_bip119_cache_synchronizer = single_threaded;

ajtowns commented at 7:53 am on September 23, 2022:

Shouldn’t this be:

0m_bip119_c_s = (f != nullptr ? f : single_threaded);

or something? Otherwise nothing seems to be done with the bip_119_cache_synchronizer_t f that was passed in other than sometimes asserting that it’s nullptr.

JeremyRubin commented at 9:09 am on September 23, 2022:

https://github.com/bitcoin/bitcoin/pull/21702/files/c24b553d3fe7db19c8c37934a196334911412a0e#diff-a0337ffd7259e8c7c9a7786d6dbd420c80abfa1afdb34ebae3261109d9ae3c19R1589 this line?

ajtowns commented at 10:42 am on September 23, 2022:

No? I’m looking at the Init() function, your link is something that calls Init() with f defaulted to nullptr?

https://github.com/bitcoin/bitcoin/pull/21702/files/c24b553d3fe7db19c8c37934a196334911412a0e#diff-a0337ffd7259e8c7c9a7786d6dbd420c80abfa1afdb34ebae3261109d9ae3c19R1496

ajtowns commented at 10:43 am on September 23, 2022:

https://github.com/bitcoin/bitcoin/pull/21702/files/c24b553d3fe7db19c8c37934a196334911412a0e#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R1736-R1737 seems to be the only place where Init’s f parameter isn’t defaulted.

JeremyRubin commented at 4:49 pm on September 24, 2022:

I think you’re right that this is broken – do you have a test condition that triggers this?

Very good catch, I think this should be eligible for the bipbounty.org bounty as a Major?

cc @jamesob can you re-do the benchmarks so we can also see if it’d be reasonable to remove these patches from the PR still?

jamesob commented at 3:17 am on September 26, 2022:

@JeremyRubin yup, I’ll do that this week.

JeremyRubin commented at 7:14 pm on September 30, 2022:

update here:

I’ve been trying to figure out how/why this was not making a test failure happen, and I think I know how to trigger it with a test case (but haven’t written one out). To trigger it, all of the following must be true:

Things that can be set up deterministically:

There is no cached evaluation of the script from a single threaded run (which can happen in the mempool), so this should be tested with a direct block submission
143 and 341 must be disabled OR there must be legacy inputs that have non-empty scriptsigs (to trigger a hash getting computed).
There is more than one CTV Input
The node is running with multiple workers

Things that happen by chance (don’t know exactly how to trigger this part via a test):

The scriptcheck jobs get scheduled on two different checkqueue workers.
There is a data-race on two inputs such that they don’t happen to correctly synchronize (CheckQueue has some locks in it)
The data-races don’t happen to both observe the fully written value – since the cache is deterministic, on most architectures the cache computations won’t result in a “garbage value”, since the write is the same. So when the single threaded computation finishes, the values will always be consistent. This is aided by the fact that the last write to the synchronizer single threaded is to set nullptr for the function, so we either do parallel identical runs, or we do one run and the other observes it. (edit: I guess also 8, that the optimizer can’t inline and reorder the effects from calling the synchronizer function, since it doesn’t know what f is in advance is relevant too)

7 is really the criticial part. Even if we end up using the single threaded initialization, it seems that it does not (on most architectures?) lead to an actual race, no matter what.

This isn’t to say that this shouldn’t get patched – it’s super clearly not formally correct – but also that it seems not possible to actually write a failing test for, which is what I would normally do under the circumstances.

JeremyRubin commented at 7:21 pm on September 30, 2022:

In any case, patch fixing the issue is here: https://github.com/bitcoin/bitcoin/pull/21702/commits/d4af88d25dd5e5d8196988bf00c2dcefcc5d96b4

ajtowns commented at 5:32 am on October 2, 2022:

I think that means that you should be able to trigger a tsan warning by having a tx with, say, 100 inputs, 10 of which are plain p2pkh, and the remaining 90 are different ctv inputs?

JeremyRubin commented at 7:03 pm on October 2, 2022:

i think it should be catchable under tsan (not entirely familiar with what it can and can’t catch), but it will require some tuning or a lot of runs to actually get it to trigger? I’ll see if I can put something together at some point to test the pre-patch post-patch behaviors.

would you prefer leaving this unresolved till such a test exists?

ajtowns commented at 8:06 am on October 7, 2022:

Here’s a test, I think: https://github.com/ajtowns/bitcoin/commit/5beeb28bde367769e189a7ff5857a190bc19c15b

When I run it under tsan without your patch, I get: “[node 1] Node returned non-zero exit code (66) when stopping”; with errors in the logs that (after lots of trimming) look like:

 0WARNING: ThreadSanitizer: data race (pid=993218)
 1  Read of size 8 at 0x7bb40001ce58 by thread T7:
 2    [#6](/bitcoin-bitcoin/6/) uint256 (anonymous namespace)::GetDefaultCheckTemplateVerifyHashEmptyScript<CTransaction>(CTransaction const&, uint256 const&, uint256 const&, unsigned int) src/./src/script/interpreter.cpp:1463:9 (bitcoind+0x11e0fd2) (BuildId: ec0
 37d2273791a5c68cd1432ff17dfea367618622)
 4
 5  Location is heap block of size 36720 at 0x7bb400014000 allocated by thread T17:
 6    [#7](/bitcoin-bitcoin/7/) CChainState::ConnectBlock(CBlock const&, BlockValidationState&, CBlockIndex*, CCoinsViewCache&, bool) src/./src/validation.cpp:2180:45 (bitcoind+0x8d4595) (BuildId: ec07d2273791a5c68cd1432ff17dfea367618622)
 7
 8  Location is heap block of size 36720 at 0x7bb400014000 allocated by thread T17:
 9    [#1](/bitcoin-bitcoin/1/) std::__new_allocator<PrecomputedTransactionData>::allocate(unsigned long, void const*) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/new_allocator.h:137:27 (bitcoind+0x95673e) (BuildId: ec07d2273791a5c68cd1432ff17dfea367618622)
10
11  Thread T7 'b-scriptch.6' (tid=993226, running) created by main thread at:
12  Thread T4 'b-scriptch.3' (tid=993223, running) created by main thread at:
13  Thread T17 'b-msghand' (tid=993243, running) created by main thread at:

configure options are ./configure --with-gui=no CC=clang CXX=clang++ --with-incompatible-bdb --enable-zmq --enable-debug --enable-werror --with-sanitizers=thread --enable-suppress-external-warnings --enable-experimental

With your patch, it passes and there aren’t any ThreadSanitizer entries in the log.

EDIT: Maybe worth noting that the test case doesn’t satisfy Jeremy’s condition 2 above (341/143 disabled or non-empty scriptSig). Not sure what that implies, if anything.

JeremyRubin commented at 6:23 pm on October 7, 2022:

condition 2 i think was incorrect, because the line:

0 m_scriptSigs_single_hash = NoScriptSigs(txTo) ? uint256{} : GetScriptSigsSHA256(txTo);

which could be written instead as

0if (!NoScriptSigs(txTo)) {
1    m_scriptSigs_single_hash = GetScriptSigsSHA256(txTo);
2}

and then the condition 2 would be accurate, I think. But because it’s using ? it always assigns, even though the new value is the same as the zero initialize.

in src/script/interpreter.h:170 in c24b553d3f outdated

167     uint256 m_spent_scripts_single_hash;
168-    //! Whether the 5 fields above are initialized.
169-    bool m_bip341_taproot_ready = false;
170+
171+    // BIP119 precomputed data (single SHA256).
172+    uint256 m_scriptSigs_single_hash;

aureleoules commented at 10:24 am on September 23, 2022:

maybe use std::optional?

JeremyRubin commented at 6:42 pm on September 30, 2022:

had this conversation around taproot times since std::optional became available around then too; probably better to keep the style consistent & do an all-at-once convert to optional in this struct if ever worthwhile?

aureleoules commented at 10:58 am on September 23, 2022: member

I verified that :

the chosen opcode for OP_CHECKTEMPLATEVERIFY is OP_NOP4 (0xb3) as mentionned in BIP 119.
there must be an element on the stack
element on the stack must be of length 32
DefaultCheckTemplateVerifyHash of the tx at the input index must be equal to the element on the stack
commitment entries correspond to the BIP (version, locktime, scriptSigs hash (if any non-null scriptSigs), number of inputs, sequences hash, number of outputs, outputs hash, and currently executing input index)
as of c24b553d3fe7db19c8c37934a196334911412a0e, this PR does not activate BIP119 but implements it

in src/script/interpreter.h:182 in d4af88d25d outdated

181+
182+    //! Whether the bip341 fields above are initialized.
183+    bool m_bip341_taproot_ready = false;
184+
185+    //! Whether the bip119 fields above are initialized directly (nullptr)
186+    //! or lazily (using sync primitves)

ajtowns commented at 5:19 am on October 2, 2022:

“primitives” – caught by linter

in src/script/interpreter.cpp:603 in d4af88d25d outdated

599+                        if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS)
600+                            return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS);
601+                        break;
602+                    }
603+
604+                    if (stack.size() < 1)

aureleoules commented at 3:10 pm on October 3, 2022:

nit: I think this is more readable, also guaranteed to be constant time

0                    if (stack.empty())

jamesob commented at 1:14 pm on October 20, 2022: member

I’m happy to report that on the basis of some new benchmarking results, CTV doesn’t negatively affect IBD performance with any statistical significance. And even better, I think the changes here can likely be simplified. The lazy building of caches that @JeremyRubin added to this changeset as a result of my previous bench results are IMO unnecessary. Egg on my face, sorry Jeremy!

ibd local range 667200 697200

The issue with the previous set of benchmarks (as noted here: #21702#pullrequestreview-859718084) is that when we unconditionally enable CTV script flags for a region of the chain that doesn’t have any segwit/taproot activity, we trigger cache building that needs to happen for those sorts of spends anyway. In other words, most of the PrecomputedTransactionData activity that goes on for CTV is needed by segwit/taproot anyway. So basically, the previous benchmarks (same benchmark on an earlier chain region) were unfairly penalizing CTV by heaping on all the precomputation that has to happen for segwit/taproot too.

My latest benchmark uses a version of this branch with all the lazy caching changes stripped out (jamesob-ctv-nocache-bench, diff from this branch). And the numbers are encouraging:

bench name	command
ibd.local.range.667200.697200	`bitcoind -dbcache=10000 -debug=coindb -debug=bench -listen=0 -connect=0 -addnode=127.0.0.1:8888 -prune=9999999 -printtoconsole=0 -assumevalid=000000000000000000176c192f42ad13ab159fdb20198b87e7ba3c001e47b876`

bench name	x	jamesob/jamesob-ctv-nocache-bench	$mergebase
ibd.local.range.667200.697200.total_secs	3	4578.0364 (± 7.0356)	4560.9678 (± 23.2994)
ibd.local.range.667200.697200.peak_rss_KiB	3	4810300.0000 (± 821.0302)	4806725.3333 (± 1218.2844)
ibd.local.range.667200.697200.cpu_kernel_secs	3	208.8000 (± 0.8003)	208.3500 (± 1.4718)
ibd.local.range.667200.697200.cpu_user_secs	3	26689.3600 (± 11.6545)	26481.8333 (± 39.7697)

bench name	x	jamesob/jamesob-ctv-nocache-bench	$mergebase
ibd.local.range.667200.697200.total_secs	3	1.004	1
ibd.local.range.667200.697200.peak_rss_KiB	3	1.001	1
ibd.local.range.667200.697200.cpu_kernel_secs	3	1.002	1
ibd.local.range.667200.697200.cpu_user_secs	3	1.008	1

The always-enable-CTV branch performance is within variance of the baseline run. This tells me that there is no performance difference in block connection with CTV enabled when witness/taproot block script flags are enabled (which I think is now the whole chain?).

The commits that I stripped off for my branch are

This PR should be rebased and personally I think the above commits should be removed. @JeremyRubin if you’d like me to do this I’m happy to. Good news, everybody!

JeremyRubin commented at 4:31 pm on October 20, 2022: contributor

Amazing news! Thank you for your dilligence in re-testing. If I understand correctly, it still might be worth it to add a patch that e.g. only applies CTV rules if we’re greater than activation height or something, so IBD has no penalty? Or is that no longer a concern either?

jamesob commented at 5:50 pm on October 20, 2022: member

If I understand correctly, it still might be worth it to add a patch that e.g. only applies CTV rules if we’re greater than activation height or something, so IBD has no penalty? Or is that no longer a concern either?

Yes I think that’s right, given that even though taproot and segwit activations are buried, we only precompute tx data in the presence of scriptWitness.

BitcoinErrorLog commented at 7:52 pm on December 2, 2022: none

NACK

This feature adds too much complexity and may have unintended outcomes for Bitcoin and its users. The great majority of the current Bitcoin community is not capable of auditing or assessing the risks of CTV. Further, CTV benefits are not significant, or in-demand enough to be worth any risk at all. Instead let’s see what the limits of creativity are by using taproot and chainless solutions (append-logs, mutual contracts, etc), before resorting to another bitcoin fork being slipped onto unknowing users.

RobinLinus commented at 12:38 pm on December 3, 2022: none

@BitcoinErrorLog people should speak for themselves and not for “the great majority of the current Bitcoin community” because that’s presumptuous.

cryptoquick commented at 7:32 pm on December 3, 2022: none

There’s some subtext in @BitcoinErrorLog’s technical opinions that he’s not being particularly upfront about. I’m sure many here are aware, but I’m somewhat of a newcomer, and I say this for other newcomers who don’t necessarily have a sense for the full story behind each conversation happening around bitcoin-core dev.

He’s CEO of Synonym, a company who is heavily bagged in tech built for the Rootstock sidechain. He has a financial interest in keeping Bitcoin simple, so as to simplify building his sidechain products, and also to oppose technologies used to build competing products that use the Bitcoin blockchain only without need for a sidechain, obviating a differentiating factor of his business model.

In my experience, he also tends to hire technically weak developers with bad attitudes for some reason, maybe they’re cheaper. But that might be why he seeks to simplify any Bitcoin-facing interfaces in making his DeFi products, like BitKit and Pear Credit.

In addition, he’s the guy who campaigned on Twitter against the inclusion of the mempoolfullrbf flag in the v24 release. After it was clear he would be unsuccessful in this attempt, even after he and developers working with him wrote several essay-length screeds on the PR itself, he threw a tantrum and declared he would run his own Bitcoin nodes on a version of bitcoin-core that not only predates Taproot, but also native SegWit. He brands this as “bitcoin conservatism”, which is even more conservative than the ossifiers (who I am actually quite sympathetic towards).

This was a thinly-veiled effort to push a proposal for using unconfirmed transactions to build payment systems that don’t need Lightning to make instant payments. He wanted to develop a solution that would let merchants accept some risk of what are essentially chargebacks built into the Bitcoin protocol in order to accept instant payments that disable the RBF flag. This is yet another example of Carvalho’s perspective of Bitcoin tech as a zero-sum game, and to antagonize developers using social attacks.

Full disclosure, I’ve decided to align myself with projects that make full use of the Bitcoin blockchain and Taproot to build layers and fat clients to extend the capabilities of Bitcoin. This includes projects like Lightning, RGB, Fedimint, Ordinals, NoLooking and yes, Sapio. I think it’s safe to say that the developers dedicating their time, often quite thanklessly, towards these projects just want Bitcoin to be better. Ideally it doesn’t come at the expense of the principles that make it the success that it is. Maybe it’s too soon for BIP-119. Maybe we may never need it for layers and wallets. It does make certain things like DLCs for stablecoins built without sidechains more efficient (and thus, pose a market threat to Carvalho’s products), and it also is said to enable the validity proofs zk rollups proposal. Those are in some ways just optimizations that scale features where it’s not yet clear there’s necessarily an indication of market demand, so I think it’s fine to hold off on merging this feature if it’s unclear yet there’s demand for those who would want to run a node with BIP-119 support included. But I don’t think it’s wise to shut the door on this discussion, either. I’d recommend continuing to give them time to build, and demonstrate to the world why such things are needed.

One of the most tangible demonstrations would be persistently high mempool fees from organic user growth that could be reduced because a significant number of transactions are paying for bytes on the blockchain that could be optimized into a much smaller on-chain footprint as long as a soft fork is put through. This would be, in essence, a demonstration of the limits of the layers built to scale the base layer after some in the community of Bitcoin node operators decided increasing the block size through a hard fork wasn’t worth splitting the chain over. Soft forks like Taproot and CTV are proof the “little blockers” were right, and likely will continue to be seen as right for a very long time, so long as we are able to make more efficient use of bytes on-chain.

In the meantime, most of us builders in the projects I mentioned don’t need anything more than Taproot and Native SegWit; those upgrades are a good excuse for ossification (especially if ossification continues to be evaluated and debated over time), which offers many great features that can be made use of already, even if some cases could be done more efficiently or trustlessly using OP_CHECKTEMPLATEVERIFY.

BitcoinErrorLog commented at 7:36 am on December 4, 2022: none

@cryptoquick I don’t know where you are getting your information from, but it seems to be wildly inaccurate. Neither myself or Synonym have anything to do with Rootstock, nor any sidechain, nor Defi. Synonym uses Bitcoin (and Lightning) only, no sidechains or nonsense. Our design choices prioritize leaving Bitcoin alone and using its current state as an assumption, not something to corrupt. I have no financial interest in keeping Bitcoin simple other than that I own bitcoin and I would like for it not to break, and I would like to help others in my position.

I have always worked to defend Bitcoin from corruption for 10 years, regardless of my employment status.

Your representation of my arguments around fullRBF and 0conf are also inaccurate, but I won’t distract people here with that, simply listen to my most recent TFTC interview or such if you care.

Most importantly, instead of refuting my concerns about CTV, you decided to attack my character and (ignorantly) my motives, which I think says plenty about your position.

cryptoquick commented at 2:01 pm on December 4, 2022: none

@BitcoinErrorLog I seem to have confused Synonym with a project called Sovryn, which builds DeFi products on Rootstock. My apologies for the misunderstanding, and using that confusion as an attack on your character.

I’m actually glad I aired my concerns, since I had trouble understanding the things you were saying when I had in the back of my mind a question around your motives. I do think it’s important to understand one’s motives to put what they’re saying in context.

I value being honest and self-critical, and you make some salient points on layers that should certainly be taken seriously. I also appreciate your concerns over adding new features to Bitcoin that could have unforeseen consequences.

I still think it’s valuable to work towards building layers and more capable wallets, because there’s certainly market demand for it. Your criticisms of them should be taken seriously and work should be done to address them, but ideally that work is done without changing the base layer.

As for soft forks that add new opcodes, I think their utility needs to not only be demonstrated, but there also needs to be broader market demand. I think it’s fair to say that remains to be seen. But it’s worthwhile to build that tools so that they are mature by the time they are needed.

Again, apologies for the misunderstanding, and if I have any future concerns, next time I’ll raise those privately.

JeremyRubin changed the base branch on Dec 4, 2022

BitcoinErrorLog commented at 8:33 am on December 5, 2022: none

@cryptoquick Apology accepted!

[TESTS] Update Transaction Tests to permit setting a flag as always on and disabling the exhaustive failure test 98f53139fb

[TESTS] Allow tx_invalid.json tests to include flag rules for if_unset: [A,B,C] then_unset: [D] e49075662a

[Backport Compatibility] Shim for block_index to be named pindex to keep patchset diff for CTV minimized for 0.23 484c8f6555

Add StandardTemplateHash definition 42c760cc8d

Add SignatureChecker method for checking DefaultCheckTemplateVerifyHash. 39f25fe03e

Add OP_CHECKTEMPLATEVERIFY Opcode as OP_NOP4

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

198cab2a40

Fixup: Make CTV Opcode use Span 1c9921bbf3

Change printing of NOP4 to CheckTemplateVerify (separate commit for ease of bisecting should downstream software expect to parse NOP4 this now fails) 13db26d8bd

[Anti-DoS Caching] Precompute the DefaultCheckTemplateVerifyHash 1ef57bc5ed

[Refactor] Document and pack the fields in the PrecomputedData 9e5007ef1f

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

[Anti-DoS Caching] Make PrecomputedTransactionData non-const and lazily init BIP-119 hash data 06255ade68

[Anti-DoS Caching] Refactor BIP-119 LazyInit to use lambda for dependency injecting sync primitives for laziness

This is required given that the script interpreter translation unit
cannot link pthread.

e958000425

Make Bare OP_CHECKTEMPLATEVERIFY basic transactions standard eda70157bd

OP_CHECKTEMPLATEVERIFY Deployment parameters 24b42832c7

[TESTS] Add OP_CHECKTEMPLATEVERIFY functional tests 203fc0ed1f

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

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

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

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

[TESTS] Fixup lazy injection f186c211a5

FIXUP: [TEST] Rename status-next to status_next for rpc_blockchain getdeploymentinfo test 02df478bac

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

JeremyRubin force-pushed on Dec 17, 2022

JeremyRubin added the label Up for grabs on Dec 17, 2022

JeremyRubin commented at 4:06 am on December 17, 2022: contributor

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.

JeremyRubin closed this on Dec 17, 2022

bitcoin locked this on Dec 17, 2023

maflcko removed the label Up for grabs on Feb 6, 2024

Implement BIP-119 Validation (CheckTemplateVerify) #21702

Reviews

Conflicts

Coverage

bench/ctv vs. $mergebase (absolute)

bench/ctv vs. $mergebase (relative)

bench/ctv vs. $mergebase (absolute)

bench/ctv vs. $mergebase (relative)