Add persistent record storage for the DSN. #862

shamil-gadelshin · 2022-10-14T14:43:13Z

This PR adds a code supporting pieces cache for the DSN. It adds a persistent implementation of the Kademlia record storage.

Changes

add ParityDb record storage (with BTreeIterator wrapper)
add put_value operation for Node
refactor record storage configuration

Comments

expiration Record field should be implemented separately

Code contributor checklist:

I have read, understood and followed contributing guide

- split providers and records traits

- add MemoryRecordStorage - add NoRecordStorage

nazar-pc

Some questions and nits.

Please try to rebase against main before opening PR and make sure all commits compile, pass checks and, ideally, all changes are as close to the final version as possible from the beginning.

crates/subspace-networking/src/behavior/custom_record_store.rs

crates/subspace-farmer/src/lib.rs

crates/subspace-networking/src/behavior.rs

crates/subspace-networking/src/create.rs

crates/subspace-networking/src/behavior/custom_record_store.rs

nazar-pc · 2022-10-17T05:58:40Z

crates/subspace-networking/src/node.rs

+    pub async fn put_value(&self, key: Multihash, value: Vec<u8>) -> Result<bool, PutValueError> {
+        let (result_sender, result_receiver) = oneshot::channel();
+
+        self.shared
+            .command_sender
+            .clone()
+            .send(Command::PutValue {
+                key,
+                value,
+                result_sender,
+            })
+            .await?;
+
+        Ok(result_receiver.await?)
+    }


I thought we discussed that we'll not have puts, instead we'll do announcements and it will be up to the target of the announcement to download and cache something or not.

We discussed it offline: I will test L2 cache using puts and later test pull solution to match "get-closest and then send add-provider" algorithm.

nazar-pc · 2022-10-17T06:04:06Z

crates/subspace-networking/src/behavior/custom_record_store.rs

+                error!(?err, "Can't create Parity DB record storage iterator.");
+
+                ParityDbRecordIterator::empty()


Why returning empty iterator instead of an error? Silencing these doesn't seem nice.

I also don't like that it doesn't fail here. You can at least discard errors and return an option as an api.

I don't like it either. However, libp2p interface is suboptimal here. The only alternative I see is to panic. Please, let me know whether I should panic here.

Please add TODO about this with link to upstream issue that these should be made fallible to allow more backend implementations.

libp2p/rust-libp2p#3035

i1i1 · 2022-10-17T08:44:54Z

crates/subspace-networking/src/behavior/custom_record_store.rs

+        match result {
+            Ok(data) => {
+                if let Some(data) = data {
+                    let db_rec_result = serde_json::from_slice::<ParityDbRecord>(&data);
+
+                    match db_rec_result {
+                        Ok(db_rec) => {
+                            trace!(?key, "Record loaded successfully from DB");
+
+                            Some(Cow::Owned(db_rec.into()))
+                        }
+                        Err(err) => {
+                            debug!(?key, ?err, "Parity DB record deserialization error");
+
+                            None
+                        }
+                    }
+                } else {
+                    trace!(?key, "No Parity DB record for given key");
+
+                    None
+                }
+            }
+            Err(err) => {
+                debug!(?key, ?err, "Parity DB record storage error");
+
+                None
+            }
+        }


You can match for literally any value:

match result { Ok(Some(data)) => { let db_rec_result = serde_json::from_slice::<ParityDbRecord>(&data); match db_rec_result { Ok(db_rec) => { trace!(?key, "Record loaded successfully from DB"); Some(Cow::Owned(db_rec.into())) } Err(err) => { debug!(?key, ?err, "Parity DB record deserialization error"); None } } } Ok(None) => { trace!(?key, "No Parity DB record for given key"); None } Err(err) => { debug!(?key, ?err, "Parity DB record storage error"); None } }

Agree. This option is better. Thanks.

i1i1 · 2022-10-17T08:47:18Z

crates/subspace-networking/src/behavior/custom_record_store.rs

+        let tx = vec![(self.column_id, key, data)];
+
+        let result = self.db.commit(tx);


