This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
client/network: Report reputation changes via response #7958
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When handling a request by a remote peer in a request response handler, one might want to in- or de-crease the reputation of the peer. E.g. one might want to decrease the reputation slightly for each request, given that it forces the local node to do work, or one might want to issue a larger reputation change due to a malformed request by the remote peer. Instead of having to pass a peerset handle to each request response handler, this commit suggests to allow handlers to isssue reputation changes via the provided `pending_response` `oneshot` channel. A reputation change issued by a request response handler via the `pending_response` channel is received by the `RequestResponsesBehaviour` which passes the reputation change up as an event to eventually be send to a peerset via a peerset handle.
mxinden
added
A0-please_review
tomaka
suggested changes
Jan 25, 2021
| pub result: Result<Vec<u8>, ()>, | ||
| /// Reputation changes accrued while handling the request. To be applied to the reputation of | ||
| /// the peer sending the request. | ||
| pub reputation_changes: Option<Vec<ReputationChange>>, |
There was a problem hiding this comment.
Suggested change
| pub reputation_changes: Option<Vec<ReputationChange>>, | |
| pub reputation_changes: Vec<ReputationChange>, |
I don't understand why this Option is needed, since None and Vec::new() represent the same thing.
There was a problem hiding this comment.
Good point. Somehow thought Vec::new allocates. Done in 3abb27a.
Given that a request-response request is not called `Request` but `InomingRequest`, rename a request-response response to `OutgoingResponse`.
tomaka
approved these changes
Jan 26, 2021
romanb
approved these changes
Jan 26, 2021
|
bot merge |
|
Trying merge. |
ghost
merged commit
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
When handling a request by a remote peer in a request response handler,
one might want to in- or de-crease the reputation of the peer. E.g. one
might want to decrease the reputation slightly for each request, given
that it forces the local node to do work, or one might want to issue a
larger reputation change due to a malformed request by the remote peer.
Instead of having to pass a peerset handle to each request response
handler, this commit suggests to allow handlers to isssue reputation
changes via the provided
pending_responseoneshotchannel.A reputation change issued by a request response handler via the
pending_responsechannel is received by theRequestResponsesBehaviourwhich passes the reputation change up as anevent to eventually be send to a peerset via a peerset handle.
Particular use-case for this change is #7895 more specifically the light client request response handler. See #7895 for details.
While I think the API proposed here is decent, I would be happy to see alternative suggestions.