feat(SPV-742): add peer manager #247
Conversation
Manual Testsℹ️ Remember to ask team members to perform manual tests and to assign |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -42,7 +42,7 @@ var version = "development" | |||
|
|
|||
| type P2PServer interface { | |||
| Start() error | |||
| Shutdown() error | |||
There was a problem hiding this comment.
Isn't it a good practice to stay with error return when shutting down? Probably we can't do anything with that, but at least we have a message and we can possibly kill it manually.
There was a problem hiding this comment.
You can see that neither implementation returns any error - so... why stay here with an error?
| addrFitlerFn := IsRoutable | ||
| if acceptLocalAddresses { | ||
| addrFitlerFn = IsRoutableWithLocal | ||
| } |
There was a problem hiding this comment.
Instead of having two separate functions IsRoutable and IsRoutableWithLocal and having this variable addrFitlerFn (funny typo there BTW 😄 ), maybe it's better to just add a param to function:
func IsRoutable(na *wire.NetAddr, checkLocal bool) boolThis would allow to remove the field addrFilterFn from AddressBook struct and just use IsRoutable function directly
| addrs []*knownAddress // addrs is a slice containing known addresses | ||
| addrsLookup map[string]int // addrLookup is a map for fast lookup of addresses, maps address key to index in addrs slice |
There was a problem hiding this comment.
I don't see the reason to have both.
IMHO all we need is map[string]*knownAddress
and when doing random peek, we can just use maps.Values(..)[random]
There was a problem hiding this comment.
We will have ~2k addresses. I'm not sure that allocating new array of this size every time we want random address is a good idea.
| } | ||
|
|
||
| // UpsertAddrs updates or adds multiple addresses. | ||
| func (a *AddressBook) UpsertAddrs(address []*wire.NetAddress) { |
There was a problem hiding this comment.
| func (a *AddressBook) UpsertAddrs(address []*wire.NetAddress) { | |
| func (a *AddressBook) AddAll(address []*wire.NetAddress) { |
| } | ||
| return peer, nil | ||
| return peer | ||
| } | ||
|
|
||
| func (p *Peer) Connect() error { | ||
| err := p.updatePeerAddr() |
There was a problem hiding this comment.
Actually this could be done in the NewPeer method
internal/transports/p2p/server.go
Outdated
| s.ctxWg.Add(1) // Wait on shutdown | ||
|
|
||
| if s.inboundPeers.Space() == 0 { | ||
| s.log.Debug().Msg("[observeInboundPeers] nothing to do") | ||
| s.noWaitingSleep(sleeDuration) | ||
| continue | ||
| } | ||
|
|
||
| s.log.Info().Msgf("listening for inbound connections on port %d", s.chainParams.DefaultPort) | ||
| s.waitForIncomingConnection() // Accept connection one-by-one to gracefully handle shutdown | ||
|
|
||
| s.ctxWg.Done() |
There was a problem hiding this comment.
It looks a little complicated.
IMHO we should run in go routine an infinite loop that
will accept connections,
try to add it to PeersCollection (or check if there is a space)
if it is ok to add it, then use peer.Connect
if it is not ok, then disconnect the connection
And in case of exiting, all we need to do is: s.listener.Close()
There was a problem hiding this comment.
Clever, changed. But, to be honest, I'm not sure if it's as error-proof as the previous implementation
arkadiuszos4chain
force-pushed
the
feat-742-peer-manager
branch
from
35090f5
to
0439f2f
Compare
…ment address book with buckets to improve geting random address; connect to one randomly chosen seed at start instead of all of them
… add other peers to the pool when sync is finished
Pull Request Checklist