search

ipfs / go-bitswap

The golang implementation of the bitswap protocol
MIT License
216 stars 112 forks source link

Sending message > 4mb fails over libp2p #588

Closed dirkmc closed 2 years ago

dirkmc commented 2 years ago

This PR adds a test to demonstrate that when a message is > 4mb in size, it fails to send over libp2p

The bitswap engine should have logic to avoid popping so much off the request queue that it exceeds the maximum message size.

Stebalien commented 2 years ago

It should... We estimate task sizes when we push work onto the queue, then pop at least 16KiB of work. We should only pass 4MiB if we have blocks well over 4MiB.

See https://github.com/ipfs/go-bitswap/blob/13c89f1ba6c65253a3dfc3b2bafcb1730b320b66/server/internal/decision/engine.go#L507

Unless the blockstore is lying about the block sizes? Or maybe we're merging messages somewhere?

(also note: libp2p isn't imposing the restriction, bitswap is when it reads messages to avoid a DoS vector)

Stebalien commented 2 years ago

I think there's something else at play here. Bitswap would be completely broken if this were actually the case.

dirkmc commented 2 years ago

The reason that the message is >4mb in the test I wrote is because I've set the target message size to be 32mb: https://github.com/ipfs/go-bitswap/pull/588/files#diff-d8b0c0072a3d07dab01dc7694e0f227fb90598b3f268b7964e367dd9a3407ed5R59

We uncovered this issue when doing some testing and trying to maximize bitswap throughput for a particular point-to-point use case. As an experiment we increased the target message size.

I believe this is a libp2p issue because when I run the same test with a mocknet (instead of a libp2p network) the test passes. I've added a test to demonstrate: https://github.com/ipfs/go-bitswap/pull/588/files#diff-d8b0c0072a3d07dab01dc7694e0f227fb90598b3f268b7964e367dd9a3407ed5R30

The test that fails is TestTwoLibp2pPeers/4 blocks. It fails because the server fails to send the message:

2022-10-03T10:42:15.563+0200    DEBUG   bitswap-server  server/server.go:339    failed to send blocks message   {"peer": "12D3KooWGAgyXMPMQESUejojggbRt9QKDhB1ff1r7XL9kPA1xy2U", "error": "stream reset"}
Stebalien commented 2 years ago

We set the max message size here: https://github.com/ipfs/go-bitswap/blob/13c89f1ba6c65253a3dfc3b2bafcb1730b320b66/network/ipfs_impl.go#L411

The test network you're using simply bypasses that logic.

Note: please don't try to optimize this by increasing message sizes. Instead, reuse the send stream to send multiple messages.

dirkmc commented 2 years ago

I agree, it makes more sense to send multiple messages rather than sending large messages.

The intent of this PR is to demonstrate that it's possible to configure bitswap in a way that causes it to behave unexpectedly (fails to send a message, and the only indication that something went wrong is a log at the debug level).

I'd suggest that if someone passes config parameters to the constructor in a way that is guaranteed not to work, we should return an error.

Stebalien commented 2 years ago

Yeah, that seems reasonable: https://github.com/ipfs/go-libipfs/issues/69