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.

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

substrate: peer_store: log warn on disconnecting because of reputation #1299

Merged
merged 5 commits into from
Aug 31, 2023

Conversation

alexggh
Copy link
Contributor

@alexggh alexggh commented Aug 30, 2023

Disconnecting and banning a peer because of negative reputation is usually an indicative of one of two things:

  1. We've got a bug that forces disconnects.
  2. We've got malicious peers that try to attack us.

In both cases I don't think we should hide this behind a trace log and we should log errors, so that things are easy to notice and debug/mitigated.

Disconnecting and banning a peer because of negative reputation is
usually an indicative of one of two things:
1. We've got a bug that forces disconnects.
2. We've got malicious peers that try to attack us.

We both cases I don't think we should hide this behind a trace log
and we should log errors, so that things are easy to notice and
debug/mitigated.

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
@alexggh alexggh added A1-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). T8-parachains_engineering labels Aug 30, 2023
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

let's see what we previously didn't see 🍿

@@ -222,7 +222,7 @@ impl PeerStoreInner {
if peer_info.reputation < BANNED_THRESHOLD {
self.protocols.iter().for_each(|handle| handle.disconnect_peer(peer_id));

log::trace!(
log::error!(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer warning instead of error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree! Done.

alexggh and others added 2 commits August 30, 2023 13:38
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
@alexggh alexggh changed the title substrate: peer_store: log error on disconnecting because of reputation substrate: peer_store: log warn on disconnecting because of reputation Aug 30, 2023
@alexggh alexggh enabled auto-merge (squash) August 30, 2023 15:11
@alexggh alexggh merged commit 5559b75 into master Aug 31, 2023
94 of 98 checks passed
@alexggh alexggh deleted the alexaggh/threshold_log_error branch August 31, 2023 06:40
Daanvdplas pushed a commit that referenced this pull request Sep 11, 2023
#1299)

* substrate: peer_store: log error on disconnecting because of reputation

Disconnecting and banning a peer because of negative reputation is
usually an indicative of one of two things:
1. We've got a bug that forces disconnects.
2. We've got malicious peers that try to attack us.

We both cases I don't think we should hide this behind a trace log
and we should log errors, so that things are easy to notice and
debug/mitigated.

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>

* Move from error to warn

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>

---------

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
paritytech#1299)

* substrate: peer_store: log error on disconnecting because of reputation

Disconnecting and banning a peer because of negative reputation is
usually an indicative of one of two things:
1. We've got a bug that forces disconnects.
2. We've got malicious peers that try to attack us.

We both cases I don't think we should hide this behind a trace log
and we should log errors, so that things are easy to notice and
debug/mitigated.

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>

* Move from error to warn

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>

---------

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
…h#1299)

* use raw balance value if tokenDecimals property is missing

* fmt
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* use raw balance value if tokenDecimals property is missing

* fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A1-insubstantial Pull request requires no code review (e.g., a sub-repository hash update).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants