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.

Sign up for GitHub

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

Jump to bottom

litep2p/req-resp: Always provide main protocol name in responses #6603

Merged
merged 4 commits into from
Nov 27, 2024
Merged

litep2p/req-resp: Always provide main protocol name in responses #6603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1dccdb3
litep2p/req-resp: Always provide main protocol name in responses
lexnv Nov 21, 2024
658154b
Add PR doc
lexnv Nov 25, 2024
99a8d1b
Merge remote-tracking branch 'origin/master' into lexnv/fix-sync-panic2
lexnv Nov 25, 2024
7085c3a
Merge branch 'master' into lexnv/fix-sync-panic2
dmitry-markin Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions prdoc/pr_6603.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Always provide main protocol name in litep2p responses

doc:
- audience: [ Node Dev, Node Operator ]
description: |
This PR aligns litep2p behavior with libp2p. Previously, litep2p network backend
would provide the actual negotiated request-response protocol that produced a
response message. After this PR, only the main protocol name is reported to other
subsystems.

crates:
- name: sc-network
bump: patch
7 changes: 2 additions & 5 deletions substrate/client/network/src/litep2p/shim/request_response/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ impl RequestResponseProtocol {
&mut self,
peer: litep2p::PeerId,
request_id: RequestId,
fallback: Option<litep2p::ProtocolName>,
_fallback: Option<litep2p::ProtocolName>,
response: Vec<u8>,
) {
match self.pending_inbound_responses.remove(&request_id) {
Expand All @@ -337,10 +337,7 @@ impl RequestResponseProtocol {
response.len(),
);

let _ = tx.send(Ok((
response,
fallback.map_or_else(|| self.protocol.clone(), Into::into),
)));
let _ = tx.send(Ok((response, self.protocol.clone())));
Copy link
Contributor

@dmitry-markin dmitry-markin Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this is not a fix for our issue because the fallback mentioned here is actually a fallback protocol, and not a fallback name. Here we need to pass the actual name of the fallback protocol for the client code to correctly implement protocol backward-compatibility (e.g., correctly process a different protobuf schema).

I have created an issue in litep2p for adding support for actual fallback names:
paritytech/litep2p#289

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To sum up the difference in operation of libp2p & litep2p network backends:

libp2p network backend

  1. libp2p backend is responsible for sending the fallback request, not libp2p.
  2. If only the fallback protocol name was used on the wire and no fallback request happened, the main protocol name is returned to the client code
  3. If the fallback request was sent, the name of the fallback request protocol is returned to the client code (as it is just another protocol from the libp2p backend perspective).

litep2p network backend

  1. litep2p sends the fallback request internally and it is the one responsible for actually performing the fallback request, not the network backend.

    Surprisingly enough, litep2p network backend also has the logic of handling fallback requests in case litep2p returned UnsupportedProtocol for some reason.

  2. If only the fallback protocol name was used on the wire and no fallback request happened, the fallback protocol name is returned to the client code. This is what makes litep2p different and leads to panics in network/sync: Panic on unexpected generic response protocol #6581.

  3. If the fallback request was sent, the name of the fallback request protocol is returned to the client code (the same as libp2p).

I will create a PR to make litep2p/backend behave in a way libp2p backend behaves.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

itep2p sends the fallback request internally and it is the one responsible for actually performing the fallback request, not the network backend.

This turned out to be not the case: litep2p network backend does not use the litep2p's try_send_request_with_fallback and uses try_send_request instead. This means fallback requests are handled completely by the backend in substrate, and not by litep2p.

So, we can be sure that a fallback reported by litep2p is just the fallback name of the main protocol and safely override it with the main protocol name in substrate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oki doki, thanks a lot for the detailed info and for confirming, this helps a lot🙏

self.metrics.register_outbound_request_success(started.elapsed());
},
}
Expand Down
Loading