Page MenuHomePhabricator

retire support of pre-0.22.8 block index upgrade
ClosedPublic

Authored by PiRK on Apr 13 2022, 16:18.

Details

Reviewers
Fabien
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.

Event Timeline

PiRK edited the test plan for this revision. (Show Details)

improve comments

PiRK published this revision for review.Apr 25 2022, 08:58
Fabien requested changes to this revision.Apr 25 2022, 14:22
Fabien added a subscriber: Fabien.

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

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

PiRK planned changes to this revision.Apr 26 2022, 05:27

re-running tests

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

Fabien requested changes to this revision.Apr 26 2022, 07:05

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

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.

PiRK planned changes to this revision.Apr 26 2022, 10:12

Testing ongoing.

PiRK requested review of this revision.Apr 26 2022, 11:01
This revision is now accepted and ready to land.Apr 27 2022, 07:40