fix: ensure transaction lookup can prune #19553
Conversation
joshieDo
added
C-bug
| return Ok(SegmentOutput { | ||
| progress: PruneProgress::Finished, | ||
| pruned: 0, | ||
| checkpoint: input | ||
| .previous_checkpoint | ||
| .map(SegmentOutputCheckpoint::from_prune_checkpoint), | ||
| }) |
There was a problem hiding this comment.
drive-by and unsure.
on one hand returning an empty checkpoint is wrong, on the other deleted_headers is empty because input.to_block falls in the middle of a static file (eg. block 10 in a range 0..=499_999k)
opted for finished, but could also be not_done with a interrupt reason, thoughts?
There was a problem hiding this comment.
why are we checking deleted_headers here?
let deleted_headers = provider
.static_file_provider()
.delete_segment_below_block(StaticFileSegment::Transactions
There was a problem hiding this comment.
unsure what the impact of this is
There was a problem hiding this comment.
deleted_headers refers to a list of SegmentHeader { block_range, tx_range} of static files that we were able to delete.
if the prune target is a block in the middle of a static file, we would return empty here
joshieDo
force-pushed
the
joshie/coordinate-tx-pruning
branch
from
7c106f5 to
718feb9
Compare
| // Check if tx_lookup will eventually need to prune any blocks <= our target | ||
| Ok(tx_lookup_mode | ||
| .next_pruned_block(tx_lookup_checkpoint) | ||
| .is_some_and(|next_block| next_block <= target) | ||
| .not()) |
There was a problem hiding this comment.
why is it not enough to just check tx_lookup_checkpoint >= target here?
There was a problem hiding this comment.
you could have a Before(100) for txlookup and a Before(200) for bodies. (lets forget sf ranges for now)
our tx_lookup_checkpoint would always be 99 and bodies would never run
| return Ok(SegmentOutput { | ||
| progress: PruneProgress::Finished, | ||
| pruned: 0, | ||
| checkpoint: input | ||
| .previous_checkpoint | ||
| .map(SegmentOutputCheckpoint::from_prune_checkpoint), | ||
| }) |
There was a problem hiding this comment.
why are we checking deleted_headers here?
let deleted_headers = provider
.static_file_provider()
.delete_segment_below_block(StaticFileSegment::Transactions
| return Ok(SegmentOutput { | ||
| progress: PruneProgress::Finished, | ||
| pruned: 0, | ||
| checkpoint: input | ||
| .previous_checkpoint | ||
| .map(SegmentOutputCheckpoint::from_prune_checkpoint), | ||
| }) |
There was a problem hiding this comment.
unsure what the impact of this is
| return Ok(SegmentOutput::not_done( | ||
| PruneInterruptReason::WaitingOnSegment(PruneSegment::TransactionLookup), | ||
| input.previous_checkpoint.map(SegmentOutputCheckpoint::from_prune_checkpoint), | ||
| )) |
joshieDo
force-pushed
the
joshie/coordinate-tx-pruning
branch
from
c04a960 to
5c00ece
Compare
| // The highest block number in the deleted files is the actual checkpoint. to_block might | ||
| // refer to a block in the middle of a undeleted file. | ||
| let checkpoint_block = deleted_headers |
There was a problem hiding this comment.
if checkpoint_block != input.to_block we can't return PruneProgress::not_done, otherwise the PrunerStage would get stuck trying over and over again
| /// # Examples | ||
| /// | ||
| /// - `Before(10)` with checkpoint at block 5 returns `Some(6)` | ||
| /// - `Before(10)` with checkpoint at block 9 returns `None` (done) | ||
| /// - `Distance(100)` with checkpoint at block 1000 returns `Some(1001)` (always has more) | ||
| /// - `Full` always returns the next block after checkpoint |
| pub const fn next_pruned_block(&self, checkpoint: Option<BlockNumber>) -> Option<BlockNumber> { | ||
| let next = match checkpoint { | ||
| Some(c) => c + 1, | ||
| None => 0, |
There was a problem hiding this comment.
should we start at genesis here or 1?
| // Transaction lookup (should be run before bodies) | ||
| .segment_opt(transaction_lookup.map(TransactionLookup::new)) | ||
| // Bodies | ||
| .segment_opt(bodies_history.map(|mode| Bodies::new(mode, transaction_lookup))) |
There was a problem hiding this comment.
can we add more note for dummies: because we still need bodies for txlookup
| /// Transaction lookup prune mode. Used to determine if we need to wait for tx lookup pruning | ||
| /// before deleting transaction bodies. | ||
| tx_lookup_mode: Option<PruneMode>, |
There was a problem hiding this comment.
okay, now this makes sense to me
| let tx_lookup_checkpoint = provider | ||
| .get_prune_checkpoint(PruneSegment::TransactionLookup)? | ||
| .and_then(|cp| cp.block_number); |
There was a problem hiding this comment.
this should also have the uncommitted txlookip checkpoint, right?
even if not, this wouldnt be an issue because this would then just trail 1 prune run behind
| // Determine the safe prune target, if any. | ||
| let to_block = match tx_lookup_mode.next_pruned_block(tx_lookup_checkpoint) { | ||
| None => Some(input.to_block), /* tx_lookup is done */ | ||
| Some(tx_lookup_next) if tx_lookup_next > input.to_block => Some(input.to_block), /* tx_lookup is ahead */ |
There was a problem hiding this comment.
can we document what this means exactly,
basically why can't we also prune up to tx_lookup_next?
| let pruned = tx_ranges.clone().map(|range| range.len()).sum::<u64>() as usize; | ||
|
|
||
| // The highest block number in the deleted files is the actual checkpoint. to_block might | ||
| // refer to a block in the middle of a undeleted file. |
There was a problem hiding this comment.
| // refer to a block in the middle of a undeleted file. | |
| // refer to a block in the middle of an undeleted file. |
Before transactions can be pruned, we need to ensure we won't delete any transactions that the tx lookup job will need for its own run.