Define behaviour of admin_addPeer regarding PeerTable

[timbeiko]

Our current implementation of admin_addPeer only connects with the peer at wire level (DevP2P) but doesn't add it to the peer table. When we advertise nodes in discovery we advertise peers from our peer table.

If the peer is not added to the peer table, it might never be discovered by other peers in the network.

Done criteria

• Investigate how admin_addPeer should behave

• Implement the change (if needed)

• Raise docs issues if changes are needed

[br0tchain]

On one side, the admin_addPeer function allows to connect to the peer using RLPx protocol through a P2PNetwork and then adds it to the current maintained peers of this P2PNetwork.

On the other side, the PeerTable is maintained by the PeerDiscoveryController. This component is the entrypoint for managing the lifecycle of peers.
When this controller handles a message for a peer that doesn't exist, it updates the PeerTable with this new peer. This way, when another peer will discover the peers on this node, it will be advertised.

With the current implementation, a peer added through the admin_addPeer command will not be stored in the PeerTable of the PeerDiscoveryController but only in the P2PNetwork peer list.
It will not be advertised through peer discovery until a message is sent to it using the PeerDiscoveryAgent. This case is very unlikely as the node which added the peer has no interest in using the discovery agent to discover a peer it already knows.

After looking in geth for how this is implemented, the following process is triggered on the admin_addPeer command :

• An event is sent through a go channel to a running server
• The server tries to ping (or pong) the peer to add
• If the peer is successfully pinged, it's added to the peer list
  When calling the discovery, the same server is called and the new added peer can then be advertised.

The current implementation of admin_addPeer allows to add a peer which is only known by the current peer except it's explicitly sent to the PeerDiscoveryController of the same node which is not intuitive.
The peer could be automatically "advertisable" if it is systematically sent to the peer discovery controller.

[AbdelStark]

@shemnon @lucassaldanha @mbaxter Can we take a decision regarding this issue ?
@br0tchain is ready to start the implementation if we want to align with other client implementations ?

[mbaxter]

Makes sense to me to match the behavior of other clients, but this is probably a question for @timbeiko .

Side note - if discovery is explicitly disabled (?discport=0), we shouldn't try broadcasting the peer to the discovery network.

[AbdelStark]

Makes sense to me to match the behavior of other clients, but this is probably a question for @timbeiko .

Side note - if discovery is explicitly disabled (?discport=0), we shouldn't try broadcasting the peer to the discovery network.

Thanks @mbaxter . I will ask @timbeiko

[lucassaldanha]

I agree with @mbaxter. Matching other clients might be the way to go.

One thing that we need to consider is that we shouldn't advertise the added peer unless we are actually connected to it.

Calling admin_addPeer, adds a peer to a special list, no matter if we actually established the connection or not (it might only be able to connect later on).

I believe it doesn't make sense to advertise peers that we haven't connected to.

Imagine a scenario in which I call admin_addPeer with the wrong IP. That connection will never be established, but if we advertise it anyway, now we have other peers trying to talk to this wrong IP.

[mbaxter]

One thing that we need to consider is that we shouldn't advertise the added peer unless we are actually connected to it.

I think @br0tchain is suggesting that we successfully bond to the peer (discovery ping / pong cycle) before adding it to our peer table, which should ensure that we don't broadcast invalid peers.

[br0tchain]

I do, if we match the other clients' behaviour, the peer is added to the discovery table once the client has been able to successfully ping it.

[davemec]

@br0tchain @mbaxter This sounds very similar to a change I currently have in draft review, #1070. If you look at the 2 line change in ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java it is no longer adding a peer to the peer table during a PING as it previously did.

[mbaxter]

@davemec - This is related, but a bit different. We're talking about broadcasting peers out to the discovery network when they're added to our node through the admin_addPeer API. So, this work basically involves initiating outbound bonding (outgoing PING) when that operation is executed. Your change relates to handling incoming bonding (how we respond to incoming PING's).

[davemec]

Thanks @mbaxter