When setting both a new index and prune size and restarting an unpruned node, the node will prune the block store first and then the index will fail to start syncing.
Fix this by setting the prune lock to 0 if the index does not yet have a best block.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36150.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Concept ACK!
Since #34897 the locator can lag the in-memory tip while SetBestBlockIndex() still advances the lock on every update, so should we maybe derive the lock from the committed locator instead (initialize in Init(), advance in Commit(), cap at the chain height on reorg, flush and drain before pruneblockchain) and cover it with a prune-mode index test?
Since #34897 the locator can lag the in-memory tip while SetBestBlockIndex() still advances the lock on every update, so should we maybe derive the lock from the committed locator instead (initialize in Init(), advance in Commit(), cap at the chain height on reorg, flush and drain before pruneblockchain) and cover it with a prune-mode index test?
That seems out of scope for what this is fixing. If you have an unpruned node, and then add both a new index and a prune value to the config and restart, it will break on indexing because it will prune first before syncing the index. This is not about a lagging locator, but startup failing to lock the prune height for an empty index.
Concept ACK
ACK 39e4a6bb41749547c983456ce05281ca9eae16fe
I hope this can get in before v32 cutoff.
If you restart your non-pruned node with a prune target and a new index, you will first prune then crash on starting to index. Then you are S.O.L. with your new index and your only recourse is to do a full -reindex. I came across this while testing #36002.
It's a very simple fix. I assume the original check for block was intended as a null guard, but it should really set it to genesis instead of skipping the prune lock.
Here's a reproducer. I'm not sure it's worth putting it in the codebase though.
#!/usr/bin/env python3
from feature_pruning import mine_large_blocks
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_greater_than
class IndexPruneStartupTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
self.num_nodes = 1
def run_test(self):
node = self.nodes[0]
self.log.info("Build an unpruned chain exceeding the minimum automatic prune target")
mine_large_blocks(node, 1020)
height = node.getblockcount()
self.log.info("Enable automatic pruning and a fresh blockfilterindex on the same restart")
self.restart_node(0, extra_args=["-prune=550", "-blockfilterindex=1"])
self.wait_until(lambda: node.getindexinfo() == {
"basic block filter index": {"synced": True, "best_block_height": height},
})
if __name__ == "__main__":
IndexPruneStartupTest(__file__).main()
This is not about a lagging locator, but startup failing to lock the prune height for an empty index.
Sure, let’s fix startup here and address the lock advancing beyond the committed index state separately.
The problem we're fixing here was introduced in #21726, which included the genesis fallback at one stage, but lost it without a regression test catching it, so we should learn from that and add one here.
Luckily, DeletePruneLock() already returns whether the lock existed, so we can check that and just reinitialize the index in blockfilter_index_initial_sync.
I’ve prepared characterization + fix commits in l0rinc/bitcoin#296 as a demo.
When setting both a new index and prune size and restarting
an unpruned node, the node will prune the block store first
and then the index will fail to start syncing.
Fix this by setting the prune lock to 0 if the index does not
yet have a best block.
Thanks @l0rinc, I added the characterization test in the first commit and updated to the regression test in the second commit. I think the functional test suite is more appropriate for this, since we want to make sure we can startup and sync a new index when we introduce pruning at the same time. This functionality could possibly fail in other ways in the future, even if the underlying prune locks are correctly applied.
470 | @@ -472,15 +471,12 @@ def run_test(self): 471 | self.sync_blocks([self.nodes[0], self.nodes[5]], wait=5, timeout=300) 472 | 473 | self.log.info("Test prune with a new index") 474 | - self.stop_node(0) 475 | + self.restart_node(0, extra_args=["-prune=550", "-blockfilterindex=1"])
6d7caa1 indexes: set prune lock to genesis before first block:
We're changing the test conditions in the fix commit, so it's not obvious what the behavior was before the fix. Can you adjust the characterization commit so that ideally only the assertions change while the conditions stay the same (so that we're actually exercising the same use case)?
it's not obvious what the behavior was before the fix.
What specifically is not obvious about it? I'd prefer to leave it like this.
you're changing the conditions, this is a different test, so you haven't characterized the behavior before the fix
But which part is not obvious to you?
Milestone
32.0