Enforce Taproot script flags whenever WITNESS is set

MarcoFalke commented at 10:59 am on November 17, 2021: member

Now that Taproot is active, it makes sense to enforce its rules on all blocks, even historic ones, regardless of the deployment status.

Benefits:

(With “script flags” I mean “taproot script verification flags”.)

Script flags are known ahead for all blocks (even blocks not yet created) and do not change. This may benefit static analysis, code review, and development of new script features that build on Taproot.
Any future bugs introduced in the deployment code won’t have any effect on the script flags, as they are independent of deployment.
Enforcing the taproot rules regardless of the deployment status makes testing easier because invalid blocks after activation are also invalid before activation. So there is no need to differentiate the two cases.
It gives belt-and-suspenders protection against a practically expensive and theoretically impossible IBD reorg attack where the node is eclipsed. While nMinimumChainWork already protects against this, the cost for a few months worth of POW might be lowered until a major version release of Bitcoin Core reaches EOL. The needed work for the attack is the difference between nMinimumChainWork and the work at block 709632.

For reference, previously the same was done for P2SH and WITNESS in commit 0a8b7b4b33c9d78574627fc606267e2d8955cd1c.

Implementation:

I found one block which fails verification with the flags applied, so I added a TaprootException, similar to the BIP16Exception.

For reference, the debug log:

0ERROR: ConnectBlock(): CheckInputScripts on b10c007c60e14f9d087e0291d4d0c7869697c6681d979c6639dbd960792b4d41 failed with non-mandatory-script-verify-flag (Witness program was passed an empty witness)
1BlockChecked: block hash=0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad state=non-mandatory-script-verify-flag (Witness program was passed an empty witness)
2InvalidChainFound: invalid block=0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad  height=692261  log2_work=92.988459  date=2021-07-23T08:24:20Z
3InvalidChainFound:  current best=0000000000000000000067b17a4c0ffd77c29941b15ad356ca8f980af137a25d  height=692260  log2_work=92.988450  date=2021-07-23T07:47:31Z
4ERROR: ConnectTip: ConnectBlock 0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad failed, non-mandatory-script-verify-flag (Witness program was passed an empty witness)

Hint for testing, make sure to set -noassumevalid.

Considerations

Obviously this change can lead to consensus splits on the network in light of massive reorgs. Currently the last block before Taproot activation, that is the last block without the Taproot script flags set, is only buried by a few days of POW. However, when and if this patch is included in the next major release, it will be buried by a few months of POW. BIP90 considerations apply when looking at reorgs this large.

laanwj added the label Consensus on Nov 17, 2021

0xB10C commented at 3:22 pm on November 17, 2021: member

Concept ACK on burying taproot deployment in the next major release :grimacing:

JeremyRubin commented at 4:04 pm on November 17, 2021: contributor

concept ACK – note that the IsNull v.s. optional doesn’t really matter; i do mildly prefer consistency though with the BIP16Exception.

DrahtBot commented at 2:51 am on November 18, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22953 (refactor: introduce single-separator split helper (boost::split replacement) by theStack)

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.

Sjors commented at 12:50 pm on November 18, 2021: member

Concept ACK. Not sure what the right amount of PoW is to burry this under. How much was SegWit burried under?

MarcoFalke commented at 1:48 pm on November 18, 2021: member

@Sjors Segwit was activated in ~summer 2017 and #11739 was created in ~fall 2017, however it wasn’t merged until April 2018, and first included in the 17.0 release (Oct 2018). If it was merged earlier it could have been included in 16.0 (Feb 2018). If the changes here are included in the next major release, they will have ~5 months of POW until 23.0 (April 2022).

in src/validation.cpp:1600 in fa75e6e79a outdated

