Remove RewindBlockIndex logic #21009

dhruv · 2021-01-26T04:53:23Z

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

DrahtBot · 2021-01-26T09:32:39Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Introduce deploymentstatus #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 · 2021-01-26T16:41:08Z

Concept ACK

dhruv · 2021-01-26T20:11:48Z

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 · 2021-01-26T21:08:49Z

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

dhruv · 2021-01-26T22:20:08Z

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

theStack · 2021-01-27T19:25:17Z

Concept ACK

ccdle12

Concept ACK

src/validation.cpp

src/init.cpp

dhruv · 2021-01-27T21:13:19Z

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

luke-jr · 2021-01-28T21:30:44Z

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 · 2021-01-29T02:29:44Z

@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 · 2021-02-01T22:24:53Z

Concept ACK.

jnewbery

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.

src/init.cpp

src/validation.h

src/validation.cpp

ariard

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.

src/validation.cpp

src/chainparams.cpp

src/net_processing.cpp

dhruv · 2021-02-06T02:42:35Z

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

src/init.cpp

src/validation.cpp

test/functional/p2p_segwit.py

dhruv · 2021-03-11T20:39:36Z

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

jonatack · 2021-03-12T16:28:19Z

ACK 6448277 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:

suggestion

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

jnewbery

utACK 6448277

dhruv · 2021-03-14T22:35:16Z

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 · 2021-03-25T14:45:35Z

Concept ACK, will review soon.

jnewbery · 2021-04-08T08:31:12Z

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

glozow

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

test/functional/p2p_segwit.py

src/init.cpp

glozow · 2021-04-19T19:43:12Z

src/validation.cpp

