Remove RewindBlockIndex logic #17862

1. 
   ariard commented at 10:53 pm on January 3, 2020: member

   RewindBlockIndex was made to allow nodes upgrading to new consensus rules post-segwit fork to download new consensus data (i.e witnesses) without having to go through a whole IBD. It has been pointed than now really few pre-segwit nodes care to upgrade and that this logic may be irrelevant.

   See #17737 (review) in review of #17737 for more context pointers.

2. ariard added the label Feature on Jan 3, 2020
3. fanquake added the label Brainstorming on Jan 3, 2020
4. 
   jnewbery commented at 8:52 am on December 14, 2020: member

   A bit more context:

   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 661100, the blockchain is around 315GB and the total number of txs is around 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.

   Removing this dead code would allow the following:

   • removal of the test_upgrade_after_activation test in p2p_segwit.py.
   • in turn, that would allow us to drop support for -segwitheight=-1, which is only supported for that test.
   • that would allow 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 would allow us to drop all of the GetLocalServices() & NODE_WITNESS checks inside net_processing, since we local services would always include NODE_WITNESS
5. 
   jnewbery commented at 8:54 am on December 14, 2020: member

   From discussions with @sdaftuar and @sipa, I think this is safe to remove. If someone upgrades with non-witness validated blocks, we should shutdown with an error that instructs the user to reindex.
6. 
   MarcoFalke commented at 8:58 am on December 14, 2020: member

   in turn, that would allow us to drop support for -segwitheight=-1, which is only supported for that test.

   I’d say to keep that test (and check for the init error), but still remove segwitheight=-1, as a previous released version can be used to create the blocksdir.

7. 
   jnewbery commented at 9:15 am on December 14, 2020: member

   a previous released version can be used to create the blocksdir.

   That’s how we used to test this upgrade code, before https://github.com/bitcoin/bitcoin/commit/95f4a03777ec2ad82a94a3e2890192a93ad83509.

   We could download v0.13 binaries to run this test, but I really don’t think it’s worth it.

8. 
   dhruv commented at 4:20 am on January 20, 2021: member

   I’ll attempt to create a patch for this in the coming days.
9. 
   dhruv commented at 8:12 pm on January 26, 2021: member

   #21009 is ready for review.
10. laanwj referenced this in commit 19a56d1519 on Apr 27, 2021
11. laanwj closed this on Apr 27, 2021

    ---

12. 
    MarcoFalke commented at 8:18 am on April 27, 2021: member

    Are the follow-ups from #17862 (comment) tracked somewhere else, now that this is closed?
13. 
    jnewbery commented at 8:28 am on April 27, 2021: member

    Yes, the follow-ups are done in #21090
14. sidhujag referenced this in commit e3a02a794a on Apr 27, 2021
15. laanwj referenced this in commit 5d83e7d714 on Jul 22, 2021
16. DrahtBot locked this on Aug 18, 2022

Contributors
ariard jnewbery MarcoFalke dhruv

Labels
Feature Brainstorming