Skip to content

feat: add reth db snapshot transactions | receipts commands#5007

Merged
joshieDo merged 39 commits intomainfrom
joshie/snapshot-bin-2
Oct 26, 2023
Merged

feat: add reth db snapshot transactions | receipts commands#5007
joshieDo merged 39 commits intomainfrom
joshie/snapshot-bin-2

Conversation

@joshieDo
Copy link
Collaborator

@joshieDo joshieDo commented Oct 13, 2023

  • moved the read + parallel hash calculation of the transaction table used on TxLookupStage ( cc @onbjerg @mattsse )
  • Adds SegmentHeader as a NippyJar header. Currently holds the block and transaction range of the snapshot.
  • adds reth db snapshot transactions command
  • adds reth db snapshot receipts command

@joshieDo joshieDo added C-enhancement New feature or request A-static-files Related to static files labels Oct 13, 2023
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Merging #5007 (e5e71be) into main (4dc15c3) will increase coverage by 44.11%.
Report is 15 commits behind head on main.
The diff coverage is 9.88%.

❗ Current head e5e71be differs from pull request most recent head efeb602. Consider uploading reports for the commit efeb602 to get more accurate results

Impacted file tree graph

Files Coverage Δ
crates/interfaces/src/provider.rs 100.00% <ø> (+100.00%) ⬆️
crates/primitives/src/transaction/mod.rs 79.69% <ø> (+42.93%) ⬆️
crates/stages/src/stages/tx_lookup.rs 97.08% <100.00%> (+94.89%) ⬆️
crates/storage/codecs/src/lib.rs 93.91% <ø> (+57.14%) ⬆️
crates/storage/db/src/snapshot.rs 84.31% <100.00%> (+84.31%) ⬆️
...torage/provider/src/providers/database/provider.rs 77.39% <100.00%> (+49.91%) ⬆️
crates/primitives/src/snapshot/segment.rs 46.66% <53.84%> (+46.66%) ⬆️
bin/reth/src/db/snapshots/headers.rs 0.00% <0.00%> (ø)
bin/reth/src/db/snapshots/bench.rs 0.00% <0.00%> (ø)
crates/storage/provider/src/traits/transactions.rs 0.00% <0.00%> (ø)
... and 8 more

... and 382 files with indirect coverage changes

Flag Coverage Δ
integration-tests 25.96% <0.00%> (-0.11%) ⬇️
unit-tests 61.74% <9.88%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 40.87% <0.00%> (+15.09%) ⬆️
blockchain tree 81.11% <ø> (+52.64%) ⬆️
pipeline 89.41% <100.00%> (+84.37%) ⬆️
storage (db) 74.96% <31.51%> (+44.98%) ⬆️
trie 95.00% <ø> (+72.46%) ⬆️
txpool 62.05% <ø> (+20.67%) ⬆️
networking 78.86% <ø> (+47.96%) ⬆️
rpc 58.91% <ø> (+32.43%) ⬆️
consensus 75.96% <ø> (+50.89%) ⬆️
revm 24.38% <ø> (+14.53%) ⬆️
payload builder 14.16% <ø> (ø)
primitives 85.68% <53.84%> (+56.50%) ⬆️

@joshieDo joshieDo changed the title feat: add reth db snapshot transactions command feat: add reth db snapshot transactions | receipts commands Oct 16, 2023
@joshieDo joshieDo marked this pull request as ready for review October 16, 2023 13:39
joshieDo and others added 10 commits October 24, 2023 11:10
Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>
Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>
Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>
Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>
Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>
Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>
Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>
@joshieDo joshieDo requested review from mattsse and shekhirin October 24, 2023 11:05
@joshieDo joshieDo requested a review from Rjected as a code owner October 24, 2023 12:01
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm,

the comment re parallel hashing can be addressed separately, I wonder, do we use the same logic in the mdbx implementation, if so we also need to revise that

let tx_range_size = tx_range.clone().count();
let tx_walker = tx_cursor.walk_range(tx_range)?;

let chunk_size = (tx_range_size / rayon::current_num_threads()).max(1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be problematic, because this can negatively impact everything else.

we're usually calling this on a per-block basis, right?

so in worst case we're spawning num CPU parallel worker threads with >1 txs.

the cutoff should be way higher so that the overhead of using the threadpool is worth it.

do we have benchmarks for this?

let's say we have 200 tx in a block and 16cores results in a chunk size of 12, which I think is a bit too low.

I think we should rather define a minimum txs per thread, maybe go with 32 or something until we have benchmarks, then we also need a variant where we don't even need to spawn a single thread.

but we can do all of this in a followup if we track it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshieDo joshieDo added this pull request to the merge queue Oct 26, 2023
Merged via the queue into main with commit 0116b80 Oct 26, 2023
@joshieDo joshieDo deleted the joshie/snapshot-bin-2 branch October 26, 2023 12:11
mattsse pushed a commit that referenced this pull request Nov 8, 2023
Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-static-files Related to static files C-enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants