Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABC1b7645404f4b: retire support of pre-0.22.8 block index upgrade

Summary

Stop supporting database upgrades for databases last updated by Bitcoin ABC < 0.22.8. These will now require a full reindex.
This removes a txdb -> blockstorage dependency which would cause an additional circular dependency in D11350.

Keep writing the current version number to the database, as this could be useful again for future upgrades and debugging.

Test Plan

Happy path 1: Start testnet IBD on this version of the node. Check that the node accepts to start from a non-existent DB.
Happy path 2: After letting the node in the previous test run for a few minutes to ensure there is a block index DB, stop it, then bump the version in src/CMakeLists.txt, recompile and restart the node, and check that IBD resumes successfully after the updating of the DB.
Stop the node, revert the previous version bumping, recompile and restart the node, check that it fails to start. This is because even though CBlockTreeDB::Upgrade returns true after finding a DB with a more recent version than the node, CBlockTreeDB::LoadBlockIndexGuts then fails to load a database with a version number different than the node.
Download a binary for an old node version prior to 0.22.8, us it to start a fresh IBD and let it run for a few minutes. Stop the node, compile and run the node for the current commit, and check that it fails to start because the DB version is too old. Restart with option -reindex, and check that this works.

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Apr 13 2022, 16:18

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 13 2022, 16:18

Harbormaster completed remote builds in B18820: Diff 33209.Apr 13 2022, 16:41

PiRK edited the summary of this revision. (Show Details)Apr 25 2022, 07:22

PiRK edited the test plan for this revision. (Show Details)Apr 25 2022, 08:52

improve comments

PiRK published this revision for review.Apr 25 2022, 08:58

PiRK mentioned this in D11348: move-only: Move ThreadImport to blockstorage.Apr 25 2022, 08:59

PiRK added a child revision: D11348: move-only: Move ThreadImport to blockstorage.Apr 25 2022, 09:00

PiRK added a child revision: D11350: move-only: Move *Disk functions to blockstorage.Apr 25 2022, 09:02

Harbormaster completed remote builds in B18872: Diff 33294.Apr 25 2022, 09:20

There is an important missing case in your test plan: normal upgrade from the last version to the next one, actually checking the happy path of this code.

src/txdb.cpp
481 ↗	(On Diff #33294)	There is a logic error here. If version has never been set then it's an old db (before the version number was written) and it needs reindexing.

This revision now requires changes to proceed.Apr 25 2022, 14:22

PiRK added inline comments.Apr 25 2022, 14:48

src/txdb.cpp
481 ↗	(On Diff #33294)	The other situation in which the version has never been set is a brand new node starting for the first time, like it happens in the tests. I need to figure out a way to handle the case you are describing without breaking the tests or making it impossible to start the node.

Fabien added inline comments.Apr 25 2022, 14:54

src/txdb.cpp
481 ↗	(On Diff #33294)	The good news is that this was handled properly before you changed that code, so you can just put it back there
481 ↗	(On Diff #33294)	Just put this block before your branch and you're done

Take into account case of an old database without version number set (so without block size either)
To simplify the if clause, drop the now uneccessary condition of CLIENT_VERSION < 0.22.8

re-running tests

Harbormaster completed remote builds in B18886: Diff 33309.Apr 26 2022, 05:42

restore the log message for the happy path ("Upgrading block index database...\n")

Harbormaster completed remote builds in B18887: Diff 33310.Apr 26 2022, 06:18

You really need to update your test plan with the happy path and not only checking for the error cases.

src/txdb.cpp
485 ↗	(On Diff #33310)	What is that log doing there ? It's plain wrong
560 ↗	(On Diff #33310)	And now if the database exists already you don't upgrade the version anymore

This revision now requires changes to proceed.Apr 26 2022, 07:05

PiRK added inline comments.Apr 26 2022, 08:53

src/txdb.cpp
485 ↗	(On Diff #33310)	It is upgrading the version number.
560 ↗	(On Diff #33310)	It is done above, in the `if (version < CLIENT_VERSION && version >= CDiskBlockIndex::TRACK_SIZE_VERSION)` part above. I just factored the "DB does not exist" and "version needs to be updated" code paths, as the action is exactly the same now: just write the version number to the DB.

Fabien added inline comments.Apr 26 2022, 09:41

src/txdb.cpp
482 ↗	(On Diff #33310)	Old versions might have blocks but no version number, and then you'll get an inconsistent state: the version is updated but the blocks are still missing the size info

Reverse the logic to hand the error first and the happy path last.
Also the cursor needs to be updated to the db key, to find out if the db actually exists. Previously I thought I could do this with the cursor to the version key, but that does not tell us if the db does not exist or is too old to have a "version" key.

Now we also take the return value of MakeBatch into account. For now, this function always returns true, so there is no change in behavior. If this ever changes, it makes sense to refuse to start the node if the database writing fails.

Testing ongoing.

Harbormaster completed remote builds in B18890: Diff 33313.Apr 26 2022, 10:28

PiRK edited the test plan for this revision. (Show Details)Apr 26 2022, 10:36

PiRK requested review of this revision.Apr 26 2022, 11:01

Fabien accepted this revision.Apr 27 2022, 07:40

This revision is now accepted and ready to land.Apr 27 2022, 07:40

Closed by commit rABC1b7645404f4b: retire support of pre-0.22.8 block index upgrade (authored by PiRK). · Explain WhyApr 27 2022, 10:18

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC1b7645404f4b: retire support of pre-0.22.8 block index upgrade.