Add PreferredNodePeerPool #744
Conversation
pipermerriam
force-pushed
the
piper/extend-HardCodedNodePeerPool-to-use-discovery
branch
2 times, most recently
from
04ae1c4
to
8bcdf3f
Compare
pipermerriam
force-pushed
the
piper/extend-HardCodedNodePeerPool-to-use-discovery
branch
from
8bcdf3f
to
da52306
Compare
pipermerriam
changed the title
p2p/peer.py
Outdated
| if options: | ||
| yield random.choice(options) | ||
| else: | ||
| yield None |
There was a problem hiding this comment.
Will the caller gracefully handle a None result here? Maybe it should just throw an exception if no bootnodes are set.
There was a problem hiding this comment.
No, this is a mistake, it should just not have the else clause and return an empty generator.
p2p/peer.py
Outdated
| """ | ||
| Returns up to `min_peers` nodes, preferring nodes from the preferred list. | ||
| """ | ||
| preferred_nodes = self._get_eligible_preferred_nodes()[:self.min_peers] |
There was a problem hiding this comment.
If I'm reading this right, every trinity install will try to connect to the first min_peers preferred nodes in the list? Seems like that would cause a usage imbalance.
One alternative direction:
_get_eligible_preferred_nodes() could be be generated from a randomized ordering of self.preferred_nodes, and _get_random_preferred_node would just return the first result from that generator. Then theoretically we could get rid of the overhead of generating the whole tuple every time in _get_eligible_preferred_nodes(), and use it as a raw generator, switching to:
preferred_nodes = take(self.min_peers, self._get_eligible_preferred_nodes())
(of course that only has a performance impact if the preferred nodes list gets big)
There was a problem hiding this comment.
I think there is a lot of room for improvement to this class, including something like you suggest here, but for now I'd like to call this good enough.
The bootnodes will not allow additional connections if all of their peer slots are full and trinity will then fallback to discovery. It should only take a moment to exhaust the pre-defined list of preferred nodes, after which discovery will take over.
pipermerriam
force-pushed
the
piper/extend-HardCodedNodePeerPool-to-use-discovery
branch
from
31e3447
to
69c8fc0
Compare
|
@carver Can you double check the body of |
pipermerriam
force-pushed
the
piper/extend-HardCodedNodePeerPool-to-use-discovery
branch
from
69c8fc0
to
51ff716
Compare
pipermerriam
force-pushed
the
piper/extend-HardCodedNodePeerPool-to-use-discovery
branch
from
51ff716
to
60d9c31
Compare
| @@ -17,9 +18,10 @@ | |||
| Generator, | |||
| Iterator, | |||
| List, | |||
| Sequence, | |||
| TYPE_CHECKING, | |||
There was a problem hiding this comment.
nitpick, this seems to screw up alphabetical ordering
There was a problem hiding this comment.
I think isort which is implemented in one of our repos (maybe eth-account?) is the programatic solution for this.
p2p/peer.py
Outdated
| @to_tuple | ||
| def _get_eligible_preferred_nodes(self) -> Generator[Node, None, None]: | ||
| """ | ||
| Returns nodes from the preferred_nodes which have not been used within |
There was a problem hiding this comment.
nitpick: Style guide prefers imperative, present tense.
Return nodes from the preferred_nodes which have not been used within the last preferred_node_recycle_time
p2p/peer.py
Outdated
|
|
||
| def _get_random_preferred_node(self) -> Node: | ||
| """ | ||
| Returns a random node from the preferred list. |
p2p/peer.py
Outdated
|
|
||
| def _get_random_bootnode(self) -> Generator[Node, None, None]: | ||
| """ | ||
| Returns a single node to bootstrap, preferring nodes from the preferred list. |
p2p/peer.py
Outdated
|
|
||
| def get_nodes_to_connect(self) -> Generator[Node, None, None]: | ||
| """ | ||
| Returns up to `min_peers` nodes, preferring nodes from the preferred list. |
| """ | ||
| preferred_nodes: Sequence[Node] = None | ||
| preferred_node_recycle_time: int = 300 | ||
| _preferred_node_tracker: Dict[Node, float] = None |
There was a problem hiding this comment.
Not as part of this PR but would it make sense to maintain a list of preferred nodes in the db or in some updated configuration file so that the list of preferred nodes becomes something well maintained (with newly discovered peers added and stale peers removed) to not fall back to the hardcoded nodes whenever there's a restart?
There was a problem hiding this comment.
Yes, verbally planned. Would you like to open an issue for this. Metrics we've talked about are.
- latency
- correct responses (doesn't send unexpected data)
- num success/failed requests
Initial idea is to have some sort of PeerSubscriber API which would be given a view into peer requests and responses and then to aggregate that data into some accessible form. Then the a FancyPeerPool class could use this data to inform what nodes/peers it returns.
There was a problem hiding this comment.
pipermerriam
deleted the
piper/extend-HardCodedNodePeerPool-to-use-discovery
branch
link: #741
What was wrong?
trinity --lightwith a niave peer pool and discovery will rarely find an LES peer to connect to.HardCodedNodesPeerPooleliminates discovery, but also locks Trinity into only being able to connect to a few nodes.How was it fixed?
Implement a new
PeerPoolclass which implements list of preferred nodes.Cute Animal Picture