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

network/metrics: Expose number of banned peers from peerstore and enable litep2p metrics #4977

Merged
merged 30 commits into from
Jul 23, 2024
Merged

network/metrics: Expose number of banned peers from peerstore and enable litep2p metrics #4977

merged 30 commits into from
Jul 23, 2024

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Jul 8, 2024 •
edited
Loading

This PR extends the metrics exposed by the peerstore with the total number of banned peers.

The new metric is exposed under substrate_sub_libp2p_peerset_num_banned_peers.

To easily extend metrics in the future, the fn num_known_peers is removed in favor of fn status.

While at it, enable the metrics for litep2p:

  • total number of peers from peerstore (needed to debug memory consumption)
  • total number of banned peers from peerstore (needed to debug reputation bans and disconnects)

Have added a couple of tests to validate that the number of banned peers is exposed properly.

Part of: #4681

Testing Done

Using subp2p-explorer have submitted random data on tx protocol.
The peer gets banned, the num of banned peers is incremented then the peer is disconnected.

cc @paritytech/networking

lexnv added 8 commits July 8, 2024 17:50
@lexnv
net/service: Remove unnecesary clone for peer store handle
9d9035c 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
peerstore: Add PeerStoreStatus for better monitoring
a68422b 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
peerstore: Cache num banned peers and add test
5a4e5f0 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
net/tests: Adjust tests to the new interface
b2c089c 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
litep2p/peerstore: Backport metrics
3e29097 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
net: Deprecate num_known_peers in favor of status
89f5251 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
litep2p: Increment peerset_num_discovered
6531390 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
metrics: Introduce peerset_num_banned_peers
9afb15a 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv added A1-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). R0-silent Changes should not be mentioned in any release notes I5-enhancement An additional feature request. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. labels Jul 8, 2024
@lexnv lexnv self-assigned this Jul 8, 2024
bkchr
bkchr reviewed Jul 8, 2024
View reviewed changes
substrate/client/network/src/peer_store.rs Outdated
Comment on lines 112 to 114

/// Get the status of the peer store.
fn status(&self) -> PeerStoreStatus;
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the implementation of this function. Why don't we update the metrics directly in the peerstore? Then we don't need to lock any mutexes?

lexnv added 9 commits July 11, 2024 12:49
@lexnv
litep2p/peerstore: Avoid unnecessary locks during initialization
d47b522 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
litep2p/peerstore: Remove unnecessary from_handle method
de715cf 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
net/metrics: Introduce peerset metrics
9b98b20 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
libp2p/peerstore: Increment peerset metrics
86838fb 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
litep2p/peerstore: Increment peerset metrics
3bfa65b 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
net/params: Expose prometheus registry to peerset initialization
35effc4 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
net: Initialize metrics directly in peerstore
a4d0b5f 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
peerset: Make metrics unique and remove status method
c63d3ee 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
templates: Fix initialization
6135f3b 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv mentioned this pull request Jul 11, 2024
Merged
lexnv added 6 commits July 11, 2024 14:54
@lexnv
network/tests: Adjust testing
8dbcec5 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
net/param: More adjustments
f24bc21 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
peerstore/tests: Adjust tests to register metrics
24b6adc 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
peerstore: Adjust testing
18d2818 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
cumulus: Pass metrics registry to full net config
1b6cde4 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Merge remote-tracking branch 'origin/master' into lexnv/peerstore-met…
c5cbcab 
…rics

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6679786

@lexnv
cumulus/test: Add prometheus registry metrics
bc80485 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
github-merge-queue bot pushed a commit that referenced this pull request Jul 15, 2024
@lexnv
network/tx: Ban peers with tx that fail to decode (#5002)
d2dff5f 
A malicious peer can submit random bytes on transaction protocol.
In this case, the peer is not disconnected or reported back to the
peerstore.

This PR ensures the peer's reputation is properly reported.

Discovered during testing:
- #4977


cc @paritytech/networking

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv and others added 4 commits July 16, 2024 18:10
@lexnv @dmitry-markin
Update substrate/client/network/src/litep2p/peerstore.rs
d815acb 
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@lexnv
net: Rename PeerSetMetrics to PeerStoreMetrics
36d1521 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
notif/tests: Add comment wrt to test initialization
80eb737 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
peerstore: Add constructor for peerstore handle
59b2fbe 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv enabled auto-merge July 16, 2024 16:36
@lexnv lexnv disabled auto-merge July 16, 2024 16:36
jpserrat pushed a commit to jpserrat/polkadot-sdk that referenced this pull request Jul 18, 2024
@lexnv @jpserrat
network/tx: Ban peers with tx that fail to decode (paritytech#5002)
0154a9f 
A malicious peer can submit random bytes on transaction protocol.
In this case, the peer is not disconnected or reported back to the
peerstore.

This PR ensures the peer's reputation is properly reported.

Discovered during testing:
- paritytech#4977


cc @paritytech/networking

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
alexggh
alexggh approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@alexggh alexggh left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@lexnv lexnv added this pull request to the merge queue Jul 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 23, 2024
@lexnv lexnv added this pull request to the merge queue Jul 23, 2024
Merged via the queue into master with commit 7f905e2 Jul 23, 2024
156 of 160 checks passed
@lexnv lexnv deleted the lexnv/peerstore-metrics branch July 23, 2024 18:56
@kianenigma
Copy link
Contributor

I am going to repeat exactly what I said in #2944 (comment) + complain about the lack of:

https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/PULL_REQUEST_TEMPLATE.md#integration

This PR should have not been marked as Silent. It is a breaking change in many ways :)

TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
@lexnv @TarekkMA
network/tx: Ban peers with tx that fail to decode (paritytech#5002)
5a8dfbc 
A malicious peer can submit random bytes on transaction protocol.
In this case, the peer is not disconnected or reported back to the
peerstore.

This PR ensures the peer's reputation is properly reported.

Discovered during testing:
- paritytech#4977


cc @paritytech/networking

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
@lexnv @dmitry-markin @TarekkMA
network/metrics: Expose number of banned peers from peerstore and ena…
4a72186 
…ble litep2p metrics (paritytech#4977)

This PR extends the metrics exposed by the peerstore with the total
number of banned peers.

The new metric is exposed under
`substrate_sub_libp2p_peerset_num_banned_peers`.

To easily extend metrics in the future, the `fn num_known_peers` is
removed in favor of `fn status`.

While at it, enable the metrics for litep2p:
- total number of peers from peerstore (needed to debug memory
consumption)
- total number of banned peers from peerstore (needed to debug
reputation bans and disconnects)

Have added a couple of tests to validate that the number of banned peers
is exposed properly.

Part of: paritytech#4681


### Testing Done
Using [subp2p-explorer](https://github.com/lexnv/subp2p-explorer) have
submitted random data on tx protocol.
The peer gets banned, the num of banned peers is incremented then the
peer is disconnected.

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
sfffaaa pushed a commit to peaqnetwork/polkadot-sdk that referenced this pull request Dec 27, 2024
@lexnv
network/tx: Ban peers with tx that fail to decode (paritytech#5002)
038995f 
A malicious peer can submit random bytes on transaction protocol.
In this case, the peer is not disconnected or reported back to the
peerstore.

This PR ensures the peer's reputation is properly reported.

Discovered during testing:
- paritytech#4977


cc @paritytech/networking

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexggh alexggh alexggh approved these changes

@dmitry-markin dmitry-markin dmitry-markin approved these changes

@bkchr bkchr bkchr approved these changes

Assignees

@lexnv lexnv

Labels
A1-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. I5-enhancement An additional feature request. R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lexnv @paritytech-cicd-pr @kianenigma @bkchr @dmitry-markin @alexggh