-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
feat: report different request errors #3857
Conversation
Codecov Report
... and 48 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -472,10 +475,27 @@ where | |||
} | |||
} | |||
|
|||
fn report_bad_message(&self, peer_id: PeerId) { | |||
trace!(target: "net::tx", ?peer_id, "Penalizing peer for bad transaction"); | |||
fn report_bad_message(&self, peer_id: PeerId, req_err: Option<RequestError>) { |
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.
we can keep this and rename it to on_request_error
and we need another function that accepts a ReputationChangeKind
.
then this function here does not need an option anymore
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.
Hey, just to make it clear, should I revert the changes on the report_bad_message
and add a new function named on_request_error
with the added functionality?
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.
Hey, just to make it clear, should I revert the changes on the report_bad_message
something in between, revert most changes for this but rename to report_message(kind)
and add a ReputationChangeKind
the new on_request_error
function then calls this
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 did some changes regarding your comment, did I get it right?
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.
last nits
@@ -492,7 +507,7 @@ where | |||
fn on_bad_import(&mut self, hash: TxHash) { | |||
if let Some(peers) = self.transactions_by_peers.remove(&hash) { | |||
for peer_id in peers { | |||
self.report_bad_message(peer_id); | |||
self.on_request_error(peer_id, RequestError::BadResponse); |
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.
last nit: we want to call the reputation kind function here, with BadTransactions
Fixes #3855
This pull request handles different transaction response errors.