Implement bitswap in the network behaviour using libp2p_bitswap. #6795

expenses · 2020-08-03T08:38:33Z

I believe this would be a good start to implementing #6075. As libp2p-bitswap still needs some work (see #6075 (comment)), this PR probably needs a lot of work in order to be merged. A few points:

* Ideally the Bitswap behaviour would be wrapped in a Toggle so that it's only activated when a chain requests it, but we can't do that until libp2p/rust-libp2p#1684 gets merged.
* I think we should be inserting the bitswap blocks into the DHT as well, so that nodes we're not directly connected to will be able to access them.
* As the idea is to allow the fetching of preimages by an IPFS client, I'm not sure if we should be messing about with IPLD as well, or if this is too low level for that.

polkadot companion: paritytech/polkadot#1550

client/network/src/service.rs

client/network/src/behaviour.rs

client/network/src/protocol/event.rs

dvc94ch · 2020-08-03T15:11:30Z

One problem is that ipfs data will leak, as there isn't a way of disposing of it. And since it's content addressed you'll need to ref count the blocks as the data is deduplicated.

To get "ipld" support you just need to implement the ipld store traits, you can easily do that, and then you get all the ipld codecs and ipld collections for free.

dvc94ch · 2020-08-04T21:52:45Z

I was planning on reimplementing bitswap as a light client notification protocol, but it doesn't seem there is a way to perform the get-providers dht query from the NetworkService. This would be an alternative that would also work for us. I'm going to make the entire network implementation of ipfs-embed replaceable so that I can drop in the light client. The db is already shared between ipfs-embed and the light-client.

@tomaka do you think exposing get-provider and start-providing in NetworkService could be a faster way forward?

tomaka · 2020-08-06T16:44:57Z

This PR doesn't do anything related to the DHT yet as far as I can tell, but one thing to be aware of is that we're customizing the name of the Kademlia protocol to include the id of the chain, as a way to prevent nodes from different chains from connecting to each other.

I suppose that it would fundamentally be better to also connected to a shared DHT, or to IPFS itself, rather than to only the DHT of a specific chain. However, this would be problematic as we are already now suffering from problems caused by the high number of TCP sockets.

a light client notification protocol

Request-response protocols, added in #6634, would be more appropriate I think. They are similar to RPC queries, whereas if you try to emulate request-reponses on top of notifications you will suffer from all sorts of issues.

@tomaka do you think exposing get-provider and start-providing in NetworkService could be a faster way forward?

That'd be totally reasonable.

dvc94ch

So I reviewed the code again. There are a couple of issues with the current implementation. The changes aren't atomic so if there is a crash half way through inserting a batch for example, the store will be in an inconsistent state. Also I noticed that the light client keeps the blocks in memory and they aren't persisted to disk.

Maybe substrate shouldn't implement the store, and instead just expose dht and bitswap operations via the Network thing.

expenses · 2020-09-18T12:53:33Z

@dvc94ch This is ready for a re-review (the button has stopped working for me 🤷‍♀️ )

dvc94ch · 2020-09-18T13:06:34Z

client/service/src/builder.rs

@@ -768,11 +768,40 @@ pub struct BuildNetworkParams<'a, TBl: BlockT, TExPool, TImpQu, TCl> {
 	pub finality_proof_request_builder: Option<BoxFinalityProofRequestBuilder<TBl>>,
 	/// An optional, shared finality proof request provider.
 	pub finality_proof_provider: Option<Arc<dyn FinalityProofProvider<TBl>>>,
+	/// The builder for an IPLD store.f


dvc94ch · 2020-09-18T13:07:45Z

client/service/src/builder.rs

+/// Something that builds an IPLD store. You generally want `()`, which always returns `None`.
+pub trait IpldStoreBuilder {
+	/// The type of store to potentially build.
+	type Store: libipld::store::Store<Params=libipld::DefaultStoreParams>+


Can this not be fixed to DefaultStoreParams?

dvc94ch · 2020-09-18T13:14:18Z

client/service/src/lib.rs

+) -> Result<(), libipld::error::Error> {
+	match event {
+		sc_network::BitswapEvent::ReceivedBlock(_, cid, data) => {
+			storage.insert(libipld::Block::new(cid.clone(), data.to_vec())?).await?;


ipfs-embed moved away from get making any changes to the stored blocks. in ipfs-embed this will actually not insert anything at all as the block isn't pinned. I see the need for recording the timestamps and having a mixture of pins, refcounts and timestamps determine if a block is live or not. That way the store can be used as a cache without leading to storage leakage.

dvc94ch · 2020-09-18T13:19:42Z

client/service/src/lib.rs

+			Ok(())
+		},
+		sc_network::BitswapEvent::ReceivedWant(peer_id, cid, _) => {
+			let block = storage.get(&cid).await?;


get will potentially try to fetch the block from the network. I think you only want to respond if you have the block locally. If the block isn't available locally or found on the network it will return a BlockNotFound. In general there is a problem that the semantics of go-ipfs, js-ipfs are not clear (or outright insufficient), and while formulating and implementing precise semantics for ipfs-embed in terms of atomicity/durability/garbage collection, they are still in flux as more experience is gained building applications on top. That's why I'd recommend completely removing any storage logic for now.

So should I just remove handle_bitswap_event and IpldStoreBuilder entirely?

Not sure how you intend to use it. While exchanging blocks with full nodes and validators could be interesting if light clients can pay them to store blocks for them, for now I'm mainly interested in using it for light client to light client data exchange. So I think removing those entirely would be fine for now.

dvc94ch

LGTM! Thanks.

gnunicorn · 2020-09-18T17:24:43Z

as this is mainly in the network, I think @tomaka should have something to say about this...

tomaka · 2020-09-22T09:00:27Z

client/network/Cargo.toml

+libp2p-bitswap = "0.7.0"
+tiny-cid = "0.2.5"


Alphabetical ordering, please!

tomaka · 2020-09-22T09:01:45Z

client/network/src/behaviour.rs

@@ -54,6 +54,8 @@ pub struct Behaviour<B: BlockT, H: ExHashT> {
 	peer_info: peer_info::PeerInfoBehaviour,
 	/// Discovers nodes of the network.
 	discovery: DiscoveryBehaviour,
+	/// Exchanges blocks of data with other nodes.
+	pub(crate) bitswap: libp2p_bitswap::Bitswap,


I'd prefer if you added methods to Behaviour rather than exposing implementation details.

tomaka · 2020-09-22T09:04:07Z

primitives/core/src/offchain/storage.rs

@@ -24,7 +24,7 @@ use std::iter::Iterator;
 /// In-memory storage for offchain workers.
 #[derive(Debug, Clone, Default)]
 pub struct InMemOffchainStorage {
-	storage: HashMap<Vec<u8>, Vec<u8>>,
+	pub(crate) storage: HashMap<Vec<u8>, Vec<u8>>,


This isn't my code, but similarly I don't think it's great software design.

tomaka · 2020-09-22T09:04:57Z

client/network/src/service.rs

+	/// Get the number of bitswap peers we are connected to.
+	pub fn bitswap_num_peers(&self) -> usize {
+		self.network_service.bitswap.peers().count()
+	}


What's a "bitswap peer" and what's a "non-bitswap peer"?

So these are open connections to other peers. It will send want requests to any peer that connects that speaks bitswap. For each open connection it maintains a list of blocks the other peer wants. If in the future the block is available it sends it to the peer. This architecture is because when I first wrote bitswap it seemed like you could have a strategy that gets a block from a friendly peer to get a block from a new peer. Now I think it should probably just ignore the want requests of blocks it doesn't have.

tomaka · 2020-09-22T09:05:40Z

client/network/src/protocol/event.rs

@@ -21,6 +21,7 @@ use bytes::Bytes;
 use libp2p::core::PeerId;
 use libp2p::kad::record::Key;
 use sp_runtime::ConsensusEngineId;
+use crate::behaviour::BitswapEvent;


Suggested change

use crate::behaviour::BitswapEvent;

pub use crate::behaviour::BitswapEvent;

tomaka · 2020-09-22T09:06:19Z

client/network/src/gossip.rs

-	) -> (Self, impl Future<Output = ()> + Send + 'static)
+	) -> (Self, impl Future<Output = ()> + 'static)


Is this change needed for this PR?
It's not a negative change, but please don't introduce changes that are unrelated to the PR.

Line 1928 of https://gitlab.parity.io/parity/substrate/-/jobs/658201#L1928 is why we need this.

tomaka · 2020-09-22T09:06:51Z

primitives/core/src/offchain/testing.rs

@@ -87,6 +87,11 @@ impl TestPersistentOffchainDB {
 			}
 		}
 	}
+
+	/// Get whether the DB is empty.
+	pub fn is_empty(&self) -> bool {


I don't really see how this is relevant for the rest of the PR.

tomaka · 2020-09-22T11:22:41Z

client/network/src/service.rs

+	/// Send a bitswap block to a peer.
+	pub fn bitswap_send_block(&self, peer_id: PeerId, cid: tiny_cid::Cid, data: Box<[u8]>) {
+		let _ = self
+			.to_worker
+			.unbounded_send(ServiceToWorkerMsg::BitswapSendBlock(peer_id, cid, data));
+	}
+
+	/// Send a bitswap block to all peers that have the block in their wantlist.
+	pub fn bitswap_send_block_all(&self, cid: tiny_cid::Cid, data: Box<[u8]>) {
+		let _ = self
+			.to_worker
+			.unbounded_send(ServiceToWorkerMsg::BitswapSendBlockAll(cid, data));
+	}
+
+	/// Send a bitswap WANT request to all peers for a block.
+	pub fn bitswap_want_block(&self, cid: tiny_cid::Cid, priority: i32) {
+		let _ = self
+			.to_worker
+			.unbounded_send(ServiceToWorkerMsg::BitswapWantBlock(cid, priority));
+	}
+
+	/// Cancel a bitswap WANT request.
+	pub fn bitswap_cancel_block(&self, cid: tiny_cid::Cid) {
+		let _ = self
+			.to_worker
+			.unbounded_send(ServiceToWorkerMsg::BitswapCancelBlock(cid));
+	}


This needs to be documented.
I don't know how bitswap works, and that's great because it should in my opinion be possible for someone who doesn't know how bitswap works to understand this API.

What's the general workflow of the thing? What happens if send_block is called on a peer that doesn't want this block?

bitswap_send_block is usually in response to a want event. the block is added to a queue and if a cancel request is received before the message is sent it will be removed from the queue. reading the code, if the cancel request comes in before the want request was processed, the cancel request will be ignored. I guess it could use some work, but it's not "terrible"

tomaka · 2020-09-22T11:23:41Z

client/network/src/behaviour.rs

+	/// A block was received.
+	ReceivedBlock(PeerId, tiny_cid::Cid, Box<[u8]>),


Can this be called multiple times with the same block? What's the relationship between the Cid and the data? Is it guaranteed to be in response to a WANT request?

Can this be called multiple times with the same block?

no, unless the events weren't polled until the block arrived and a new want request was submitted.

What's the relationship between the Cid and the data

the cid hash matches the data, the cid is the one submitted in the want request

Is it guaranteed to be in response to a WANT request?

it's guaranteed to be in response to a want request.

tomaka · 2020-09-22T15:22:48Z

client/network/src/behaviour.rs

+	///
+	/// There are no guarantees about the recieved block:
+	/// * The CID and block contents might not match.
+	/// * We might have receieved a block without having sent out a want request first.


That sounds like an attack vector.
@dvc94ch Can a node just connect and send, say, 2 GiB of data without us having asked for it, and we will simply buffer it?

This should probably be combined with the reputation system right?

no this can't happen https://github.com/ipfs-rust/libp2p-bitswap/blob/master/src/behaviour.rs#L216

@dvc94ch right, but that 2 GiB of data is still held in memory for a period of time, right?

Well that depends on the transport. A message is processed from the queue, and dropped if it wasn't requested. We don't have a sophisticated mechanism for disconnecting from peers, I thought that substrate handles bandwidth limiting, telemetry and reputation, and instead of handling this stuff myself in bitswap/ipfs-embed, I thought we can leverage that. I'm glad to redesign bitswap if there is some input on how it should work

A message is processed from the queue, and dropped if it wasn't requested. We don't have a sophisticated mechanism for disconnecting from peers, I thought that substrate handles bandwidth limiting, telemetry and reputation, and instead of handling this stuff myself in bitswap/ipfs-embed, I thought we can leverage that. I'm glad to redesign bitswap if there is some input on how it should work

I'm not familiar with the bitswap protocol or the internals of bitswap, so I'm just naively asking.
The problem I have in mind is: someone can just connect, open 1000 substreams (or whatever the maximum is), negotiate bitswap, send a length prefix of N, and then send N - 1 bytes but never the last byte. Do this multiple times simultaneously and you can probably make the node run out of memory.
This is entirely specific to the bitswap protocol, and not something Substrate can handle.

What we do for other protocols is enforce a limit of one substream per protocol per connection, and make sure that the remote can never send a large amount of data without having explicitly requested it first.

What we do for other protocols is enforce a limit of one substream per protocol per connection

ipfs-rust/libp2p-bitswap#7, can you point me to a specific protocol that handles this, so I can look how it's done?

make sure that the remote can never send a large amount of data without having explicitly requested it first.

I think this is already handled. Does this satisfy the requirement? https://github.com/ipfs-rust/libp2p-bitswap/blob/master/src/behaviour.rs#L216 Or is more work needed?

can you point me to a specific protocol that handles this, so I can look how it's done?

I'm not familiar with this code unfortunately, but libp2p's request-response should do that

I think this is already handled. Does this satisfy the requirement? https://github.com/ipfs-rust/libp2p-bitswap/blob/master/src/behaviour.rs#L216 Or is more work needed?

As far as I understand, you're still entirely buffering the message, and then only discard it, so it really depends on the maximum size of the message. If it's 512 kiB it's ok, if it's 2 GiB it's not.

I think these issues are handled much better now ipfs-rust/libp2p-bitswap#8.

@tomaka how concerned are you with interopability with other implementations? In particular now we avoid packing multiple requests responses in the same message, and reuse the substream to send multiple messages. Also even if we add that supporting bitswap 1.1.0 and 1.0.0 is not something I'm particularly interested in working on, and the api is really focused around bitswap 1.2.0 which works completely different from previous versions.

how concerned are you with interopability with other implementations?

If this feature is used specifically for large files storage using Substrate, I guess we don't really care.

dvc94ch · 2020-09-23T15:11:08Z

So where do we go from here? I guess I could pr libp2p-bitswap to rust-libp2p, then we could review that first before moving on to the substrate specifics?

dvc94ch · 2020-09-26T08:57:58Z

@expenses it seems the docs are still incorrect. If you're busy with other stuff, feel free to close the pr and I'll merge the changes that make sense into mine.

expenses · 2020-09-28T07:05:42Z

@dvc94ch ok, that makes sense 👍

tomaka · 2020-09-28T09:15:59Z

Link for note keeping: #7133

expenses added 4 commits July 30, 2020 16:50

WIP, good start

2b67bbb

Add networking tests

d5d3130

Merge remote-tracking branch 'origin/master' into ashley-bitswap

5bd7cb7

Reindent bitswap tests using tabs

6fa276d

expenses added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Aug 3, 2020

expenses requested a review from tomaka August 3, 2020 08:38

expenses added 2 commits August 3, 2020 10:38

Merge remote-tracking branch 'origin/master' into ashley-bitswap

926575f

Use OffchainStorage instead of AuxStore

471b171

tomaka previously requested changes Aug 3, 2020

View reviewed changes

Apply suggestions

83bc9c6

expenses requested a review from tomaka August 3, 2020 11:58

expenses added 2 commits August 3, 2020 15:06

Convert spaces to tabs

be7baf5

Merge remote-tracking branch 'origin/master' into ashley-bitswap

6147c6a

Merge remote-tracking branch 'origin/master' into ashley-bitswap

f90a39c

expenses added 4 commits August 5, 2020 13:59

Implement IPLD storage traits

2496778

Merge remote-tracking branch 'origin/master' into ashley-bitswap

3244af9

Documentation

8d028d4

Merge remote-tracking branch 'origin/master' into ashley-bitswap

ae5fd43

expenses marked this pull request as ready for review August 6, 2020 09:12

github-actions bot added the A7-needspolkadotpr label Aug 6, 2020

expenses added 3 commits August 6, 2020 12:14

Fix line widths

ccb5910

Fix browser build

3da54fa

Merge remote-tracking branch 'origin/master' into ashley-bitswap

6f3d206

This was referenced Aug 6, 2020

Companion PR for Implement bitswap in the network behaviour using libp2p_bitswap paritytech/polkadot#1550

Closed

Light Client: Implement checkpoints in chain_spec to speed up syncing #6804

Open

Merge remote-tracking branch 'origin/master' into ashley-bitswap

f80e0ad

dvc94ch reviewed Sep 12, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into ashley-bitswap

4b954d2

dvc94ch mentioned this pull request Sep 17, 2020

Suggestion for adding bitswap to substrate #7133

Closed

expenses added 3 commits September 18, 2020 12:18

Remove storage impl and update

83de229

Merge remote-tracking branch 'origin/master' into ashley-bitswap

a8a0f8b

Fix wasm build

1f9c171

expenses requested a review from seunlanlege September 18, 2020 12:46

dvc94ch reviewed Sep 18, 2020

View reviewed changes

expenses added 2 commits September 18, 2020 15:51

Remove stuff

9923b11

Remove bitswap network tests

30fd353

dvc94ch approved these changes Sep 18, 2020

View reviewed changes

expenses added 3 commits September 21, 2020 12:41

Merge remote-tracking branch 'origin/master' into ashley-bitswap

38f4667

Merge remote-tracking branch 'origin/master' into ashley-bitswap

8187024

Merge remote-tracking branch 'origin/master' into ashley-bitswap

5de7605

github-actions bot removed the A7-needspolkadotpr label Sep 22, 2020

tomaka suggested changes Sep 22, 2020

View reviewed changes

expenses added 2 commits September 22, 2020 15:24

Remove changes that slipped in accidentally

da1c738

Improve documentation somewhat

b8f0981

expenses requested a review from tomaka September 22, 2020 14:11

github-actions bot added the A7-needspolkadotpr label Sep 22, 2020

tomaka reviewed Sep 22, 2020

View reviewed changes

expenses closed this Sep 28, 2020

expenses deleted the ashley-bitswap branch December 10, 2020 14:34

	use crate::behaviour::BitswapEvent;

	pub use crate::behaviour::BitswapEvent;

		) -> (Self, impl Future<Output = ()> + Send + 'static)
		) -> (Self, impl Future<Output = ()> + 'static)

		/// A block was received.
		ReceivedBlock(PeerId, tiny_cid::Cid, Box<[u8]>),

Implement bitswap in the network behaviour using libp2p_bitswap. #6795

Implement bitswap in the network behaviour using libp2p_bitswap. #6795

Conversation

expenses commented Aug 3, 2020 • edited Loading

dvc94ch commented Aug 3, 2020

dvc94ch commented Aug 4, 2020

tomaka commented Aug 6, 2020

dvc94ch left a comment

Choose a reason for hiding this comment

expenses commented Sep 18, 2020

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

dvc94ch left a comment

Choose a reason for hiding this comment

gnunicorn commented Sep 18, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvc94ch Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvc94ch Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvc94ch Sep 28, 2020 • edited Loading

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

dvc94ch commented Sep 23, 2020

dvc94ch commented Sep 26, 2020

expenses commented Sep 28, 2020

tomaka commented Sep 28, 2020

expenses commented Aug 3, 2020 •

edited

Loading

dvc94ch Sep 22, 2020 •

edited

Loading

dvc94ch Sep 23, 2020 •

edited

Loading

dvc94ch Sep 28, 2020 •

edited

Loading