client/network: Use request response for light client requests #7895

mxinden · 2021-01-13T17:57:13Z

This is the last step to close #6968, namely to use libp2p-request-response introduced in #6634 for light client requests. The patch is similar to the block request changes done in #7478.

Given that the list of changes required is already large, this pull request is scoped to refactorings within client/network for now. Once merged, this pull request enables many more refactorings, especially around light-client logic initialization outside of client/network.

mxinden

This pull request is ready to be reviewed.

There are still 3 outstanding / in-progress issues, highlighted as comments below.

mxinden · 2021-01-19T16:05:39Z

client/network/src/light_client_requests/handler.rs

+	// TODO: Still need to figure out how to pass the peerset to the handler. Can't do it in
+	// `builder.rs` as the peerset is only constructed later on in `protocol.rs`.
+	// /// Handle to use for reporting misbehaviour of peers.
+	// peerset: PeersetHandle,


Architectural issue for which I have not yet found a neat solution.

Linking to #7958 here for future reference.

client/network/src/light_client_requests/handler.rs

mxinden · 2021-01-19T16:06:48Z

client/network/src/behaviour.rs

+			CustomMessageOutcome::SyncConnected(peer_id) => {
+				// TODO: Should this be tied to sync connections, or to connections in general?
+				self.light_client_request_sender.inject_connected(peer_id);
+				self.events.push_back(BehaviourOut::SyncConnected(peer_id))
+			}
+			CustomMessageOutcome::SyncDisconnected(peer_id) => {
+				// TODO: Should this be tied to sync connections, or to connections in general?
+				self.light_client_request_sender.inject_disconnected(peer_id);
+				self.events.push_back(BehaviourOut::SyncDisconnected(peer_id))
 			}


Two arguments for using nodes in the sync peer set:

Simplicity

Likely long-lived, thus good for requests

Sounds OK to me, but maybe @tomaka has a more educated opinion on the matter, being intimately familiar with the peer sets.

@tomaka any thoughts on this?

Sorry, forgot to answer.
Yes, that is the right thing to do. Being "sync connected" proves that they're on the correct chain.

Sentry nodes are deprecated. Thus there is no need for `.maintain/sentry-node` to spin up a sentry node test environment. Instead this commit rewrites the setup to contain two full-connected validators and one light client. With the steps below one can now spin up a local test network with two validators, one light-client, Prometheus and Grafana. - cargo build --release - sudo docker-compose -f .maintain/local-docker-test-network/docker-compose.yml up

mxinden · 2021-01-20T19:52:07Z

With 5d1d682 I have rewritten .maintain/sentry-node and renamed it to ./maintain/local-docker-test-network. One can now spin up a test environment with two validators, one light client, Prometheus and Grafana via docker-compose -f .maintain/local-docker-test-network/docker-compose.yml up.

Bug that @svyatonik spotted, namely that polkadot.js/apps only reports "Remote fetch failed" is fixed with 6da6207. UI is still facing issues fetching data via light-client request API. Same issues appear on current master. I need to investigate this further, as it might as well be due to my setup.

svyatonik · 2021-01-21T11:57:13Z

client/network/src/light_client_requests/sender.rs

+				continue
+			}
+
+			pending_request.attempts_left -= 1;


I know that it is something that has already been in the master and you've just moved that code, but this actually causing this error in logs:

Light: [2021-01-21 14:42:48] 2021-01-21 14:42:48.245 DEBUG tokio-runtime-worker sc_network::light_client_requests::sender: No peer available to send request to. Light: [2021-01-21 14:42:48] 2021-01-21 14:42:48.246 WARN tokio-runtime-worker txpool: Failed to fetch block body: RemoteFetchFailed

Couple of lines below (see // Break in case there is no idle peer.), the request is returned to the queue if there are no peers or all peers are busy with other requests. The initial idea was that request only should fail if we have received enough "reject" from full nodes. E.g. 2 different full nodes have responded with error to the same request.

Probably this is causing an issues? I'd just move this line (pending_request.attempts_left -= 1;) just before converting it into_sent request. Wdyt?

Suggestion sounds good and indeed this solves the remaining connection issues. Thanks @svyatonik!

Commit 2477f34 does the corresponding change.

svyatonik

I can't say I have gone through all the code as I was looking mostly on light handler + sender. But: (1) what I have seen looks fine and (2) it works in my local tests. So OK from me

Allow request response protocol handlers to issue reputation changes, by sending them back along with the response payload.

romanb

👍 I've only left insignificant comments and references for other and future readers.

romanb · 2021-01-25T08:39:31Z

client/network/src/light_client_requests/handler.rs

+	StorageProof,
+	light
+};
+// use sc_peerset::{PeersetHandle, ReputationChange};


Suggested change

// use sc_peerset::{PeersetHandle, ReputationChange};

romanb · 2021-01-25T08:42:08Z

client/network/src/light_client_requests/handler.rs

+	// TODO: Still need to figure out how to pass the peerset to the handler. Can't do it in
+	// `builder.rs` as the peerset is only constructed later on in `protocol.rs`.
+	// /// Handle to use for reporting misbehaviour of peers.
+	// peerset: PeersetHandle,


Linking to #7958 here for future reference.

romanb · 2021-01-25T08:49:34Z

client/network/src/request_responses.rs

@@ -233,7 +233,8 @@ impl RequestResponsesBehaviour {
 		if let Some((protocol, _)) = self.protocols.get_mut(protocol) {
 			if protocol.is_connected(target) {
 				let request_id = protocol.send_request(target, request);
-				self.pending_requests.insert(request_id, (Instant::now(), pending_response));
+				let prev_req_id = self.pending_requests.insert(request_id, (Instant::now(), pending_response));
+				debug_assert!(prev_req_id.is_none(), "Expect request id to be unique.");


Just for reference, this assertion can fail with this code, which is addressed in #7957.

romanb · 2021-01-25T09:05:12Z

client/network/src/behaviour.rs

+			CustomMessageOutcome::SyncConnected(peer_id) => {
+				// TODO: Should this be tied to sync connections, or to connections in general?
+				self.light_client_request_sender.inject_connected(peer_id);
+				self.events.push_back(BehaviourOut::SyncConnected(peer_id))
+			}
+			CustomMessageOutcome::SyncDisconnected(peer_id) => {
+				// TODO: Should this be tied to sync connections, or to connections in general?
+				self.light_client_request_sender.inject_disconnected(peer_id);
+				self.events.push_back(BehaviourOut::SyncDisconnected(peer_id))
 			}


Sounds OK to me, but maybe @tomaka has a more educated opinion on the matter, being intimately familiar with the peer sets.

romanb · 2021-01-25T09:09:21Z

So do I understand correctly that this should only be merged after #7957 and possibly after #7958 (or an equivalent solution)? Or is #7958 not considered blocking for this PR?

mxinden · 2021-01-25T09:36:02Z

So do I understand correctly that this should only be merged after #7957 and possibly after #7958 (or an equivalent solution)? Or is #7958 not considered blocking for this PR?

Correct. Paraphrasing the above. This pull request should not be merged before #7957. In addition this pull request should not be merged before finding a solution for the LightClientRequestHandler to be able to issue reputation changes with #7958 being one possible solution.

mxinden · 2021-02-01T15:59:36Z

I have tested this once more via the docker-compose setup. All outstanding comments and TODOs are addressed. I will go ahead and merge this pull request. Thanks @svyatonik @romanb @tomaka for the help!

mxinden · 2021-02-01T15:59:43Z

bot merge

ghost · 2021-02-01T15:59:47Z

Trying merge.

mxinden added 10 commits January 8, 2021 14:32

client/network: Re-enable light_client_handler.rs unit tests

bcdf9fa

client/network: Add scaffolding for light client using req-resp

f9ef812

client/network: Make it compile

20ee04d

client/network: Rename OutEvent SendRequest

8596782

client/network: Restructure light client request client and handler

00a092a

client/network: Rename light client request client to sender

1fccb36

client/network: Remove light client prepare_request

1a45ab7

client/network/src/light: Rework configuration

2bb5740

client/network: Formatting

43b30a8

Merge branch 'paritytech/master' into light-client-request-response

a9d8b4f

mxinden added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jan 13, 2021

mxinden mentioned this pull request Jan 13, 2021

Refactor the block requests, finality requests, and light client requests networking protocol to use the new generic request-response system #6968

Closed

3 tasks

mxinden added 16 commits January 13, 2021 21:25

client/network/light: Remove RequestId

5262e2d

client/network/light: Make request functions methods

be5a591

client/network/light: Refactor request wrapping

48592e1

client/network/light: Fix warnings

8be4473

client/network/light: Serialize request in method

feeef54

client/network/light: Make returning response a method

c893aff

client/network/light: Depend on request response to timeout requests

4a2edde

client/network: Fix test compilation

fcece3a

client/network/light: Re-enable connection test

30eb9ae

client/network/light: Re-enable timeout test

32774d0

client/network/light: Re-enable incorrect_response test

db2d9b5

client/network/light: Re-enable wrong_response_type test

c86671a

client/network/light: Re-enable retry_count_failures test

f2d1487

client/network/light: Re-enable issue_request tests

310efa3

client/network/light: Re-enable send_receive tests

6d33c95

client/network/light: Deduplicate test logic

7efd5c3

mxinden added 5 commits January 18, 2021 18:49

client/network: Fix test compilation

984fa95

client/network: Rename light client request client to sender

8e24f29

client/network: Handle too-many-requests error

31d6791

Merge branch 'paritytech/master' into light-client-request-response

3a04470

client/network: Update outdated comments

d4a2be6

mxinden commented Jan 19, 2021

View reviewed changes

mxinden added 2 commits January 20, 2021 20:23

client/network/light: Choose any peer if none has best block defined

6da6207

mxinden mentioned this pull request Jan 20, 2021

.maintain: Replace sentry-node with local-docker-test-network #7943

Merged

romanb self-requested a review January 21, 2021 09:06

svyatonik reviewed Jan 21, 2021

View reviewed changes

mxinden added 5 commits January 21, 2021 16:26

client/network/light: Handle oneshot cancellation

5a609ec

client/network/light: Do not reduce retry count on missing peer

2477f34

client/network/request-response: Assert in debug request id to be unique

6dfb7c1

client/network/light: Choose same limit as block request protocol

b39509a

Merge branch 'paritytech/master' into light-client-request-response

e5bf6cd

svyatonik approved these changes Jan 22, 2021

View reviewed changes

client/network: Report reputation changes via response

c4ac336

Allow request response protocol handlers to issue reputation changes, by sending them back along with the response payload.

mxinden mentioned this pull request Jan 22, 2021

client/network: Report reputation changes via response #7958

Merged

romanb approved these changes Jan 25, 2021

View reviewed changes

mxinden added 3 commits January 27, 2021 22:30

Merge branch 'paritytech/master' into light-client-request-response

74671e3

Merge branch 'paritytech/master' into light-client-request-response

c4106c2

client/network: Remove resolved TODOs

b5d9636

ghost merged commit 5a5f06e into paritytech:master Feb 1, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/network: Use request response for light client requests #7895

client/network: Use request response for light client requests #7895

mxinden commented Jan 13, 2021 •

edited

Loading

mxinden left a comment

mxinden Jan 19, 2021

romanb Jan 25, 2021

mxinden Jan 19, 2021

romanb Jan 25, 2021

mxinden Jan 27, 2021

tomaka Feb 1, 2021

mxinden commented Jan 20, 2021

svyatonik Jan 21, 2021

mxinden Jan 21, 2021

svyatonik left a comment

romanb left a comment

romanb Jan 25, 2021

romanb Jan 25, 2021

romanb Jan 25, 2021

romanb Jan 25, 2021

romanb commented Jan 25, 2021

mxinden commented Jan 25, 2021

mxinden commented Feb 1, 2021

mxinden commented Feb 1, 2021

ghost commented Feb 1, 2021

client/network: Use request response for light client requests #7895

client/network: Use request response for light client requests #7895

Conversation

mxinden commented Jan 13, 2021 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Jan 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svyatonik left a comment

Choose a reason for hiding this comment

romanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanb commented Jan 25, 2021

mxinden commented Jan 25, 2021

mxinden commented Feb 1, 2021

mxinden commented Feb 1, 2021

ghost commented Feb 1, 2021

mxinden commented Jan 13, 2021 •

edited

Loading