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

network: Update libp2p to 0.54.1 #5996

Open
nazar-pc opened this issue Oct 9, 2024 · 10 comments · May be fixed by #6248
Open

network: Update libp2p to 0.54.1 #5996

nazar-pc opened this issue Oct 9, 2024 · 10 comments · May be fixed by #6248
Assignees

Comments

@nazar-pc
Copy link
Contributor

nazar-pc commented Oct 9, 2024

Substrate currently uses libp2p 0.52.4 with lots of outdated dependencies, libp2p 0.53.0 was released 11 months ago and 0.54.0 was released 2 months ago in time for September's release cut.

This is way too slow, please consider upgrading libp2p to not lag behind for a year or more, this is bad for downstream users due to lots of duplicated and outdated dependencies, can also be problematic from security standpoint.

@bkchr
Copy link
Member

bkchr commented Oct 9, 2024

CC @paritytech/networking

@lexnv lexnv changed the title Update libp2p to 0.54.1 network: Update libp2p to 0.54.1 Oct 10, 2024
@lexnv
Copy link
Contributor

lexnv commented Oct 10, 2024

It's definitely something we want to do!

I would wait a bit to update libp2p to the latest version:

  • the amount of breaking changes from version to version is consuming a lot of resources (I gave up after half a day of hacking at the new API to get it just to compile).
  • we need a better process for catching regressions in networking backend updates. Last time we updated, we discovered a few weeks later a CPU regression of around 20-30%. Here is a good document written by @AndreiEres https://hackmd.io/@AndreiEres/networking-bench (and Benchmark network stack CPU usage  #5220) with a path forward.
  • Ideally this could happen after Litep2p stabilizes. We'd need a safe fallback for unexpected issues, and the version prior to 0.52.4 started to manifest panics for nodes running for 5-6 days 🙈

After we update libp2p to latest version, we could start looking at performance issues: #5221

@lexnv lexnv added this to Networking Oct 10, 2024
@nazar-pc
Copy link
Contributor Author

nazar-pc commented Oct 10, 2024

We jump onto latest version of libp2p at https://github.com/autonomys/subspace fairly quickly and while I agree it is annoying to do all the changes needed to migrate to new version, it typically takes me a couple of hours to do. And I am yet to see show-stopping issues after upgrade itself (though we do not have as robust testing and benchmarking capabilities yet).

I can probably do this upgrade for you (in two steps: first to 0.53.x and then to 0.54.x to make review easier), but it takes you guys a really long time to review with regular merge conflicts in between (I have 4 PRs that I opened that are waiting for review for over a week each, some are waiting for way longer than a week), so while I understand you probably have good reasons for this, it is a big investment.

Let me know if you're willing to commit to timely review and I'll take a look at doing this in the near future myself, it isn't a big deal.

Substrate is a large source of outdated dependencies that triggered RUSTSEC alerts in the past already as well as causing other issues, like pulling old version of libp2p-quic (seems impossible to disable right now) that pulls old version of ring that fails to compile on some platforms, all for dependency we don't even use in the runtime.

@lexnv
Copy link
Contributor

lexnv commented Oct 21, 2024

@nazar-pc We would love to get some help on this if you have some extra bandwidth! 🙏

I'll have a pass soon for the remaining networking PRs, thanks for contributing again!

@nazar-pc
Copy link
Contributor Author

Noted, I'll update once I get to it

@nazar-pc nazar-pc linked a pull request Oct 27, 2024 that will close this issue
2 tasks
@nazar-pc
Copy link
Contributor Author

nazar-pc commented Oct 27, 2024

0.53.2 is done here: #6248

Please assign this issue to me, I'll take care of the rest too.

UPD: 0.54.1 is also there, it was too easy to do to delay it further.

@nazar-pc
Copy link
Contributor Author

Now https://rustsec.org/advisories/RUSTSEC-2024-0421.html affects us due to old libp2p version as well

@lexnv
Copy link
Contributor

lexnv commented Dec 10, 2024

@nazar-pc what is the impact of https://rustsec.org/advisories/RUSTSEC-2024-0421.html issue for your team?

IIUC, https://rustsec.org/advisories/RUSTSEC-2024-0421.html can lead to privilege escalation if and only if host domain name check is part of a privilege check / authorization check.

From the substrate perspective, we use crypto/noise on top of every connection, we verify and validate the remote is in possession of the private keys derived from the PeerID.

In other words, even if the DNS resolver decides that example.org and xn--example-.org are equal, we do rely on the signatures from crypto/noise to establish the encrypted connection with the remote address.

Let me know if my understanding is correct 🙏

@nazar-pc
Copy link
Contributor Author

Probably not very large, but I really don't like to suppress advisories and I had to do that more than once due to old libp2p and litep2p versions in Substrate already.

@lexnv
Copy link
Contributor

lexnv commented Dec 10, 2024

Yep totally understandable, thanks for the swift reply 🙏

We'll test until the EoW the libp2p update which should enable us to easily upgrade to the latest unaffected version 👍

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

Successfully merging a pull request may close this issue.

3 participants