blockstorage: Drop legacy -txindex check

[maflcko]

The only reason for the check was to print a warning about an increase in storage use. Now that 22.x is EOL and everyone should have migrated (or decided to not care about storage use), remove the check.

Also, a move-only commit is included. (Rebased from #22242)

[DrahtBot]

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, stickies-v
Concept ACK	theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #28392 (test: Use pathlib over os path by ns-xvrn)
• #28311 ([WIP] BIP300 (Drivechains) consensus-level logic by luke-jr)
• #28226 (util/blockstorage: Add TRACE_RAII, slightly faster -reindex-chainstate with CBufferedFile by martinus)
• #28051 (Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly by ryanofsky)
• #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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.

[theStack]

Concept ACK

There are still two instances where blocks_path can be used (sometimes it's quite annoying that python also allows single quote strings):

test/functional/feature_pruning.py:        self.prunedir = os.path.join(self.nodes[2].chain_path, 'blocks', '')
test/functional/wallet_backup.py:            shutil.rmtree(os.path.join(self.nodes[2].chain_path, 'blocks'))

With my very limited sed skills, I'd just add another line
sed -i "s|].chain_path, 'blocks'|].blocks_path|g" $(git grep -l chain_path)
to the scripted-diff (probably also possible with only one).

[maflcko]

Thanks, fixed up the scripted-diff

[fanquake]

cc @TheCharlatan

[TheCharlatan]

ACK fae4055, though I lack historical context to really judge the second commit fa86855.

I was curious if it might be possible to easily make one of the CBlockIndex* in BlockTreeDB a CBlockIndex&, but the change became a bit sprawling.

[maflcko]

though I lack historical context to really judge the second commit

The basic idea is that the legacy txindex was stored inside the block index db. Then a new src/index/ infrastructure was added and there was migration code. Then, the migration code was removed and a warning was added that a previous version is needed (or a full -reindex) to do the migration. Now that all those previous versions are EOL, and thus there should no one be left, who needs a migration, the warning can be removed. Even if someone is left behind, the worst that can happen is they'll be left with "dangling" db entries (using storage). And they can at any time clear the storage with a full -reindex.

[stickies-v]

ACK fae4055

The upgrade path when coming from legacy txindex is straightforward and clear enough, no need to keep this extra check around for such an old version imo.

[stickies-v]

Should we at some point sunset this test completely? Given that the txindex construction is completely separate from BlockTreeDB, I don't think there's really any compatibility testing happening anymore? It's good to have the test around for this pull, but I'm not sure when it'll be useful in the future.

[maflcko]

Correct. Seems fine to remove it in the future.

[maflcko]

I mean it is still checking that the dangling BlockTreeDB entries like t T, or txindex do not cause any issues, but at some point we can drop this test.

[stickies-v]

Okay, thx. I quite like adding "# TODO" comments to make it easier to find future cleanup items, but I don't really know how I'd phrase the "when" clause so no concrete suggestion.

[maflcko]

Maybe create an issue (after merge) to remove it after +1 release of this being merged?

[maflcko]

rfm, or is anything left to be done here?