1594@@ -1595,6 +1595,13 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
1595     {
1596         // Enforce WITNESS rules whenever P2SH is in effect
1597         flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS;
1598+        // Enforce Taproot (BIP340-BIP342) whenever WITNESS is in effect,
1599+        // unless except
1600+        if (!consensusparams.TaprootException.has_value() ||          // no Taproot exception on this chain

jamesob commented at 7:53 pm on November 18, 2021:

Can just be !consensusparams.TaprootException (https://en.cppreference.com/w/cpp/utility/optional/operator_bool).

MarcoFalke commented at 2:28 pm on November 22, 2021:

Is there a reason to prefer one over the other?

MarcoFalke commented at 10:34 am on November 25, 2021:

Gone

jamesob commented at 7:55 pm on November 18, 2021: member

Concept ACK, for consistency with previous soft fork deployments and the benefits well articulated in the PR description.

dunxen commented at 5:30 pm on November 21, 2021: contributor

Concept ACK

Running verification since genesis now.

in test/functional/feature_taproot.py:1172 in fa75e6e79a outdated

1170@@ -1171,13 +1171,9 @@ def spenders_taproot_inactive():
1171 
1172     # Test that keypath spending is valid & non-standard, regardless of validity.

Sjors commented at 12:13 pm on November 22, 2021:

Needs to drop “regardless of validity” (except for scriptpath_invalid_unkleaf)

MarcoFalke commented at 2:28 pm on November 22, 2021:

Good point. I’ll fix the comment on the next force push with any other nits, if there are any. The force push is currently scheduled to happen after #23512 is merged.

MarcoFalke commented at 10:34 am on November 25, 2021:

Reworked test

Sjors commented at 2:03 pm on November 22, 2021: member

Code in fa75e6e79a1635480786a8a001d2ebff07e8bc8d looks good.

Verified that signet and testnet3 indeed do not need an exception block. Currently verifying that mainnet does need the exception (by dropping it), and whether that’s the only one.

Update 28-11: also tested mainnet

DrahtBot added the label Needs rebase on Nov 25, 2021

MarcoFalke force-pushed on Nov 25, 2021

MarcoFalke commented at 10:36 am on November 25, 2021: member

Rebased with the following changes:

Two refactoring cleanup commits per myself
Use plain uint256 (not optional) per @JeremyRubin
Rework test per @Sjors

DrahtBot removed the label Needs rebase on Nov 25, 2021

in src/consensus/params.h:10 in fa386f6a27 outdated

 6@@ -7,6 +7,7 @@
 7 #define BITCOIN_CONSENSUS_PARAMS_H
 8 
 9 #include <uint256.h>
10+

JeremyRubin commented at 5:29 pm on November 25, 2021:

nit

MarcoFalke commented at 5:33 pm on November 25, 2021:

Yes, there should be a newline between project-internal includes and project-external ones.

MarcoFalke commented at 3:55 pm on November 30, 2021:

Force pushed to remove the nit-fix, since it is being fixed in another pull already.

JeremyRubin commented at 5:34 pm on November 25, 2021: contributor

if tests pass this should be OK, but the removal of the nullptr check and change to assert does seem a bit creepy to me.

gruve-p commented at 5:35 pm on November 25, 2021: contributor

Concept ACK

MarcoFalke commented at 5:40 pm on November 25, 2021: member

if tests pass this should be OK, but the removal of the nullptr check and change to assert does seem a bit creepy to me.

Can you elaborate why this is creepy? All call sites assume the hash to be existing. See the commit message and for example:

https://github.com/bitcoin/bitcoin/blob/fa65b65821add01e28118899d4f2eef98aa4c37a/src/validation.cpp#L1643

fjahr commented at 5:05 pm on November 27, 2021: member

Concept ACK and light Code Review ACK

I will do some testing in the next days.

MarcoFalke force-pushed on Nov 30, 2021

MarcoFalke commented at 3:56 pm on November 30, 2021: member

Force pushed last commit, only change is whitespace related

in src/validation.cpp:1593 in fa5fb6f2f2 outdated

1588@@ -1589,43 +1589,42 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
1589     // mainnet and testnet), so for simplicity, always leave P2SH
1590     // on except for the one violating block.
1591     if (consensusparams.BIP16Exception.IsNull() || // no bip16 exception on this chain
1592-        pindex->phashBlock == nullptr || // this is a new candidate block, eg from TestBlockValidity()
1593-        *pindex->phashBlock != consensusparams.BIP16Exception) // this block isn't the historical exception
1594+        block_index.phashBlock == nullptr || // this is a new candidate block, eg from TestBlockValidity()
1595+        *block_index.phashBlock != consensusparams.BIP16Exception) // this block isn't the historical exception

Sjors commented at 5:36 am on December 1, 2021:

Maybe clarify this into *(block_index.phashBlock)?

I tested that this indeed syncs past the BIP16Exception block, and that - conversely - chaning the exception block to a non-existing block causes a failure at height 170,060.

MarcoFalke commented at 7:38 am on December 1, 2021:

In a later commit this will change to *Assert(block_index.phashBlock), so I think there is nothing left to do here?

Sjors commented at 11:47 am on December 1, 2021:

Assuming that Assert() stays where it is, then indeed it already has the brackets.

in src/validation.cpp:942 in fa5fb6f2f2 outdated

938@@ -939,7 +939,7 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws)
939     // There is a similar check in CreateNewBlock() to prevent creating
940     // invalid blocks (using TestBlockValidity), however allowing such
941     // transactions into the mempool can be exploited as a DoS attack.
942-    unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(m_active_chainstate.m_chain.Tip(), chainparams.GetConsensus());
943+    unsigned int currentBlockScriptVerifyFlags{GetBlockScriptFlags(*m_active_chainstate.m_chain.Tip(), chainparams.GetConsensus())};

Sjors commented at 5:43 am on December 1, 2021:

CChain does not guarantee that Tip() != nullptr. Maybe add an assert as well as a comment for why it’s safe to assume in this context Tip() exists?

MarcoFalke commented at 7:37 am on December 1, 2021:

Seems unrelated to the changes here? Generally, it is not possible to run a mempool without a chainstate or a chainstate without genesis block (i.e. no tip). If the assert is added, I recommend doing it in PreChecks, not in the checks that are done last. You may review #23630 which adds the assert.

Sjors commented at 11:47 am on December 1, 2021:

It’s in reference to the * which dereferences Tip()

MarcoFalke commented at 11:51 am on December 1, 2021:

Ok, the first time that Tip is dereferenced is in if (!CheckFinalTx(m_active_chainstate.m_chain.Tip() in PreChecks.

The function actually has the assert you are asking for:

0    assert(active_chain_tip); // TODO: Make active_chain_tip a reference

in src/validation.cpp:1592 in fa65b65821 outdated

1588@@ -1589,8 +1589,7 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Co
1589     // mainnet and testnet), so for simplicity, always leave P2SH
1590     // on except for the one violating block.
1591     if (consensusparams.BIP16Exception.IsNull() || // no bip16 exception on this chain
1592-        block_index.phashBlock == nullptr || // this is a new candidate block, eg from TestBlockValidity()
1593-        *block_index.phashBlock != consensusparams.BIP16Exception) // this block isn't the historical exception
1594+        *Assert(block_index.phashBlock) != consensusparams.BIP16Exception) // this block isn't the historical exception

Sjors commented at 6:57 am on December 1, 2021:

It seems more readable to me to move this assert into the main function body. On mainnet this doesn’t change anything, and I checked that it doesn’t break any tests.

MarcoFalke commented at 7:40 am on December 1, 2021:

the main function already has the assert: #23536 (comment)

This is just another belt-and-suspenders assert.

in src/validation.cpp:1598 in fa3335bcc1 outdated

1592@@ -1593,6 +1593,13 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Co
1593     {
1594         // Enforce WITNESS rules whenever P2SH is in effect
1595         flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS;
1596+        // Enforce Taproot (BIP340-BIP342) whenever WITNESS is in effect,
1597+        // unless except
1598+        if (!consensusparams.TaprootException.IsNull() ||                        // no Taproot exception on this chain

Sjors commented at 7:13 am on December 1, 2021:

Did you mean to drop the !?

MarcoFalke commented at 7:47 am on December 1, 2021:

Ah yes, good catch!

Maybe my preference would be to drop the IsNull check and purely rely on the the actual comparison, which is already sufficient and necessary.

Sjors commented at 11:45 am on December 1, 2021:

It’s nice for it to be identifcal to the BIP16 check, which made this mistake easier to see. It’s also probably more similar to what future code will look like that has to handle multiple blocks.

Sjors commented at 7:16 am on December 1, 2021: member

Mostly happy with fa3335bcc143beb7585ebf8c01c028b1762fd77a

I still have to re-run the above sanity checks with -assumevalid=0

refactor: Pass const reference instead of pointer to GetBlockScriptFlags

The function dereferences the pointer and can not accept nullptr. Change
the arg to a const reference to clarify this for the caller.

faadc606c7

Remove nullptr check in GetBlockScriptFlags

Commit d59b8d6aa1102ffac980c89e96105ddec9cfb579 removed the need for
this check and it was never needed.

fa42299411

MarcoFalke force-pushed on Dec 1, 2021

Sjors approved

Sjors commented at 2:06 pm on December 2, 2021: member

tACK fa9a4d103ff673724dfeb53b10a2707a81539d68

ajtowns commented at 4:22 am on December 8, 2021: member

For what it’s worth, I don’t find the listed benefits very persuasive:

Script verification flags are constant and do not change as the chain progresses.

The script verification flags aren’t constant – there’s an exception for a particular block in the chain that matters most.

Any future bugs introduced in the deployment code won’t have any effect on the script flags, as they are independent of deployment.

Depends a lot on how severe the bugs are; undefined behaviour could do anything after all. Simply burying the deployment to a fixed height would achieve this goal better, I think.

Enforcing the taproot rules regardless of the deployment status makes testing easier because invalid blocks after activation are also invalid before activation. So there is no need to differentiate the two cases.

For testing on regtest or signet, taproot is already always active; there’s no need to change the deployment rules to have taproot rules applied to the mempool. I believe with this patch, we’re unable to test the code path that’s executed for mainnet block 692261 except by validating the entire mainnet chain up until that point, where prior to this patch we test it automatically on regtest as part of feature_taproot.

It may alert a miner earlier if they produce taproot invalid blocks. For example, if they test their system on a test chain where taproot isn’t active yet. Producing blocks with invalid taproot spends may hurt them only when they switch to mainnet.

The only way this would happen is if they’re mining their own fork of testnet without having synced to anytime since July; and they’d need to be mining pay to taproot spends into those blocks (which they’re presumably not getting from public testnet, since they’re not synced to the public testnet), and then mining non-standard transactions spend them without following taproot rules (which again they’re not getting from public testnet or they’d be synced with public testnet, so are somehow generating them themselves?). That doesn’t sound very plausible to me…

So I don’t think those benefits would justify this change. But…

For reference, previously the same was done for P2SH and WITNESS in commit 0a8b7b4.

That references an irc meeting which seems to have a different rationale: that if you’re in IBD on mainnet, you won’t accept a forked chain with invalid taproot spends (even if you’re eclipsed). eg:

333 2017-10-12T19:12:53 <morcos> sipa: i just really hate the attack scenarios that involve feeding alternate chains that eventually get reorged out but possibly with poor consequences... perhaps this is not a problem with segwit, but perhaps it is... say your wallet loses money in an unexpected way or something?

I’m not sure that’s a huge benefit; but I could see there being potential for a reorg as IBD completes to somehow leave you with an invalid taproot spend in the mempool that you might try to include in a block you mine later invalidating that block, or, more plausibly, that you might relay a tx paying to your own taproot address, that then gets mined on the fork and spend via an invalid taproot spend, making you incorrectly think that you’ve lost funds.

Again, not sure that’s a huge benefit, but that does seem to me to justify the change. So ConceptACK.

in src/validation.cpp:1599 in fa9a4d103f outdated

1592@@ -1593,6 +1593,13 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Co
1593     {
1594         // Enforce WITNESS rules whenever P2SH is in effect
1595         flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS;
1596+        // Enforce Taproot (BIP340-BIP342) whenever WITNESS is in effect,
1597+        // unless except
1598+        if (consensusparams.TaprootException.IsNull() ||                         // no Taproot exception on this chain
1599+            *Assert(block_index.phashBlock) != consensusparams.TaprootException) // this block is not the historical exception

ajtowns commented at 4:27 am on December 8, 2021:

Arguably it would also be correct to write this as:

0const auto& h = *Assert(block_index.phashBlock);
1if (h.IsNull() || (h != BIP16Exception && h != TaprootException)) {
2    flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT;
3}

since the taproot exception block has already been validated to comply with p2sh and segwit tx rules (and the inclusion of witness data is handled via BLOCK_OPT_WITNESS elsewhere)

MarcoFalke commented at 10:11 am on December 10, 2021:

It seems there is a typo in your idea. h.IsNull() should never evaluate to true. Maybe you meant to write

0const auto& h = *Assert(block_index.phashBlock);
1if (BIP16Exception.IsNull() || TaprootException.IsNull() || (h != BIP16Exception && h != TaprootException)) {
2    flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT;
3}

Though, this doesn’t work, as I expect it to fail to sync testnet3.

As I mentioned earlier, just dropping the IsNull check seems simpler (https://github.com/bitcoin/bitcoin/pull/23536#discussion_r759927632).

So your code would be

0const auto& h = *Assert(block_index.phashBlock);
1if (h != BIP16Exception && h != TaprootException) {
2    flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT;
3}

which should work (Haven’t synced any chain with it yet). Happy to switch if reviewers think it is worth it.

ajtowns commented at 9:42 am on December 17, 2021:

I don’t think it’s worth including in this PR – if it’s worth including at all, doing so in an eventual followup is fine.

If we’re sure h.IsNull() is never true, then BIP16Exception.IsNull() implies h != BIP16Exception so you don’t need the IsNull test at all. I thought it was supposed to be able to be null if you were generating a new block and trying to work out which txs you were allowed to include. The way I’m thinking of this is:

features that will be used by many wallets (segwit, taproot, p2sh) should be “always” enforced if possible; this reduces the risk of bugs that might be triggered by an attacker against a node while it’s doing IBD
there have to be some exceptions unfortunately, but the exceptions aren’t the interesting part at all, so should be emphasising the enforcement path more than the exception logic
since the block hash already commits to everything that goes into proving the validity, actually running the checks is only useful to validate the code hasn’t changed it’s behaviour, not that the block is valid or not. that is, if we ever report that the exception blocks are invalid, that’s a bug in the code, not a problem with the block.

Maybe a cleaner approach would be:

 0// add to Consensus::Params or CChainState
 1// (lists the rules that *don't* apply to exception blocks)
 2const unordered_map<uint256, uint32_t, BlockHasher> script_verify_exception_blocks = {
 3    { BIP16Exception, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT },
 4    { TaprootException, SCRIPT_VERIFY_TAPROOT },
 5};
 6
 7// validation:
 8flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT;
 9
10const auto& i = script_verify_exception_blocks.find(*Assert(block_index.phashBlock))
11if (i != script_verify_exception_blocks.end()) {
12    // block is special case exception to some rules
13    flags &= ~(i->second);
14}

Then for chains where there’s no exceptions, the map is just empty.

MarcoFalke commented at 10:08 am on December 17, 2021:

If we’re sure h.IsNull() is never true

Yes, I think we can be pretty certain about this. Both call sites assume the hash of the passed block is not null:

E.g.

0    assert(*pindex->phashBlock == block.GetHash());

The IsNull was added based on this comment: #11739 (review)

MarcoFalke commented at 10:35 am on December 17, 2021:

Maybe a cleaner approach would be:

Not sure if I like your suggestion to split “flags math” into two different files. This would make reading the code harder. Based on your earlier suggestion, just treating a set of block hashes as “assumed valid” with regard to script verification flags seems cleaner:

0
1const std::set<uint256> script_verify_exception_blocks;
2
3const auto& it_ex{script_verify_exception_blocks.find(*Assert(block_index.phashBlock))};
4if (it_ex != script_verify_exception_blocks.end()) {
5    flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT;
6}

ajtowns commented at 10:39 am on December 17, 2021:

Putting the map in CChainState rather than Consensus::Params would allow keeping it validation rather than splitting it into validation/chainparams fwiw. Agree that your suggestion also works fine.

MarcoFalke commented at 1:00 pm on December 17, 2021:

This is the full diff (checked that it compiles, but didn’t sync the chain). Maybe instead of a follow-up we can decide whether it is a good idea, and if yes, just push it here?

  0commit bb064cf5ba34ac3214577a55aa40702e1dc4d19c
  1Author: MarcoFalke <falke.marco@gmail.com>
  2Date:   Fri Dec 17 13:57:39 2021 +0100
  3
  4    set
  5
  6diff --git a/src/chainparams.cpp b/src/chainparams.cpp
  7index b0bded4355..a14633a80a 100644
  8--- a/src/chainparams.cpp
  9+++ b/src/chainparams.cpp
 10@@ -65,8 +65,10 @@ public:
 11         consensus.signet_blocks = false;
 12         consensus.signet_challenge.clear();
 13         consensus.nSubsidyHalvingInterval = 210000;
 14-        consensus.BIP16Exception = uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22");
 15-        consensus.TaprootException = uint256S("0x0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad");
 16+        consensus.script_verify_exception_blocks.emplace( // BIP16 exception
 17+            uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"));
 18+        consensus.script_verify_exception_blocks.emplace( // Taproot exception
 19+            uint256S("0x0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad"));
 20         consensus.BIP34Height = 227931;
 21         consensus.BIP34Hash = uint256S("0x000000000000024b89b42a942fe0d9fea3bb44ab7bd1b19115dd6a759c0808b8");
 22         consensus.BIP65Height = 388381; // 000000000000000004c2b624ed5d7756c508d90fd0da2c7c679febfa6c4735f0
 23@@ -185,8 +187,8 @@ public:
 24         consensus.signet_blocks = false;
 25         consensus.signet_challenge.clear();
 26         consensus.nSubsidyHalvingInterval = 210000;
 27-        consensus.BIP16Exception = uint256S("0x00000000dd30457c001f4095d208cc1296b0eed002427aa599874af7a432b105");
 28-        consensus.TaprootException = uint256::ZERO;
 29+        consensus.script_verify_exception_blocks.emplace( // BIP16 exception
 30+            uint256S("0x00000000dd30457c001f4095d208cc1296b0eed002427aa599874af7a432b105"));
 31         consensus.BIP34Height = 21111;
 32         consensus.BIP34Hash = uint256S("0x0000000023b3a96d3484e5abb3755c413e7d41500f8e2a5c3f0dd01299cd8ef8");
 33         consensus.BIP65Height = 581885; // 00000000007f6655f22f98e72ed80d8b06dc761d5da09df0fa1dc4be4f861eb6
 34@@ -325,8 +327,6 @@ public:
 35         consensus.signet_blocks = true;
 36         consensus.signet_challenge.assign(bin.begin(), bin.end());
 37         consensus.nSubsidyHalvingInterval = 210000;
 38-        consensus.BIP16Exception = uint256{};
 39-        consensus.TaprootException = uint256::ZERO;
 40         consensus.BIP34Height = 1;
 41         consensus.BIP34Hash = uint256{};
 42         consensus.BIP65Height = 1;
 43@@ -394,8 +394,6 @@ public:
 44         consensus.signet_blocks = false;
 45         consensus.signet_challenge.clear();
 46         consensus.nSubsidyHalvingInterval = 150;
 47-        consensus.BIP16Exception = uint256();
 48-        consensus.TaprootException = uint256::ZERO;
 49         consensus.BIP34Height = 1; // Always active unless overridden
 50         consensus.BIP34Hash = uint256();
 51         consensus.BIP65Height = 1;  // Always active unless overridden
 52diff --git a/src/consensus/params.h b/src/consensus/params.h
 53index 97e755a80b..b9b9b6de78 100644
 54--- a/src/consensus/params.h
 55+++ b/src/consensus/params.h
 56@@ -7,7 +7,9 @@
 57 #define BITCOIN_CONSENSUS_PARAMS_H
 58 
 59 #include <uint256.h>
 60+
 61 #include <limits>
 62+#include <set>
 63 
 64 namespace Consensus {
 65 
 66@@ -70,10 +72,13 @@ struct BIP9Deployment {
 67 struct Params {
 68     uint256 hashGenesisBlock;
 69     int nSubsidyHalvingInterval;
 70-    /* Block hash that is excepted from BIP16 enforcement, or Null if none exists */
 71-    uint256 BIP16Exception;
 72-    /* Block hash that is excepted from SCRIPT_VERIFY_TAPROOT enforcement, or Null if none exists */
 73-    uint256 TaprootException;
 74+    /**
 75+     * Hashes of blocks that are
 76+     * - known to be consensus valid, and
 77+     * - buried in the chain, and
 78+     * - fail if script flags from the chain tip are applied.
 79+     */
 80+    std::set<uint256> script_verify_exception_blocks;
 81     /** Block height and hash at which BIP34 becomes active */
 82     int BIP34Height;
 83     uint256 BIP34Hash;
 84diff --git a/src/validation.cpp b/src/validation.cpp
 85index a1c5a2f286..5677ff4952 100644
 86--- a/src/validation.cpp
 87+++ b/src/validation.cpp
 88@@ -1586,20 +1586,15 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Co
 89     // BIP16 didn't become active until Apr 1 2012 (on mainnet, and
 90     // retroactively applied to testnet)
 91     // However, only one historical block violated the P2SH rules (on both
 92-    // mainnet and testnet), so for simplicity, always leave P2SH
 93-    // on except for the one violating block.
 94-    if (consensusparams.BIP16Exception.IsNull() || // no bip16 exception on this chain
 95-        *Assert(block_index.phashBlock) != consensusparams.BIP16Exception) // this block isn't the historical exception
 96-    {
 97-        // Enforce WITNESS rules whenever P2SH is in effect
 98-        flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS;
 99-        // Enforce Taproot (BIP340-BIP342) whenever WITNESS is in effect,
100-        // unless except
101-        if (consensusparams.TaprootException.IsNull() ||                         // no Taproot exception on this chain
102-            *Assert(block_index.phashBlock) != consensusparams.TaprootException) // this block is not the historical exception
103-        {
104-            flags |= SCRIPT_VERIFY_TAPROOT;
105-        }
106+    // mainnet and testnet).
107+    // Similarly, only one historical block violated the TAPROOT rules on
108+    // mainnet.
109+    // For simplicity, always leave P2SH+WITNESS+TAPROOT on except for the two
110+    // violating blocks.
111+
112+    const auto& it_exception{consensusparams.script_verify_exception_blocks.find(*Assert(block_index.phashBlock))};
113+    if (it_exception != consensusparams.script_verify_exception_blocks.end()) {
114+        flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT;
115     }
116 
117     // Enforce the DERSIG (BIP66) rule

ajtowns commented at 1:37 pm on December 17, 2021:

If you’re using a set rather than a map, you can just say script_verify_exception_blocks.count(...) != 0 and avoid the iterator and the .end() check.

MarcoFalke commented at 10:23 am on December 20, 2021:

Ok, done in #23536 (comment). Happy to revert to previous ACKed commit if needed.

in test/functional/feature_taproot.py:1188 in fa9a4d103f outdated

1190-    # Same for scriptpath spending (and features like annex, leaf versions, or OP_SUCCESS don't change this)
1191     add_spender(spenders, "inactive/scriptpath_valid", key=sec, tap=tap, leaf="pk", standard=Standard.V23, inputs=[getter("sign")])
1192-    add_spender(spenders, "inactive/scriptpath_invalidsig", key=sec, tap=tap, leaf="pk", standard=False, inputs=[getter("sign")], sighash=bitflipper(default_sighash))
1193-    add_spender(spenders, "inactive/scriptpath_invalidcb", key=sec, tap=tap, leaf="pk", standard=False, inputs=[getter("sign")], controlblock=bitflipper(default_controlblock))
1194+
1195+    # Test that features like annex, leaf versions, or OP_SUCCESS are valid but non-standard

ajtowns commented at 4:32 am on December 8, 2021:

Is there any value to keeping any of the inactive/ test cases? They aren’t being tested with taproot inactive anymore, and the behaviour when taproot is active should already be being tested elsewhere…

MarcoFalke commented at 10:32 am on December 10, 2021:

Good point. Currently they are used to run against previous releases, but given that the tests passed once one the previous releases (and previous releases are “constant”), we can probably remove this code. Happy to look into this as a follow-up unless reviewers want me to do it here.

Sjors commented at 7:19 pm on January 29, 2022:

I’m confused about what the test is doing with respect to previous releases, and wonder if the name spenders_taproot_inactive is still appropriate.

When run with --previous_release, it runs against v0.20.1 which does not have Taproot, so that makes sense. But otherwise it sets -vbparams=taproot:1:1, which is a no-op as of this PR (except for the getblockchaininfo RPC).

MarcoFalke commented at 8:38 am on January 30, 2022:

I’ll rename the function name with the other test cleanups in a follow-up. I’d say we should focus on the consensus change here and leave style changes in the test for later.

MarcoFalke commented at 12:28 pm on April 1, 2022:

Renamed the function (among other cleanup) in https://github.com/bitcoin/bitcoin/pull/24737

ajtowns commented at 4:45 am on December 8, 2021: member

utACK fa9a4d103ff673724dfeb53b10a2707a81539d68

Checked that testnet reindexes to tip with this patch and noassumevalid. Have not checked mainnet.

MarcoFalke commented at 10:29 am on December 10, 2021: member

Thank you for reviewing the pull request description @ajtowns . The use of the word “constant” was indeed wrong. I’ve replaced it with “known in advance”. Also, I’ve removed the section talking about miner testing.

333 2017-10-12T19:12:53 sipa: i just really hate the attack scenarios that involve feeding alternate chains that eventually get reorged out but possibly with poor consequences… perhaps this is not a problem with segwit, but perhaps it is… say your wallet loses money in an unexpected way or something? I’m not sure that’s a huge benefit; but I could see there being potential for a reorg as IBD completes to somehow leave you with an invalid taproot spend in the mempool that you might try to include in a block you mine later invalidating that block, or, more plausibly, that you might relay a tx paying to your own taproot address, that then gets mined on the fork and spend via an invalid taproot spend, making you incorrectly think that you’ve lost funds. Again, not sure that’s a huge benefit, but that does seem to me to justify the change. So ConceptACK.

Unless I am missing something this seems to be a reason against this change, no? Currently we won’t process any blocks that are not part of a chain that has at least the amount of work of the main chain at the time of the software release. So the attack that this patch is supposed to “fix” assumes a massive reorg. With a reorg this massive the change here isn’t safe because it may split network consensus into two. And having split consensus in two, you can launch other attacks.

So I don’t think the patch here should be thought as a countermeasure against some kind of (reorg) attack. The motivation for it is mostly nifty. Or to quote the IRC meeting you linked to: “cleanest”, “very nice property”.

dunxen commented at 5:45 am on December 12, 2021: contributor

tACK fa9a4d1

Reverified mainnet chain with assumevalid=0 and also confirmed that the exception is needed.

ajtowns commented at 10:35 am on December 17, 2021: member

Unless I am missing something this seems to be a reason against this change, no? Currently we won’t process any blocks that are not part of a chain that has at least the amount of work of the main chain at the time of the software release. So the attack that this patch is supposed to “fix” assumes a massive reorg. With a reorg this massive the change here isn’t safe because it may split network consensus into two. And having split consensus in two, you can launch other attacks.

I think the massive reorg that’s assumed is from a fake, lower-work chain to the current chain which is much easier than doing a “real” reorg. The idea being:

an attacker eclipses your node during IBD so that you can only follow their chain
your node is not running this code, so accepts a non-standard taproot invalid tx because they include it in some block in their chain

Then either,

the attacker then allows you to reorg to the main chain, at which point the taproot invalid tx perhaps enters your mempool
despite the huge reorg and eventual activation of taproot that tx doesn’t get removed from your mempool
you then include that tx in blocks you attempt to mine, rendering them invalid

or

the invalid tx spends to or from an address of yours, causing your wallet balance to be incorrect
due to the shock, you have a heart attack and die, losing access to your coins?

Because you’re reorging to the main chain, you can do those attacks with substantially less resources than it would take to reorg the current chain back prior to taproot’s activation. Hence I think that’s a reason for this change (and why it’s a “nice property” to have).

Considering three ways of enforcing taproot:

bip341 rules (current)
unconditional activation at height 709632 (#23505)
activation except for block 0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad (this pr)

then the first two will only result in differences if there is a reorg back to block 687283 (and taproot was prevented from activating via bip341 rules, or timewarp was used to delay activation to a later block), whereas the latter could result in differences if there is a reorg back to block 709630 (with block 709631 being taproot invalid). I think the longer reorg would require about 4.6x as much work as the latter at present. Presuming this change isn’t released until April, that’s another ~15000 blocks, and the ratio would presumably be 1.8x.

But either would already be over 4800 blocks (and by April would be ~20,000 blocks), which I’d expect to result in a network split independently of any activation rules (due to people invalidating the reorg and continuing to follow/mine the lower work chain)… Hence I don’t think the risk of a higher-work reorg of the current chain is a reason against this change.

MarcoFalke commented at 11:02 am on December 17, 2021: member

Sorry, I still don’t follow how an eclipse attack can disable our protection against reorgs during IBD.

The code:

0    if (!fRequested) {  // If we didn't ask for it:
1// .. snip ..
2
3        // Protect against DoS attacks from low-work chains.
4        // If our tip is behind, a peer could try to send us
5        // low-work blocks on a fake chain that we would never
6        // request; don't process these.
7        if (pindex->nChainWork < nMinimumChainWork) return true;
8    }

And fRequested is never set for “boring” peers. See:

0    if (...snip... || state->pindexBestKnownBlock->nChainWork < nMinimumChainWork) {
1        // This peer has nothing interesting.
2        return;
3    }

If this protection didn’t work, we should fix it. Again, I don’t think seeing this pull request as a “bug fix” for an attack is the right approach. If there was a bug, it would be exploitable on current master (with witness v2 outputs maybe?).

ajtowns commented at 12:39 pm on December 17, 2021: member

Witness v2 outputs don’t matter I think – they won’t affect your wallet (for the second case) and won’t be any more valid/invalid on the main chain than on a hypothetical lower work fork (so won’t invalidate your blocks in the first case).

The minimum chain work check would currently just mean you’d need to start your fake chain after block 691719, but presumably it’d be easiest to start just prior to taproot’s activation anyway (709632). Updating min chainwork prior to 23.0 to something well past the chain work as at taproot’s activation would likely make this attack prohibitively expensive against people running 23.0, even if this PR weren’t included. But this patch would prevent that attack even if someone were willing to do that much work (eg, perhaps there’s a huge improvement in sha256d asics making such an attack much cheaper). My feeling is that’s not super important, but still a worthwhile additional protection.

But if I’m wrong and this change isn’t protecting against bugs for people simply following mainnet, I’m not sure it makes sense to be touching consensus code in this way though: better to just bury the deployment rather than adding consensus logic for something with no tangible benefits. Burial gains both the first two benefits listed in the pr description (if you know the block’s height, you know whether taproot is enforced or not; and bugs in bip9/whatever logic no affect it), and for the third, the only cases where taproot isn’t activated is if you manually configure regtest that way, or you’re trying a massive reorg of testnet or mainnet, so presuming you don’t do those things, you can treat a buried deployment of taproot as always active too. Burial has a higher cost to creating a consensus split due to needing a deeper reorg, and seems like it has a lower risk of introducing bugs (due to less new code). (The same argument would apply to making p2sh and segwit rules apply to (almost) all blocks originally, and would also presumably apply to reverting that change or p2sh/segwit now).

MarcoFalke commented at 1:22 pm on December 17, 2021: member

Thanks, I’ve summarize your point in OP in three sentences.

MarcoFalke commented at 4:17 pm on December 17, 2021: member

the attacker then allows you to reorg to the main chain, at which point the taproot invalid tx perhaps enters your mempool

Re-reading this, I think it shouldn’t be possible for the invalid tx to enter the mempool, since MaybeUpdateMempoolForReorg calls ATMP, which requires txs to be policy and consensus valid, no?

ajtowns commented at 4:54 pm on December 17, 2021: member

the attacker then allows you to reorg to the main chain, at which point the taproot invalid tx perhaps enters your mempool

Re-reading this, I think it shouldn’t be possible for the invalid tx to enter the mempool, since MaybeUpdateMempoolForReorg calls ATMP, which requires txs to be policy and consensus valid, no?

Yeah, that’s what I’d expect (or at least hope for). But that logic’s more complicated than just “you can’t ever make a new valid block with a taproot-invalid tx”, so there’s more chance of that code having a bug, either already or sometime in the future.

MarcoFalke force-pushed on Dec 20, 2021

MarcoFalke commented at 10:21 am on December 20, 2021: member

Ok, adjusted the point in OP to “It gives belt-and-suspenders protection against a practically expensive and theoretically impossible IBD reorg attack”.

Also, apologies to reviewers, since I completely reworked the implementation of the last commit. I think std::set gives a bunch of nice benefits (over a verbose list of uint256 members):

No need to add an exception if there is none. That is, no need to handle the confusing uint256::IsNull case (or std::optional::has_value).
No nested ifs. So the code is easier to parse for humans and less susceptible to coding errors.

There is a minor downside in that P2SH and WITNESS are no longer set when verifying the one taproot exception block on mainnet. I think this is acceptable, as the block is known to be fully valid already, so there is no general need to redo all script verify checks on it. In fact, with default -assumevalid settings of the next major release, this block already won’t be re-checked.

Though, I am happy to revert to the previous ACKed commit, if needed.

MarcoFalke force-pushed on Dec 20, 2021

MarcoFalke commented at 9:18 am on December 26, 2021: member

Synced the chains with this patch locally

0xB10C commented at 1:43 pm on January 12, 2022: member

ACK faa40fc1e7e6f12f71a7e157dd920feffb3ede31

Synced faa40fc1e7e6f12f71a7e157dd920feffb3ede31 on mainnet, testnet, and signet with noassumevalid=1. Changes look good to me. Compared the taproot block hash to the hash I had in my notes… BIP-16 hash didn’t change. I think not checking P2SH and WITNESS for the taproot block isn’t a blocker (as Marco mentioned: the block won’t be checked once the assumevalid settings are update for the next release).

~While looking at the log of the nodes, I’ve noticed that they are opening a new block-relay-only outbound connection every 5 minutes (lots of New outbound peer connected: version: 70016, blocks=718290, peer=X (block-relay-only)). I don’t remember seeing this this frequent. I don’t think it’s related to this change, maybe another change that I’m not aware of.~ Apparently this is normal behavior. I just never noticed it before.

jamesob commented at 3:45 pm on January 19, 2022: member

Github ACK https://github.com/bitcoin/bitcoin/pull/23536/commits/faa40fc1e7e6f12f71a7e157dd920feffb3ede31

Can do a more formal review soon.

ajtowns commented at 6:31 am on January 28, 2022: member

ACK faa40fc1e7e6f12f71a7e157dd920feffb3ede31

Checked validation of mainnet and testnet, and that validation fails at both mainnet blocks if their hashes aren’t included in the exception blocks set (didn’t check the testnet exception).

Would be possible (in a followup PR) to allow adding exception blocks to regtest via a command line parameter, which would then allow testing validation of exception blocks, and improve code coverage of the functional tests.

Sjors commented at 9:11 am on January 28, 2022: member

There is a minor downside in that P2SH and WITNESS are no longer set when verifying the one taproot exception block on mainnet. I think this is acceptable, as the block is known to be fully valid already, so there is no general need to redo all script verify checks on it. In fact, with default -assumevalid settings of the next major release, this block already won’t be re-checked.

Though, I am happy to revert to the previous ACKed commit, if needed.

I don’t think this is a good idea. It goes beyond burying the taproot deployment. If a user wants to check for themselves if these two blocks obey all the other consensus rules, they would have to make a non-trivial code change and recompile.

ajtowns commented at 10:11 am on January 28, 2022: member

I don’t think this is a good idea. It goes beyond burying the taproot deployment. If a user wants to check for themselves if these two blocks obey all the other consensus rules, they would have to make a non-trivial code change and recompile.

This PR isn’t burying the taproot deployment – it’s applying it to old blocks and thus invalidating many theoretically possible large reorgs of mainnet or testnet, which naturally goes far beyond simply burying the deployment.

The only block with other unchecked consensus rules is the taproot exception block – for the p2sh exception blocks, there are no other consensus rules that are not being checked, since none of p2sh nor segwit nor taproot were active at that point on either mainnet or testnet.

Rather than making a code change and recompiling the easy way to validate that block would be using an old release of bitcoind prior to this change being merged, which doesn’t seem terrible to me. What that doesn’t achieve is allowing you to use that particular block as a regression test of the current bitcoind p2sh/segwit implementation, but that seems like it’s a pretty minimal benefit.

Otherwise, not really seeing the benefit of checking that that particular hardcoded historical block complies with p2sh/segwit rules; but this approach (or something like it) would allow doing that if it really is important.

Sjors commented at 11:41 am on January 28, 2022: member

@ajtowns I guess the term “burying” is ambiguous. It could mean as you say, only fixing the height of activation. I meant it as burying it all the way to genesis, with the existing block grandfathered in. In either case, this PR goes beyond that, because it no longer validates p2sh rules for a (single) block that was created long after p2sh was activated.

Downloading an earlier version does not seem like a very practical solution. I would imagine a user story like this: they first run with the default -assumevalid=1. After going down a rabbit hole, or maybe because they want to ACK a new assumevalid value, they decide to sync again with -assumevalid=0. But then they go even deeper down the rabbit hole to script_verify_exception_blocks. And let’s say the year is 2030 and we’ve added a few more soft fork exception blocks. They’d have to download multiple older binaries, which may or may not run on their system anymore. Then they have to sync them up to, but not too far beyond, the activation of each soft fork, and then switch the next version.

sdaftuar commented at 2:05 pm on January 28, 2022: member

There is a minor downside in that P2SH and WITNESS are no longer set when verifying the one taproot exception block on mainnet. I think this is acceptable, as the block is known to be fully valid already, so there is no general need to redo all script verify checks on it. In fact, with default -assumevalid settings of the next major release, this block already won’t be re-checked.

Though I have not reviewed this patch in any detail, I share @Sjors’s skepticism of this idea. I think there’s a social/cultural difference between having code that is designed to let users run all the validation checks that we believe should have been run at all points in the past, and not letting users do that. If we were debating whether to make it so that the assumevalid setting could not be turned off without a code recompile, I think we would reject the idea – yet this approach takes us down that path towards making that be the case for some of the blockchain history.

MarcoFalke force-pushed on Jan 28, 2022

MarcoFalke commented at 2:15 pm on January 28, 2022: member

Thank you for the feedback @Sjors and @sdaftuar. While I don’t see a practical need to set the p2sh flags on this particular block, I see why it goes against the general sentiment to give users the choice to “maximally” verify the chain. For the same reason assume-utxo will run validation in the background and assume-valid can be disabled at runtime.

I’ve reverted to the less controversial initial version, which already had (and hopefully still has) 3 ACKs. Apologies to everyone who reviewed the now stale branch.

ajtowns commented at 2:34 pm on January 28, 2022: member

Downloading an earlier version does not seem like a very practical solution. I would imagine a user story like this: they first run with the default -assumevalid=1. After going down a rabbit hole, or maybe because they want to ACK a new assumevalid value, they decide to sync again with -assumevalid=0. But then they go even deeper down the rabbit hole to script_verify_exception_blocks. And let’s say the year is 2030 and we’ve added a few more soft fork exception blocks. They’d have to download multiple older binaries, which may or may not run on their system anymore. Then they have to sync them up to, but not too far beyond, the activation of each soft fork, and then switch the next version.

(I think -assumevalid=1 will behave the same as -assumevalid=0 – you have to supply a hash, not true/false)

I don’t think that makes sense; saying “yes, this is an okay hash for assumevalid” means that the block with that hash is (a) valid, (b) is relatively recent, and (c) is buried by enough work that it’s unlikely that the chain will reorg past it. If you want to break down what “valid” means, then it means “bitcoin node version X will accept it, whatever set of parameters you use” for various values of X; but it doesn’t matter whether version X is running the same code as version Y to work that out, just whether or not they end up giving the same answer.

Likewise, even if version 2030.1 is running some block through its taproot code that doesn’t necessarily tell you whether version 22.0 will accept the block as valid – the code’s likely to have changed substantially inbetween, and unintentional bugs may have been introduced, so if you really want to be sure both versions give the same answer, you have to validate using both versions anyway.

(A big difference between an exception for a single block and excepting the entirety of history is that it makes it much harder to use the real blockchain as a regression test for the current code; so I don’t agree with generalising “we wouldn’t hardcode assumevalid” to “we shouldn’t hardcode any block” follows)

MarcoFalke commented at 2:42 pm on January 28, 2022: member

We can always switch to the flatter std::set approach later when the next deployment is backdated to genesis, but right now it seems minimally more controversial and the first version of the patch perfectly achieves the primary goal of this pull request (taproot since genesis).

ajtowns commented at 2:57 pm on January 28, 2022: member

I think you can get the best of both worlds using a map instead of a set:

  0diff --git a/src/chainparams.cpp b/src/chainparams.cpp
  1index b0bded4355..5943b87f37 100644
  2--- a/src/chainparams.cpp
  3+++ b/src/chainparams.cpp
  4@@ -10,6 +10,7 @@
  5 #include <deploymentinfo.h>
  6 #include <hash.h> // for signet block challenge hash
  7 #include <util/system.h>
  8+#include <script/interpreter.h>
  9 
 10 #include <assert.h>
 11 
 12@@ -65,8 +66,8 @@ public:
 13         consensus.signet_blocks = false;
 14         consensus.signet_challenge.clear();
 15         consensus.nSubsidyHalvingInterval = 210000;
 16-        consensus.BIP16Exception = uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22");
 17-        consensus.TaprootException = uint256S("0x0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad");
 18+        consensus.ScriptFlagExceptions.emplace(uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"), SCRIPT_VERIFY_NONE);
 19+        consensus.ScriptFlagExceptions.emplace(uint256S("0x0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad"), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS);
 20         consensus.BIP34Height = 227931;
 21         consensus.BIP34Hash = uint256S("0x000000000000024b89b42a942fe0d9fea3bb44ab7bd1b19115dd6a759c0808b8");
 22         consensus.BIP65Height = 388381; // 000000000000000004c2b624ed5d7756c508d90fd0da2c7c679febfa6c4735f0
 23@@ -185,8 +186,7 @@ public:
 24         consensus.signet_blocks = false;
 25         consensus.signet_challenge.clear();
 26         consensus.nSubsidyHalvingInterval = 210000;
 27-        consensus.BIP16Exception = uint256S("0x00000000dd30457c001f4095d208cc1296b0eed002427aa599874af7a432b105");
 28-        consensus.TaprootException = uint256::ZERO;
 29+        consensus.ScriptFlagExceptions.emplace(uint256S("0x00000000dd30457c001f4095d208cc1296b0eed002427aa599874af7a432b105"), SCRIPT_VERIFY_NONE);
 30         consensus.BIP34Height = 21111;
 31         consensus.BIP34Hash = uint256S("0x0000000023b3a96d3484e5abb3755c413e7d41500f8e2a5c3f0dd01299cd8ef8");
 32         consensus.BIP65Height = 581885; // 00000000007f6655f22f98e72ed80d8b06dc761d5da09df0fa1dc4be4f861eb6
 33@@ -325,8 +325,6 @@ public:
 34         consensus.signet_blocks = true;
 35         consensus.signet_challenge.assign(bin.begin(), bin.end());
 36         consensus.nSubsidyHalvingInterval = 210000;
 37-        consensus.BIP16Exception = uint256{};
 38-        consensus.TaprootException = uint256::ZERO;
 39         consensus.BIP34Height = 1;
 40         consensus.BIP34Hash = uint256{};
 41         consensus.BIP65Height = 1;
 42@@ -394,8 +392,6 @@ public:
 43         consensus.signet_blocks = false;
 44         consensus.signet_challenge.clear();
 45         consensus.nSubsidyHalvingInterval = 150;
 46-        consensus.BIP16Exception = uint256();
 47-        consensus.TaprootException = uint256::ZERO;
 48         consensus.BIP34Height = 1; // Always active unless overridden
 49         consensus.BIP34Hash = uint256();
 50         consensus.BIP65Height = 1;  // Always active unless overridden
 51diff --git a/src/consensus/params.h b/src/consensus/params.h
 52index 97e755a80b..527c5e5907 100644
 53--- a/src/consensus/params.h
 54+++ b/src/consensus/params.h
 55@@ -8,6 +8,7 @@
 56 
 57 #include <uint256.h>
 58 #include <limits>
 59+#include <map>
 60 
 61 namespace Consensus {
 62 
 63@@ -70,10 +71,8 @@ struct BIP9Deployment {
 64 struct Params {
 65     uint256 hashGenesisBlock;
 66     int nSubsidyHalvingInterval;
 67-    /* Block hash that is excepted from BIP16 enforcement, or Null if none exists */
 68-    uint256 BIP16Exception;
 69-    /* Block hash that is excepted from SCRIPT_VERIFY_TAPROOT enforcement, or Null if none exists */
 70-    uint256 TaprootException;
 71+    /* Block hashes that do not have default script flags */
 72+    std::map<uint256,uint32_t> ScriptFlagExceptions;
 73     /** Block height and hash at which BIP34 becomes active */
 74     int BIP34Height;
 75     uint256 BIP34Hash;
 76diff --git a/src/validation.cpp b/src/validation.cpp
 77index a1c5a2f286..e8cc5b2f6e 100644
 78--- a/src/validation.cpp
 79+++ b/src/validation.cpp
 80@@ -1581,25 +1581,11 @@ static ThresholdConditionCache warningcache[VERSIONBITS_NUM_BITS] GUARDED_BY(cs_
 81 
 82 static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Consensus::Params& consensusparams)
 83 {
 84-    unsigned int flags = SCRIPT_VERIFY_NONE;
 85-
 86-    // BIP16 didn't become active until Apr 1 2012 (on mainnet, and
 87-    // retroactively applied to testnet)
 88-    // However, only one historical block violated the P2SH rules (on both
 89-    // mainnet and testnet), so for simplicity, always leave P2SH
 90-    // on except for the one violating block.
 91-    if (consensusparams.BIP16Exception.IsNull() || // no bip16 exception on this chain
 92-        *Assert(block_index.phashBlock) != consensusparams.BIP16Exception) // this block isn't the historical exception
 93-    {
 94-        // Enforce WITNESS rules whenever P2SH is in effect
 95-        flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS;
 96-        // Enforce Taproot (BIP340-BIP342) whenever WITNESS is in effect,
 97-        // unless except
 98-        if (consensusparams.TaprootException.IsNull() ||                         // no Taproot exception on this chain
 99-            *Assert(block_index.phashBlock) != consensusparams.TaprootException) // this block is not the historical exception
100-        {
101-            flags |= SCRIPT_VERIFY_TAPROOT;
102-        }
103+    unsigned int flags = SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT;
104+
105+    // Some blocks are treated as exceptions and verified with a subset of the default flags above
106+    const auto it = consensusparams.ScriptFlagExceptions.find(*Assert(block_index.phashBlock));
107+    if (it != consensusparams.ScriptFlagExceptions.end()) {
108+        flags = it->second;
109     }
110 
111     // Enforce the DERSIG (BIP66) rule

Sjors commented at 3:21 pm on January 28, 2022: member

I like the ScriptFlagExceptions map approach too.

In my example above I should have said “not setting -assumevalid”, rather than -assumevalid=1, because indeed it takes a hash argument. My point being that users will probably do whatever the default is first, and then try -assumevalid=0. I was confused with the (proposed) -assumeutxo, where we don’t allow a user to provide a different hash at all without recompiling.

If you want to break down what “valid” means, then it means “bitcoin node version X will accept it, whatever set of parameters you use” for various values of X; but it doesn’t matter whether version X is running the same code as version Y to work that out, just whether or not they end up giving the same answer.

Likewise, even if version 2030.1 is running some block through its taproot code that doesn’t necessarily tell you whether version 22.0 will accept the block as valid – the code’s likely to have changed substantially inbetween, and unintentional bugs may have been introduced, so if you really want to be sure both versions give the same answer, you have to validate using both versions anyway.

I think the difference between accidental and intentional consensus changes is important though.

When looking for accidental consensus changes you can’t really avoid running multiple versions. This also applies to comparison with alternative implementations. This is what ForkMonitor does, it’s constantly looking for a block that is considered valid by some nodes yet invalid by others. This type of consensus incompatibility between versions most likely shows up very quickly, not in a deeply buried block. This is because not everyone upgrades at the same time.

An intentional consensus change, e.g. where developers help themselves - or a crazy litigious evildoer - to some coins, is something else. Now exploiting the burial mechanisms probably isn’t a practical attack at all, but it’s still very nice if a user can ensure this didn’t happen, by running just one piece of software, and reviewing one 1 set of code. This is especially relevant for a new user, in case they believe millions of others conspired against him in the past :-)

MarcoFalke force-pushed on Jan 29, 2022

MarcoFalke commented at 1:46 pm on January 29, 2022: member

I like the ScriptFlagExceptions map approach too.

Ok, going to push that soon.

Enforce Taproot script flags whenever WITNESS is set cccc1e70b8

MarcoFalke force-pushed on Jan 29, 2022

in src/validation.cpp:1589 in cccc1e70b8

1593-    {
1594-        // Enforce WITNESS rules whenever P2SH is in effect
1595-        flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS;
1596+    // mainnet and testnet).
1597+    // Similarly, only one historical block violated the TAPROOT rules on
1598+    // mainnet.

Sjors commented at 6:46 pm on January 29, 2022:

0// No historical blocks violated the `WITNESS` rules on mainnet and testnet.

MarcoFalke commented at 8:28 am on January 30, 2022:

Seems like a suggestion unrelated to this pull, that can be added in a separate commit or even pull request on master right now without causing a conflict here.

in src/consensus/params.h:79 in cccc1e70b8

76-    uint256 BIP16Exception;
77+    /**
78+     * Hashes of blocks that
79+     * - are known to be consensus valid, and
80+     * - buried in the chain, and
81+     * - fail if the default script verify flags are applied.

Sjors commented at 6:57 pm on January 29, 2022:

0 * We manually apply the flags that applied at the time the block was created.

MarcoFalke commented at 8:32 am on January 30, 2022:

That’d be ambiguous. You’d also have to specify that it means the flags that this version (the version before this commit) applied. Also, I don’t see a downside in adding more flags on top, if it makes sense.

So I am going to leave this as-is for now. Could be improved in a follow-up, if needed.

Sjors commented at 3:53 pm on January 30, 2022:

I thought about suggesting to add the WITNESS flag to the P2SH block, but decided that would just be confusing. And I’m not even sure if it’s sane, because part of SegWit uses P2SH.

in src/consensus/params.h:81 in cccc1e70b8

78+     * Hashes of blocks that
79+     * - are known to be consensus valid, and
80+     * - buried in the chain, and
81+     * - fail if the default script verify flags are applied.
82+     */
83+    std::map<uint256, uint32_t> script_flag_exceptions;

Sjors commented at 6:59 pm on January 29, 2022:

Slightly confusing variable name: it suggest that the flags are expected, which might make one think that the provided flags are to be subtracted. But it’s actually the blocks that are excepted from the usual flags, and we manually provide the correct flags. However I can’t think of a better name atm.

JeremyRubin commented at 7:33 pm on January 29, 2022:

script_flag_overrides_for_block

MarcoFalke commented at 8:34 am on January 30, 2022:

script_flags_exceptions means the default script flags used for exception blocks. It doesn’t mean the script flags of this block are overridden completely.

Going to leave this as-is for now, but a separate scripted-diff commit may change the name once one is found.

Sjors commented at 3:54 pm on January 30, 2022:

exception_block_script_flags is what came to me in a dream. But yes, not worth touching now.

Sjors commented at 7:28 pm on January 29, 2022: member

Currently testing cccc1e70b8a14430cc94143da97936a60d6c83d3 (so far so good with the P2SH exception, still syncing to Taproot)

ajtowns commented at 5:47 am on January 30, 2022: member

I think the difference between accidental and intentional consensus changes is important though.

I guess what I’m trying to say is that I don’t think that is the difference in this case – the set and map approaches give the same “valid” / “invalid” response to any block you might present to them, and likewise to any transaction; there’s no consensus change between the two approaches, so there’s nothing for the accidental/intentional distinction to apply to.

it’s still very nice if a user can ensure this didn’t happen, by running just one piece of software, and reviewing one 1 set of code. This is especially relevant for a new user, in case they believe millions of others conspired against him in the past :-)

I mean “millions” of users did conspire against them in the past – they all got together to produce a consensus on who owned how much bitcoin while excluding said new user? All validating the chain can tell the new user is what, if any, rules that conspiracy might have adhered to up until now.

I think as a new user of a bitcoin node, what you want is to ensure (a) you have the same chain tip as everyone else, (b) that your utxo state is the same as everyone else’s, (c) that you’re making the same accept/reject decisions for future blocks as everyone else, and (d) that the chain rules make sense and meet whatever your goals are. And you probably only really care about (b) as a means of achieving (c) and perhaps validating (d), eg that there’s no secret stash of spendable bitcoin beyond what you expected.

I think revalidating rules on known historical blocks (ie, disabling assumevalid and checking p2sh/segwit on the taproot exception block) only helps those things fairly weakly: -assumevalid=0 checks that the current code that will run against new blocks gives the same answers as historical code when run against old blocks (which helps with (c), but in a pretty similar way to having a test suite in general does), and it tells you whether different rules were used in the past than apply now, which might cause you to question whether the current rules really do make sense, ie (d).

That doesn’t really change the conclusion, though: being able to throw as much data as possible through the test suite is good, and map/BIPxxxException works better than set on that count.

MarcoFalke commented at 2:49 pm on January 31, 2022: member

Tested cccc1e70b8a14430cc94143da97936a60d6c83d3 for all 6 combinations. (3 chains unmodified and 3 exceptions removed)

Sjors commented at 12:52 pm on February 1, 2022: member

I also tested cccc1e70b8a14430cc94143da97936a60d6c83d3 on mainnet, as follows:

comment out both exception blocks
src/bitcoind -assumevalid=0 (pro tip: -dbcache=20000 if you have enough RAM)
wait for invalid block error for the first exception block (should be pretty quick)
uncomment first exception block, recompile, restart node
src/bitcoin-cli reconsiderblock HASH
wait for invalid block error for the second exception block (rinse and repeat: uncomment, recompile, reconsider block)
sync to the point Taproot activated (or just to the tip)

in src/validation.cpp:1593 in cccc1e70b8

1597+    // Similarly, only one historical block violated the TAPROOT rules on
1598+    // mainnet.
1599+    // For simplicity, always leave P2SH+WITNESS+TAPROOT on except for the two
1600+    // violating blocks.
1601+    uint32_t flags{SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT};
1602+    const auto it{consensusparams.script_flag_exceptions.find(*Assert(block_index.phashBlock))};

ajtowns commented at 8:14 am on February 4, 2022:

This would be more efficient if script_flag_exceptions were std::unordered_map<int, std::pair<uint256, uint32_t>> – that is if it mapped a height to its hash and script flags; then you’re only looking up a hash table, rather than doing uint256 comparisons to search through a tree; you only look at the block hash when you’re at the actual height.

Code changes would look something like:

0    std::unordered_map<int, std::pair<uint256, uint32_t>> script_flag_exceptions;
1
2    consensus.script_flag_exceptions.try_emplace( // BIP16 exception
3        170060, uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"), SCRIPT_VERIFY_NONE);
4
5    const auto it{consensusparams.script_flag_exceptions.find(block_index.nHeight)};
6    if (it != consensusparams.script_flag_exceptions.end() && *Assert(block_index.phashBlock) == it->second.first) {
7        flags = it->second.second;
8    }

MarcoFalke commented at 8:25 am on February 7, 2022:

Thanks, will leave that for a follow up, because:

comparison is already used in current master.
I don’t expect any speedup, since calculating the block hash is far more expensive than one (to currently two) comparisons.

Sjors commented at 9:34 am on February 7, 2022:

Having the height in either the code or a comment would be nice too, but I agree it can wait, and could use a bench.

JeremyRubin commented at 5:21 pm on February 7, 2022:

i would also suggest binary searching a sorted prevector<std::pair<uint256, uint23>, N> since that will have much superior cache locality on the search, probably better than map/unordered_map.

MarcoFalke commented at 9:01 am on February 8, 2022:

Thanks, leaving for a follow-up.

I’d be shocked to see any real-world (IBD) performance difference due to less comparisons or “superior cache locality”, as I expect calculating the hash of the block is far more expensive than everything combined in this method. Fun fact: We currently calculate the block hash at least twice for no reason, so a better way to improve performance would be to review (or benchmark) #23819.

laanwj commented at 1:10 pm on March 25, 2022:

Yes, I think micro-optimizing the data structure used to store two items is a waste of time. Binary search or linear search in particular will make no difference.

ajtowns approved

ajtowns commented at 8:10 am on February 7, 2022: member

ACK cccc1e70b8a14430cc94143da97936a60d6c83d3 ; code review; wrote a “getblockscriptflags” rpc to quickly check that blocks just had bit 17 (taproot) added; review of earlier revisions had established non-exception blocks do validate with taproot rules enabled.

MarcoFalke commented at 7:04 am on February 18, 2022: member

This missed the 23.0 feature freeze, but it would be good to decide if we want this for 24.0. From the previous comments it looks like three people tested the code, but I couldn’t infer if the same number also reviewed the code.

Sjors commented at 12:03 pm on February 18, 2022: member

v24.0 is fine

I had tested the update, and reviewed the initial version, but not (as thoroughly) the update. Until now…

tACK cccc1e70b8a14430cc94143da97936a60d6c83d3

I would like a followup that clarifies and/or cleans up the backward compatibility tests (as promised in the comment): #23536 (review)

MarcoFalke added this to the milestone 24.0 on Feb 18, 2022

ajtowns commented at 3:08 am on March 4, 2022: member

Here’s the getblockscriptflags code I used in case anyone else wants a cheaper way of checking there aren’t consensus changes compared to master than a full revalidation of the entire blockchain. Idea is that you apply this patch, startup bitcoind, save the output of getblockscriptflags, then apply the changes in this PR, restart bitcoind and check the output of getblockscriptflags hasn’t changed [EDIT: err, except to add the taproot flag on all non-exception blocks]. You’ll need to call the RPC at multiple heights since it only returns info for up to 20k blocks. (Note that after merging this PR, the call to GetBlockScriptFlags in rpc/blockchain.cpp will need to become *pindex due to the declaration change in this PR)

  0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
  1index 7cbe7e6159..9202b671f4 100644
  2--- a/src/rpc/blockchain.cpp
  3+++ b/src/rpc/blockchain.cpp
  4@@ -831,6 +831,42 @@ static RPCHelpMan getblockfrompeer()
  5     };
  6 }
  7 
  8+std::vector<std::tuple<int, uint256, uint32_t>> Get20kBlockScriptFlags(
  9const CBlockIndex* pindex, const Consensus::Params& chainparams);
 10+
 11+static RPCHelpMan getblockscriptflags()
 12+{
 13+    return RPCHelpMan{"getblockscriptflags", "blah",
 14+                {
 15+                    {"height", RPCArg::Type::NUM, RPCArg::Optional::NO,
 16 "The height index"},
 17+                },
 18+                RPCResult{RPCResult::Type::ARR, "script flags", "blah",
 19+                    {RPCResult{RPCResult::Type::STR, "info", "blah"}}},
 20+                RPCExamples{
 21+                    HelpExampleCli("getblockscriptflags", "1000")
 22+                },
 23+                [&](const RPCHelpMan& self, const JSONRPCRequest& reque
 24st) -> UniValue
 25+{
 26+    ChainstateManager& chainman = EnsureAnyChainman(request.context);
 27+    LOCK(cs_main);
 28+    const CChain& active_chain = chainman.ActiveChain();
 29+
 30+    int nHeight = request.params[0].get_int();
 31+    if (nHeight < 0 || nHeight > active_chain.Height())
 32+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of 
 33range");
 34+
 35+    CBlockIndex* blockindex = active_chain[nHeight];
 36+
 37+    UniValue res(UniValue::VARR);
 38+    auto r = Get20kBlockScriptFlags(blockindex, Params().GetConsensus());
 39+
 40+    for (const auto& x : r) {
 41+        res.push_back(strprintf("%d %08x %s", std::get<0>(x), std::get<2>(x), std::get<1>(x).GetHex()));
 42+    }
 43+    return res;
 44+},
 45+    };
 46+}
 47+
 48 static RPCHelpMan getblockhash()
 49 {
 50     return RPCHelpMan{"getblockhash",
 51@@ -2839,6 +2875,7 @@ static const CRPCCommand commands[] =
 52     { "blockchain",         &getblock,                           },
 53     { "blockchain",         &getblockfrompeer,                   },
 54     { "blockchain",         &getblockhash,                       },
 55aj@cobalt:~/P/bitcoin/bitcoin/chk/src$ git show 1d2c0e25bcf128773ade612ca068ebc4404f4a86 | cat
 56commit 1d2c0e25bcf128773ade612ca068ebc4404f4a86
 57Author: Anthony Towns <aj@erisian.com.au>
 58Date:   Mon Feb 7 17:33:52 2022 +1000
 59
 60    getblockscriptflags
 61
 62diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
 63index 7cbe7e6159..9202b671f4 100644
 64--- a/src/rpc/blockchain.cpp
 65+++ b/src/rpc/blockchain.cpp
 66@@ -831,6 +831,42 @@ static RPCHelpMan getblockfrompeer()
 67     };
 68 }
 69 
 70+std::vector<std::tuple<int, uint256, uint32_t>> Get20kBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& chainparams);
 71+
 72+static RPCHelpMan getblockscriptflags()
 73+{
 74+    return RPCHelpMan{"getblockscriptflags", "blah",
 75+                {
 76+                    {"height", RPCArg::Type::NUM, RPCArg::Optional::NO, "The height index"},
 77+                },
 78+                RPCResult{RPCResult::Type::ARR, "script flags", "blah",
 79+                    {RPCResult{RPCResult::Type::STR, "info", "blah"}}},
 80+                RPCExamples{
 81+                    HelpExampleCli("getblockscriptflags", "1000")
 82+                },
 83+                [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 84+{
 85+    ChainstateManager& chainman = EnsureAnyChainman(request.context);
 86+    LOCK(cs_main);
 87+    const CChain& active_chain = chainman.ActiveChain();
 88+
 89+    int nHeight = request.params[0].get_int();
 90+    if (nHeight < 0 || nHeight > active_chain.Height())
 91+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range");
 92+
 93+    CBlockIndex* blockindex = active_chain[nHeight];
 94+
 95+    UniValue res(UniValue::VARR);
 96+    auto r = Get20kBlockScriptFlags(blockindex, Params().GetConsensus());
 97+
 98+    for (const auto& x : r) {
 99+        res.push_back(strprintf("%d %08x %s", std::get<0>(x), std::get<2>(x), std::get<1>(x).GetHex()));
100+    }
101+    return res;
102+},
103+    };
104+}
105+
106 static RPCHelpMan getblockhash()
107 {
108     return RPCHelpMan{"getblockhash",
109@@ -2839,6 +2875,7 @@ static const CRPCCommand commands[] =
110     { "blockchain",         &getblock,                           },
111     { "blockchain",         &getblockfrompeer,                   },
112     { "blockchain",         &getblockhash,                       },
113+    { "blockchain",         &getblockscriptflags,                },
114     { "blockchain",         &getblockheader,                     },
115     { "blockchain",         &getchaintips,                       },
116     { "blockchain",         &getdifficulty,                      },
117diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
118index c480a093a4..a614c3228f 100644
119--- a/src/rpc/client.cpp
120+++ b/src/rpc/client.cpp
121@@ -62,6 +62,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
122     { "getbalance", 3, "avoid_reuse" },
123     { "getblockfrompeer", 1, "peer_id" },
124     { "getblockhash", 0, "height" },
125+    { "getblockscriptflags", 0, "height" },
126     { "waitforblockheight", 0, "height" },
127     { "waitforblockheight", 1, "timeout" },
128     { "waitforblock", 1, "timeout" },
129diff --git a/src/validation.cpp b/src/validation.cpp
130index c12dc9e8b6..ae693c26fc 100644
131--- a/src/validation.cpp
132+++ b/src/validation.cpp
133@@ -285,6 +285,18 @@ bool CheckSequenceLocks(CBlockIndex* tip,
134 // Returns the script flags which should be checked for a given block
135 static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& chainparams);
136 
137+std::vector<std::tuple<int, uint256, uint32_t>> Get20kBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& chainparams)
138+{
139+    std::vector<std::tuple<int, uint256, uint32_t>> r;
140+    int c = 20000;
141+    while (c > 0 && pindex != nullptr) {
142+        r.emplace_back(pindex->nHeight, *pindex->phashBlock, GetBlockScriptFlags(pindex, chainparams));
143+        --c;
144+        pindex = pindex->pprev;
145+    }
146+    return r;
147+}
148+
149 static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache, size_t limit, std::chrono::seconds age)
150     EXCLUSIVE_LOCKS_REQUIRED(pool.cs, ::cs_main)
151 {

in src/validation.cpp:1592 in cccc1e70b8

1598+    // mainnet and testnet).
1599+    // Similarly, only one historical block violated the TAPROOT rules on
1600+    // mainnet.
1601+    // For simplicity, always leave P2SH+WITNESS+TAPROOT on except for the two
1602+    // violating blocks.
1603+    uint32_t flags{SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT};

ajtowns commented at 7:16 am on March 10, 2022:

Should MANDATORY_SCRIPT_VERIFY_FLAGS from script/standard.h (#3843) match the values here? The description of that constant seems mostly applicable, but it doesn’t seem to have been updated for any of the post-P2SH forks.

MarcoFalke commented at 7:22 am on March 10, 2022:

The goal of this pull request is to change the block verify flags (consensus). I recommend creating a separate brainstorming issue (or pull) to discuss policy and p2p changes.

jamesob approved

jamesob commented at 2:59 pm on March 10, 2022: member

ACK cccc1e70b8a14430cc94143da97936a60d6c83d3 (jamesob/ackr/23536.1.MarcoFalke.enforce_taproot_script_f)

Built and reviewed each commit locally. I haven’t done testing that’s as material as @Sjors and @ajtowns (nice work guys), but I have convinced myself that the code works as intended. script_flag_exceptions is a big improvement on the previous BIP16Exception-style approach.

The test changes are a bit more opaque to me as I haven’t read through the taproot functional tests before.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK cccc1e70b8a14430cc94143da97936a60d6c83d3 ([`jamesob/ackr/23536.1.MarcoFalke.enforce_taproot_script_f`](https://github.com/jamesob/bitcoin/tree/ackr/23536.1.MarcoFalke.enforce_taproot_script_f))
 4
 5Built and reviewed each commit locally. I haven't done testing that's as material as [@Sjors](/bitcoin-bitcoin/contributor/sjors/) and [@ajtowns](/bitcoin-bitcoin/contributor/ajtowns/) (nice work guys), but I have convinced myself that the code works as intended. `script_flag_exceptions` is a big improvement on the previous `BIP16Exception`-style approach.
 6
 7The test changes are a bit more opaque to me as I haven't read through the taproot functional tests before.
 8-----BEGIN PGP SIGNATURE-----
 9
10iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmIqEgsACgkQepNdrbLE
11TwWjag/+Oqnptx0jUlbCLHA6N/w7wJM8WOtkCDkKCMGLIzeRtpoxcwNNQQo9vYC/
12QcRwmpbVQHCaAYn3UMNTgvVgIWIk+ONFprIb/mL+Xt0+XkEx+sBPeDbvyrHm269/
13Yvi11FrlbuN5mSsah7YTh/gBSm6uY1o8vyL6qpTfBG6+4yxjDAOUwDUXowVQZFI0
147id7AAyzo1Nv0s0dpUarQutqxWuxrp99FTxDG6Ru2zI4kqpSvGSXP+JnK0UkoTNJ
15feGlZy9zSxfZT+I70bFDYjYNUytZ8H5DnUN9YoQIDlVR6YzKNNbCEkKgxyKQdPW0
16SFMmSECL+RA8jh96dk9zCvpbbMcrgemQPXTb+E10ATa6IsLf65BEerqybd8DIHNG
17GiaV6iQKMCJk1QbjWAs51O3/3ayvEQqhr2QIhTc5l82xTtqBs7qoZvK4uOoO+IL/
18B/RC4t+6GKJ1VA5Z1nfWscJpw2gu8Q/mz9inUmfwOpOjGCxe7k9IHKuTFVzytpip
19QC9F6M2m8YJcL7XP1tkEkuDyKqUdkOtvghTbilpc9CrqQA2QvUtL4OQFrLV0GUIF
20kNxN/bgyIWFywrTRMj1rKsFr6ETox5WWkR61wXEH0WDtzyRsiyE7+8s8+PpfLprR
21qfFoMOG4MLb9EN2ckD9Iz6r5z9EIQLqR+OCuEHTsDJ5KDv5dv4w=
22=29PR
23-----END PGP SIGNATURE-----

0Tested on Linux-5.10.0-11-amd64-x86_64-with-glibc2.31
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/local/bin/clang++ CC=/usr/local/bin/clang --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i
5
6Compiler version: Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63

achow101 commented at 5:50 pm on March 16, 2022: member

ACK cccc1e70b8a14430cc94143da97936a60d6c83d3

Reviewed the code and did a full chain revalidation.

laanwj commented at 11:45 am on March 25, 2022: member

Code review ACK cccc1e70b8a14430cc94143da97936a60d6c83d3 I like the generalization of BIP16Exception to script_flag_exceptions.

laanwj merged this on Mar 25, 2022

laanwj closed this on Mar 25, 2022

MarcoFalke deleted the branch on Mar 25, 2022

sidhujag referenced this in commit 9d6124fdf8 on Apr 2, 2022

DrahtBot locked this on Apr 1, 2023

Enforce Taproot script flags whenever WITNESS is set #23536

Benefits:

Implementation:

Considerations

Conflicts