net: Strip previous p2p prefix before concatenating addresses#11078
net: Strip previous p2p prefix before concatenating addresses#11078
Conversation
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
| if matches!(addr.iter().last(), Some(multiaddr::Protocol::P2p(_))) { | ||
| addr.pop(); | ||
| } |
There was a problem hiding this comment.
Would it be difficult to ensure that self.multiaddr never contains the peer ID in the first place?
We should investigate how peer IDs end up in the address that is supposed to be without a peer ID.
There was a problem hiding this comment.
Litep2p appends the /p2p prefix on all listen addresses on initialization:
I don't believe this will cause issues since we are dialing from the transport manager either way:
- a different peer cannot trick us into dialing since we compare the addresses stripped of
/p2pprefix - don't believe other protocols make use of this
I would still keep this one as a tiny defensive check for future proofing
AndreiEres
left a comment
There was a problem hiding this comment.
Not a networking expert but LGTM
| let proto = multiaddr::Protocol::P2p(From::from(self.peer_id)); | ||
| self.multiaddr.clone().with(proto) | ||
| let mut addr = self.multiaddr.clone(); | ||
| if matches!(addr.iter().last(), Some(multiaddr::Protocol::P2p(_))) { |
There was a problem hiding this comment.
Maybe worth adding a comment why we do it
Co-authored-by: Bastian Köcher <git@kchr.de>
|
/cmd prdoc --audience node_dev --bump patch |
…e_dev --bump patch'
This PR ensures that a `MultiaddrWithPeerId` will always remain valid after a concatenating process. The litep2p code will concatanate the provided multiaddr-with-peerID regardless if the address already contains a `p2p/..` prefix: https://github.com/paritytech/polkadot-sdk/blob/e85be1c07adedcc86554affa4f6af536c4a2efc1/substrate/client/network/src/litep2p/service.rs#L427-L434 This then can lead to addresses for reserved peers to never be dialed by the network backend as they are considered invalid. This has been discovered by running the networking benchmarks: https://github.com/paritytech/polkadot-sdk/blob/e85be1c07adedcc86554affa4f6af536c4a2efc1/substrate/client/network/benches/notifications_protocol.rs#L143-L148 Part of the cleanups for: - #10425 --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit e30a412)
|
Successfully created backport PR for |
Backport #11078 into `stable2512` from lexnv. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR ensures that a `MultiaddrWithPeerId` will always remain valid after a concatenating process. The litep2p code will concatanate the provided multiaddr-with-peerID regardless if the address already contains a `p2p/..` prefix: https://github.com/paritytech/polkadot-sdk/blob/e85be1c07adedcc86554affa4f6af536c4a2efc1/substrate/client/network/src/litep2p/service.rs#L427-L434 This then can lead to addresses for reserved peers to never be dialed by the network backend as they are considered invalid. This has been discovered by running the networking benchmarks: https://github.com/paritytech/polkadot-sdk/blob/e85be1c07adedcc86554affa4f6af536c4a2efc1/substrate/client/network/benches/notifications_protocol.rs#L143-L148 Part of the cleanups for: - #10425 --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit e30a412)
|
Successfully created backport PR for |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin stable2512
git worktree add -d .worktree/backport-11078-to-stable2512 origin/stable2512
cd .worktree/backport-11078-to-stable2512
git switch --create backport-11078-to-stable2512
git cherry-pick -x e30a412c6cd957afedbb3157dac9cf4b424b0fa6 |
Backport #11078 into `stable2506` from lexnv. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Backport paritytech#11078 into `stable2512` from lexnv. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR ensures that a
MultiaddrWithPeerIdwill always remain valid after a concatenating process.The litep2p code will concatanate the provided multiaddr-with-peerID regardless if the address already contains a
p2p/..prefix:polkadot-sdk/substrate/client/network/src/litep2p/service.rs
Lines 427 to 434 in e85be1c
This then can lead to addresses for reserved peers to never be dialed by the network backend as they are considered invalid.
This has been discovered by running the networking benchmarks:
polkadot-sdk/substrate/client/network/benches/notifications_protocol.rs
Lines 143 to 148 in e85be1c
Part of the cleanups for: