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)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Reviewers, this pull request conflicts with the following ones:
TRACE_RAII, slightly faster -reindex-chainstate with CBufferedFile by martinus)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.
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):
0test/functional/feature_pruning.py: self.prunedir = os.path.join(self.nodes[2].chain_path, 'blocks', '')
1test/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=.
-BEGIN VERIFY SCRIPT-
sed -i 's|CBlockTreeDB|BlockTreeDB|g' $( git grep -l CBlockTreeDB )
-END VERIFY SCRIPT-
ACK fae405556d56f6f13ce57f69a06b9ec1e825422b, though I lack historical context to really judge the second commit fa8685597e7302fc136f21b6dd3a4b187fa8e251.
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.
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.
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.
BlockTreeDB entries like t T, or txindex do not cause any issues, but at some point we can drop this test.
53@@ -55,23 +54,13 @@ def run_test(self):
54 drop_index_chain_dir = self.nodes[1].chain_path
55 shutil.rmtree(drop_index_chain_dir)
56 shutil.copytree(legacy_chain_dir, drop_index_chain_dir)
57- self.nodes[1].assert_start_raises_init_error(
58- extra_args=["-txindex"],
59- expected_msg="Error: The block index db contains a legacy 'txindex'. To clear the occupied disk space, run a full -reindex, otherwise ignore this error. This error message will not be displayed again.",
60- )
61 # Build txindex from scratch and check there is no error this time
62 self.start_node(1, extra_args=["-txindex"])
Could add these lines to check that if user is upgrading from an older node, they’ll still get a helpful message on how to use txindex again (albeit without cleaning up unused space)?
0 self.log.info("Check that \"Use -txindex\" error is thrown and -txindex works normally")
1 self.start_node(1)
2 assert_raises_rpc_error(-5, "Use -txindex", lambda: self.nodes[1].getrawtransaction(txid=spend_utxo["txid"]))
3 self.restart_node(1, extra_args=["-txindex"])
if user is upgrading from an older node, they’ll still get a helpful message on how to use txindex again
I don’t think your suggested test is related to this pull. IIUC you are checking that getrawtransaction recommends to set -txindex. However, if a user previously had it set, they won’t need to change it, and if a user previously had it unset, they’d have to set it in the previous version and here, unrelated of the changes here.
There is a different test in a another file that checks this already, no?
Yeah you’re right, this doesn’t add anything. I wanted to check that a user had clear instructions on how to build the new txindex, but they wouldn’t have to do anything at all, as they’re already expected to keep -txindex in startup options every time.
Could explicitly add a check that we’re indeed building a new txindex (as opposed to somehow using the legacy one), but that doesn’t seem like a likely issue either so may not be worth testing? No strong view, so can be marked as resolved.
0 with self.nodes[1].assert_debug_log(expected_msgs=['Syncing txindex with block chain from height 0']):
1 self.start_node(1, extra_args=["-txindex"])
ACK fae405556d56f6f13ce57f69a06b9ec1e825422b
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.