feat(snapshots): write Receipt to snapshots during ExecutionStage
#6103
Conversation
This reverts commit b9c51e9.
Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>
| // `maybe_snapshotter` already prunes receipts if it detects that there are more | ||
| // elements in the static files than expected. | ||
| if let Some(stage_checkpoint) = stage_checkpoint.as_mut() { | ||
| for block_number in range { |
There was a problem hiding this comment.
this is to "unwind" the stage progress?
There was a problem hiding this comment.
added more context, lmk if it makes sense
| } | ||
| reverse_walker.delete_current()?; | ||
| None => { |
There was a problem hiding this comment.
i'd like if we could either make maybe_snapshotter more verbose in its naming, split it out into two separate functions, or add some more comments here. it is a bit hard to follow what exactly goes on since maybe_snapshotter (as a name) doesn't say much, but seems to do a lot?
There was a problem hiding this comment.
changed the name and added context, lmk if it makes sense
joshieDo
force-pushed
the
joshie/tx-snap-sync
branch
from
ead73cc
to
782f7e3
Compare
…eceipts-snap-sync
| // for receipts. | ||
| let snapshotter = try_prepare_snapshotter(&self.prune_modes, provider, start_block)?; |
There was a problem hiding this comment.
this function is standalone for testing reasons?
then this makes sense, though I'd also add a fn try_prepare_snapshotter(&self if possible
There was a problem hiding this comment.
i guess this was doing too much and not very clear
- check if
snapshotteris to be used. - put it outside of the function - consistency check of the static file data (eg. do i have more receipts than database expects? then unwind static files)
added more docs that hopefully clarify
| if let Some(stage_checkpoint) = stage_checkpoint.as_mut() { | ||
| stage_checkpoint.progress.processed -= receipt.cumulative_gas_used; | ||
| if let Some(stage_checkpoint) = stage_checkpoint.as_mut() { | ||
| stage_checkpoint.progress.processed -= receipt.cumulative_gas_used; |
There was a problem hiding this comment.
because this is part of the unwind and we need to set the total gas progress back .
it already existed beforehand, this is my understanding
| /// | ||
| /// `omit_changed_check` should be set to true of bundle has some of it data | ||
| /// detached, This would make some original values not known. | ||
| pub fn write_to_db<TX: DbTxMut + DbTx>( | ||
| pub fn write_to_storage<TX: DbTxMut + DbTx>( |
There was a problem hiding this comment.
I'd prefer a where clause instead
| let (_, body_indices) = | ||
| bodies_cursor.seek_exact(block_number)?.unwrap_or_else(|| { | ||
| let mut body_indices = |block_number: BlockNumber| { | ||
| Ok::<_,DatabaseError>(bodies_cursor.seek_exact(block_number)?.unwrap_or_else(|| { |
There was a problem hiding this comment.
this large Ok looks very strange
can you do this in two steps?
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
…eceipts-snap-sync
…th into joshie/receipts-snap-sync
…eceipts-snap-sync
| @@ -264,6 +264,13 @@ impl SnapshotProvider { | |||
| index.insert(tx_end, current_block_range.clone()); | |||
| }) | |||
| .or_insert_with(|| BTreeMap::from([(tx_end, current_block_range)])); | |||
| } else if let Some(1) = tx_index.get(&segment).map(|index| index.len()) { | |||
| // Only happens if we unwind all the txs/receipts from the first static file. | |||
| // Should only happen in test scenarios. | |||
There was a problem hiding this comment.
hmm, isn't it possible that we have index of length one during the normal node operation?
There was a problem hiding this comment.
i may be missing something, but i think highly unlikely...
Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>
attention: it only writes to static files if there's no pruning/configuration enabled for receipts (includes
--fullwhich only includes deposit contract receipts). Meaning: it keeps them on databaseon top of #5733