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

Bitswap 1.2.0 #269

Merged
merged 8 commits into from
Aug 26, 2022
Merged

Bitswap 1.2.0 #269

merged 8 commits into from
Aug 26, 2022

Conversation

aschmahmann
Copy link
Contributor

The current Bitswap spec is a bit unwieldly and out of date and generally seemed in need of some cleaning up. The spec here was a rewrite but some notable changes include:

  • Added /ipfs/bitswap/1.2.0
  • Clarified the protocol identifiers used to identify the protocol versions
  • Clarified the specified protocol message format per-protocol version
  • Specified the maximum message size (4MiB)
  • Specified the minimum compliant maximum block size (2MiB)
  • Specified that messages are varint-delimited protobufs

@aschmahmann aschmahmann force-pushed the feat/bitswap-spec-improvement branch 4 times, most recently from 5c7acf9 to 235ff8e Compare March 4, 2022 02:20

There are two primary flows that Bitswap manages: requesting blocks from and serving blocks to peers. Block requests are primarily mediated by the want-manager, which tells our peers whenever we want a new block. Serving blocks is primarily handled by the decision engine, which decides how resources should be allocated among our peers.
- `/ipfs/bitswap/1.0.0` - Initial version
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also seen /ipfs/bitswap and /bitswap/1.0.0 around digging through the GitHub history. I'm not sure if these are the same as /ipfs/bitswap/1.0.0 or different earlier versions.

Perhaps someone with sufficient history can clarify, or I can do more GitHub archaeology to learn more.

BITSWAP.md Outdated Show resolved Hide resolved

Sender:
Wantlist wantlist = 1;
repeated Block payload = 3;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it appears to be supported in go-bitswap I removed repeated bytes blocks = 2; from versions of the protocol >1.0.0 as that field is deprecated and superseded by another field (i.e. payload). Given that we use libp2p and multistream to negotiate protocol versions there seems to be no reason why a /ipfs/bitswap/1.1.0 node should ever receive a message with the blocks field since a node needing the blocks field should have negotiated for /ipfs/bitswap/1.0.0.

However, given that this behavior is likely implemented across the various implementations today I'm happy to put it back in the spec if people think it should be there.

bool full = 2; // whether this is the full wantlist. default to false
}
message Block {
bytes prefix = 1; // CID prefix (all of the CID components except for the digest of the multihash)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous comment here seemed to indicate that this had to be a CIDv1 when it could be a CIDv0 as well.

Comment on lines +3 to +4
**Author(s)**:
- Adin Schmahmann
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just guessed this was alphabetical order by first name 🤷.

- David Dias
- Jeromy Johnson
- Juan Benet

**Maintainers(s)**:
**Maintainer(s)**:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this is supposed to be used for/if it really makes sense to keep around. Perhaps this is an interest group that I should be adding more people in the IPFS community who are interested in the Bitswap specs and improving them or being particularly involved in protocol upgrade proposals.

BITSWAP.md Outdated Show resolved Hide resolved
BITSWAP.md Outdated Show resolved Hide resolved
BITSWAP.md Outdated Show resolved Hide resolved
BITSWAP.md Show resolved Hide resolved
BITSWAP.md Outdated Show resolved Hide resolved
BITSWAP.md Show resolved Hide resolved
BITSWAP.md Outdated Show resolved Hide resolved
@aschmahmann aschmahmann marked this pull request as ready for review March 31, 2022 15:55
BITSWAP.md Outdated Show resolved Hide resolved
alanshaw pushed a commit to nftstorage/nft.storage that referenced this pull request Apr 28, 2022
Bitswap spec is being updated to clarify the 2MiB limit: ipfs/specs#269

go-ipfs chunker accepts a max size of 1MiB but protobuf wrapping can make it a bit bigger. Switching to 2MiB allows us to support blocks created using that max chunker size.
alanshaw pushed a commit to nftstorage/nft.storage that referenced this pull request Apr 29, 2022
Bitswap spec is being updated to clarify the 2MiB limit: ipfs/specs#269

go-ipfs chunker accepts a max size of 1MiB but protobuf wrapping can make it a bit bigger. Switching to 2MiB allows us to support blocks created using that max chunker size.

refs web3-storage/web3.storage#1269
BITSWAP.md Outdated Show resolved Hide resolved
@lidel lidel changed the title Update the Bitswap spec Bitswap 1.2.0 Aug 5, 2022
@BigLep
Copy link
Contributor

BigLep commented Aug 24, 2022

@aschmahmann : can we merge this? This seems like a quick thing to close out before you head out.

@aschmahmann
Copy link
Contributor Author

@BigLep yep, will get this cleaned up and ready for merge today

BITSWAP.md Show resolved Hide resolved
BITSWAP.md Outdated Show resolved Hide resolved
@aschmahmann aschmahmann merged commit c889e0b into main Aug 26, 2022
@aschmahmann aschmahmann deleted the feat/bitswap-spec-improvement branch August 26, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants