This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Implement bitswap in the network behaviour using libp2p_bitswap. #6795
Closed
Closed
Changes from all commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
2b67bbb
WIP, good start
expenses d5d3130
Add networking tests
expenses 5bd7cb7
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses 6fa276d
Reindent bitswap tests using tabs
expenses 926575f
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses 471b171
Use OffchainStorage instead of AuxStore
expenses 83bc9c6
Apply suggestions
expenses be7baf5
Convert spaces to tabs
expenses 6147c6a
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses f90a39c
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses 2496778
Implement IPLD storage traits
expenses 3244af9
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses 8d028d4
Documentation
expenses ae5fd43
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses ccb5910
Fix line widths
expenses 3da54fa
Fix browser build
expenses 6f3d206
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses e7ffe2a
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses 8e4c008
Insert bitswap blocks into the dht, rework to put bitswap methods on …
expenses 435fa52
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses d61bddc
Merge remote-tracking branch 'origin/master' into HEAD
expenses b8804df
Fix wasm build, but via patch
expenses f80e0ad
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses 4b954d2
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses 83de229
Remove storage impl and update
expenses a8a0f8b
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses 1f9c171
Fix wasm build
expenses 9923b11
Remove stuff
expenses 30fd353
Remove bitswap network tests
expenses 38f4667
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses 8187024
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses 5de7605
Merge remote-tracking branch 'origin/master' into ashley-bitswap
expenses da1c738
Remove changes that slipped in accidentally
expenses b8f0981
Improve documentation somewhat
expenses File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be combined with the reputation system right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no this can't happen https://github.com/ipfs-rust/libp2p-bitswap/blob/master/src/behaviour.rs#L216
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dvc94ch right, but that 2 GiB of data is still held in memory for a period of time, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 sendN - 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ipfs-rust/libp2p-bitswap#7, can you point me to a specific protocol that handles this, so I can look how it's done?
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with this code unfortunately, but libp2p's request-response should do that
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this feature is used specifically for large files storage using Substrate, I guess we don't really care.