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

Yamux is broken #334

Closed
wants to merge 7 commits into from
Closed

Yamux is broken #334

wants to merge 7 commits into from

Conversation

ianopolous
Copy link
Contributor

@ianopolous ianopolous commented Sep 28, 2023

@Nashatyrev There are 2 things in this PR

  1. Don't try and dial DNSADDR addresses in TCP transport
    These should mainly be used for bootstrap nodes with a DNS TXT record lookup to expand to list of nodes (which we've implemented downstream for kademlia). This removes a lot of log spam from failed dial attempts in Kademlia.

  2. We tried updating downstream to include the recent yamux refactoring but it broke sending large blobs. We have a downstream unit test demonstrating it, but it relies on the bitswap protocol. Here I've implemented a new Blob protocol just for testing which sends a length encoded byte array and echos it back. This works for sizes < 1MiB-4, but hangs and times out for larger sizes.

These should mainly be used for bootstrap with a DNS TXT
record lookup to expand to list of nodes

This removes a lot of log spam from failed dial attempts
…h yamux

This is trying to emulate a downstream test using bitswap,
which currently fails with 1MiB blocks.
@ianopolous ianopolous changed the title Don't try and dial DNSADDR addresses in TCP transport Yamux is broken Oct 4, 2023
@Nashatyrev
Copy link
Collaborator

Yes, thanks for the unit test 👍
I have added it to the refactored Yamux branch and it passes here: #328
You may want to address DNSADDR separately, but it just make sense to wait for #335 merged to avoid that mess with the gradlew.bat

@ianopolous
Copy link
Contributor Author

ianopolous commented Oct 4, 2023

@Nashatyrev Excellent. Any idea what might have broken/fixed it?

I will wait, and redo the DNSADDR in a new PR later.

@Nashatyrev Nashatyrev mentioned this pull request Oct 9, 2023
@Nashatyrev
Copy link
Collaborator

Added largeBlolb test as a separate PR here #337

@Nashatyrev
Copy link
Collaborator

I will wait, and redo the DNSADDR in a new PR later.

Then I'm closing this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants