Send list of peers before dropping a peer

[hashmap]

It turns out that we first accept and then randomly drop connections if we have an excess of peers. This PR adds logging and sending the current list of peers to a peer to be disconnected. It should improve peer discovery.

[DavidBurkett]

This looks good, but we're still going to change it to only drop the most recent incoming connections, right?

[JeremyRubin]

Does this open us up to new network mapping attacks?

[DavidBurkett]

@JeremyRubin In what way? Nodes can always request a peer list. The only thing peers are learning from this is that the node is connected to too many peers.

[hashmap]

@JeremyRubin as David said of you are connected to a peer you can request a peer list. Here we prevent bootstrap problem.
@DavidBurkett I'm opening an issue to discuss and track it, I proposed some support for that #2724, however there may be some better solutions

[garyyu]

Pls let me paste the related part here from #2726 (comment)

IIRC, Peer request (send_peer_request(p2p::Capabilities::PEER_LIST)) should only happen when it's needed.

How about enhance the "Ping / Pong" ( or "Hand / Shake" ? I don't remember well) message? and clearly present the "Peer Request" on the "Hand" (for example) message if needed, means if the remote peer is full of connections, it should at least send back a peer list before refusing/dropping the connection request.

[hashmap]

Handshake actually happens even if we exceed number of peers, clean up happens periodically after that, so no need to embellish handshake. In this pr we don't wait for a peer list request, but unconditionally send the list before dropping the connection.

[antiochp]

Just thinking out loud - does this open us up to any kind of DoS attack?
Previously if we had too many inbound connection attempts we just dropped connections. Now we provide (all) connection attempts with a relatively large chunk of data (that we need to read from our db).
I'm not sure it is an issue - but might be worth discussing.

Scenario - I drown a node in a large number of connection attempts, each one forcing the node to hit its db and build a large message payload.

Maybe we prevent this via our "recent attempts" tracking (at least for the case where these repeated attempts come from the same ip address)?

[hashmap]

@antiochp we read once per clean cycle from db no matter how many peers we have, it's not related to connection attempt. To be on the same page - we accept all handshakes and after that we may remove excess of peers doing periodical checks. So we are more like Google Play, not like AppStore.

I'm not sure we should accept handshake if we have too many peers, perhaps we should have hard_max_peers threshold so when we cross it we reject any connection attempts, because what we have now is a DoS attack vector.

[garyyu]

unconditionally send the list before dropping the connection is quite rude.

As I said in #2725 (comment), I'm proposing to enhance the hand/shake, if the peer need the peer list, it can indicate in hand message, when the remote peer receive this hand message with a peer list request flag, but exceed its maximum peers limitation, it can send out the peer list before dropping; if not exceeding, it just ignore this indicator.

And we should duplicate the excessing logic on the connection request part, instead of current accept / drop / accept / drop / ...

[DavidBurkett]

@garyyu Adding a field to the hand message would require a protocol bump, which would cause all kinds of havoc on the network, or it requires abusing the capabilities field. I personally don't believe sending a few dozen addresses to a peer is a big deal. They're only 18 bytes each.

[hashmap]

What if we send the list only to peers connected in last n seconds? We have this info now.

[ignopeverell]

I think the proper fix for this is running actual seeds instead of full nodes. That would allow any node to get enough addresses to bootstrap (which seems to be the goal here). So perhaps @quentinlesceller https://github.com/mimblewimble/seeder should get some love.

[hashmap]

It seems I found the reason of peer dropping, closing in favor of #2780