ADR-14: bootstrap from previously seen peers #2024
Conversation
derrandz
requested review from
adlerjohn,
liamsi,
renaynay,
Wondertan,
vgonkivs,
distractedm1nd and
walldiss
as code owners
Codecov Report
@@ Coverage Diff @@
## main #2024 +/- ##
==========================================
+ Coverage 54.19% 55.41% +1.22%
==========================================
Files 217 209 -8
Lines 14031 13324 -707
==========================================
- Hits 7604 7384 -220
+ Misses 5600 5194 -406
+ Partials 827 746 -81
... and 71 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
|
||
| ### Database | ||
|
|
||
| We will use a badgerDB datastore to store the addresses of previously seen peers. The database will be stored in the `data` directory, and will be named `peers.db`. The database will have a single key-value pair, where the key is the peer ID, and the value is the peer multiaddress. |
There was a problem hiding this comment.
My suggestion is to check out the checkpointStore implementation inside of the daser because the on-disk peerstore will look similar (a prefixed-store + a single key where the value is the "good peers").
| + p.onBlockedPeer(PIDToAddrInfo(pID)) | ||
| } | ||
| ``` | ||
| We are assuming a function named `PIDToAddrInfo` that converts a `peer.ID` to a `peer.AddrInfo` struct for this example's purpose. |
derrandz
force-pushed
the
adr/bootstrap-from-previous-peers
branch
from
2f38ded to
91682c8
Compare
derrandz
changed the title
There was a problem hiding this comment.
My only suggestion is to decrease the amount of code snippets you include for approach A
I think the most important thing is to outline where the hooks will be passed, why they occur inside peer tracker, and what the actual hooks will look like themselves. The other stuff is unnecessary and distracting - it'll also probably look different in the implementation. The idea here is just to outline how it'd be integrated.
ideally this ADR could also exist inside go-header somehow if we decide to go with approach A :)
derrandz
force-pushed
the
adr/bootstrap-from-previous-peers
branch
from
bbb2f84 to
670fe64
Compare
derrandz
force-pushed
the
adr/bootstrap-from-previous-peers
branch
from
52701cf to
c8cd4da
Compare
| type PeerStore interface { | ||
| // Put adds a list of peers to the store. | ||
| Put([]peer.AddrInfo) error | ||
| // Get returns a peer from the store. | ||
| Load() ([]peer.AddrInfo, error) | ||
| } |
There was a problem hiding this comment.
There is no need for interface, if there is only one implementation planned.
There was a problem hiding this comment.
In implementation yes, but in ADR I think an interface is eloquent and transmits the message clearly
| // chainID is an identifier of the chain. | ||
| chainID string | ||
| + | ||
| + peerstore Peerstore |
There was a problem hiding this comment.
It seems unusual to pass dependancy object by Params. Is there particular reason for it, or it is just a draft, that will be changed when actual implementation will be create?
There was a problem hiding this comment.
It is odd indeed, but we want this dependency to be optional and given the current signature, we got to do it this way. The implementation is in celestiaorg/go-header#36 please give it a review
There was a problem hiding this comment.
On second thought, I think it shouldn't be a param.
cc @renaynay @Wondertan
| + peerset = ex.trustedPeers | ||
| + } else { | ||
| + // otherwise, node has a chain head that is NOT outdated so we can actually request random peers in addition to trusted | ||
| + peerset = append(ex.trustedPeers, ex.params.peerstore.Get()) |
There was a problem hiding this comment.
Would it make sense on Put operation to store additional time.Now value and check the time passed since last store operation on Load, to identify if stored information is likely to be outdate and should be ignored?
There was a problem hiding this comment.
Good point, I think yes.
derrandz
requested review from
liamsi,
walldiss,
renaynay,
distractedm1nd and
Wondertan
fix: implement latest requested changes
derrandz
force-pushed
the
adr/bootstrap-from-previous-peers
branch
from
932f6c9 to
bf5d30d
Compare
|
Closing this in favour of a doc on go-header instead as the solution is almost fully contained to that repo |
Overview
Self-explanatory, closes #1851
Checklist