Suggestion for adding bitswap to substrate #7133

dvc94ch · 2020-09-17T19:56:52Z

I can fix the build failures, but wanted to get some early feedback first. This is intended to superceed the PR by @expenses #6795 in a way that it is useful for sunshine-protocol to use this feature effectively.

ipfs-embed now supports plugging in an external networking stack. [0] is the corresponding PR to use the NetworkService of a substrate light client as the backend for ipfs-embed in the sunshine client.

all the changes outside of sc-networking come from adding a generic parameter to the NetworkService that implements the MultihashDigest trait from tiny-multihash. This is used by bitswap to verify the hashes of ipfs blocks. (We require this as we have a custom hasher based on sp-trie that checks that the content identifier matches the trie root).

adds providers, provide and unprovide methods to NetworkService for registering content in the dht.
adds a BootstrapComplete event to NetworkService, and starts bootstrapping the dht. (not sure why this wasn't required before)
adds bitswap send, send-all, want and cancel methods to NetworkService for making and handling bitswap requests.
when a peer is discovered it calls the connect method on the bitswap NetworkBehaviour.
uncomments the role checking causing light client peers being dropped prematurely.

[0] Networking sunshine-protocol/sunshine-core#5

expenses · 2020-09-18T10:16:59Z

bin/node-template/node/src/service.rs

@@ -91,7 +92,7 @@ pub fn new_full(config: Configuration) -> Result<TaskManager, ServiceError> {
 		GrandpaFinalityProofProvider::new_for_service(backend.clone(), client.clone());

 	let (network, network_status_sinks, system_rpc_tx, network_starter) =
-		sc_service::build_network(sc_service::BuildNetworkParams {
+		sc_service::build_network::<_, _, _, _, Multihash>(sc_service::BuildNetworkParams {


Ideally, I think we want Multihash to be a default field here, so we don't have to do this whole _, _, _, thing. As functions can't have generic fields, but structs can, we could move build_network to be a method of BuildNetworkParams instead. This would look uglier and be less consistent, but it would atleast solve this problem.

Not sure if BuildNetworkParams should have a pub field of type PhantomData. I made build_network a method that only takes one generic type parameter, the Multihash.

dvc94ch · 2020-09-18T14:24:15Z

This can be rebased on top of #6795 as a follow-up.

dvc94ch · 2020-09-18T17:43:45Z

@tomaka what are your thoughts on supporting light client to light client network protocols?

tomaka · 2020-09-21T07:05:51Z

wanted to get some early feedback first

I'm not exactly sure what you would like to get feedback on.
If the question is whether the feature should go in Substrate, judging by the number of 🚀 emojis I guess the answer is yes.
If the question is whether this should be implemented using a NetworkBehaviour, well that's what matches our architecture so yes.
I can give a review of small things that could be tweaked, but I suppose you're not at this point yet.

@tomaka what are your thoughts on supporting light client to light client network protocols?

What do you mean by that? Light clients can connect and can theoretically talk to each other. In practice there isn't anything that a light client could ask to another light client at the moment.

If you're asking in the context of the DHT, our general assumption at the moment is that light clients are short-lived and that they're more or less parasites for all networking-related purposes. There is a plan to do #3303, but since it hasn't been implemented yet there isn't anything preventing light clients from behaving like a full client on the DHT.

dvc94ch · 2020-09-21T10:55:31Z

I'm not exactly sure what you would like to get feedback on

Whether or not this belongs in substrate and if a network behaviour is the way to go. Question answered, thanks.

What do you mean by that? Light clients can connect and can theoretically talk to each other. In practice there isn't anything that a light client could ask to another light client at the moment.

The intention of this PR is to change that.

I can give a review of small things that could be tweaked, but I suppose you're not at this point yet.

I think it's ready for review, just waiting for ci to turn green.

tomaka · 2020-09-22T13:35:49Z

I gave a review in #6795

I'd prefer if possible to document in sc-network how bitswap works from a high-level perspective so that people can actually understand how to use it, rather than just throwing an API in the air and implicitly asking users to look at the code of libp2p-bitswap to understand what to do.

tomaka · 2020-09-28T09:17:00Z

Reposting relevant review comment: #6795 (comment)

gnunicorn · 2020-11-02T10:01:18Z

Closed b/c of staleness.

dvc94ch requested review from andresilva, mxinden and tomusdrw as code owners September 17, 2020 19:56

expenses reviewed Sep 18, 2020

View reviewed changes

dvc94ch mentioned this pull request Sep 18, 2020

storage keys paged and query storage at not implemented in light client #6884

Open

dvc94ch force-pushed the dvc-bitswap branch from 4396ee2 to a613f8f Compare September 21, 2020 10:29

tomusdrw removed their request for review September 21, 2020 11:56

dvc94ch force-pushed the dvc-bitswap branch 2 times, most recently from 629b2b3 to b1714fc Compare September 21, 2020 18:51

dvc94ch added 8 commits September 24, 2020 11:20

Add bitswap support.

2dda224

Remove BAD_ROLE.

1d6becb

Propagate generic.

3ca6480

Update Cargo.lock

25048f7

Make build_network a method on BuildNetworkParams.

1db72aa

Fix tests.

93d02fb

Fix line width.

6c04d78

Fix tests.

de30ac0

dvc94ch force-pushed the dvc-bitswap branch from b1714fc to de30ac0 Compare September 24, 2020 09:21

tomaka mentioned this pull request Sep 28, 2020

Implement bitswap in the network behaviour using libp2p_bitswap. #6795

Closed

gnunicorn added A0-please_review Pull request needs code review. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Oct 14, 2020

gnunicorn closed this Nov 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggestion for adding bitswap to substrate #7133

Suggestion for adding bitswap to substrate #7133

dvc94ch commented Sep 17, 2020 •

edited

Loading

expenses Sep 18, 2020

dvc94ch Sep 21, 2020

dvc94ch commented Sep 18, 2020

dvc94ch commented Sep 18, 2020

tomaka commented Sep 21, 2020 •

edited

Loading

dvc94ch commented Sep 21, 2020

tomaka commented Sep 22, 2020 •

edited

Loading

tomaka commented Sep 28, 2020

gnunicorn commented Nov 2, 2020

Suggestion for adding bitswap to substrate #7133

Suggestion for adding bitswap to substrate #7133

Conversation

dvc94ch commented Sep 17, 2020 • edited Loading

expenses Sep 18, 2020

Choose a reason for hiding this comment

dvc94ch Sep 21, 2020

Choose a reason for hiding this comment

dvc94ch commented Sep 18, 2020

dvc94ch commented Sep 18, 2020

tomaka commented Sep 21, 2020 • edited Loading

dvc94ch commented Sep 21, 2020

tomaka commented Sep 22, 2020 • edited Loading

tomaka commented Sep 28, 2020

gnunicorn commented Nov 2, 2020

dvc94ch commented Sep 17, 2020 •

edited

Loading

tomaka commented Sep 21, 2020 •

edited

Loading

tomaka commented Sep 22, 2020 •

edited

Loading