blockstorage: Drop legacy -txindex check by maflcko · Pull Request #28195 · bitcoin/bitcoin

maflcko · 2023-08-01T10:29:42Z

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 · 2023-08-01T10:29:45Z

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

-BEGIN VERIFY SCRIPT- sed -i 's|].chain_path, .blocks.|].blocks_path|g' $(git grep -l chain_path) -END VERIFY SCRIPT-

The block index (CBlockTreeDB) is required to write and read blocks, so move it to blockstorage. This allows to drop the txdb.h include from `node/blockstorage.h`. Can be reviewed with: --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

Can be reviewed with --word-diff-regex=.

maflcko · 2023-08-01T15:00:19Z

Thanks, fixed up the scripted-diff

fanquake · 2023-08-01T15:29:00Z

cc @TheCharlatan

-BEGIN VERIFY SCRIPT- sed -i 's|CBlockTreeDB|BlockTreeDB|g' $( git grep -l CBlockTreeDB ) -END VERIFY SCRIPT-

sedited · 2023-08-10T12:17:47Z

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 · 2023-08-15T09:27:28Z

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 · 2023-08-31T11:32:30Z

test/functional/feature_txindex_compatibility.py

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.

Correct. Seems fine to remove it in the future.

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.

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.

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

test/functional/feature_txindex_compatibility.py

maflcko · 2023-09-05T07:23:02Z

rfm, or is anything left to be done here?

897d6dd Remove feature_txindex_compatibility.py in V27 (Brandon Odiwuor) Pull request description: Fixes #28421, see [#28195 (comment)](fa86855#r1311494362) Remove feature_txindex_compatibility.py in V27, follow up to #28195 being merged which is included in v26 ACKs for top commit: maflcko: lgtm ACK 897d6dd theStack: ACK 897d6dd stickies-v: ACK 897d6dd Tree-SHA512: 53102d39f6fdbdcf1bb13b6feb2f446b0e9e8e3fe294c0e6fe37e7731713fb9fd5b048e19b6edf80579f5edbcf762b51d56d57bdcda67ec3527706891dc3572b

DrahtBot changed the title ~~blockstorage: Drop legacy -txindex check~~ blockstorage: Drop legacy -txindex check Aug 1, 2023

DrahtBot added the Block storage label Aug 1, 2023

This was referenced Aug 1, 2023

test: Drop 22.x node from TxindexCompatibilityTest #28070

Merged

kernel: Prune leveldb headers #28186

Merged

maflcko force-pushed the 2307-test-blocks- branch from fa6c48a to faedae4 Compare August 1, 2023 10:36

DrahtBot added the CI failed label Aug 1, 2023

maflcko force-pushed the 2307-test-blocks- branch from faedae4 to fa518b7 Compare August 1, 2023 11:45

theStack reviewed Aug 1, 2023

View reviewed changes

MarcoFalke added 4 commits August 1, 2023 15:26

scripted-diff: Use blocks_path where possible

fa69148

-BEGIN VERIFY SCRIPT- sed -i 's|].chain_path, .blocks.|].blocks_path|g' $(git grep -l chain_path) -END VERIFY SCRIPT-

index: Drop legacy -txindex check

fa86855

Fixup style of moved code

faf6303

Can be reviewed with --word-diff-regex=.

maflcko force-pushed the 2307-test-blocks- branch from fa518b7 to faf6303 Compare August 1, 2023 13:28

DrahtBot removed the CI failed label Aug 1, 2023

DrahtBot mentioned this pull request Aug 2, 2023

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

scripted-diff: Rename CBlockTreeDB -> BlockTreeDB

fae4055

-BEGIN VERIFY SCRIPT- sed -i 's|CBlockTreeDB|BlockTreeDB|g' $( git grep -l CBlockTreeDB ) -END VERIFY SCRIPT-

DrahtBot mentioned this pull request Aug 6, 2023

util/blockstorage: Add TRACE_RAII, slightly faster -reindex-chainstate with CBufferedFile #28226

Closed

fanquake requested a review from stickies-v August 10, 2023 13:14

This was referenced Aug 22, 2023

[WIP] BIP300 (Drivechains) consensus-level logic #28311

Closed

Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

Merged

stickies-v approved these changes Aug 31, 2023

View reviewed changes

DrahtBot mentioned this pull request Sep 2, 2023

test: Use pathlib over os path #28392

Merged

fanquake merged commit ecab855 into bitcoin:master Sep 5, 2023

maflcko deleted the 2307-test-blocks- branch September 5, 2023 10:41

stickies-v mentioned this pull request Sep 6, 2023

test: Remove feature_txindex_compatibility.py in v27 #28421

Closed

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023

Merge bitcoin#28195: blockstorage: Drop legacy -txindex check

cf0e1cc

BrandonOdiwuor mentioned this pull request Oct 30, 2023

test: Remove feature_txindex_compatibility.py #28753

Merged

bitcoin locked and limited conversation to collaborators Sep 4, 2024

Conversation

maflcko commented Aug 1, 2023

Uh oh!

DrahtBot commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

theStack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 1, 2023

Uh oh!

fanquake commented Aug 1, 2023

Uh oh!

sedited commented Aug 10, 2023

Uh oh!

maflcko commented Aug 15, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Sep 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Aug 1, 2023 •

edited

Loading

theStack left a comment •

edited

Loading

stickies-v Aug 31, 2023 •

edited

Loading