feat: add peer to PeerTable on admin_addPeer #1177
Conversation
| peerPermissions, | ||
| natService, | ||
| metricsSystem, | ||
| maintainedPeers::subscribeAdd, |
There was a problem hiding this comment.
This feels too convoluted and inverted. Passing just the subscribe methods does not keep the type from needing to be known by the classes doing the subscriptions. It also impairs readability so I have no idea what object I am ultimately subscribing to. I think it would be cleaner to pass the maintainedPeers instance as part of the constructor chain instead of two curried methods and then in the PeerDiscoveryController method call
maintainedPeers.subscribeAdd(this::onPeerAdded);
maintainedPeers.subscribeRemove(this::onPeerRemoved);
If we want the discovery controller to be removable and re-installable we could then track the subscription IDs and then also keep a reference to the maintainedPeers object for un-installation.
There was a problem hiding this comment.
Hi @shemnon, thank you for your review. Unfortunately I won't have enough time to fix this before leaving on holidays so I'll get back into it when as soon as I can.
There was a problem hiding this comment.
Hi @shemnon, I've fixed this as you suggested, it's more readable this way :)
|
@br0tchain @shemnon - will this change anything around the peer modifications not being persisted? For context, this is what's currently included in the docs around |
|
Hey @MadelineMurray , sorry for the late response. No it won't change anything about the peer persistence, it will just add it to the Peer Table so it will be erased when the peer reboots. |
br0tchain
force-pushed
the
feature/560-addpeer-to-peer-table
branch
2 times, most recently
from
d8513aa
to
9631974
Compare
…subscription Signed-off-by: Alexandre PARIS-VERGNE <alexpv14@gmail.com>
Signed-off-by: Alexandre PARIS-VERGNE <alexpv14@gmail.com>
br0tchain
force-pushed
the
feature/560-addpeer-to-peer-table
branch
from
9631974
to
08444d8
Compare
Signed-off-by: Alexandre PARIS-VERGNE <alexpv14@gmail.com>
|
@br0tchain - no worries and thanks for the clarification. |
|
@br0tchain is this ready for merge? |
|
I think so, I've just updated it with the latest master. |
| @@ -195,6 +201,18 @@ private PeerDiscoveryController( | |||
| "type"); | |||
| } | |||
|
|
|||
| private void onPeerAdded(final Peer peer, final boolean wasAdded) { | |||
| if (wasAdded) { | |||
| addToPeerTable(DiscoveryPeer.fromEnode(peer.getEnodeURL())); | |||
There was a problem hiding this comment.
This doesn't look right. As we discussed in the issue, we should bond to the peer rather than just adding them to our peer table and immediately advertising the node to the network.
There was a problem hiding this comment.
Added a follow-up ticket to address since this is already merged: #1278
| @@ -81,13 +82,15 @@ | |||
| /* Is discovery enabled? */ | |||
| private boolean isActive = false; | |||
| protected final Subscribers<PeerBondedObserver> peerBondedObservers = Subscribers.create(); | |||
| private final MaintainedPeers maintainedPeers; | |||
There was a problem hiding this comment.
A simpler way of implementing this would be to call the publicly exposed method PeerDiscoveryAgent.bond() from P2PNetwork.addMaintainConnection.
|
|
||
| private void onPeerRemoved(final Peer peer, final boolean wasRemoved) { | ||
| if (wasRemoved) { | ||
| peerTable.tryEvict(peer); |
There was a problem hiding this comment.
Do we need this logic? Honestly not sure if we do or not, but it's a little unexpected at first glance.
PR description
Handle peerAdded event on admin_addPeer to add the peer to the PeerTable
Handle peerRemoved event on admin_removePeer to remove the peer from the PeerTable
Fixed Issue(s)
fixes #560
Signed-off-by: Alexandre PARIS-VERGNE alexpv14@gmail.com