feat(shwap/bitswap): Shwap optimized Bitswap constructors #3536
Conversation
Wondertan
requested review from
renaynay,
vgonkivs,
distractedm1nd,
walldiss and
ramin
as code owners
Wondertan
force-pushed
the
shwap-bitswap-construct
branch
from
37c89c6
to
8d11121
Compare
Wondertan
force-pushed
the
shwap-bitswap-construct
branch
from
8d11121
to
f1e8d6a
Compare
| // TODO(@Wondertan): PR to bitswap to make this timeout configurable | ||
| // Higher timeout increases the probability of successful reconstruction |
There was a problem hiding this comment.
I think this todo should not be part of code base. Lets park it into issue instead
| // maxServerWantListsPerPeer defines the limit for maximum possible cached wants/requests per peer | ||
| // in the Bitswap. Exceeding this limit will cause Bitswap server to drop requested wants leaving | ||
| // client stuck for sometime. | ||
| // Thus, we make the limit a bit generous, so we minimize the chances of this happening. | ||
| // This is relevant until https://github.com/ipfs/boxo/pull/629#discussion_r1653362485 is fixed. | ||
| maxServerWantListsPerPeer = 8096 |
There was a problem hiding this comment.
Do you think It will affect the max size of blocks we can reconstruct? WIf yes, would be great to specify max eds size we can support with this value in use.
There was a problem hiding this comment.
If the limit is reached, reconstruction will slow down for N*30 secs, but it should still succeed. The N represents how many times all the samples didn't fit into the want list.
There was a problem hiding this comment.
Where 30sec comes from? If the peer sends 16k Iwants to the server, looks like the server will just drop first 8k of requests. which means it might never reply to those iwant requests. For example, 8096 will not fit 128*128 samples, which might be a potential limitation point for that setting value
There was a problem hiding this comment.
30 seconds comes from WANT resend timeout in Bitswap MessageeQueue. Client side resends the WANTs every 30 secs basically.
| if network == "" { | ||
| return "" | ||
| } | ||
| return protocol.ID(fmt.Sprintf("/%s/shwap", network)) |
There was a problem hiding this comment.
Maybe we should add versioning in case protocol breaking changes?
There was a problem hiding this comment.
Breaking changes on the ID/Container level would be reflected in CID's codec bump, so we don't have to worry about it here.
There was a problem hiding this comment.
Another question I have about protocol ID is the idea we discussed to separate bitswap networks into samples and reconstruction related. Any plans to make constructors aware of network type in this PR? it seems like a PR those changes would fit well
There was a problem hiding this comment.
I thought of including that in this PR, but decided against that as those constructors should land next to reconstruction retrieval.
This PR abstracts away Bitswap parameterization and provides simple constructors optimally configured for Shwap protocol composition.