feat: share `SnapshotProvider` through `ProviderFactory` #5249

joshieDo · 2023-10-31T11:46:59Z

Adds a shared SnapshotProvider to ProviderFactory & DatabaseProvider
Share it to all reth components on boot-up

joshieDo · 2023-10-31T11:50:12Z

bin/reth/src/node/mod.rs

@@ -295,7 +295,9 @@ impl<Ext: RethCliExt> NodeCommand<Ext> {
        let head = self.lookup_head(Arc::clone(&db)).wrap_err("the head block is missing")?;

        // setup the blockchain provider
-        let factory = ProviderFactory::new(Arc::clone(&db), Arc::clone(&self.chain));
+        let (highest_snapshots_tx, highest_snapshots_rx) = watch::channel(None);
+        let factory = ProviderFactory::new(Arc::clone(&db), Arc::clone(&self.chain))


am i correct to understand that this will be shared across all components that might create providers?

through blockchain_db

mattsse

It's getting a bit harder for me to maintain an overview of all components and how they'll fit together.
we need a proper overview and docs.

The Snapshotter::new needs more docs, unclear what the watch channel does.

after having a closer look at how the channel is currently used
do we care if the value changed or if is sufficient to just get the current highest snapshot?

because this is essentially an Rwlock that keeps track of changes

mattsse · 2023-10-31T14:01:53Z

crates/storage/provider/src/providers/database/mod.rs

+    /// Snapshot Provider
+    snapshot_provider: Option<Arc<SnapshotProvider>>,


I think this is a reasonable location for the snapshot integration, otherwise we'd need to integrate it into the BlockchainProvider, which I guess would also be suitable.

joshieDo · 2023-11-01T20:23:06Z

ref #5270 <- overview docs

joshieDo · 2023-11-01T20:25:01Z

do we care if the value changed or if is sufficient to just get the current highest snapshot?

It should be sufficient to get the current one. Even if a new one comes the moment i'm reading the old one, it just means that the new snapshot files were just created. Data hasn't been deleted yet, that's the Pruners job after it gets the very same notification.

mattsse · 2023-11-02T10:43:42Z

crates/blockchain-tree/src/blockchain_tree.rs

@@ -1071,6 +1071,7 @@ impl<DB: Database, EF: ExecutorFactory> BlockchainTree<DB, EF> {
        let provider = DatabaseProvider::new_rw(
            self.externals.db.tx_mut()?,
            self.externals.chain_spec.clone(),
+            None,


I don't like that this extra argument is now required

mattsse · 2023-11-02T10:45:39Z

bin/reth/src/node/mod.rs

@@ -295,7 +295,9 @@ impl<Ext: RethCliExt> NodeCommand<Ext> {
        let head = self.lookup_head(Arc::clone(&db)).wrap_err("the head block is missing")?;

        // setup the blockchain provider
-        let factory = ProviderFactory::new(Arc::clone(&db), Arc::clone(&self.chain));
+        let (highest_snapshots_tx, highest_snapshots_rx) = watch::channel(None);


where does the sender go here, looks unused now.

I think we should consider hiding this channel and move instantiation to the Snapshotter instead, it can keep both halfs and return a new receiver on demand

reth/bin/reth/src/node/mod.rs

Lines 469 to 474 in 113ef23

let _snapshotter = reth_snapshot::Snapshotter::new(

db,

self.chain.clone(),

self.chain.snapshot_block_interval,

highest_snapshots_tx,

);

i think it still makes sense to move it inside

crates/snapshot/src/snapshotter.rs

crates/storage/provider/src/providers/snapshot/manager.rs

shekhirin · 2023-11-09T16:58:18Z

crates/storage/provider/src/providers/snapshot/manager.rs


 /// SnapshotProvider
 #[derive(Debug, Default)]
 pub struct SnapshotProvider {
    /// Maintains a map which allows for concurrent access to different `NippyJars`, over different
    /// segments and ranges.
    map: DashMap<(BlockNumber, SnapshotSegment), LoadedJar>,
+    /// Tracks the latest and highest snapshot of every segment.
+    highest_tracker: Option<watch::Receiver<Option<HighestSnapshots>>>,


can we reuse HighestSnapshotsTracker here?

that would mean adding tokio as a dependency to primitives, and I didn't want to add that tbh

that would mean adding tokio as a dependency to primitives

sorry I don't understand, HighestSnapshotsTracker is in reth-snapshot crate that we already use as a dependency in reth-provider, no?

Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>

mattsse

suggesting to replace the watch channel with SnapshotTracker {Arc<Rwlock>} wrapper type

crates/snapshot/src/snapshotter.rs

mattsse · 2023-11-13T12:59:09Z

crates/snapshot/src/snapshotter.rs

+        let _ = self.highest_snapshots_notifier.send(Some(self.highest_snapshots)).map_err(|_| {
            warn!(target: "snapshot", "Highest snapshots channel closed");
        });
    }


this error can safely be ignored because it's kept open by having both as fields

mattsse · 2023-11-13T13:01:45Z

crates/snapshot/src/snapshotter.rs

    highest_snapshots: HighestSnapshots,
-    highest_snapshots_tracker: watch::Sender<Option<HighestSnapshots>>,
+    /// Channel sender to notify other components of the new highest snapshots
+    highest_snapshots_notifier: watch::Sender<Option<HighestSnapshots>>,


do we expect features that need to listen for changed snapshots?

if not perhaps we could just use an rwlock wrapper type? because a watch channel is also just an rwlock with some add ons.

if we need listeners for snapshots we can add this separately

yeah, agree. i'll do it in a further PR if that's okay

mattsse

lgtm,

okay with doing the rwlock refactor separately.

pending @shekhirin

mattsse · 2023-11-14T15:28:13Z

bin/reth/src/node/mod.rs

@@ -295,7 +295,9 @@ impl<Ext: RethCliExt> NodeCommand<Ext> {
        let head = self.lookup_head(Arc::clone(&db)).wrap_err("the head block is missing")?;

        // setup the blockchain provider
-        let factory = ProviderFactory::new(Arc::clone(&db), Arc::clone(&self.chain));
+        let (highest_snapshots_tx, highest_snapshots_rx) = watch::channel(None);
+        let factory = ProviderFactory::new(Arc::clone(&db), Arc::clone(&self.chain))


joshieDo added 5 commits October 31, 2023 11:35

add snapshots_path to ChainPath

eb00e6d

move HighestSnapshots to primitives crate

a8ef221

add with_highest_tracker to SnapshotProvider

f1f32d7

add a shared snapshot provvider to providerfactory and dbprovider

f5cadf8

add snapshot provider to shared blockchain_db

1acd28f

joshieDo added C-enhancement New feature or request A-static-files Related to static files labels Oct 31, 2023

joshieDo requested review from shekhirin and mattsse October 31, 2023 11:47

joshieDo requested review from rakita, rkrasiuk, onbjerg and gakonst as code owners October 31, 2023 11:47

allow unused for now

470a6b6

joshieDo commented Oct 31, 2023

View reviewed changes

add get_highest_snapshot to SnapshotProvider

113ef23

mattsse reviewed Oct 31, 2023

View reviewed changes

mattsse requested changes Nov 2, 2023

View reviewed changes

joshieDo added 3 commits November 2, 2023 16:29

move hihgest snapshot channel inside snapshotter

73fd00f

add default receiver to snapshotter

2fef51f

replace with with_snapshot_provider on db provider

33dc284

joshieDo requested a review from mattsse November 2, 2023 17:07

joshieDo mentioned this pull request Nov 3, 2023

feat: add directory paths to Snapshotter and SnapshotProvider #5283

Merged

Merge remote-tracking branch 'origin/main' into joshie/db-snap-provider

3be09cd

shekhirin reviewed Nov 9, 2023

View reviewed changes

joshieDo and others added 3 commits November 13, 2023 11:40

Update crates/snapshot/src/snapshotter.rs

d0699f7

Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>

Update crates/snapshot/src/snapshotter.rs

892b260

Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>

Update crates/storage/provider/src/providers/snapshot/manager.rs

756d00b

Co-authored-by: Alexey Shekhirin <a.shekhirin@gmail.com>

mattsse requested changes Nov 13, 2023

View reviewed changes

mattsse approved these changes Nov 14, 2023

View reviewed changes

joshieDo added this pull request to the merge queue Nov 14, 2023

Merged via the queue into main with commit d21e346 Nov 14, 2023
25 checks passed

joshieDo deleted the joshie/db-snap-provider branch November 14, 2023 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: share `SnapshotProvider` through `ProviderFactory` #5249

feat: share `SnapshotProvider` through `ProviderFactory` #5249

joshieDo commented Oct 31, 2023

joshieDo Oct 31, 2023

joshieDo Oct 31, 2023

mattsse Nov 14, 2023

mattsse left a comment

mattsse Oct 31, 2023

joshieDo commented Nov 1, 2023 •

edited

Loading

joshieDo commented Nov 1, 2023

mattsse Nov 2, 2023

mattsse Nov 2, 2023

shekhirin Nov 2, 2023

joshieDo Nov 2, 2023

shekhirin Nov 9, 2023

joshieDo Nov 13, 2023

shekhirin Nov 13, 2023

mattsse left a comment

mattsse Nov 13, 2023

mattsse Nov 13, 2023

joshieDo Nov 14, 2023

mattsse left a comment

mattsse Nov 14, 2023

		/// Snapshot Provider
		snapshot_provider: Option<Arc<SnapshotProvider>>,

	let _snapshotter = reth_snapshot::Snapshotter::new(
	db,
	self.chain.clone(),
	self.chain.snapshot_block_interval,
	highest_snapshots_tx,
	);

feat: share SnapshotProvider through ProviderFactory #5249

feat: share SnapshotProvider through ProviderFactory #5249

Conversation

joshieDo commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshieDo commented Nov 1, 2023 • edited Loading

joshieDo commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: share `SnapshotProvider` through `ProviderFactory` #5249

feat: share `SnapshotProvider` through `ProviderFactory` #5249

joshieDo commented Nov 1, 2023 •

edited

Loading