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

chore: add config to limit peerstore capacity #770

Merged
merged 6 commits into from
Sep 27, 2023

Conversation

chaitanyaprem
Copy link
Collaborator

Description

go-libp2p doesn't provide an option to limit peer-store capacity which may cause it to grow unbounded.
This PR includes a config flag which is already there in nwaku to set peerStore capacity. If not set, set it to 5*maxConnections flag.

Ref waku-org/nwaku#1525

Changes

  • New config flag for peer-store-capacity
  • Fail adding peers in case this capacity is breached.

Tests

Ran all unit tests.
Ran service-node with and without setting peer-store-capacity flag to confirm values are set appropriately.

Did not add any new unit tests as changes seem trivial.

@status-im-auto
Copy link

status-im-auto commented Sep 25, 2023

Jenkins Builds

Click to see older builds (18)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 4349239 #1 2023-09-25 11:54:12 ~2 min nix-flake 📄log
✔️ 4349239 #1 2023-09-25 11:56:34 ~4 min linux 📦deb
✔️ 4349239 #1 2023-09-25 11:56:41 ~4 min tests 📄log
✔️ 4349239 #1 2023-09-25 11:57:07 ~5 min tests 📄log
✔️ 4349239 #1 2023-09-25 11:58:15 ~6 min android 📦tgz
✔️ 4349239 #1 2023-09-25 12:02:55 ~11 min ios 📦tgz
✖️ 8c446ba #2 2023-09-26 01:35:59 ~22 sec tests 📄log
✖️ 8c446ba #2 2023-09-26 01:36:32 ~1 min nix-flake 📄log
8c446ba #2 2023-09-26 01:36:33 ~1 min linux 📄log
✖️ 8c446ba #2 2023-09-26 01:36:57 ~1 min tests 📄log
8c446ba #2 2023-09-26 01:38:32 ~3 min android 📄log
8c446ba #2 2023-09-26 01:39:09 ~3 min ios 📄log
✔️ 82c0f4a #3 2023-09-26 04:27:28 ~1 min nix-flake 📄log
✔️ 82c0f4a #3 2023-09-26 04:27:49 ~2 min linux 📦deb
✔️ 82c0f4a #3 2023-09-26 04:28:35 ~2 min tests 📄log
✔️ 82c0f4a #3 2023-09-26 04:28:43 ~3 min tests 📄log
✔️ 82c0f4a #3 2023-09-26 04:29:29 ~3 min android 📦tgz
✔️ 82c0f4a #3 2023-09-26 04:29:36 ~4 min ios 📦tgz
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 04dd94f #4 2023-09-27 05:19:24 ~1 min linux 📦deb
✔️ 04dd94f #4 2023-09-27 05:20:36 ~2 min nix-flake 📄log
✔️ 04dd94f #4 2023-09-27 05:21:36 ~3 min android 📦tgz
✔️ 04dd94f #4 2023-09-27 05:21:38 ~3 min tests 📄log
✔️ 04dd94f #4 2023-09-27 05:21:48 ~3 min tests 📄log
✔️ 04dd94f #4 2023-09-27 05:21:59 ~3 min ios 📦tgz
✔️ a86466a #5 2023-09-27 06:36:57 ~1 min linux 📦deb
✔️ a86466a #5 2023-09-27 06:37:30 ~1 min tests 📄log
✔️ a86466a #5 2023-09-27 06:37:49 ~1 min nix-flake 📄log
✔️ a86466a #5 2023-09-27 06:37:57 ~1 min tests 📄log
✔️ a86466a #5 2023-09-27 06:39:11 ~3 min android 📦tgz
✔️ a86466a #5 2023-09-27 06:40:32 ~4 min ios 📦tgz

@richard-ramos
Copy link
Member

Thinking more about this PR, something that i find concerning is that peerID are ephemeral (just logout and login in desktop and you get a new peerID). Wouldn't this be problematic? I wonder you'd end up filling slots in the peerstore with peers that might no longer exist, and reach a limit in the peerstore

@chaitanyaprem
Copy link
Collaborator Author

Thinking more about this PR, something that i find concerning is that peerID are ephemeral (just logout and login in desktop and you get a new peerID). Wouldn't this be problematic? I wonder you'd end up filling slots in the peerstore with peers that might no longer exist, and reach a limit in the peerstore

Good point. At some point during this I was thinking if there is a TTL for not connected peers in the peerstore. That should take care of it. If not, another approach we could take is remove peers from the store once they are disconnected, anyways now we have discv5 running constantly doing discovery which would help find peers as and when peers are less.

@chaitanyaprem
Copy link
Collaborator Author

For now, I have set address expiry of discovered peers.
When we want to connect to those peers again and addresses are not available, removing them from peerStore.
This should take care of concern you have highlighted.

Looks like we need to handle this in Waku layer in a different way. I looked at Prysm and noticed that they don't use the libp2p peerStore at all and have their own peerStore. Probably we may have to take #579 up to handle such scenarios.

@chaitanyaprem chaitanyaprem merged commit 359b28a into feat/sharded-peer-mgmt Sep 27, 2023
6 checks passed
@chaitanyaprem chaitanyaprem deleted the chore/peer-store-capacity branch September 27, 2023 06:41
chaitanyaprem added a commit that referenced this pull request Sep 27, 2023
* feat: connect/disconnect to peers based on node topic sub/unsub

* feat: maintain healty relay connections per pubSubTopic

Co-authored-by: richΛrd <info@richardramos.me>

* chore: add config to limit peerstore capacity (#770)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants