celestiaorg / go-header

Go library with all the services needed to request, sync and store blockchain headers.
Apache License 2.0
19 stars 18 forks source link

refactor(p2p)!: PIDStore interface should take `peer.AddrInfo` and not `peer.ID` #86

Closed renaynay closed 1 year ago

renaynay commented 1 year ago

The whole original point of using an on-disk peerstore is to quickly bootstrap the node with previously-seen good peers such that it can use them as quickly as possible.

If we only store the peer.ID, the host will have no addr information on that peer so it will not be able to connect to it early on until it actually discovers that peer later on and then gets its address information.

While this PR is breaking, I doubt anyone except celestia-node is using the pidstore interface at all so hopefully this isn't too intrusive.

codecov-commenter commented 1 year ago

Codecov Report

Merging #86 (6520262) into main (87ab66e) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #86   +/-   ##
=======================================
  Coverage   67.27%   67.27%           
=======================================
  Files          35       35           
  Lines        2827     2827           
=======================================
  Hits         1902     1902           
  Misses        772      772           
  Partials      153      153           
Impacted Files Coverage Δ
p2p/peer_tracker.go 77.92% <100.00%> (ø)
renaynay commented 1 year ago

We must use on disk libp2p peer store instead that will keep addresses.

@Wondertan then we do not need pidstore interface whatsoever -- we won't actually get any granularity around which peers we are storing/bootstrapping off of - it'll just be every peer that we maintain in our peerstore. I just POC'd a workaround that purely uses the on disk peerstore to bootstrap the peer_tracker and it seems to work for bootstrapping node from prev seen peers well enough.

I'm okay with this b/c it's less LOC entirely and we can remove ugliness from exchange + peer_tracker. I wish we would've done this from the start.