Vector is heap allocated. You can use either of those instead:

self.db.commit(Some((self.column_id, key, data))); self.db.commit(std::iter::once((self.column_id, key, data)));

Ok. I will change to Option but with a separate line for it.

I believe you can also just use [] or &[], Option is not natural here even though it happens to implement IntoIterator.

i1i1 · 2022-10-17T08:59:39Z

crates/subspace-networking/src/behavior/custom_record_store.rs

+                error!(?err, "Can't create Parity DB record storage iterator.");
+
+                ParityDbRecordIterator::empty()


I also don't like that it doesn't fail here. You can at least discard errors and return an option as an api.

crates/subspace-networking/src/behavior/custom_record_store.rs

# Conflicts: # crates/subspace-farmer/src/single_plot_farm.rs

shamil-gadelshin added 6 commits October 10, 2022 18:46

networking: Refactor CustomRecordStore

406815d

- split providers and records traits

networking: Add new record store types.

4913706

- add MemoryRecordStorage - add NoRecordStorage

Refactor custom record store for Kademlia configuration.

3102e91

networking: Add ParityDb record storage.

0560bfb

networking: Add ‘PutValue’ command to Node.

4be9129

networking: Introduce ParityDB record storage.

cfb661e

shamil-gadelshin added the networking Subspace networking (DSN) label Oct 14, 2022

shamil-gadelshin self-assigned this Oct 14, 2022

shamil-gadelshin requested review from nazar-pc, rg3l3dr and i1i1 as code owners October 14, 2022 14:43

Merge branch 'main' into dsn2-push-pieces-from-node

eef8714

nazar-pc reviewed Oct 17, 2022

View reviewed changes

i1i1 reviewed Oct 17, 2022

View reviewed changes

shamil-gadelshin linked an issue Oct 17, 2022 that may be closed by this pull request

DSN. Add piece publishing to cache (L2) for farmers. #870

Closed

networking: Fix review comments.

a9feff9

shamil-gadelshin requested review from nazar-pc and i1i1 October 17, 2022 16:35

nazar-pc reviewed Oct 18, 2022

View reviewed changes

shamil-gadelshin added 3 commits October 18, 2022 13:28

networking: Fix review comments.

ff288de

Merge branch 'main' into dsn2-push-pieces-from-node

ae39d38

# Conflicts: # crates/subspace-farmer/src/single_plot_farm.rs

networking: Fix review comments.

06cbab9

shamil-gadelshin requested a review from nazar-pc October 18, 2022 07:08

nazar-pc approved these changes Oct 18, 2022

View reviewed changes

i1i1 approved these changes Oct 18, 2022

View reviewed changes

shamil-gadelshin merged commit f901f75 into main Oct 18, 2022

shamil-gadelshin deleted the dsn2-push-pieces-from-node branch October 18, 2022 11:21

shamil-gadelshin mentioned this pull request Oct 24, 2022

DSN. Add DSN support for consensus v2 #845

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add persistent record storage for the DSN. #862

Add persistent record storage for the DSN. #862

shamil-gadelshin commented Oct 14, 2022

nazar-pc left a comment

nazar-pc Oct 17, 2022

shamil-gadelshin Oct 17, 2022

nazar-pc Oct 17, 2022

i1i1 Oct 17, 2022

shamil-gadelshin Oct 17, 2022

nazar-pc Oct 18, 2022

shamil-gadelshin Oct 18, 2022

i1i1 Oct 17, 2022

shamil-gadelshin Oct 17, 2022

i1i1 Oct 17, 2022

shamil-gadelshin Oct 17, 2022

nazar-pc Oct 18, 2022

i1i1 Oct 17, 2022

		error!(?err, "Can't create Parity DB record storage iterator.");

		ParityDbRecordIterator::empty()

		let tx = vec![(self.column_id, key, data)];

		let result = self.db.commit(tx);

Add persistent record storage for the DSN. #862

Add persistent record storage for the DSN. #862

Conversation

shamil-gadelshin commented Oct 14, 2022

Changes

Comments

Code contributor checklist:

nazar-pc 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment