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

feat: KCP should reduce aggregrate bandwidth usage and increase performance #1918

Closed
wants to merge 174 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jan 1, 2024

This PR makes kcp the default network transport protocol for comet.

Note: please merge #1907 first because I have a strong preference for working on linted code.

This should

  • reduce bandwidth consumption
  • reduce the number of roundtrips
  • be suitable for high performance chains
  • resolve some of the p2p storms issues
  • allow comet packets to sail through censorship, like the great firewall

I need help with:

  • quantifying the improvements
  • ensuring total compatibility

note:

This is surely a breaking change, but while we are bandwidth constrained I think this is a very sensible approach, as it may reduce traffic needs by >50% because kcp is a reliable udp protocol.

https://github.com/xtaci/kcp-go/releases/tag/v5.6.5

To my knowledge, there aren't any downsides here. KCP has all of the features we know and love about tcp, but is inherently less chatty.


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@faddat
Copy link
Contributor Author

faddat commented Jan 16, 2024

Good news: I think I just missed a small address format change thing and that is why e2e fails. All other failures seem to have already existed and probably best to ignore them.

@faddat faddat marked this pull request as ready for review January 16, 2024 04:31
@faddat faddat requested review from a team as code owners January 16, 2024 04:31
@adizere
Copy link
Member

adizere commented Jan 16, 2024

Really appreciate the experiment here. Adopting KCP overnight is probably not very sensible towards users, so let's find a way to ship it responsibly.

This PR would deserve due-diligence. Comet team can do that, namely quantifying the improvements and ensuring total compatibility. Our capacity is full and we're all on-deck for v1. Since v1 has already a lot of changes slated (we want to get v1 out the door as soon as PBTS is ready, not pile up more stuff in it, as that would delay the release due to additional testing), this change will need to wait.

That being said, @faddat we have two options basically: KCP waits until v1 ships, and then Comet team can look into testing & compatibility concerns; or you start some testing and experiments yourself or with other teams.

Side note, this PR has many changes unrelated to KCP, but I assume those will go away once #1907 is done.

@adizere adizere added this to the 2024-Q2 milestone Jan 16, 2024
@faddat
Copy link
Contributor Author

faddat commented Jan 16, 2024

Oh definitely not overnight

I think that step one is to get it working well

For example, what I can see right now from the benchmarks? It's slower than TCP. But reality is, I believe that we can make it much faster than TCP and of course much more reliable than UDP making it a really good choice plus it sails over certain crazy walls, was once used to make a dog tunnel (plz decide metaphor by self)

@faddat
Copy link
Contributor Author

faddat commented Jan 16, 2024

But you are correct about the unrelated changes. I merged several PRS into this PR so that I could have a more stable branch than main

Admittedly I got mixed results

@faddat
Copy link
Contributor Author

faddat commented Jan 16, 2024

Well, so I'm going to throw out a third option which is like. First of all I make it work well. I don't think that the kcp Branch works well yet, and I think that really we need to make sure that the branch itself works well and then okay look. This is a little bit radical but we've been rolling that way and I think that's a good thing:

  • We need to get rid of all the other database. Back-ends, the conversions are actually easy
  • We swap out TCP for kcp
  • We write new benchmarks
  • informal or icf pays by percentage performance and stability improvement over time

Since like yourself I think that this is a multi-month process, you know I'm just going to kind of cook on it here. But please do understand that one of the things that I think is really important to accomplish on comet over the next few months is that we get rid of CGO all together. Please read the paper on CGO by cockroachdb.

Honestly, that paper is the single and solitary reason why I think gno just might have a chance

So if you go and you read about rocks, for example, if you read a lot, you're going to find that. It's probably the absolute fastest, but if you read a lot about using it in go, you're also going to find that anytime you cross that evil evil CGO boundary. You actually lose so much RAM and CPU that fast kind of doesn't matter anymore.

@faddat
Copy link
Contributor Author

faddat commented Jan 23, 2024

@adizere

v1 > kcp

eg: defintely, v1 is more important.

@faddat faddat changed the title KCP should reduce aggregrate bandwidth usage and increase performance feat: KCP should reduce aggregrate bandwidth usage and increase performance Jan 24, 2024
Copy link

github-actions bot commented Feb 4, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale For use by stalebot label Feb 4, 2024
@github-actions github-actions bot closed this Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale For use by stalebot
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants