feat: async peerstore #182

achingbrain · 2021-12-31T08:07:49Z

Refactors interfaces and classes used by libp2p-interfaces to use the async peer store from libp2p/js-libp2p#1058

Fixes a memory leak where peer data (multiaddrs, protocols, etc) is never evicted from memory.

BREAKING CHANGE: peerstore methods and pubsub start/stop are now all async

codecov-commenter · 2021-12-31T08:17:53Z

Codecov Report

Merging #182 (a8f5849) into master (38ee6e1) will not change coverage.
The diff coverage is n/a.

❗ Current head a8f5849 differs from pull request most recent head 5c3491c. Consider uploading reports for the commit 5c3491c to get more accurate results

@@           Coverage Diff           @@
##           master     #182   +/-   ##
=======================================
  Coverage   76.58%   76.58%           
=======================================
  Files           2        2           
  Lines        1905     1905           
  Branches      141      144    +3     
=======================================
  Hits         1459     1459           
  Misses        446      446

Impacted Files	Coverage Δ
test/utils/index.js	`87.50% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38ee6e1...5c3491c. Read the comment docs.

achingbrain · 2021-12-31T08:29:24Z

package.json

-    "libp2p": "^0.35.0",
-    "libp2p-floodsub": "^0.28.0",
-    "libp2p-interfaces-compliance-tests": "^2.0.3",
+    "libp2p": "libp2p/js-libp2p#feat/async-peerstore",


This will need to be removed after the next libp2p release.

Longer term we need to break the circular dependencies of these components.

We can, this was already done for pubsub as part of libp2p/js-libp2p#857 but we need to do for the other modules. Just opened a PR to remove the dev deps: libp2p/js-libp2p#1107

dapplion · 2021-12-31T08:57:21Z

Fixes a memory leak where peer data (multiaddrs, protocols, etc) is never evicted from memory.

Where was the leak exactly?

achingbrain · 2021-12-31T09:13:52Z

Where was the leak exactly?

See the discussion in libp2p/js-libp2p#1058

In brief the peer store has various Maps that we add peer data to when we discover them on the network - multiaddrs, protocols, etc. The leak is that we never evict anything from the Map so it just grows and grows.

The fix is to use the datastore to store and read peer data instead of holding it all in memory.

achingbrain · 2021-12-31T14:10:27Z

There seem to be quite a few flaky tests here, from what it looks like, the errors are the same as in master & other PRs - @dapplion any ideas?

dapplion · 2022-01-02T09:06:05Z

The fix is to use the datastore to store and read peer data instead of holding it all in memory.

So everytime libp2p needs to lookup protocols for a peer it will do a database disk read?

achingbrain · 2022-01-02T09:24:43Z

It’ll do a datastore read - that datastore could have an in-memory cache which would replicate the previous behaviour but you could add an eviction strategy to stop your nodes eventually falling over.

dapplion · 2022-01-03T08:04:40Z

I feel a better solution would be to fix the in-memory cache first instead of moving to a disk database. If those values have to be persisted, the app layer can choose when to do so, for example right before shutting down.

achingbrain · 2022-01-03T08:53:44Z

This PR enables the fix for the in-memory cache and does not move to a disk database. It allows libp2p to use a datastore which makes the actual storage configurable - it could be in-memory or a database or a combination of both.

At the moment peer store access is synchronous, which means you are stuck with in-memory storage since every storage method that's not in-memory requires async accessors.

You then can't evict peer information as there's no way to retrieve it later from a secondary async storage medium without requiring the calling code to null-guard on every peer store access and go to the secondary storage medium instead. This would add significant complexity to any part of the stack that touches the peer store which is quite a lot of it.

The change here is just to move to async accessors which then lets you configure the datastore as your application requires.

For the next IPFS release this will be an in-memory datastore with a configurable LRU cache that falls back to something that can store larger amounts of data without impacting memory use such as level or IndexedDB while also periodically writing cache data into the store in the same way the current persistent peer store works.

If you want to be able to hold the peer info of every node in the network in memory as we do now, just configure the LRU cache to be something very large. Even then you'll still gain some protection against OOM errors as the LRU cache will have a limit.

dapplion · 2022-01-03T11:32:44Z

That sounds good @achingbrain ! Thanks for the thorough explanation 👍

vasco-santos

LGTM

vasco-santos · 2022-01-05T16:17:57Z

package.json

-    "libp2p": "^0.35.0",
-    "libp2p-floodsub": "^0.28.0",
-    "libp2p-interfaces-compliance-tests": "^2.0.3",
+    "libp2p": "libp2p/js-libp2p#feat/async-peerstore",


We can, this was already done for pubsub as part of libp2p/js-libp2p#857 but we need to do for the other modules. Just opened a PR to remove the dev deps: libp2p/js-libp2p#1107

vasco-santos · 2022-01-05T16:22:39Z

test/go-gossipsub.js

@@ -2,7 +2,9 @@
 * These tests were translated from:
 *   https://github.com/libp2p/go-libp2p-pubsub/blob/master/gossipsub_test.go
 */
-const { expect } = require('chai')
+const chai = require('chai')
+chai.use(require('dirty-chai'))


nit: use aegir exported chai instead

That would be my preference too, but it's not used anywhere else in this module so I did the manual .use to be consistent with the other tests.

wemeetagain · 2022-01-06T14:50:13Z

Generally LGTM but there is a type error:
https://github.com/ChainSafe/js-libp2p-gossipsub/runs/4717165393?check_suite_focus=true#step:4:29

achingbrain · 2022-01-08T08:21:06Z

@wemeetagain the type error has been resolved, the CI problems are back to the flakiness observed earlier.

Refactors interfaces and classes used by `libp2p-interfaces` to use the async peer store from libp2p/js-libp2p#1058 Fixes a memory leak where peer data (multiaddrs, protocols, etc) is never evicted from memory. BREAKING CHANGE: peerstore methods and pubsub start/stop are now all async

wemeetagain · 2022-01-20T15:44:56Z

This PR introduces some subtle difference in timing that results in the tests inconsistently failing.
IMO we need to diagnose the issue/resolve it before merging this

ts/index.ts

wemeetagain · 2022-01-20T16:59:56Z

Maybe we can just disable the unreliable tests for now

ChainSafe/js-libp2p-gossipsub#182 shipped so the fork is no longer required.

achingbrain force-pushed the feat/async-peerstore branch from b3c60f8 to 0af1865 Compare December 31, 2021 08:28

achingbrain commented Dec 31, 2021

View reviewed changes

achingbrain mentioned this pull request Dec 31, 2021

feat: async peerstore backed by datastores libp2p/js-libp2p#1058

Merged

7 tasks

achingbrain force-pushed the feat/async-peerstore branch from 6734396 to 6ec4b76 Compare December 31, 2021 13:23

dapplion requested review from wemeetagain, twoeths and vasco-santos January 3, 2022 11:33

vasco-santos previously approved these changes Jan 5, 2022

View reviewed changes

achingbrain dismissed vasco-santos’s stale review via a8f5849 January 8, 2022 08:06

achingbrain force-pushed the feat/async-peerstore branch from a8f5849 to 5c3491c Compare January 15, 2022 08:38

achingbrain mentioned this pull request Jan 17, 2022

feat: async peerstore libp2p/js-libp2p-daemon#62

Merged

dapplion previously approved these changes Jan 19, 2022

View reviewed changes

wemeetagain reviewed Jan 20, 2022

View reviewed changes

ts/index.ts Outdated Show resolved Hide resolved

wemeetagain added 2 commits January 20, 2022 11:17

chore: create px messages concurrently

74f764a

chore: skip unreliable tests

cda02f9

wemeetagain dismissed dapplion’s stale review via cda02f9 January 20, 2022 17:19

wemeetagain mentioned this pull request Jan 20, 2022

Re-enable unreliable tests #183

Closed

wemeetagain approved these changes Jan 20, 2022

View reviewed changes

wemeetagain merged commit f938782 into ChainSafe:master Jan 20, 2022

achingbrain added a commit to libp2p/js-libp2p-daemon that referenced this pull request Jan 25, 2022

chore: remove gossipsub fork

b2de54c

ChainSafe/js-libp2p-gossipsub#182 shipped so the fork is no longer required.

achingbrain mentioned this pull request Jan 25, 2022

chore: remove gossipsub fork libp2p/js-libp2p-daemon#67

Merged

achingbrain added a commit to libp2p/js-libp2p-daemon that referenced this pull request Jan 25, 2022

chore: remove gossipsub fork (#67)

0ea657b

ChainSafe/js-libp2p-gossipsub#182 shipped so the fork is no longer required.

twoeths mentioned this pull request Mar 9, 2022

Migrate to libp2p > 0.35.8 and gossipsub 0.12.2 ChainSafe/lodestar#3661

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: async peerstore #182

feat: async peerstore #182

achingbrain commented Dec 31, 2021

codecov-commenter commented Dec 31, 2021 •

edited

Loading

achingbrain Dec 31, 2021

vasco-santos Jan 5, 2022

dapplion commented Dec 31, 2021

achingbrain commented Dec 31, 2021

achingbrain commented Dec 31, 2021

dapplion commented Jan 2, 2022

achingbrain commented Jan 2, 2022

dapplion commented Jan 3, 2022

achingbrain commented Jan 3, 2022

dapplion commented Jan 3, 2022

vasco-santos left a comment

vasco-santos Jan 5, 2022

vasco-santos Jan 5, 2022

achingbrain Jan 5, 2022

wemeetagain commented Jan 6, 2022

achingbrain commented Jan 8, 2022

wemeetagain commented Jan 20, 2022

wemeetagain commented Jan 20, 2022

feat: async peerstore #182

feat: async peerstore #182

Conversation

achingbrain commented Dec 31, 2021

codecov-commenter commented Dec 31, 2021 • edited Loading

Codecov Report

achingbrain Dec 31, 2021

Choose a reason for hiding this comment

vasco-santos Jan 5, 2022

Choose a reason for hiding this comment

dapplion commented Dec 31, 2021

achingbrain commented Dec 31, 2021

achingbrain commented Dec 31, 2021

dapplion commented Jan 2, 2022

achingbrain commented Jan 2, 2022

dapplion commented Jan 3, 2022

achingbrain commented Jan 3, 2022

dapplion commented Jan 3, 2022

vasco-santos left a comment

Choose a reason for hiding this comment

vasco-santos Jan 5, 2022

Choose a reason for hiding this comment

vasco-santos Jan 5, 2022

Choose a reason for hiding this comment

achingbrain Jan 5, 2022

Choose a reason for hiding this comment

wemeetagain commented Jan 6, 2022

achingbrain commented Jan 8, 2022

wemeetagain commented Jan 20, 2022

wemeetagain commented Jan 20, 2022

codecov-commenter commented Dec 31, 2021 •

edited

Loading