Enforce Taproot script flags whenever WITNESS is set #23536
Conversation
|
Concept ACK on burying taproot deployment in the next major release 😬 |
|
concept ACK -- note that the IsNull v.s. optional doesn't really matter; i do mildly prefer consistency though with the BIP16Exception. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK. Not sure what the right amount of PoW is to burry this under. How much was SegWit burried under? |
|
@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). |
|
Concept ACK, for consistency with previous soft fork deployments and the benefits well articulated in the PR description. |
|
Concept ACK Running verification since genesis now. |
There was a problem hiding this comment.
Code in fa75e6e 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
maflcko
force-pushed
the
2111-taprootGenesis
branch
from
fa75e6e
to
fa386f6
Compare
|
Rebased with the following changes:
|
|
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. |
|
Concept ACK |
Can you elaborate why this is creepy? All call sites assume the hash to be existing. See the commit message and for example: Line 1643 in fa65b65 |
|
Concept ACK and light Code Review ACK I will do some testing in the next days. |
maflcko
force-pushed
the
2111-taprootGenesis
branch
from
fa386f6
to
fa3335b
Compare
|
Force pushed last commit, only change is whitespace related |
The function dereferences the pointer and can not accept nullptr. Change the arg to a const reference to clarify this for the caller.
Commit d59b8d6 removed the need for this check and it was never needed.
maflcko
force-pushed
the
2111-taprootGenesis
branch
from
fa3335b
to
fa9a4d1
Compare
|
For what it's worth, I don't find the listed benefits very persuasive:
The script verification flags aren't constant -- there's an exception for a particular block in the chain that matters most.
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.
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.
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...
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:
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. |
| add_spender(spenders, "inactive/scriptpath_invalidsig", key=sec, tap=tap, leaf="pk", standard=False, inputs=[getter("sign")], sighash=bitflipper(default_sighash)) | ||
| add_spender(spenders, "inactive/scriptpath_invalidcb", key=sec, tap=tap, leaf="pk", standard=False, inputs=[getter("sign")], controlblock=bitflipper(default_controlblock)) | ||
|
|
||
| # Test that features like annex, leaf versions, or OP_SUCCESS are valid but non-standard |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Renamed the function (among other cleanup) in #24737
|
I think you can get the best of both worlds using a map instead of a set: diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index b0bded4355..5943b87f37 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -10,6 +10,7 @@
#include <deploymentinfo.h>
#include <hash.h> // for signet block challenge hash
#include <util/system.h>
+#include <script/interpreter.h>
#include <assert.h>
@@ -65,8 +66,8 @@ public:
consensus.signet_blocks = false;
consensus.signet_challenge.clear();
consensus.nSubsidyHalvingInterval = 210000;
- consensus.BIP16Exception = uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22");
- consensus.TaprootException = uint256S("0x0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad");
+ consensus.ScriptFlagExceptions.emplace(uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"), SCRIPT_VERIFY_NONE);
+ consensus.ScriptFlagExceptions.emplace(uint256S("0x0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad"), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS);
consensus.BIP34Height = 227931;
consensus.BIP34Hash = uint256S("0x000000000000024b89b42a942fe0d9fea3bb44ab7bd1b19115dd6a759c0808b8");
consensus.BIP65Height = 388381; // 000000000000000004c2b624ed5d7756c508d90fd0da2c7c679febfa6c4735f0
@@ -185,8 +186,7 @@ public:
consensus.signet_blocks = false;
consensus.signet_challenge.clear();
consensus.nSubsidyHalvingInterval = 210000;
- consensus.BIP16Exception = uint256S("0x00000000dd30457c001f4095d208cc1296b0eed002427aa599874af7a432b105");
- consensus.TaprootException = uint256::ZERO;
+ consensus.ScriptFlagExceptions.emplace(uint256S("0x00000000dd30457c001f4095d208cc1296b0eed002427aa599874af7a432b105"), SCRIPT_VERIFY_NONE);
consensus.BIP34Height = 21111;
consensus.BIP34Hash = uint256S("0x0000000023b3a96d3484e5abb3755c413e7d41500f8e2a5c3f0dd01299cd8ef8");
consensus.BIP65Height = 581885; // 00000000007f6655f22f98e72ed80d8b06dc761d5da09df0fa1dc4be4f861eb6
@@ -325,8 +325,6 @@ public:
consensus.signet_blocks = true;
consensus.signet_challenge.assign(bin.begin(), bin.end());
consensus.nSubsidyHalvingInterval = 210000;
- consensus.BIP16Exception = uint256{};
- consensus.TaprootException = uint256::ZERO;
consensus.BIP34Height = 1;
consensus.BIP34Hash = uint256{};
consensus.BIP65Height = 1;
@@ -394,8 +392,6 @@ public:
consensus.signet_blocks = false;
consensus.signet_challenge.clear();
consensus.nSubsidyHalvingInterval = 150;
- consensus.BIP16Exception = uint256();
- consensus.TaprootException = uint256::ZERO;
consensus.BIP34Height = 1; // Always active unless overridden
consensus.BIP34Hash = uint256();
consensus.BIP65Height = 1; // Always active unless overridden
diff --git a/src/consensus/params.h b/src/consensus/params.h
index 97e755a80b..527c5e5907 100644
--- a/src/consensus/params.h
+++ b/src/consensus/params.h
@@ -8,6 +8,7 @@
#include <uint256.h>
#include <limits>
+#include <map>
namespace Consensus {
@@ -70,10 +71,8 @@ struct BIP9Deployment {
struct Params {
uint256 hashGenesisBlock;
int nSubsidyHalvingInterval;
- /* Block hash that is excepted from BIP16 enforcement, or Null if none exists */
- uint256 BIP16Exception;
- /* Block hash that is excepted from SCRIPT_VERIFY_TAPROOT enforcement, or Null if none exists */
- uint256 TaprootException;
+ /* Block hashes that do not have default script flags */
+ std::map<uint256,uint32_t> ScriptFlagExceptions;
/** Block height and hash at which BIP34 becomes active */
int BIP34Height;
uint256 BIP34Hash;
diff --git a/src/validation.cpp b/src/validation.cpp
index a1c5a2f286..e8cc5b2f6e 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1581,25 +1581,11 @@ static ThresholdConditionCache warningcache[VERSIONBITS_NUM_BITS] GUARDED_BY(cs_
static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Consensus::Params& consensusparams)
{
- unsigned int flags = SCRIPT_VERIFY_NONE;
-
- // BIP16 didn't become active until Apr 1 2012 (on mainnet, and
- // retroactively applied to testnet)
- // However, only one historical block violated the P2SH rules (on both
- // mainnet and testnet), so for simplicity, always leave P2SH
- // on except for the one violating block.
- if (consensusparams.BIP16Exception.IsNull() || // no bip16 exception on this chain
- *Assert(block_index.phashBlock) != consensusparams.BIP16Exception) // this block isn't the historical exception
- {
- // Enforce WITNESS rules whenever P2SH is in effect
- flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS;
- // Enforce Taproot (BIP340-BIP342) whenever WITNESS is in effect,
- // unless except
- if (consensusparams.TaprootException.IsNull() || // no Taproot exception on this chain
- *Assert(block_index.phashBlock) != consensusparams.TaprootException) // this block is not the historical exception
- {
- flags |= SCRIPT_VERIFY_TAPROOT;
- }
+ unsigned int flags = SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT;
+
+ // Some blocks are treated as exceptions and verified with a subset of the default flags above
+ const auto it = consensusparams.ScriptFlagExceptions.find(*Assert(block_index.phashBlock));
+ if (it != consensusparams.ScriptFlagExceptions.end()) {
+ flags = it->second;
}
// Enforce the DERSIG (BIP66) rule |
|
I like the ScriptFlagExceptions map approach too. In my example above I should have said "not setting
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 :-) |
maflcko
force-pushed
the
2111-taprootGenesis
branch
from
fa9a4d1
to
faa40fc
Compare
Ok, going to push that soon. |
maflcko
force-pushed
the
2111-taprootGenesis
branch
from
faa40fc
to
cccc1e7
Compare
| flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS; | ||
| // mainnet and testnet). | ||
| // Similarly, only one historical block violated the TAPROOT rules on | ||
| // mainnet. |
There was a problem hiding this comment.
// No historical blocks violated the `WITNESS` rules on mainnet and testnet.
There was a problem hiding this comment.
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.
| * Hashes of blocks that | ||
| * - are known to be consensus valid, and | ||
| * - buried in the chain, and | ||
| * - fail if the default script verify flags are applied. |
There was a problem hiding this comment.
* We manually apply the flags that applied at the time the block was created.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| * - buried in the chain, and | ||
| * - fail if the default script verify flags are applied. | ||
| */ | ||
| std::map<uint256, uint32_t> script_flag_exceptions; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
script_flag_overrides_for_block
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
exception_block_script_flags is what came to me in a dream. But yes, not worth touching now.
| add_spender(spenders, "inactive/scriptpath_invalidsig", key=sec, tap=tap, leaf="pk", standard=False, inputs=[getter("sign")], sighash=bitflipper(default_sighash)) | ||
| add_spender(spenders, "inactive/scriptpath_invalidcb", key=sec, tap=tap, leaf="pk", standard=False, inputs=[getter("sign")], controlblock=bitflipper(default_controlblock)) | ||
|
|
||
| # Test that features like annex, leaf versions, or OP_SUCCESS are valid but non-standard |
There was a problem hiding this comment.
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).
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.
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: 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. |
|
Tested cccc1e7 for all 6 combinations. (3 chains unmodified and 3 exceptions removed) |
|
I also tested cccc1e7 on mainnet, as follows:
|
There was a problem hiding this comment.
ACK cccc1e7 ; 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.
| // For simplicity, always leave P2SH+WITNESS+TAPROOT on except for the two | ||
| // violating blocks. | ||
| uint32_t flags{SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT}; | ||
| const auto it{consensusparams.script_flag_exceptions.find(*Assert(block_index.phashBlock))}; |
There was a problem hiding this comment.
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:
std::unordered_map<int, std::pair<uint256, uint32_t>> script_flag_exceptions;
consensus.script_flag_exceptions.try_emplace( // BIP16 exception
170060, uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"), SCRIPT_VERIFY_NONE);
const auto it{consensusparams.script_flag_exceptions.find(block_index.nHeight)};
if (it != consensusparams.script_flag_exceptions.end() && *Assert(block_index.phashBlock) == it->second.first) {
flags = it->second.second;
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
|
v24.0 is fine I had tested the update, and reviewed the initial version, but not (as thoroughly) the update. Until now... tACK cccc1e7 I would like a followup that clarifies and/or cleans up the backward compatibility tests (as promised in the comment): #23536 (comment) |
|
Here's the Patch belowdiff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 7cbe7e6159..9202b671f4 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -831,6 +831,42 @@ static RPCHelpMan getblockfrompeer()
};
}
+std::vector<std::tuple<int, uint256, uint32_t>> Get20kBlockScriptFlags(
const CBlockIndex* pindex, const Consensus::Params& chainparams);
+
+static RPCHelpMan getblockscriptflags()
+{
+ return RPCHelpMan{"getblockscriptflags", "blah",
+ {
+ {"height", RPCArg::Type::NUM, RPCArg::Optional::NO,
"The height index"},
+ },
+ RPCResult{RPCResult::Type::ARR, "script flags", "blah",
+ {RPCResult{RPCResult::Type::STR, "info", "blah"}}},
+ RPCExamples{
+ HelpExampleCli("getblockscriptflags", "1000")
+ },
+ [&](const RPCHelpMan& self, const JSONRPCRequest& reque
st) -> UniValue
+{
+ ChainstateManager& chainman = EnsureAnyChainman(request.context);
+ LOCK(cs_main);
+ const CChain& active_chain = chainman.ActiveChain();
+
+ int nHeight = request.params[0].get_int();
+ if (nHeight < 0 || nHeight > active_chain.Height())
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of
range");
+
+ CBlockIndex* blockindex = active_chain[nHeight];
+
+ UniValue res(UniValue::VARR);
+ auto r = Get20kBlockScriptFlags(blockindex, Params().GetConsensus());
+
+ for (const auto& x : r) {
+ res.push_back(strprintf("%d %08x %s", std::get<0>(x), std::get<2>(x), std::get<1>(x).GetHex()));
+ }
+ return res;
+},
+ };
+}
+
static RPCHelpMan getblockhash()
{
return RPCHelpMan{"getblockhash",
@@ -2839,6 +2875,7 @@ static const CRPCCommand commands[] =
{ "blockchain", &getblock, },
{ "blockchain", &getblockfrompeer, },
{ "blockchain", &getblockhash, },
aj@cobalt:~/P/bitcoin/bitcoin/chk/src$ git show 1d2c0e25bcf128773ade612ca068ebc4404f4a86 | cat
commit 1d2c0e25bcf128773ade612ca068ebc4404f4a86
Author: Anthony Towns <aj@erisian.com.au>
Date: Mon Feb 7 17:33:52 2022 +1000
getblockscriptflags
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 7cbe7e6159..9202b671f4 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -831,6 +831,42 @@ static RPCHelpMan getblockfrompeer()
};
}
+std::vector<std::tuple<int, uint256, uint32_t>> Get20kBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& chainparams);
+
+static RPCHelpMan getblockscriptflags()
+{
+ return RPCHelpMan{"getblockscriptflags", "blah",
+ {
+ {"height", RPCArg::Type::NUM, RPCArg::Optional::NO, "The height index"},
+ },
+ RPCResult{RPCResult::Type::ARR, "script flags", "blah",
+ {RPCResult{RPCResult::Type::STR, "info", "blah"}}},
+ RPCExamples{
+ HelpExampleCli("getblockscriptflags", "1000")
+ },
+ [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
+ ChainstateManager& chainman = EnsureAnyChainman(request.context);
+ LOCK(cs_main);
+ const CChain& active_chain = chainman.ActiveChain();
+
+ int nHeight = request.params[0].get_int();
+ if (nHeight < 0 || nHeight > active_chain.Height())
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range");
+
+ CBlockIndex* blockindex = active_chain[nHeight];
+
+ UniValue res(UniValue::VARR);
+ auto r = Get20kBlockScriptFlags(blockindex, Params().GetConsensus());
+
+ for (const auto& x : r) {
+ res.push_back(strprintf("%d %08x %s", std::get<0>(x), std::get<2>(x), std::get<1>(x).GetHex()));
+ }
+ return res;
+},
+ };
+}
+
static RPCHelpMan getblockhash()
{
return RPCHelpMan{"getblockhash",
@@ -2839,6 +2875,7 @@ static const CRPCCommand commands[] =
{ "blockchain", &getblock, },
{ "blockchain", &getblockfrompeer, },
{ "blockchain", &getblockhash, },
+ { "blockchain", &getblockscriptflags, },
{ "blockchain", &getblockheader, },
{ "blockchain", &getchaintips, },
{ "blockchain", &getdifficulty, },
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index c480a093a4..a614c3228f 100644
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -62,6 +62,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "getbalance", 3, "avoid_reuse" },
{ "getblockfrompeer", 1, "peer_id" },
{ "getblockhash", 0, "height" },
+ { "getblockscriptflags", 0, "height" },
{ "waitforblockheight", 0, "height" },
{ "waitforblockheight", 1, "timeout" },
{ "waitforblock", 1, "timeout" },
diff --git a/src/validation.cpp b/src/validation.cpp
index c12dc9e8b6..ae693c26fc 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -285,6 +285,18 @@ bool CheckSequenceLocks(CBlockIndex* tip,
// Returns the script flags which should be checked for a given block
static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& chainparams);
+std::vector<std::tuple<int, uint256, uint32_t>> Get20kBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& chainparams)
+{
+ std::vector<std::tuple<int, uint256, uint32_t>> r;
+ int c = 20000;
+ while (c > 0 && pindex != nullptr) {
+ r.emplace_back(pindex->nHeight, *pindex->phashBlock, GetBlockScriptFlags(pindex, chainparams));
+ --c;
+ pindex = pindex->pprev;
+ }
+ return r;
+}
+
static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache, size_t limit, std::chrono::seconds age)
EXCLUSIVE_LOCKS_REQUIRED(pool.cs, ::cs_main)
{ |
| // mainnet. | ||
| // For simplicity, always leave P2SH+WITNESS+TAPROOT on except for the two | ||
| // violating blocks. | ||
| uint32_t flags{SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ACK cccc1e7 (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.
Show signature data
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK cccc1e70b8a14430cc94143da97936a60d6c83d3 ([`jamesob/ackr/23536.1.MarcoFalke.enforce_taproot_script_f`](https://github.com/jamesob/bitcoin/tree/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.
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmIqEgsACgkQepNdrbLE
TwWjag/+Oqnptx0jUlbCLHA6N/w7wJM8WOtkCDkKCMGLIzeRtpoxcwNNQQo9vYC/
QcRwmpbVQHCaAYn3UMNTgvVgIWIk+ONFprIb/mL+Xt0+XkEx+sBPeDbvyrHm269/
Yvi11FrlbuN5mSsah7YTh/gBSm6uY1o8vyL6qpTfBG6+4yxjDAOUwDUXowVQZFI0
7id7AAyzo1Nv0s0dpUarQutqxWuxrp99FTxDG6Ru2zI4kqpSvGSXP+JnK0UkoTNJ
feGlZy9zSxfZT+I70bFDYjYNUytZ8H5DnUN9YoQIDlVR6YzKNNbCEkKgxyKQdPW0
SFMmSECL+RA8jh96dk9zCvpbbMcrgemQPXTb+E10ATa6IsLf65BEerqybd8DIHNG
GiaV6iQKMCJk1QbjWAs51O3/3ayvEQqhr2QIhTc5l82xTtqBs7qoZvK4uOoO+IL/
B/RC4t+6GKJ1VA5Z1nfWscJpw2gu8Q/mz9inUmfwOpOjGCxe7k9IHKuTFVzytpip
QC9F6M2m8YJcL7XP1tkEkuDyKqUdkOtvghTbilpc9CrqQA2QvUtL4OQFrLV0GUIF
kNxN/bgyIWFywrTRMj1rKsFr6ETox5WWkR61wXEH0WDtzyRsiyE7+8s8+PpfLprR
qfFoMOG4MLb9EN2ckD9Iz6r5z9EIQLqR+OCuEHTsDJ5KDv5dv4w=
=29PR
-----END PGP SIGNATURE-----
Show platform data
Tested on Linux-5.10.0-11-amd64-x86_64-with-glibc2.31
Configured 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
Compiled 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
Compiler version: Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63
|
ACK cccc1e7 Reviewed the code and did a full chain revalidation. |
|
Code review ACK cccc1e7 |
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".)
nMinimumChainWorkalready 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 betweennMinimumChainWorkand the work at block 709632.For reference, previously the same was done for P2SH and WITNESS in commit 0a8b7b4.
Implementation:
I found one block which fails verification with the flags applied, so I added a
TaprootException, similar to theBIP16Exception.For reference, the debug log:
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.