Remove RewindBlockIndex logic

dhruv commented at 4:53 am on January 26, 2021: member

Context from original comment (minor edits):

RewindBlockIndex() is a mechanism to allow nodes to be upgraded after segwit activation, while still keeping their chainstate/datadir in a consistent state. It works as follows:

A pre-segwit (i.e. v0.13.0 or older) node is running.
Segwit activates. The pre-segwit node remains sync’ed to the tip, but is not enforcing the new segwit rules.
The user upgrades the node to a segwit-aware version (v0.13.1 or newer).
On startup, in AppInitMain(), RewindBlockIndex() is called. This walks the chain backwards from the tip, disconnecting and erasing blocks that from after segwit activation that weren’t validated with segwit rules.
those blocks are then redownloaded (with witness data) and validated with segwit rules.

This logic probably isn’t required any more since:

Segwit activated at height 481824, when the block chain was 130GB and the total number of txs was 250 million. Today, we’re at height 667704, the blockchain is over 315GB and the total number of txs is over 600 million. Even if 20% of that added data is witness data (a high estimate), then around 150GB of transactions would need to be rewound to get back to segwit activation height. It’d probably be faster to simply validate from genesis, especially since we won’t be validating any scripts before the assumevalid block. It’s also unclear whether rewinding 150GB of transactions would even work. It’s certainly never been tested.
Bitcoin Core v0.13 is hardly used any more. https://luke.dashjr.org/programs/bitcoin/files/charts/software.html shows less than 50 nodes running it. The software was EOL on Aug 1st 2018. It’s very unlikely that anyone is running 0.13 and will want to upgrade to 0.22.

This PR introduces NeedsRedownload() which merely checks for insufficiently validated segwit blocks and requests that the user restarts the node with -reindex. Reindexing the block files upon restart will make the node rebuild chain state and block index from the blk*.dat files on disk. The node won’t be able to index the blocks with BLOCK_OPT_WITNESS, so they will be missing from the chain and be re-downloaded, with witness data.

Removing this code allows the following (done in follow-up #21090):

removal of tests using segwitheight=-1 in p2p_segwit.py.
in turn, that allows us to drop support for -segwitheight=-1, which is only supported for that test.
that allows us to always set NODE_WITNESS in our local services. The only reason we don’t do that is to support -segwitheight=-1.
that in turn allows us to drop all of the GetLocalServices() & NODE_WITNESS checks inside net_processing.cpp, since our local services would always include NODE_WITNESS

fanquake added the label Validation on Jan 26, 2021

DrahtBot commented at 9:32 am on January 26, 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:

#19438 (Introduce deploymentstatus by ajtowns)

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.

jnewbery commented at 4:41 pm on January 26, 2021: member

Concept ACK

dhruv commented at 8:11 pm on January 26, 2021: member

The linter was failing on all pull requests with the same error when I pushed so it is likely unrelated to the commits here and will get resolved with the next push. Ready for review.

practicalswift commented at 9:08 pm on January 26, 2021: contributor

@dhruv Rebase on master and the boost/thread/mutex.hpp warning will go away: it was fixed in #21010 :)

dhruv force-pushed on Jan 26, 2021

dhruv commented at 10:20 pm on January 26, 2021: member

Rebased against master and linter is passing. Thanks, @practicalswift.

laanwj added the label UTXO Db and Indexes on Jan 27, 2021

in src/validation.cpp:4390 in 7c919a15ab outdated

4514-            if (!FlushStateToDisk(params, state, FlushStateMode::ALWAYS)) {
4515-                LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", state.ToString());
4516-                return false;
4517+        for (CBlockIndex* block = m_chain.Genesis(); block != nullptr; block = m_chain.Next(block)) {
4518+            if (IsWitnessEnabled(block->pprev, params.GetConsensus()) && !(block->nStatus & BLOCK_OPT_WITNESS)) {
4519+                // block is insuficiently validated for a segwit client

ccdle12 commented at 6:52 pm on January 27, 2021:

nit: insuficiently -> insufficiently

dhruv commented at 9:12 pm on January 27, 2021:

Done.

in src/init.cpp:1821 in 7c919a15ab outdated

1817@@ -1821,11 +1818,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1818         }
1819     }
1820 
1821-    if (chainparams.GetConsensus().SegwitHeight != std::numeric_limits<int>::max()) {
1822-        // Advertise witness capabilities.
1823-        // The option to not set NODE_WITNESS is only used in the tests and should be removed.
1824-        nLocalServices = ServiceFlags(nLocalServices | NODE_WITNESS);
1825-    }
1826+    nLocalServices = ServiceFlags(nLocalServices | NODE_WITNESS);

ccdle12 commented at 6:59 pm on January 27, 2021:

nit: since we are always using the NODE_WITNESS flag, would it make sense to pass it on initialization for nLocalServices?

dhruv commented at 9:12 pm on January 27, 2021:

That makes sense. Thanks!

theStack commented at 7:25 pm on January 27, 2021: member

Concept ACK

ccdle12 commented at 7:47 pm on January 27, 2021: contributor

Concept ACK

dhruv force-pushed on Jan 27, 2021

dhruv commented at 9:13 pm on January 27, 2021: member

Thanks for the review @ccdle12. Comments addressed. Ready for further review.

luke-jr commented at 9:30 pm on January 28, 2021: member

Makes sense, if the rationale was simply to ensure the block gets redownloaded.

If we focus on validation, however, we would want this around for Taproot. But I’m not sure that’s what its purpose is.

Re-validating blocks on upgrade seems like a feature we don’t have today, and should be implemented separately from this (without redownloading).

(Therefore, Concept ACK)

dhruv commented at 2:29 am on January 29, 2021: member

@luke-jr You’re right, the removed code erases insufficiently validated blocks (which do not have witness data and can’t be properly validated by a segwit-aware node) and re-downloads them. AFAICT, with Taproot, the post-activation blocks will have witness data, so we’ll need to implement a different function to re-validate the insufficiently validated blocks after upgrade.

ariard commented at 10:24 pm on February 1, 2021: member

Concept ACK.

in src/init.cpp:1691 in 79970c9291 outdated

1686@@ -1687,26 +1687,23 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1687                 break;
1688             }
1689 
1690-            bool failed_rewind{false};
1691-            // Can't hold cs_main while calling RewindBlockIndex, so retrieve the relevant
1692+            bool needs_ibd{false};
1693+            // Can't hold cs_main while calling NeedsIBD, so retrieve the relevant

jnewbery commented at 2:53 pm on February 2, 2021:

This isn’t true. NeedsIBD() grabs cs_main and holds throughout. You could change it to requires cs_main, then run this entire block under one cs_main lock.

dhruv commented at 8:34 pm on February 5, 2021:

Done.

in src/validation.h:676 in 79970c9291 outdated

672@@ -673,7 +673,7 @@ class CChainState
673 
674     /** Replay blocks that aren't fully applied to the database. */
675     bool ReplayBlocks(const CChainParams& params);
676-    bool RewindBlockIndex(const CChainParams& params) LOCKS_EXCLUDED(cs_main);
677+    bool NeedsIBD(const CChainParams& params) LOCKS_EXCLUDED(cs_main);

jnewbery commented at 2:54 pm on February 2, 2021:

Make [nodiscard]?

dhruv commented at 8:34 pm on February 5, 2021:

Great idea. Done.

in src/validation.cpp:4387 in 79970c9291 outdated

4497-            return false;
4498-        }
4499-    }
4500-
4501     {
4502         LOCK(cs_main);

jnewbery commented at 2:54 pm on February 2, 2021:

Perhaps just annotate this function as requiring cs_main and assert that it’s held.

dhruv commented at 8:35 pm on February 5, 2021:

Done.

in src/validation.cpp:4388 in 79970c9291 outdated

4512-            // it'll get called a bunch real soon.
4513-            BlockValidationState state;
4514-            if (!FlushStateToDisk(params, state, FlushStateMode::ALWAYS)) {
4515-                LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", state.ToString());
4516-                return false;
4517+        for (CBlockIndex* block = m_chain.Genesis(); block != nullptr; block = m_chain.Next(block)) {

jnewbery commented at 2:54 pm on February 2, 2021:

Can you use a range based for loop since you’re iterating over all members of this container?

dhruv commented at 8:36 pm on February 5, 2021:

I took @ariard’s suggestion and started from the params.SegwitHeight() - also changed it to a while loop that reads cleaner.

Out of curiosity, is a defined [] operator all that’s needed to use a range-based for loop?

ajtowns commented at 10:40 pm on February 7, 2021:

range-based for loops need an x.begin() and x.end() (or begin(x) and end(x)) that return iterators, and the iterators need to accept ++it and *it and be comparable with != – https://en.cppreference.com/w/cpp/language/range-for

jnewbery commented at 9:42 am on February 13, 2021:

My mistake. For some reason I thought m_chain was a container here.

jnewbery commented at 2:55 pm on February 2, 2021: member

Concept ACK. I think it might make sense to split this into two PRs. The second commit is quite large and makes a lot of changes.

in src/validation.cpp:4389 in bf5c3e1327 outdated

4513-            BlockValidationState state;
4514-            if (!FlushStateToDisk(params, state, FlushStateMode::ALWAYS)) {
4515-                LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", state.ToString());
4516-                return false;
4517+        for (CBlockIndex* block = m_chain.Genesis(); block != nullptr; block = m_chain.Next(block)) {
4518+            if (IsWitnessEnabled(block->pprev, params.GetConsensus()) && !(block->nStatus & BLOCK_OPT_WITNESS)) {

ariard commented at 12:02 pm on February 3, 2021:

Do we really need to iterate on the whole block index from genesis ?

Segwit activation height has been hardcoded by #16060. I think you can start the witness-valid iteration from the hardcoded height, whatever the network, minus one ?

dhruv commented at 8:37 pm on February 5, 2021:

Great idea. Updated to start from params.SegwitHeight()

in src/chainparams.cpp:464 in 79970c9291 outdated

460@@ -461,11 +461,8 @@ void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
461 {
462     if (args.IsArgSet("-segwitheight")) {
463         int64_t height = args.GetArg("-segwitheight", consensus.SegwitHeight);
464-        if (height < -1 || height >= std::numeric_limits<int>::max()) {
465+        if (height < 0 || height >= std::numeric_limits<int>::max()) {

ariard commented at 1:14 pm on February 3, 2021:

Can you commit-split the changes around segwithheight from the ones around NODE_WITNESS ? Better to review any changes in net_processing on their own.

dhruv commented at 8:37 pm on February 5, 2021:

Done - moved the second commit to #21090 and split it up into several there.

in src/net_processing.cpp:1951 in 79970c9291 outdated

1947@@ -1948,7 +1948,7 @@ void static ProcessGetData(CNode& pfrom, Peer& peer, const CChainParams& chainpa
1948 
1949 static uint32_t GetFetchFlags(const CNode& pfrom) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
1950     uint32_t nFetchFlags = 0;
1951-    if ((pfrom.GetLocalServices() & NODE_WITNESS) && State(pfrom.GetId())->fHaveWitness) {
1952+    if (State(pfrom.GetId())->fHaveWitness) {

ariard commented at 1:22 pm on February 3, 2021:

I think this is buggy. Always setting NODE_WITNESS to our local service flags doesn’t mean we will never meet peers which aren’t advertising it. Of course it’s unlikely there is that much of those peers deployed but otherwise I believe we’ll wrongly unserialize received txn.

jnewbery commented at 2:15 pm on February 3, 2021:

Why do you think this is buggy? We’ll ~un~serialize using MSG_WITNESS_FLAG iff fHaveWitness is set to true, which happens if the peer includes NODE_WITNESS in its version message.

EDIT: I wrote “unserialize” here when it should be “serialize”. GetFetchFlags() is used to set our serialization when sending messages.

ariard commented at 1:42 pm on February 4, 2021:

I think we unserialize if fAllowWitness=true, MSG_WITNESS_FLAG is used by a getdata sender to require serialization of witnesses but not at unserialization itself?

That said, you’re right that fHaveWitness implies they advertise their local services with NODE_WITNESS. And those ones are static so it should be good. Further, the conditional could be already reduced to fHaveWitness check only, a test node with -segwitheight== std::numeric_limits<int>::max() will have fHaveWitness=false ?

jnewbery commented at 2:16 pm on February 4, 2021:

MSG_WITNESS_FLAG is used by a getdata sender to require serialization of witnesses but not at unserialization itself

Sorry, yes - I wrote “unserialize” above when I meant “serialize”

Further, the conditional could be already reduced to fHaveWitness check only, a test node with -segwitheight== std::numeric_limits::max() will have fHaveWitness=false ?

I don’t think that’s right. Before this PR, it’s checking that both:

we have signaled NODE_WITNESS to the peer
the peer has signaled NODE_WITNESS to us

we can only use witness serialization on the connection when both of those conditions are true.

After this PR, we unconditionally signal NODE_WITNESS to all peers, so we only need to check that they signaled NODE_WITNESS to us (which is what is indicated by fHaveWitness).

ariard commented at 12:28 pm on February 5, 2021:

Gotcha, reading comment L869 in src/net_processing.cpp : “Note that pnode->GetLocalServices() is a reflection of the local services we were offering when the CNode object was created for this peer” makes me understood this is our local services, not their. Renaming the method GetAnnouncedToPeerServices or something similar would be clearer.

But further, if our local services are static beyond initialization does it make sense to track them per-peer, the same set of flags should be announced to all our peers during a runtime lifecycle ?

we can only use witness serialization on the connection when both of those conditions are true.

Does BIP144 really require double opt-in to fetch a tx and its witnesses with a getdata ? Not clear reading the BIP no more the getdata processing code.

0            // WTX and WITNESS_TX imply we serialize with witness
1            int nSendFlags = (inv.IsMsgTx() ? SERIALIZE_TRANSACTION_NO_WITNESS : 0);
2            connman.PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *tx));

jnewbery commented at 2:08 pm on February 5, 2021:

I’m sure there are lots of improvements that could be made around how we track services (and indeed will be made as part of #19398). However, that seems orthogonal to this PR. Here, we just want to ensure that mainnet behaviour is unaffected.

in src/net_processing.cpp:2719 in 79970c9291 outdated

2715@@ -2717,7 +2716,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2716         bool fAnnounceUsingCMPCTBLOCK = false;
2717         uint64_t nCMPCTBLOCKVersion = 0;
2718         vRecv >> fAnnounceUsingCMPCTBLOCK >> nCMPCTBLOCKVersion;
2719-        if (nCMPCTBLOCKVersion == 1 || ((pfrom.GetLocalServices() & NODE_WITNESS) && nCMPCTBLOCKVersion == 2)) {
2720+        if (nCMPCTBLOCKVersion == 1 || nCMPCTBLOCKVersion == 2) {

ariard commented at 1:22 pm on February 3, 2021:

Maybe better to defer those change to #20799, which will achieve the same IIRC but can be reasoned on their own ?

dhruv commented at 8:40 pm on February 5, 2021:

This diff is now moved to #21090 but I left it in because the check is redundant if this code is merged. IIUC, #20799 eliminates the entire conditional so they can both be reasoned for independently?

ariard commented at 1:25 pm on February 3, 2021: member

If we focus on validation, however, we would want this around for Taproot. But I’m not sure that’s what its purpose is.

I don’t think we need it for Taproot as it doesn’t introduce new consensus data but extend meaning of already existing witness ?

We might need again that kind of logic in the future when we do have a soft-fork introducing new consensus data but AFAIK there is no such studied proposal. And if this happen I hope we can be smarter to avoid re-downloading.

dhruv force-pushed on Feb 5, 2021

dhruv commented at 2:42 am on February 6, 2021: member

Comments addressed. The second commit has been broken out into #21090. Ready for further review.

in src/init.cpp:1694 in 9bae47d529 outdated

1704+            bool needs_ibd{false};
1705+            {
1706+                LOCK(cs_main);
1707+                for (CChainState* chainstate : chainman.GetAll()) {
1708+                    if (!fReset) {
1709+                        uiInterface.InitMessage(_("Checking if IBD is needed...").translated);

jnewbery commented at 10:15 am on February 8, 2021:

This will print the message twice if there are two CChainStates.

dhruv commented at 5:23 pm on February 8, 2021:

Done.

in src/init.cpp:1690 in 9bae47d529 outdated

1700-                            "Unable to rewind the database to a pre-fork state. "
1701-                            "You will need to redownload the blockchain");
1702-                        failed_rewind = true;
1703-                        break; // out of the per-chainstate loop
1704+            bool needs_ibd{false};
1705+            {

jnewbery commented at 10:21 am on February 8, 2021:

We can avoid some deep nesting and the need for a temporary variable to pass the result of the predicate out of the loop by using an stl algorithm:

 0--- a/src/init.cpp
 1+++ b/src/init.cpp
 2@@ -1686,27 +1686,19 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
 3                 break;
 4             }
 5 
 6-            bool needs_ibd{false};
 7-            {
 8-                LOCK(cs_main);
 9-                for (CChainState* chainstate : chainman.GetAll()) {
10-                    if (!fReset) {
11-                        uiInterface.InitMessage(_("Checking if IBD is needed...").translated);
12-                        if (chainstate->NeedsIBD(chainparams)) {
13-                            strLoadError = _(
14-                                "Segwit blocks are insufficiently validated. "
15-                                "Please delete blocks dir and chain state dir and restart.");
16-                            needs_ibd = true;
17-                            break; // out of the per-chainstate loop
18-                        }
19-                    }
20+            if (!fReset) {
21+                uiInterface.InitMessage(_("Checking if redownload is needed...").translated);
22+                LOCK(cs_main);
23+                auto chainstates{chainman.GetAll()};
24+                if (std::any_of(chainstates.begin(), chainstates.end(),
25+                                [&chainparams] (CChainState* cs) {return cs->NeedsIBD(chainparams);})) {
26+                    strLoadError = _(
27+                        "Segwit blocks are insufficiently validated. "
28+                        "Please delete blocks dir and chain state dir and restart.");
29+                    break;
30                 }
31             }
32 
33-            if (needs_ibd) {
34-                break; // out of the chainstate activation do-while
35-            }
36-
37             bool failed_verification = false;

This is exactly what std::any_of is designed for. Instead of creating a boolean, manually iterating through a range, executing a predicate on each member and then setting the boolean and breaking out if the predicate returns true, we can let the standard library do the heavy lifting. That makes the code more expressive by raising the level of abstraction and communicating what we want the code to do, and not what the individual steps to do it are.

dhruv commented at 5:24 pm on February 8, 2021:

Beautiful change. Thank you for teaching so patiently!

jnewbery commented at 5:47 pm on February 8, 2021:

Ooops. I guess the lambda needs an annotation too. Something like this maybe:

0                if (std::any_of(chainstates.begin(), chainstates.end(),
1                                [&chainparams] (CChainState* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {return cs->NeedsIBD(chainparams);})) {

dhruv commented at 11:35 pm on February 8, 2021:

That worked. For my education, what happened there and how did you know the lambda annotation would help?

I suspect the annotation is evaluated with the limited scope of the bound variables, so it does not “know” about the locks that are acquired in the outer scope. Is that even close?

jnewbery commented at 9:44 am on February 13, 2021:

As discussed, there was an error message about NeedsIBD() requiring cs_main. Since the compiler does not know that when it’s inside the lambda, it’s in a scope that already has cs_main, we need to annotate it.

in src/validation.cpp:4393 in 9bae47d529 outdated

4389@@ -4390,143 +4390,22 @@ bool CChainState::ReplayBlocks(const CChainParams& params)
4390     return true;
4391 }
4392 
4393-//! Helper for CChainState::RewindBlockIndex
4394-void CChainState::EraseBlockData(CBlockIndex* index)
4395+bool CChainState::NeedsIBD(const CChainParams& params)

jnewbery commented at 10:23 am on February 8, 2021:

Consider renaming this function NeedsRedownload. All nodes that are starting fresh or restarting after some time need IBD. This function is specifically for nodes that need to delete blocks and redownload them.

dhruv commented at 5:24 pm on February 8, 2021:

Makes sense. Done.

in test/functional/p2p_segwit.py:1997 in 9bae47d529 outdated

1995+        assert softfork_active(self.nodes[0], 'segwit')
1996+        assert softfork_active(self.nodes[1], 'segwit')
1997         assert softfork_active(self.nodes[2], 'segwit')
1998 
1999-        # Make sure this peer's blocks match those of node0.
2000+        # Make sure all peers have the same blocks.

jnewbery commented at 10:25 am on February 8, 2021:

This loop seems useless. sync_blocks() ensures that the nodes have the same block hash at their tip. We assume that if the block hash at the tip is the same then all blocks in the chain are the same (if not, then bitcoin is fundamentally broken).

dhruv commented at 5:25 pm on February 8, 2021:

Fair enough. I didn’t remember that sync_blocks() verifies that the best hashes are equal. Done.

dhruv force-pushed on Feb 8, 2021

dhruv commented at 11:35 pm on February 8, 2021: member

Comments addressed. Rebased. Ready for further review.

jnewbery commented at 9:51 am on February 13, 2021: member

Code review ACK 0af05b95e9c829ff581bb76a0998b5a90386e27a

in src/init.cpp:1698 in 0af05b95e9 outdated

1713+                auto chainstates{chainman.GetAll()};
1714+                if (std::any_of(chainstates.begin(), chainstates.end(),
1715+                                [&chainparams](CChainState* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(chainparams); })) {
1716+                    strLoadError = _(
1717+                        "Segwit blocks are insufficiently validated. "
1718+                        "Please delete blocks dir and chain state dir and restart.");

ariard commented at 1:02 pm on February 13, 2021:

Should we be more cautious with directly summoning users to delete directories ?

Maybe we should give a more detailed path (.bitcoin/blocks, .bitcoin/chainstate) but not sure if it’s platform dependent. I just want to minimize the risk of someone deleting by mistake its wallet.

dhruv commented at 1:15 am on February 15, 2021:

Good idea! Done.

ariard commented at 1:03 pm on February 13, 2021: member

Code Review ACK 0af05b95 pending on comment about error message.

dhruv force-pushed on Feb 15, 2021

dhruv commented at 1:15 am on February 15, 2021: member

Updated to log the blocksdir and chainstate dir to the user so they do not accidentally delete their wallet. Ready for further review.

dhruv force-pushed on Feb 15, 2021

in src/init.cpp:1699 in d9370c1c5b outdated

1714+                if (std::any_of(chainstates.begin(), chainstates.end(),
1715+                                [&chainparams](CChainState* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(chainparams); })) {
1716+                    auto chainstate_dir = GetDataDir() / CHAINSTATE_DIRNAME;
1717+                    strLoadError = strprintf(_(
1718+                        "Segwit blocks are insufficiently validated. "
1719+                        "Please delete blocks dir(%s) and chainstate dir(%s) and restart."), GetBlocksDir(), chainstate_dir);

MarcoFalke commented at 11:45 am on February 15, 2021:

A -reindex should do the same with less effort?

dhruv commented at 6:39 pm on February 15, 2021:

When a pre-segwit node upgrades to segwit, it has no way to sufficiently validate to the segwit consensus rules from the blocks on disk since witness data was never relayed to it. Help for -reindex says “Rebuild chain state and block index from the blk*.dat files on disk”. Looking at the code also does not suggest to me that -reindex would cause the node to re-download.

Am I missing something?

jnewbery commented at 10:11 am on February 16, 2021:

Ah, I think this is true. -reindex will cause the node to try to rebuild the chain state from the block files, which will work only as far as the segwit activation height if the blocks after that are serialized without witness. The node will then try to sync by downloading all blocks from peers as normal. The test passes with the following change:

 0diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
 1index 219c15f673..76d8967a41 100755
 2--- a/test/functional/p2p_segwit.py
 3+++ b/test/functional/p2p_segwit.py
 4@@ -1973,15 +1973,8 @@ class SegWitTest(BitcoinTestFramework):
 5         with self.nodes[2].assert_debug_log(expected_msgs=["Segwit blocks are insufficiently validated. Please delete blocks dir and chain state dir and restart."], timeout=10):
 6             self.nodes[2].start(["-segwitheight={}".format(SEGWIT_HEIGHT)])
 7 
 8-        # As directed, the user deletes the blocks and chainstate directories and restarts the node
 9-        datadir = get_datadir_path(self.options.tmpdir, 2)
10-        blocks_dir = os.path.join(datadir, "regtest", "blocks")
11-        chainstate_dir = os.path.join(datadir, "regtest", "chainstate")
12-        shutil.rmtree(blocks_dir)
13-        shutil.rmtree(chainstate_dir)
14-
15-        self.start_node(2, extra_args=["-segwitheight={}".format(SEGWIT_HEIGHT)])
16-        assert_equal(self.nodes[2].getblockcount(), 0)
17+        self.start_node(2, extra_args=["-reindex=1", "-segwitheight={}".format(SEGWIT_HEIGHT)])
18+        assert_equal(self.nodes[2].getblockcount(), SEGWIT_HEIGHT - 1)
19         self.connect_nodes(0, 2)
20 
21         # We reconnect more than 100 blocks, give it plenty of time

There are a bunch of AcceptBlock errors since the blocks after height SEGWIT_HEIGHT aren’t serialized properly:

0node2 2021-02-16T10:05:38.943607Z [loadblk] ERROR: AcceptBlock: bad-witness-nonce-size, ContextualCheckBlock : invalid witness reserved value size 
1 node2 2021-02-16T10:05:38.943663Z [loadblk] ERROR: AcceptBlock: bad-witness-nonce-size, ContextualCheckBlock : invalid witness reserved value size 
2 node2 2021-02-16T10:05:38.943712Z [loadblk] ERROR: AcceptBlock: bad-witness-nonce-size, ContextualCheckBlock : invalid witness reserved value size 
3[...]

and when the node connects to its peers, it downloads and syncs to the tip, validating segwit rules as it goes.

One downside is that the old blocks are retained on disk and never deleted. However, that was the case before this PR as well.

dhruv commented at 4:32 pm on February 16, 2021:

Ah, what I wasn’t thinking about is that it is normal for the block index to not be a 1:1 representation of the blk*.dat files. It happens all the time with reorgs, etc. So, upon -reindex the node won’t be able to validate [SEGWIT_HEIGHT, consensusTip] blocks, will have tip == SEGWIT_HEIGHT - 1. It will then proceed to sync headers and therefore re-download [SEGWIT_HEIGHT, consensusTip].

Thank you for the great suggestion @MarcoFalke! Makes for much simpler code. Updated.

dhruv commented at 6:40 pm on February 15, 2021: member

Force pushed d9370c1 to fix failing test.

ariard commented at 12:03 pm on February 16, 2021: member

Code Review ACK d9370c1, thanks for taking the suggestion, Marco one sounds fine to me if you want to take it.

dhruv force-pushed on Feb 16, 2021

in src/validation.h:560 in 361300e038 outdated

556@@ -556,7 +557,7 @@ class CChainState
557         size_t cache_size_bytes,
558         bool in_memory,
559         bool should_wipe,
560-        std::string leveldb_name = "chainstate");
561+        std::string leveldb_name = CHAINSTATE_DIRNAME);

jnewbery commented at 4:39 pm on February 16, 2021:

Why is this change needed?

dhruv commented at 4:43 pm on February 16, 2021:

You are right - it is no longer needed with the -reindex change (I was previously using it to create the chainstate dir path). Removed.

dhruv force-pushed on Feb 16, 2021

dhruv commented at 4:46 pm on February 16, 2021: member

Force pushed to use -reindex instead of asking the user to delete blocks dir and chainstatedir. Ready for further review!

in test/functional/p2p_segwit.py:1972 in 5e81857852 outdated

1968+        # Restarting node 2 should result in a shutdown because the blockchain consists of
1969+        # insufficiently validated blocks per segwit consensus rules.
1970+        self.stop_node(2)
1971+        with self.nodes[2].assert_debug_log(expected_msgs=[
1972+                'Segwit blocks are insufficiently validated. Please restart with -reindex.'], timeout=10):
1973+            self.nodes[2].start(["-segwitheight={}".format(SEGWIT_HEIGHT)])

jnewbery commented at 5:43 pm on February 16, 2021:

(please don’t change the branch for this, but if you need to retouch for some other reason) you can use an f-string here:

0            self.nodes[2].start([f"-segwitheight={SEGWIT_HEIGHT}"])

f-strings are supported from python 3.6, and have a slightly cleaner syntax.

You can also use an f-string in the extra_args argument below.

dhruv commented at 8:25 pm on February 27, 2021:

Thanks for showing me that. Done.

jnewbery commented at 5:44 pm on February 16, 2021: member

utACK 5e818578524ee8419bcaf610cf1394817d867663

The CI failure seemed unrelated. I’ve restarted it.

in src/init.cpp:1696 in 5e81857852 outdated

1711+                uiInterface.InitMessage(_("Checking if redownload is needed...").translated);
1712+                LOCK(cs_main);
1713+                auto chainstates{chainman.GetAll()};
1714+                if (std::any_of(chainstates.begin(), chainstates.end(),
1715+                                [&chainparams](CChainState* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(chainparams); })) {
1716+                    strLoadError = _("Segwit blocks are insufficiently validated. Please restart with -reindex.");

ajtowns commented at 7:10 am on February 19, 2021:

Perhaps "Witness data for blocks after height %d requires validation.Please restart with -reindex.", params.SegwitHeight might be better?

Seems like the commit message should also be updated now this isn’t recommending deleting the blocks dir.

I suppose with assumeutxo, we could conceivably have a -reindex-while-running option, that downgraded the current tip to “assumeutxo-valid”, and did the reindex/redownload in the background until it caught up.

dhruv commented at 8:25 pm on February 27, 2021:

Done.

in src/validation.cpp:4398 in 5e81857852 outdated

4441-                EraseBlockData(entry.second);
4442-            }
4443-        }
4444-    }
4445+    // At and above params.SegwitHeight, segwit consensus rules must be validated
4446+    CBlockIndex* block = m_chain[params.GetConsensus().SegwitHeight];

ajtowns commented at 7:15 am on February 19, 2021:

I’m a little surprised this doesn’t go from m_chain.Tip() backwards to SegwitHeight?

You’re only trying to figure out if any blocks didn’t have witness data checked, not which is the lowest one, and if there are any without witness data, the tip should also not have witness data (if the node software that downloaded the tip was segwit aware it would have rewound or demanded a reindex, rather than downloading the tip). I think it’s still probably smart to check every block back to SegwitHeight in case of some weird bug in old software, so the normal case wouldn’t be any faster.

dhruv commented at 8:26 pm on February 27, 2021:

Yeah, it’s a little more intuitive to go backwards. It’s also faster. Updated.

Note that the code was not checking each block in [segwit_height, tip]. It was merely looking for one insufficiently validated block and returning true. It continues to do the same thing which is why going backwards from tip is faster.

ajtowns commented at 10:59 am on March 2, 2021:

In the normal case it checks every block in [segwit_height, tip] and returns false because it didn’t find a block. :) (Unless I’m very confused?)

dhruv commented at 1:44 pm on March 2, 2021:

You’re correct. I misunderstood what you were saying.

in src/init.cpp:1691 in 5e81857852 outdated

1706-            }
1707 
1708-            if (failed_rewind) {
1709-                break; // out of the chainstate activation do-while
1710+            if (!fReset) {
1711+                uiInterface.InitMessage(_("Checking if redownload is needed...").translated);

ajtowns commented at 7:18 am on February 19, 2021:

Is this message really needed on every startup, rather than just a message when some action is needed?

dhruv commented at 8:26 pm on February 27, 2021:

Nice idea. Removed.

in src/validation.h:687 in 5e81857852 outdated

683@@ -684,7 +684,7 @@ class CChainState
684 
685     /** Replay blocks that aren't fully applied to the database. */
686     bool ReplayBlocks(const CChainParams& params);
687-    bool RewindBlockIndex(const CChainParams& params) LOCKS_EXCLUDED(cs_main);
688+    [[nodiscard]] bool NeedsRedownload(const CChainParams& params) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

ajtowns commented at 5:12 am on February 20, 2021:

Add a doxygen comment? /** Check if chain state needs to be redownloaded due to lack of witness data */ ?

Should be a const method, as far as I can see.

dhruv commented at 8:26 pm on February 27, 2021:

Done and done.

ajtowns commented at 5:19 am on February 20, 2021: member

Here’s some suggested changes. Looks good to me though.

dhruv force-pushed on Feb 27, 2021

dhruv commented at 8:28 pm on February 27, 2021: member

Thanks for the reviews @ajtowns and @jnewbery. My apologies on the delay. Our twins arrived a few days ago so we are still finding our new routine :)

Comments addressed. Ready for further review.

jnewbery commented at 2:14 pm on February 28, 2021: member

utACK e750a774b8375dcbab9b804e2fd1334f1f62d6e4

DrahtBot added the label Needs rebase on Mar 4, 2021

dhruv force-pushed on Mar 4, 2021

jnewbery commented at 5:42 pm on March 4, 2021: member

Your latest rebase has reintroduced the EraseBlockData() declaration in validation.h L776.

dhruv force-pushed on Mar 4, 2021

dhruv commented at 5:56 pm on March 4, 2021: member

Ah, yes. I was just fixing it while you noticed as well. Thanks, @jnewbery. Rebased. Ready for further review.

jnewbery commented at 6:07 pm on March 4, 2021: member

utACK 1aecaac8b46a936634245739efff852f03c32b55

DrahtBot removed the label Needs rebase on Mar 4, 2021

ariard commented at 4:22 pm on March 9, 2021: member

Code Review ACK 1aecaac

in src/validation.cpp:4441 in 1aecaac8b4 outdated

4565-            BlockValidationState state;
4566-            if (!FlushStateToDisk(params, state, FlushStateMode::ALWAYS)) {
4567-                LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", state.ToString());
4568-                return false;
4569-            }
4570+    while (block != nullptr && block->nHeight >= params.GetConsensus().SegwitHeight) {

jonatack commented at 3:14 pm on March 10, 2021:

0-    CBlockIndex* block = m_chain.Tip();
1+    CBlockIndex* block{m_chain.Tip()};
2+    const int segwit_height{params.GetConsensus().SegwitHeight};
3 
4-    while (block != nullptr && block->nHeight >= params.GetConsensus().SegwitHeight) {
5+    while (block != nullptr && block->nHeight >= segwit_height) {

dhruv commented at 8:36 pm on March 11, 2021:

Done

in test/functional/p2p_segwit.py:1964 in 1aecaac8b4 outdated

1961+        assert_equal(self.nodes[0].getblockcount(), self.nodes[2].getblockcount())
1962+        assert_equal(self.nodes[1].getblockcount(), self.nodes[2].getblockcount())
1963+        assert SEGWIT_HEIGHT < self.nodes[2].getblockcount()
1964+        assert softfork_active(self.nodes[0], 'segwit')
1965+        assert softfork_active(self.nodes[1], 'segwit')
1966+        assert 'segwit' not in self.nodes[2].getblockchaininfo()['softforks']

jonatack commented at 3:15 pm on March 10, 2021:

0         # All nodes are caught up and node 2 is a pre-segwit node that will soon upgrade.
1+        for n in range(2):
2+            assert_equal(self.nodes[n].getblockcount(), self.nodes[2].getblockcount())
3+            assert softfork_active(self.nodes[n], "segwit")
4-        assert_equal(self.nodes[0].getblockcount(), self.nodes[2].getblockcount())
5-        assert_equal(self.nodes[1].getblockcount(), self.nodes[2].getblockcount())
6         assert SEGWIT_HEIGHT < self.nodes[2].getblockcount()
7-        assert softfork_active(self.nodes[0], 'segwit')
8-        assert softfork_active(self.nodes[1], 'segwit')
9         assert 'segwit' not in self.nodes[2].getblockchaininfo()['softforks']

dhruv commented at 8:36 pm on March 11, 2021:

Done

in test/functional/p2p_segwit.py:1989 in 1aecaac8b4 outdated

1986+        # The upgraded node syncs headers and performs redownload
1987+        assert_equal(self.nodes[0].getblockcount(), self.nodes[2].getblockcount())
1988+        assert_equal(self.nodes[1].getblockcount(), self.nodes[2].getblockcount())
1989+        assert softfork_active(self.nodes[0], 'segwit')
1990+        assert softfork_active(self.nodes[1], 'segwit')
1991         assert softfork_active(self.nodes[2], 'segwit')

jonatack commented at 3:15 pm on March 10, 2021:

0         # The upgraded node syncs headers and performs redownload
1-        assert_equal(self.nodes[0].getblockcount(), self.nodes[2].getblockcount())
2-        assert_equal(self.nodes[1].getblockcount(), self.nodes[2].getblockcount())
3-        assert softfork_active(self.nodes[0], 'segwit')
4-        assert softfork_active(self.nodes[1], 'segwit')
5-        assert softfork_active(self.nodes[2], 'segwit')
6+        for n in range(2):
7+            assert_equal(self.nodes[n].getblockcount(), self.nodes[2].getblockcount())
8+        for n in range(3):
9+            assert softfork_active(self.nodes[n], "segwit")

dhruv commented at 8:36 pm on March 11, 2021:

Done

in src/validation.h:727 in 1aecaac8b4 outdated

723@@ -724,7 +724,9 @@ class CChainState
724 
725     /** Replay blocks that aren't fully applied to the database. */
726     bool ReplayBlocks(const CChainParams& params);
727-    bool RewindBlockIndex(const CChainParams& params) LOCKS_EXCLUDED(cs_main);
728+
729+    /** Check if chain state needs to be redownloaded due to lack of witness data */
730+    [[nodiscard]] bool NeedsRedownload(const CChainParams& params) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);

jonatack commented at 3:18 pm on March 10, 2021:

0-    /** Check if chain state needs to be redownloaded due to lack of witness data */
1-    [[nodiscard]] bool NeedsRedownload(const CChainParams& params) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
2+    /** Whether the chain state needs to be reindexed due to lack of witness data. */
3+    [[nodiscard]] bool NeedsReindex(const CChainParams& params) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
4     bool LoadGenesisBlock(const CChainParams& chainparams);

dhruv commented at 8:38 pm on March 11, 2021:

I left it as NeedsRedownload as that represents more accurately, what’s needed. Restarting with -reindex is just the suggestion we make to the user that accomplishes the redownload. To me, NeedsReindex suggests merely rebuilding an index from what we have would be enough.

Updated the comment though.

jonatack commented at 3:21 pm on March 10, 2021: member

Tested Approach ACK, a few non-blocking thoughts below

in src/init.cpp:1700 in 1aecaac8b4 outdated

1713-                        failed_rewind = true;
1714-                        break; // out of the per-chainstate loop
1715-                    }
1716-                }
1717-            }
1718

jonatack commented at 3:24 pm on March 10, 2021:

(nit, remove extra blank line)

dhruv commented at 8:38 pm on March 11, 2021:

Done

DrahtBot added the label Needs rebase on Mar 11, 2021

dhruv force-pushed on Mar 11, 2021

dhruv commented at 8:39 pm on March 11, 2021: member

Thank you for the review @jonatack. Comments addressed. Rebased. Ready for further review.

DrahtBot removed the label Needs rebase on Mar 11, 2021

jonatack commented at 4:28 pm on March 12, 2021: member

ACK 644827722b9eba8af40576505a3d5444272f29c3 per git range-diff e0bc27a 1aecaac 6448277 and debug build/ran bitcoind

Small suggestion to save a few getblockcount() calls, happy to re-ACK if you update. Sorry for not noticing on the first pass:

 0         # All nodes are caught up and node 2 is a pre-segwit node that will soon upgrade.
 1+        node_2_height = self.nodes[2].getblockcount()
 2         for n in range(2):
 3-            assert_equal(self.nodes[n].getblockcount(), self.nodes[2].getblockcount())
 4+            assert_equal(self.nodes[n].getblockcount(), node_2_height)
 5             assert softfork_active(self.nodes[n], "segwit")
 6-        assert SEGWIT_HEIGHT < self.nodes[2].getblockcount()
 7+        assert SEGWIT_HEIGHT < node_2_height
 8         assert 'segwit' not in self.nodes[2].getblockchaininfo()['softforks']
 9 
10         # Restarting node 2 should result in a shutdown because the blockchain consists of
11@@ -1981,8 +1982,9 @@ class SegWitTest(BitcoinTestFramework):
12         self.sync_blocks(timeout=240)
13 
14         # The upgraded node syncs headers and performs redownload
15+        node_2_height = self.nodes[2].getblockcount()
16         for n in range(2):
17-            assert_equal(self.nodes[n].getblockcount(), self.nodes[2].getblockcount())
18+            assert_equal(self.nodes[n].getblockcount(), node_2_height)

jnewbery commented at 4:43 pm on March 12, 2021: member

utACK 644827722b9eba8af40576505a3d5444272f29c3

dhruv commented at 10:35 pm on March 14, 2021: member

Small suggestion to save a few getblockcount() calls, happy to re-ACK if you update. Sorry for not noticing on the first pass:

Thanks, @jonatack. I will update it if I end up rebasing or addressing other comments.

jamesob commented at 2:45 pm on March 25, 2021: member

Concept ACK, will review soon.

jnewbery commented at 8:31 am on April 8, 2021: member

@ariard do you mind re-reviewing this? I believe all of your review comments are addressed.

in test/functional/p2p_segwit.py:1985 in 644827722b outdated

1991-            assert_equal(block_hash, self.nodes[0].getblockhash(height))
1992-            assert_equal(self.nodes[0].getblock(block_hash), self.nodes[2].getblock(block_hash))
1993-            height -= 1
1994+        # The upgraded node syncs headers and performs redownload
1995+        for n in range(2):
1996+            assert_equal(self.nodes[n].getblockcount(), self.nodes[2].getblockcount())

glozow commented at 7:17 pm on April 19, 2021:

Perhaps worth verifying the blocks are equivalent, like in the original test?

0        height = self.nodes[2].getblockcount()
1        blockhash = self.nodes[2].getblockhash(height)
2        block = self.nodes[2].getblock(blockhash)
3        for n in [0, 1]:
4            assert_equal(self.nodes[n].getblockcount(), height)
5            assert_equal(self.nodes[n].getblockhash(height), blockhash)
6            assert_equal(self.nodes[n].getblock(blockhash), block)

jnewbery commented at 8:01 pm on April 19, 2021:

self.sync_blocks() ensures that all the nodes are synced to the same block (see #21009 (review)).

glozow commented at 2:50 pm on April 20, 2021:

is the assert_equal for getblockcount() necessary?

dhruv commented at 11:11 pm on April 21, 2021:

You’re right. sync_blocks() checks for the best block hash as mentioned, so the getblockcount() check is really just checking for very unlikely hash collisions. Updated.

in src/init.cpp:1705 in 644827722b outdated

1716-                    }
1717+            if (!fReset) {
1718+                LOCK(cs_main);
1719+                auto chainstates{chainman.GetAll()};
1720+                if (std::any_of(chainstates.begin(), chainstates.end(),
1721+                                [&chainparams](CChainState* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(chainparams); })) {

glozow commented at 7:31 pm on April 19, 2021:

0                                [&chainparams](const CChainState* const cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(chainparams); })) {

jnewbery commented at 7:59 pm on April 19, 2021:

There’s no harm in doing this, but I don’t think making the pointer const (CChainstate* const) is an improvement. In c++, when you pass a pointer, the called function makes a copy of that pointer, so it being const or not communicates nothing to the caller.

Making the pointer point to const data (const CChainstate*) on the other hand is useful. It means that the function can’t mutate the thing that cs is pointing to (and can’t be called with a pointer to non-const).

glozow commented at 9:20 pm on April 19, 2021:

Ah true 👍 the latter const is what I was going for :D

dhruv commented at 11:11 pm on April 21, 2021:

Done.

in src/validation.cpp:4443 in 644827722b outdated

4567-            if (!FlushStateToDisk(params, state, FlushStateMode::ALWAYS)) {
4568-                LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", state.ToString());
4569-                return false;
4570-            }
4571+    while (block != nullptr && block->nHeight >= segwit_height) {
4572+        if (!(block->nStatus & BLOCK_OPT_WITNESS)) {

glozow commented at 7:43 pm on April 19, 2021:

I did a quick grep for BLOCK_OPT_WITNESS and came across #19806 which added a “fake” BLOCK_OPT_WITNESS “so that RewindBlockIndex() doesn’t zealously unwind the assumed-valid chain.” I don’t have much background on AssumeUTXO but is there an interaction there? Should that be cleaned up as well?

jamesob commented at 3:34 pm on April 21, 2021:

Good call, @glozow. I’ll make a note to look at whether that’s still necessary after the merge of this PR.

glozow commented at 7:45 pm on April 19, 2021: member

Concept ACK to removing code that’s no longer necessary and the simplifications it allows

[validation] RewindBlockIndex no longer needed

Instead of rewinding blocks, we request that the user restarts with
-reindex

d831e711ca

dhruv force-pushed on Apr 21, 2021

dhruv commented at 11:13 pm on April 21, 2021: member

Addressed comments from @glozow and simplified the functional test a bit - previous code was checking segwit activation on all nodes, but we only need the check on the upgrading node. Ready for further review.

in test/functional/p2p_segwit.py:1974 in d831e711ca

1989-        height = self.nodes[2].getblockcount()
1990-        while height >= 0:
1991-            block_hash = self.nodes[2].getblockhash(height)
1992-            assert_equal(block_hash, self.nodes[0].getblockhash(height))
1993-            assert_equal(self.nodes[0].getblock(block_hash), self.nodes[2].getblock(block_hash))
1994-            height -= 1

jamesob commented at 11:30 pm on April 21, 2021:

So based on the removal of this test code (and the SEGWIT_HEIGHT - 1 assertion above), the implication seems to be we expect users who are upgrading from pre-segwit to remove their datadir and sync to tip from scratch? Based on the assertions here, -reindex will not bring them to tip, so what’s the point of telling the user to reindex at all? Or is this behavior specific to the functional tests, and in practice -reindex would bring the upgraded node to tip? (I’ll need to review the reindex code; I can’t remember what to expect there.)

If we expect users to remove their blocksdir before restarting with -reindex, you might consider mentioning that in the error message you’ve included in init.cpp.

jnewbery commented at 7:51 am on April 22, 2021:

Reindexing will result in the node discarding the blocks without witness after segwit activation and redownloading them from peers and validating. See #21009 (review). The reason the node doesn’t sync to tip here is because in our functional tests, the nodes don’t make automatic connections to each other. As soon as the nodes are connected, the upgraded node will resync from its peers.

dhruv commented at 3:07 pm on April 22, 2021:

Ah, I should have updated the PR description. Updated to now say:

This PR introduces NeedsRedownload() which merely checks for insufficiently validated segwit blocks and requests that the user restarts the node with -reindex. Reindexing the block files upon restart will make the node rebuild chain state and block index from the blk*.dat files on disk. The node won’t be able to index the blocks with BLOCK_OPT_WITNESS, so they will be missing from the chain and be re-downloaded, with witness data.

Sorry for the confusion.

As for the -reindex code, the call path is: src/init.cpp::Threadimport() -> validation.cpp:CChainState::LoadExternalBlockFile() -> validation.cpp:CChainState::AcceptBlock() -> validation.cpp:ContextualCheckBlock(). The last function tries to validate for witness commitments and is unable to thereby causing the block to not be accepted into the chain upon a restart with -reindex

Using combine_logs.py for test/functional/p2p_segwit.py you can see lines like:

0 node2 2021-04-22T14:56:56.543661Z [loadblk] [util/system.h:52] [error] ERROR: AcceptBlock: bad-witness-nonce-size, ContextualCheckBlock : invalid witness reserved value size
1 node2 2021-04-22T14:56:56.544812Z [loadblk] [util/system.h:52] [error] ERROR: AcceptBlock: bad-witness-nonce-size, ContextualCheckBlock : invalid witness reserved value size

jamesob commented at 3:23 pm on April 22, 2021:

Ah, thanks all. Makes sense.

jamesob commented at 11:33 pm on April 21, 2021: member

Looks good generally and I’m very much in favor of this change.

glozow commented at 2:09 am on April 22, 2021: member

utACK d831e711cab83c70bf2ded62fe33f484844e73dd

jnewbery commented at 7:46 am on April 22, 2021: member

utACK d831e711cab83c70bf2ded62fe33f484844e73dd

jamesob commented at 3:24 pm on April 22, 2021: member

ACK https://github.com/bitcoin/bitcoin/pull/21009/commits/d831e711cab83c70bf2ded62fe33f484844e73dd

Will be testing locally today.

jamesob commented at 5:29 pm on April 22, 2021: member

Built and ran tests locally. Grepped around for lingering references and found a few in documentation that can be cleaned up after this PR. Snapshot activation docs should also be changed in a follow-up PR but the existing logic there is still necessary: we still need to “fake” BLOCK_OPT_WITNESS in the way that we are now to avoid tripping the new NeedsRedownload() check.

Thanks for this change @dhruv. Nice work!

laanwj commented at 8:14 am on April 27, 2021: member

Cursory code review ACK d831e711cab83c70bf2ded62fe33f484844e73dd. Agree with the direction of the change, thanks for simplifying the logic here.

laanwj merged this on Apr 27, 2021

laanwj closed this on Apr 27, 2021

sidhujag referenced this in commit e3a02a794a on Apr 27, 2021

dhruv commented at 2:39 pm on April 30, 2021: member

Grepped around for lingering references and found a few in documentation that can be cleaned up after this PR @jamesob I’ve attempted to update references in #21816

laanwj referenced this in commit 5d83e7d714 on Jul 22, 2021

fanquake referenced this in commit 23e8c702bc on Mar 11, 2022

gwillen referenced this in commit 95ab1aa7cb on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

Remove RewindBlockIndex logic #21009

Conflicts