This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
chore: update libp2p to 0.52.1 #14429
chore: update libp2p to 0.52.1 #14429
Changes from 44 commits
3102dbd
54778bd
839641b
45ff813
12ba133
fe656e8
37e7408
d5b8198
81f5cd0
4d068dd
3e54dd1
076635b
d1fd05c
f01de8b
bea250f
405ebcb
394eba1
ac5cfd3
3d04207
5f511cc
f141f03
6970c11
49e0ba7
477ebc1
dd4a718
c4f0a9c
350902c
b67be46
3c179f0
720a068
7ffa62a
07f2db8
0370270
b25efe5
343a86e
965cc2d
3b1c8f2
a506670
6c78a74
09f1ab6
e22dc73
2b1377c
6aa6c8d
8538215
a6e2e25
c45b465
d464408
8b9309d
73df4d5
c34f538
d44efb5
ff76e2a
925cad0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is adding this line intended? It modifies the networking protocols of Substrate/Polkadot, and is a pretty substantial change for what looks a dependency version bump PR.
If intended, this should IMO go through an RFC: https://github.com/polkadot-fellows/RFCs/pulls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it a bit worrisome that modifying the networking protocol would have successfully passed review without raising alarms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you @melekes elaborate on why this is added in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🙏 Will create an RFC today. I've reverted the code to manually confirming the observed address (although it's not safe, I believe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/libp2p/rust-libp2p/blob/master/protocols/identify/CHANGELOG.md#0430
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set_mode
like it is done in tests), but I don't think it's desirable.Discovered new external address for our node
) toFromSwarm::ExternalAddrConfirmed
case, so we don't log all the candidates.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's how it works today.
There's no safety aspect here. We must already consider every peer as potentially malicious. Increasing the chance that peers report accurate information should neither increase not decrease safety. If it does, it means that we don't actually consider every peer as potentially malicious and this should be fixed.
I'm not necessarily against the principle of AutoNAT. What I'm completely against is sneaking changes to the networking protocol in minor version bumps, and I'm quite annoyed about rust-libp2p if they do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note from the rust-libp2p side:
libp2p-autonat
is not required.libp2p-autonat
is not automatically enabled.Long story short, Substrate should be able to upgrade to
libp2p
v0.52.0
without the introduction oflibp2p-autonat
.