-                return false;
-            }
+    while (block != nullptr && block->nHeight >= segwit_height) {
+        if (!(block->nStatus & BLOCK_OPT_WITNESS)) {


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?

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

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

dhruv · 2021-04-21T23:13:00Z

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.

jamesob

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

jamesob · 2021-04-21T23:30:58Z

test/functional/p2p_segwit.py

-            block_hash = self.nodes[2].getblockhash(height)
-            assert_equal(block_hash, self.nodes[0].getblockhash(height))
-            assert_equal(self.nodes[0].getblock(block_hash), self.nodes[2].getblock(block_hash))
-            height -= 1


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.

Reindexing will result in the node discarding the blocks without witness after segwit activation and redownloading them from peers and validating. See #21009 (comment). 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.

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:

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 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

Ah, thanks all. Makes sense.

glozow

utACK d831e71

jnewbery · 2021-04-22T07:46:59Z

utACK d831e71

jamesob · 2021-04-22T15:24:30Z

ACK d831e71

Will be testing locally today.

jamesob · 2021-04-22T17:29:02Z

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 · 2021-04-27T08:14:34Z

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

d831e71 [validation] RewindBlockIndex no longer needed (Dhruv Mehta) Pull request description: Closes bitcoin#17862 Context from [original comment](bitcoin#17862 (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 bitcoin#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` ACKs for top commit: jnewbery: utACK d831e71 jamesob: ACK bitcoin@d831e71 laanwj: Cursory code review ACK d831e71. Agree with the direction of the change, thanks for simplifying the logic here. glozow: utACK d831e71 Tree-SHA512: 3eddf5121ccd081ad7f15a5c6478ef867083edc8ba0bf1ee759e87bc070ee3d2f0698a3feba8db8dc087987c8452887b6f72cff05b3e178f41cb10a515fb8053

dhruv · 2021-04-30T14:39:49Z

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

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta) Pull request description: Builds on #21009 and makes progress on remaining items in #17862 Removing `RewindBlockIndex()` in #21009 allows the following: - removal of tests using `segwitheight=-1` in `p2p_segwit.py`. - move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime - 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` ACKs for top commit: mzumsande: Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647 Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

40e871d [miner] always assume we can create witness blocks (glozow) Pull request description: Given the low possibility of a reorg reverting the segwit soft fork, there is no longer a need to check whether segwit is active to see if it's okay to add to the block template (see also #23512, #21009, etc). `TestBlockValidity()` is also run on the block template at the end of `CreateNewBlock()`, so any invalid block would be caught there. ACKs for top commit: gruve-p: ACK 40e871d jnewbery: utACK 40e871d, although I disagree about changing the test for segwit transaction in mempool before activagtion, instead of just removing it: #24421 (comment). achow101: ACK 40e871d theStack: Code-review ACK 40e871d Tree-SHA512: bf4860bf2bed8339622d05228d11d60286edb0c32a9a3c434b8d154913c07ea56e50649f4af7009c2a1c6a58a81d2299ab43b41a6f16dee7d08cc89cc1603019

fanquake added the Validation label Jan 26, 2021

dhruv mentioned this pull request Jan 26, 2021

Remove RewindBlockIndex logic #17862

Closed

dhruv force-pushed the rewindblockindex-2021 branch from 02934c0 to 7c919a1 Compare January 26, 2021 22:04

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

ccdle12 reviewed Jan 27, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

dhruv force-pushed the rewindblockindex-2021 branch from 7c919a1 to 79970c9 Compare January 27, 2021 21:12

DrahtBot mentioned this pull request Jan 28, 2021

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

DrahtBot mentioned this pull request Feb 1, 2021

[Bundle 3/n] Prune remaining g_chainman usage in validation functions #21055

Merged

2 tasks

jnewbery reviewed Feb 2, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

ariard reviewed Feb 3, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/chainparams.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

ariard mentioned this pull request Feb 4, 2021

validation: UTXO snapshot activation #19806

Merged

dhruv force-pushed the rewindblockindex-2021 branch from 79970c9 to 9bae47d Compare February 5, 2021 20:28

dhruv mentioned this pull request Feb 5, 2021

Default to NODE_WITNESS in nLocalServices #21090

Merged

jnewbery reviewed Feb 8, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

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

dhruv force-pushed the rewindblockindex-2021 branch from 9bae47d to 416854c Compare February 8, 2021 17:23

DrahtBot removed the Needs rebase label Mar 11, 2021

jnewbery reviewed Mar 12, 2021

View reviewed changes

This was referenced Mar 15, 2021

wallet: ensure wallet files are not reused across chains #18554

Merged

doc: Address feedback from Transifex translator community #21463

Closed

DrahtBot mentioned this pull request Mar 24, 2021

validation: UpdateTip/CheckBlockIndex assumeutxo support #21526

Merged

glozow reviewed Apr 19, 2021

View reviewed changes

[validation] RewindBlockIndex no longer needed

d831e71

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

dhruv force-pushed the rewindblockindex-2021 branch from 6448277 to d831e71 Compare April 21, 2021 23:09

jamesob reviewed Apr 21, 2021

View reviewed changes

glozow reviewed Apr 22, 2021

View reviewed changes

laanwj merged commit 19a56d1 into bitcoin:master Apr 27, 2021

dhruv mentioned this pull request Apr 30, 2021

[docs] Update references to RewindBlockIndex() #21816

Closed

michaelfolkson mentioned this pull request Jun 3, 2021

doc: Various validation doc fixups #22121

Merged

glozow mentioned this pull request Feb 22, 2022

miner: always assume we can build witness blocks #24421

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 19a56d1 into merged_master (Bitcoin PR bitcoin/bitcoin#21009)

95ab1aa

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Remove RewindBlockIndex logic #21009

Remove RewindBlockIndex logic #21009

Conversation

dhruv commented Jan 26, 2021 • edited

DrahtBot commented Jan 26, 2021 • edited

Conflicts

jnewbery commented Jan 26, 2021

dhruv commented Jan 26, 2021

practicalswift commented Jan 26, 2021

dhruv commented Jan 26, 2021

theStack commented Jan 27, 2021

ccdle12 left a comment

Choose a reason for hiding this comment

dhruv commented Jan 27, 2021

luke-jr commented Jan 28, 2021

dhruv commented Jan 29, 2021

ariard commented Feb 1, 2021

jnewbery left a comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

dhruv commented Feb 6, 2021

dhruv commented Mar 11, 2021 • edited

jonatack commented Mar 12, 2021

jnewbery left a comment

Choose a reason for hiding this comment

dhruv commented Mar 14, 2021

jamesob commented Mar 25, 2021

jnewbery commented Apr 8, 2021

glozow left a comment

Choose a reason for hiding this comment

glozow Apr 19, 2021 • edited

Choose a reason for hiding this comment

jamesob Apr 21, 2021

Choose a reason for hiding this comment

dhruv commented Apr 21, 2021

jamesob left a comment

Choose a reason for hiding this comment

jamesob Apr 21, 2021

Choose a reason for hiding this comment

jnewbery Apr 22, 2021

Choose a reason for hiding this comment

dhruv Apr 22, 2021 • edited

Choose a reason for hiding this comment

jamesob Apr 22, 2021

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

jnewbery commented Apr 22, 2021

jamesob commented Apr 22, 2021

jamesob commented Apr 22, 2021

laanwj commented Apr 27, 2021

dhruv commented Apr 30, 2021

dhruv commented Jan 26, 2021 •

edited

DrahtBot commented Jan 26, 2021 •

edited

dhruv commented Mar 11, 2021 •

edited

glozow Apr 19, 2021 •

edited

dhruv Apr 22, 2021 •

edited