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

remove the database-backed peerstore #2329

Open
marten-seemann opened this issue Jun 3, 2023 · 4 comments
Open

remove the database-backed peerstore #2329

marten-seemann opened this issue Jun 3, 2023 · 4 comments
Labels
kind/discussion Topical discussion; usually not changes to codebase

Comments

@marten-seemann
Copy link
Contributor

marten-seemann commented Jun 3, 2023

As far as I can tell, it's only used by drand in PLN: https://github.com/drand/drand/blob/ade43023d83b5b06f15f6563ec0e7b246b0829db/lp2p/ctor.go#L21
And by 0xmesh: https://github.com/0xProject/0x-mesh/blob/a40744bd096d2888fc16941bb940959254ea7c38/p2p/opts.go#L18
(There might be more)

The database-backed peerstore is what required #2231 and the mess we got into by adding contexts (see #2327).

It doesn't make a lot of sense to have a database-backed peerstore in the first place. The peerstore uses the disconnect event to garbage collect old entries, so there's little point in persisting this information. You'd just end up with entries that will never be evicted.

Note that this proposal is only to remove our pstoreds implementation of the peerstore interfaces. Users that still believe that there's value in having a database-backed peerstore are free to adopt that code: Applications can provide their own implementation of the peerstore interface in the libp2p constructor.

@marten-seemann marten-seemann added the kind/discussion Topical discussion; usually not changes to codebase label Jun 3, 2023
@marten-seemann marten-seemann changed the title remote the database-backed peerstore remove the database-backed peerstore Jun 3, 2023
@l0k18
Copy link

l0k18 commented Jul 21, 2023

This metadata store, it surprises me a lot that it is not used enough to warrant maintaining it. Do all the consuming applications bootstrap the metadata of peers every time or do they all have their own persistence?

I am using it, but the lack of iterators in the interface is quite a problem, can't count the records using it, would either have to separately and redundantly implement an index table and keep it synced or write a new implementation and extend it with a new interface that has this necessary stuff by embedding the limited base interfaces.

If there is no reason relating to supporting other languages accessing the on disk database, why would there be any sense in replacing it with sqlite, when it is not optimised for a large dataset? Sqlite is an option for small data sets that is easily accessed by other languages. It would be inefficient to have it handle more than a hundred megabytes of data.

@marten-seemann
Copy link
Contributor Author

You make some excellent points why we should remove it :)

First of all, we need to ask if libp2p itself needs the metadata store? With a super easy refactor, the answer is no.
The next question we need to ask is if libp2p should provide this functionality to applications, even though it doesn't need it itself. In my view, libp2p is a p2p networking library. It should provide networking functionality, but not random other functions like a persistent data store. As you describe yourself, the abstractions currently provided are inadequate for that (no contexts, no iterators, no gc, ...), and that's a symptom of libp2p trying to solve more problems than it should try to solve in the first place.

@l0k18
Copy link

l0k18 commented Jul 21, 2023

Aside from simple peer discovery, essentially advertising keys going along with addresses, most other importers of this library are not running a gossip system to propagate network state information, as their protocol has already covered every part of that. You are correct.

If people need this stuff it will be at our repository.

@l0k18
Copy link

l0k18 commented Jul 30, 2023

I want to report another thing I discovered related to this. It stores the libp2p.Host private key in the data store, in cleartext.

That also means that all libp2p apps are storing at least one excess copy of this key outside of the conusming app's own key storage. It probably should be fixed regardless of how else the rest of this works, to at least document this.

I will just be aiming to make it use an encrypted Badger instance instead. We already have one in our codebase and deduplicating it was already planned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/discussion Topical discussion; usually not changes to codebase
Projects
Status: Discuss
Development

No branches or pull requests

2 participants