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 problems over websocket transport since switch to nhooyr websocket #2188

Closed
hannahhoward opened this issue Mar 16, 2023 · 5 comments · Fixed by #2193
Closed

Bitswap problems over websocket transport since switch to nhooyr websocket #2188

hannahhoward opened this issue Mar 16, 2023 · 5 comments · Fixed by #2193
Assignees
Labels
kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP

Comments

@hannahhoward
Copy link
Contributor

hannahhoward commented Mar 16, 2023

Bug: attempting to fetch UnixFS files over bitswap with peers that only expose websockets will fail on large blocks. Note: this is all of Elastic IPFS/Web3.storage

Steps To Reproduce:

Build Kubo v0.18.0:

Start the daemon:

./cmd/ipfs/ipfs daemon
./cmd/ipfs/ipfs get bafybeiarntxetfbzpyp7baj7kqvqljrn5uyoobq2y4b3zegbbem5joazgy 

This is a CID I stored on web3.storage. It works.

Now switch to Kubo master and do the same. It now hangs.

Heavy debugging identifies the following error generated in the websocket conn: failed to read: read limited at 32769 bytes -- this comes from here I believe: https://github.com/nhooyr/websocket/blob/master/read.go#L385

Note that actually everything works fine until we get to the first 1MB raw block, I assume because it's over that limit.

This was originally discovered in Lassie and in debugging why it couldn't fetch while Kubo 0.18.0 could, I found that Kubo master couldn't either.

@p-shahi p-shahi added the kind/bug A bug in existing code (including security flaws) label Mar 16, 2023
@p-shahi
Copy link
Member

p-shahi commented Mar 16, 2023

flagging this for @MarcoPolo

Without knowing much:
It looks like it has a default connection read limit set to 32768 bytes + one more byte in case of a fin frame:
https://github.com/nhooyr/websocket/blob/master/read.go#L69
Can we change the SetReadLimit?

There also seems to be this issue that a user ran into yesterday with the limit: coder/websocket#382 which shows an option to disable the read limit (but no ETA for a release)

Meta information:
introduced in #1982

@p-shahi p-shahi added the P0 Critical: Tackled by core team ASAP label Mar 16, 2023
@MarcoPolo MarcoPolo self-assigned this Mar 16, 2023
@BigLep
Copy link
Contributor

BigLep commented Mar 16, 2023

Thanks for finding and reporting @hannahhoward .

Thanks for jumping onto it @MarcoPolo .

Please make sure we get a regression test for this added. Ideally we could apply such a test to all transports.

@BigLep
Copy link
Contributor

BigLep commented Mar 16, 2023

Thanks @MarcoPolo for getting the PR out. What timeline are you thinking here for merging and getting it released? I'm asking since it's important for the Kubo 0.19 release (prevent regression) and impacting Rhea/Lassie work. Assuming @marten-seemann is out, who else would you like to pull in?

@MarcoPolo
Copy link
Collaborator

Thanks for the review @hannahhoward. If @marten-seemann doesn't get to it by eod, I'll merge and cut a patch release for Kubo with this.

@p-shahi
Copy link
Member

p-shahi commented Mar 17, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants