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

litep2p/discovery: Fix memory leak in litep2p.public_addresses() #5998

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Oct 9, 2024

This PR ensures that the litep2p.public_addresses() never grows indefinitely.

  • effectively fixes subtle memory leaks
  • fixes authority DHT records being dropped due to size limits being exceeded
  • provides a healthier subset of public addresses to the /identify protocol

This PR adds a new ExternalAddressExpired event to the litep2p/discovery process.

Substrate uses an LRU address_confirmations bounded to 32 address entries.
The oldest entry is propagated via the ExternalAddressExpired event when a new address is added to the list (if capacity is exceeded).

The expired address is then removed from the litep2p.public_addresses(), effectively limiting its size to 32 entries (the size of address_confirmations LRU).

cc @paritytech/networking @alexggh

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv added T0-node This PR/Issue is related to the topic “node”. I2-bug The node fails to follow expected behavior. labels Oct 9, 2024
@lexnv lexnv self-assigned this Oct 9, 2024
substrate/client/network/src/litep2p/discovery.rs Outdated Show resolved Hide resolved
Co-authored-by: Bastian Köcher <git@kchr.de>
@lexnv lexnv added the A4-needs-backport Pull request must be backported to all maintained releases. label Oct 10, 2024
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@dmitry-markin
Copy link
Contributor

Why do we have so many (>32) external addresses confirmed? Is there a linked issue in polkadot-sdk?

prdoc/pr_5998.prdoc Outdated Show resolved Hide resolved
substrate/client/network/src/litep2p/mod.rs Outdated Show resolved Hide resolved
@lexnv
Copy link
Contributor Author

lexnv commented Oct 11, 2024

Why do we have so many (>32) external addresses confirmed? Is there a linked issue in polkadot-sdk?

This issue was reproduced in Rococo on litep2p validators, don't believe we have an gitissue for it. It looks like over time peers report via identify protocol connections on different ports.
Libp2p has a few events on which we clean up the addresses (AddressExpired / ExpiredListenAddr / ListenerClosed / port-reuse logic).

lexnv and others added 5 commits October 11, 2024 14:56
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
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
Labels
A4-needs-backport Pull request must be backported to all maintained releases. I2-bug The node fails to follow expected behavior. T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants