Add pieces announcement through DSN. #847

shamil-gadelshin · 2022-10-03T11:58:09Z

This PR starts adding the archival storage layer support for the DSN v2. Fixes #846

Changes

add multihash conversion to PieceIndexHash primitive
add announce_piece and get_piece_providers methods to Node in the networking crate
change gossipsub archival data publication to Kademlia version
add provider support to custom_data_store in the networking crate
added 'announce-piece' example to the networking crate

Comments

custom_data_store doesn't support providers' removing yet

Code contributor checklist:

I have read, understood and followed contributing guide

- add libp2p crate dependency

- introduce ‘announce_piece’ for Node - introduce ‘get_piece_providers for Node - add provider’s support for the custom_data_store

nazar-pc

Announcement is great, but we also need to have a way to cancel announcement. What I imagine is that announce returns some guard object you need to keep around for as long as you're providing the data and once you're not anymore you just drop it and cancellation will be sent over the network.

nazar-pc · 2022-10-04T10:34:37Z

crates/subspace-core-primitives/Cargo.toml

@@ -19,6 +19,7 @@ dusk-bls12_381 = { version = "0.11", default-features = false, features = ["allo
 dusk-bytes = "0.1"
 dusk-plonk = { version = "0.12.0", default-features = false, features = ["alloc"], git = "https://github.com/subspace/plonk", rev = "193e68ba3d20f737d730e4b6edc757e4f639e7c3" }
 hex = { version  = "0.4.3", default-features = false }
+libp2p = {version = "0.46.1", optional=true, default-features = false }


Hm... I think it is really quite undesirable to make subspace-core-primitives depend on libp2p 😞

Also attention to detail:

Suggested change

libp2p = {version = "0.46.1", optional=true, default-features = false }

libp2p = { version = "0.46.1", optional = true, default-features = false }

Last time we had this conversation, you suggested adding dependencies. We need such a conversion (PieceIndexHash -> Multihash), we'll probably need some more conversions for primitive types. An alternative would be creating explicit conversion methods in the networking crate.

Well, PieceIndexHash already has AsRef<[u8]> implemented for it. Which means all you need to write is MultihashDigest::digest(&Code::Identity, piece_index_hash.as_ref()), which depending on how frequently that is used likely not worth building libp2p, which takes non-negligible amount of time, just to have From conversion.

Also one whitespace is still missing with latest changes.

crates/subspace-networking/src/node.rs

crates/subspace-service/src/dsn.rs

shamil-gadelshin

I agree that we need a provider removing mechanism. I wanted to add it using Kademlia's builtin features (provider ttl and republication interval). However, I wanted this changes in the next PR closer to the new plotting, when we will have pulling pieces.

It's not a problem though, please, let me know if you want it in this PR - I'll initialize these constants and add providers' removal code to the PR.

shamil-gadelshin · 2022-10-04T13:57:47Z

crates/subspace-core-primitives/Cargo.toml

@@ -19,6 +19,7 @@ dusk-bls12_381 = { version = "0.11", default-features = false, features = ["allo
 dusk-bytes = "0.1"
 dusk-plonk = { version = "0.12.0", default-features = false, features = ["alloc"], git = "https://github.com/subspace/plonk", rev = "193e68ba3d20f737d730e4b6edc757e4f639e7c3" }
 hex = { version  = "0.4.3", default-features = false }
+libp2p = {version = "0.46.1", optional=true, default-features = false }


Last time we had this conversation, you suggested adding dependencies. We need such a conversion (PieceIndexHash -> Multihash), we'll probably need some more conversions for primitive types. An alternative would be creating explicit conversion methods in the networking crate.

nazar-pc

TTL and republish are useful, but I believe there is also a way to explicitly remove it, which we can use for convenience. I wish there was a TODO for that, but nothing in this PR is necessarily blocking.

nazar-pc · 2022-10-05T12:00:17Z

crates/subspace-core-primitives/Cargo.toml

@@ -19,6 +19,7 @@ dusk-bls12_381 = { version = "0.11", default-features = false, features = ["allo
 dusk-bytes = "0.1"
 dusk-plonk = { version = "0.12.0", default-features = false, features = ["alloc"], git = "https://github.com/subspace/plonk", rev = "193e68ba3d20f737d730e4b6edc757e4f639e7c3" }
 hex = { version  = "0.4.3", default-features = false }
+libp2p = {version = "0.46.1", optional=true, default-features = false }


Well, PieceIndexHash already has AsRef<[u8]> implemented for it. Which means all you need to write is MultihashDigest::digest(&Code::Identity, piece_index_hash.as_ref()), which depending on how frequently that is used likely not worth building libp2p, which takes non-negligible amount of time, just to have From conversion.

nazar-pc · 2022-10-05T12:01:26Z

crates/subspace-core-primitives/Cargo.toml

@@ -19,6 +19,7 @@ dusk-bls12_381 = { version = "0.11", default-features = false, features = ["allo
 dusk-bytes = "0.1"
 dusk-plonk = { version = "0.12.0", default-features = false, features = ["alloc"], git = "https://github.com/subspace/plonk", rev = "193e68ba3d20f737d730e4b6edc757e4f639e7c3" }
 hex = { version  = "0.4.3", default-features = false }
+libp2p = {version = "0.46.1", optional=true, default-features = false }


Also one whitespace is still missing with latest changes.

shamil-gadelshin · 2022-10-05T12:58:48Z

TTL and republish are useful, but I believe there is also a way to explicitly remove it, which we can use for convenience. I wish there was a TODO for that, but nothing in this PR is necessarily blocking.

Turns out that TTL works only for the receiving peers and doesn't work for announcing peers. So, the stop_providing is mandatory. I will add it to the next PR.

I will format Cargo.toml again, but I'm not following about libp2p as a dependency. I agree that it is a relatively heavy dependency, but we have heavy dependencies already like dusk crates.

nazar-pc · 2022-10-05T13:01:37Z

I agree that it is a relatively heavy dependency, but we have heavy dependencies already like dusk crates.

Dusk crates are foundational, you'll have to write a lot of tricky code to get the same results as API provided by subspace-core-primitives. libp2p doesn't provide any new APIs, just a convenience conversion that saves you a few characters, but doesn't achieve anything substantial to justify adding it to dependencies IMHO.

shamil-gadelshin · 2022-10-05T13:06:02Z

Dusk crates are foundational, you'll have to write a lot of tricky code to get the same results as API provided by subspace-core-primitives. libp2p doesn't provide any new APIs, just a convenience conversion that saves you a few characters, but doesn't achieve anything substantial to justify adding it to dependencies IMHO.

Let's wait for some issues with this code, I'm not sure we will find any but feel free to move the conversion code to the networking crate any moment you found one.

nazar-pc · 2022-10-05T13:10:50Z

I mean the immediate issue is that everything (quite literally) depends on subspace-core-primitives, now everything effectively depends on libp2p and all of its numerous dependencies too, sequentializing/slowing down build times.

shamil-gadelshin added 4 commits October 3, 2022 14:17

primitives: Add multihash converstion to PieceIndexHash

6db75ec

- add libp2p crate dependency

networking: Add add- and get- providers for pieces.

c4c356f

- introduce ‘announce_piece’ for Node - introduce ‘get_piece_providers for Node - add provider’s support for the custom_data_store

networking: Add ‘announce-piece’ example

92e77bc

node: Change gossipsub pieces publishing to kad-version.

e12fd0e

shamil-gadelshin added networking Subspace networking (DSN) node Node (service library/node app) labels Oct 3, 2022

shamil-gadelshin requested a review from nazar-pc as a code owner October 3, 2022 11:58

shamil-gadelshin self-assigned this Oct 3, 2022

shamil-gadelshin requested review from rg3l3dr and liuchengxu as code owners October 3, 2022 11:58

nazar-pc reviewed Oct 4, 2022

View reviewed changes

Fix review comments.

8901af9

shamil-gadelshin commented Oct 4, 2022

View reviewed changes

nazar-pc approved these changes Oct 5, 2022

View reviewed changes

Merge branch 'main' into dsn2-announce-pieces

174e481

shamil-gadelshin enabled auto-merge October 5, 2022 12:59

shamil-gadelshin merged commit 37e044f into subspace:main Oct 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pieces announcement through DSN. #847

Add pieces announcement through DSN. #847

shamil-gadelshin commented Oct 3, 2022 •

edited by nazar-pc

nazar-pc left a comment

nazar-pc Oct 4, 2022

shamil-gadelshin Oct 4, 2022

nazar-pc Oct 5, 2022

nazar-pc Oct 5, 2022

shamil-gadelshin left a comment

shamil-gadelshin Oct 4, 2022

nazar-pc left a comment

nazar-pc Oct 5, 2022

nazar-pc Oct 5, 2022

shamil-gadelshin commented Oct 5, 2022

nazar-pc commented Oct 5, 2022

shamil-gadelshin commented Oct 5, 2022

nazar-pc commented Oct 5, 2022 •

edited

	libp2p = {version = "0.46.1", optional=true, default-features = false }
	libp2p = { version = "0.46.1", optional = true, default-features = false }

Add pieces announcement through DSN. #847

Add pieces announcement through DSN. #847

Conversation

shamil-gadelshin commented Oct 3, 2022 • edited by nazar-pc

Changes

Comments

Code contributor checklist:

nazar-pc left a comment

Choose a reason for hiding this comment

nazar-pc Oct 4, 2022

Choose a reason for hiding this comment

shamil-gadelshin Oct 4, 2022

Choose a reason for hiding this comment

nazar-pc Oct 5, 2022

Choose a reason for hiding this comment

nazar-pc Oct 5, 2022

Choose a reason for hiding this comment

shamil-gadelshin left a comment

Choose a reason for hiding this comment

shamil-gadelshin Oct 4, 2022

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

nazar-pc Oct 5, 2022

Choose a reason for hiding this comment

nazar-pc Oct 5, 2022

Choose a reason for hiding this comment

shamil-gadelshin commented Oct 5, 2022

nazar-pc commented Oct 5, 2022

shamil-gadelshin commented Oct 5, 2022

nazar-pc commented Oct 5, 2022 • edited

shamil-gadelshin commented Oct 3, 2022 •

edited by nazar-pc

nazar-pc commented Oct 5, 2022 •

edited