Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protocols/bitswap: Add BitSwap implementation #2632

Closed
SionoiS opened this issue May 4, 2022 · 15 comments
Closed

protocols/bitswap: Add BitSwap implementation #2632

SionoiS opened this issue May 4, 2022 · 15 comments

Comments

@SionoiS
Copy link

SionoiS commented May 4, 2022

Hi,

Just wanted to get the ball rolling on this. As discussed in Discord.

Some links from a quick search.
https://github.com/rs-ipfs/rust-ipfs/tree/master/bitswap
https://github.com/ipfs-rust/libp2p-bitswap
https://github.com/ChainSafe/libp2p-bitswap/
https://docs.substrate.io/rustdocs/latest/src/sc_network/bitswap.rs.html
https://github.com/n0-computer/iroh/tree/main/iroh-bitswap/src
Drop your own below!

My experience with Bitswap is limited. I had to slightly modify rust-ipfs impl. of it for my Testground uses.

Here's some points I find important;

  1. Allow any block providing strategy.
  2. Provide all possible events.
  3. Stats tracking.

rust-ipfs impl. did allow for different strategy but was missing a block sent complete event I needed.

Is there official spec written somewhere?
Do we start from scratch or modify one of the existing implementations?
Could we not implement GraphSync instead?

Thanks.

@mxinden
Copy link
Member

mxinden commented May 6, 2022

Is there official spec written somewhere?

Spec: https://github.com/ipfs/specs/blob/master/BITSWAP.md ipfs/specs#269 and ipfs/specs#270

Do we start from scratch or modify one of the existing implementations?

I would have to review the existing implementations before making an informed decision here.

Could we not implement GraphSync instead?

If I am not mistaken that would force IPLD as a dependency onto libp2p. Not a blocker, though something to keep in mind. Also, in case I am not mistaken, Bitswap is significantly simpler and thus a good first protocol for exchanging bytes in libp2p.

//CC @aschmahmann as the Bitswap expert.

@aschmahmann
Copy link

Is there official spec written somewhere?

269 is the best to use at the moment (270 is for a new version of the Bitswap protocol). Sorry it's not merged into master yet 😬.

Could we not implement GraphSync instead?

Basically I agree with Max here. Bitswap is going to be a simpler protocol to build out, and while many Bitswap consumers end up using IPLD on top of it to create and fetch larger groups of content it's not required as with GraphSync. No reason both couldn't be included down the line though.

Some motivating reasons to implement Bitswap first:

  • It's simpler to implement and has fewer dependencies (making it easier to be used cross-language and implementation)
  • It currently has wider adoption which is both an indicator of demand and the systems you'll become interoperable with. There are at least 5 implementations of Bitswap in Rust alone (another one that's being developed is in https://github.com/n0-computer/iroh/tree/main/iroh-bitswap), and then there's other languages like Go and JS with the applications build on top of them
  • It's simplicity makes it easier to build strategies to fetch data from multiple peers

Given the 5 Rust implementations of Bitswap you may want to decide what it is that rust-libp2p wants out of a Bitswap implementation? Is it to unify the existing implementation efforts, to have a different opinion on extensibility or how the client and server implementations prioritize who to fetch data from and serve it to, etc?

As I mentioned on Discord I suspect it'd be quite useful to have a single codebase that handles the basic mechanics of the protocol for you (e.g. processing the protobuf messages) and then people may have different thoughts on how to orchestrate things like sessions or prioritization. However, maybe perhaps the right abstraction might let people share more code here.

Within the go-libp2p-kad-dht code I did a refactor extracting out the protocol message pieces that's been helpful in letting people play around with new client implementations (https://github.com/libp2p/go-libp2p-kad-dht/blob/f0569715b6e50119f425f708ab6673b536725139/pb/protocol_messenger.go#L34).

@SionoiS
Copy link
Author

SionoiS commented May 6, 2022

I read the specs.

We are looking at supporting all versions of bitswap, not just v1.3.0 right?

Also, is priority explained anywhere?

Of the 5 it seams that rs-ip/rust-ipfs impl. is the most generic. Although I don't think having list of wants and blocks is appropriate.
Tracking who want what and what to send where should be left for the user IMO.

@carsonfarmer
Copy link
Contributor

carsonfarmer commented May 6, 2022

Oh cool to see this discussion kicked off from the Discord chat! I think from my perspective, "a single codebase for handling the basic mechanics of the protocol for you" is exactly what we would want out of having this protocol. An abstraction that allows a custom client to easily decide what to send to whom in a way similar how the kad protocol exposes this for responding to provider queries would be quite nice. I think the iroh implementation from @dignifiedquire is already leaning in this direction.

