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.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--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.
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?
The conditions are the same. The node is restarted with with a prune target and a new index. It's not obvious to me how to make this more obvious.
Can you please unresolve this comment
The characterization test could do a start, wait_until_stopped, start_node as well, so that the fix commit only changes the wait to wait_until - instead of changing assert_start_raises_init_error to wait_until which is hard to interpret, it's not obvious how this changes the behavior, or what we're even testing before.
I think I will just keep it as is for now.
Re-ACK 6d7caa1c23346fadd15183db0f57385207db113b
ACK 6d7caa1c23346fadd15183db0f57385207db113b
480 | - expected_msg="basic block filter index best block of the index goes beyond pruned data", 481 | - match=ErrorMatch.PARTIAL_REGEX, 482 | - ) 483 | - self.start_node(0, extra_args=["-prune=550"]) 484 | + height = node.getblockcount() 485 | + self.wait_until(lambda: node.getindexinfo() == {
This is an extended (runtime) script, so can take longer and isn't run by all jobs. That said, for me it runs in:
1/1 - feature_pruning.py passed, Duration: 95 s
TEST | STATUS | DURATION
feature_pruning.py | ✓ Passed | 95 s
ALL | ✓ Passed | 95 s (accumulated)
Runtime: 95 s
What hardware are you on?
In the CI this PR took:
alpine: feature_pruning.py | ✓ Passed | 301 s
previous releases: feature_pruning.py | ✓ Passed | 121 s
most recent merge to master: alpine: feature_pruning.py | ✓ Passed | 173 s
previous releases: feature_pruning.py | ✓ Passed | 90 s
So it's certainly adding to the runtime, but I'd say that's expected, as we are pruning and indexing instead of init erroring...
I'm surprised it makes this much of a difference in the CI, not seeing much of a difference locally.
EDIT: It's probably debug mode again.
Instead of syncing completely might it be enough to sync a few blocks instead?
Fixed. We only wait until at least the first 10 blocks are synced.
Code review ACK 6d7caa1c23346fadd15183db0f57385207db113b
The genesis prune lock is set before startup pruning, preserving the blocks the index needs when they're available.
Did not run the reproducer from the comments.
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.
Re-ACK 9b2299514028f5055d4314e402776cbb45c5f538
503 | @@ -504,9 +504,9 @@ void BaseIndex::SetBestBlockIndex(const CBlockIndex* block) 504 | { 505 | assert(!m_chainstate->m_blockman.IsPruneMode() || AllowPrune()); 506 | 507 | - if (AllowPrune() && block) { 508 | + if (AllowPrune()) { 509 | node::PruneLockInfo prune_lock; 510 | - prune_lock.height_first = block->nHeight; 511 | + prune_lock.height_first = block ? block->nHeight : 0;
nit: Wouldn't have hurt to add a comment here, to the effect of what's in the commit message.
Code review ACK 9b2299514028f5055d4314e402776cbb45c5f538
Post-merge reACK 9b2299514028f5055d4314e402776cbb45c5f538
Milestone
32.0