@mxinden
Copy link
Member

mxinden commented Jun 3, 2022

@SionoiS you raised in the last community call that you would be interested in working on this, supported through a grant 🎉. What would be the timeline on your end? When would you have capacity to work on this?

@SionoiS
Copy link
Author

SionoiS commented Jun 3, 2022

@mxinden As soon as my current grant work is done. I'm already ahead of schedule but I'm not sure how long building the website is going to take. The end of this month at the latest I will start and I planned 4 months. So I guess ~ November.

OR if it's ok with PL I could work on both at the same time but it's going to take longer.

In the Mean time, I would be cool to gather what ppl want to see and don't want for Bitswap.
If anyone has thoughts please share!

@mxinden
Copy link
Member

mxinden commented Jun 8, 2022

OR if it's ok with PL I could work on both at the same time but it's going to take longer.

I would prefer doing the two in sequence instead of doing them in parallel. Also gives me more time to figure out the administrative side of things.

@SionoiS
Copy link
Author

SionoiS commented Nov 2, 2022

Ready to start working on this! Looking at the diff between iroh and rust-ipfs implementation of bitswap. where do we want the libp2p implementation, on a rust-ipfs <-> iroh spectrum.

Seams like people don't like bitswap and are building new things. If thing are changing might be good to keep the scope of this project small.

  • impl. of ConnectionHandler allow managing resources, iroh is a good example. rust-ipfs use OneShotHandler which is already part of libp2p. May end up with a custom impl but I'll try without first.
  • impl. of In/OutboundUpgrade & NetworkBehavious.
  • Testground setup for interop testing
  • No stat tracking
  • Support only v1.1 of bitswap
  • docs
  • example

That's the plan for now let me know what you think.

@thomaseizinger
Copy link
Contributor

My 2c are:

We definitely want a custom ConnectionHandler. Those run on a separate task which means we don't as easily block the main event loop of network behaviours.

@SionoiS
Copy link
Author

SionoiS commented Nov 3, 2022

We definitely want a custom ConnectionHandler. Those run on a separate task which means we don't as easily block the main event loop of network behaviours.

So to get decent perf. I need to impl one, ok make sense!

Is perf the only reason or is there things you can't do without an ConnectionHandler impl?

@SionoiS
Copy link
Author

SionoiS commented Nov 9, 2022

Ok I'll recap the meeting let me know if I got anything wrong.

Bitswap is a flawed protocol, no amount of engineering can fix it. A working group has been assembled to build a new protocol.

A test plan could be made for go-bitswap <-> rust-bitswap but what would that accomplish?

A naive spec-compliant rust-bitswap would not interop well with go-bitswap because of it's idiosyncrasies.

A rust-libp2p bitswap implementation that is compatible with go-bitswap would require high maintenance.

None of the options seams adequate...

@mxinden @b5

notes: https://www.notion.so/Rust-Bitswap-Implementation-s-60e4114cdac243ba9a78875a65511134

@b5
Copy link

b5 commented Nov 9, 2022

Bitswap is a flawed protocol, no amount of engineering can fix it. A working group has been assembled to build a new protocol.

dang, when you spell it out like that, sounds rough 😅 . I would add the context that this is one team's view after having written an implementation, but I'm on that team, and do hold this view.

A test plan could be made for go-bitswap <-> rust-bitswap but what would that accomplish?

it would accomplish at least one thing: It would test our team's assumptions & give a repeatable-ish context for analyzing failures.

A naive spec-compliant rust-bitswap would not interop well with go-bitswap because of it's idiosyncrasies.

fully agreed.

A rust-libp2p bitswap implementation that is compatible with go-bitswap would require high maintenance.

fully agreed.

I think the best thing for our community to do is move on, write a new data transfer protocol that has multiple implementations & a proper spec from the start. If folks need go-bitswap interoperability, I'd encourage them to use & contribute to iroh's implementation. We need to maintain this in some capacity for the foreseeable future.

@dvc94ch

This comment was marked as off-topic.

@dvc94ch

This comment was marked as off-topic.

@dvc94ch

This comment was marked as off-topic.

mxinden added a commit to mxinden/rust-libp2p that referenced this issue Jan 20, 2023
We will not implement Bitswap anytime soon. See
libp2p#2632 (comment) for rational.
mergify bot pushed a commit that referenced this issue Jan 22, 2023
We will not implement Bitswap anytime soon. See
#2632 (comment) for rational.
@dariusc93 dariusc93 closed this as not planned Won't fix, can't repro, duplicate, stale Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants