hosts table (db) should include only valid entries

[offerm]

A valid entry is an entry that can be used by the node and by nodes connected to it to open outbound connections.

record an entry only upon successful connection.

For inbound connection:
a. don't record it in the table at all, or
b. record it in the table only if the connected peer provided it's listen port
(I opt for a. since we can't validate the listen port for an incoming connection.

don't add a record if the host is localhost or 127.0.0.1 or node didn't provide listen port or the peer PubKey is the same as our PubKey (self-connection).

For outbound connection:
don't add a record if the host is localhost or 127.0.0.1 or the peer PubKey is the same as our PubKey (self-connection).

[offerm]

@kilrau my recommendation, for now, is not to record in the hosts table only inbound connections.

[kilrau]

Agree with most of it. Summarized, we want to improve the hosts sharing in the following way:

• add outbound connection to hosts db only upon successful connection (applies for the manual CLI connect, as well as getHosts from other xuds) and if the xud can present a valid nodePubKey in the handshake (@sangaman is on this). Otherwise don't add and disconnect.
• add inbound connection to hosts db only if host sends P2P port in handshake, P2P listen port is reachable (either try connect to it (check [Discussion] Socket connection can be opened twice #271) and if the xud can present a valid nodePubKey. For checking the port: we can could use nmap -p 8885 IP or even better just connect&disconnect immediately since this doesn't add a dependency.
  OR
  we just do what offer suggested as a temporary fix: don't add inbound connections at all

don't add a record if the host is localhost or 127.0.0.1 or node didn't provide listen port or the peer PubKey is the same as our PubKey (self-connection).
For outbound connection:
don't add a record if the host is localhost or 127.0.0.1 or the peer PubKey is the same as our PubKey (self-connection).

Already in the works: #177

@kilrau my recommendation, for now, is not to record in the hosts table only inbound connections.

Don't get it 100%, could you rephrase? @offerm I guess you mean don't record inbound connections at all.

[kilrau]

As correctly decribed in @offerm 's https://hackmd.io/s/r1NhWtbrX, "It is very hard to disconnect from nodes. If A connects with B, disconnects with B, connects with C that is connected to B, A will become connected to B again."

If we decide to don't add inbound hosts at all, it's fixed.

If we decide to try doing it, I'd suggest to either create a new disconnectedHosts db to record manually disconnected hosts and make a check on this on connect via getHosts. Once a node is connected again manually via CLI/gRPC , it gets removed from this db. Optionally we could also use the bannedHosts for this and add an additional field saying if the host was just disconnected or really banned.

Another thing:
In the case of banned host, we would want to add a confirmation to the CLI "This host was previously banned, connecting to it now will remove it from the banned hosts list. Continue?" <Y/n.>

[kilrau]

@moshababo @sangaman @michael1011 @itsbalamurali Please add your opinions!

[sangaman]

I think the proper behavior might be to only connect to peers specified in HOSTS packet if they are new to us. If we know that host exists, then we will have already had a chance to connect to them.

I think the banned confirmation prompt is a good idea. Or maybe better we can add a flag to the connect rpc command to override bans, and if it's not specified we can return an error saying something like "this node has been previously banned, to connect anyway set unban to true". That way we can have the same functionality through gRPC calls where a prompt is not practical.

[kilrau]

add a flag to the connect rpc command to override bans

Sounds good: add something like --unban true to the connect rpc. I'd want the CLI to behave a bit more intelligent though, thus simply asking for a Y/n and just using the --unban true grpc call.

I think the proper behavior might be to only connect to peers specified in HOSTS packet if they are new to us. If we know that host exists, then we will have already had a chance to connect to them.

That also sounds good, but that's only client-side filtering part on connection attempt. How about your opinion on these (mark the ones we already have) @moshababo @sangaman @michael1011 :

• add outbound connection to hosts db only upon successful connection
• add inbound connection to hosts db only if host sends P2P port in handshake & is reachable
• add connection to hosts db only if host presents valid nodePubKey
• how to disconnect permanently (feel free to open a separate issue)

[sangaman]

#315 takes care of those 4 bullet points. Unless for the last one, do you mean disconnecting even beyond a server restart? Right now you'd need to ban to do that, but we could introduce a way to stop disconnecting to a certain node but not quite banning them.

[kilrau]

#315 takes care of those 4 bullet points. Unless for the last one, do you mean disconnecting even beyond a server restart? Right now you'd need to ban to do that, but we could introduce a way to stop disconnecting to a certain node but not quite banning them.

Nice! Also the 'P2P Port reachable' test? Will do some tests in a bit.

Disconnect: Yes, I mean disconnecting beyond a restart, given that we might change the logic into continuously connecting to new hosts at runtime, not only on start. It sounds like a separate issue tho.

[sangaman]

OK, yes since it will only add a node to the db on a successful connection with handshake it should required that the port be reachable, otherwise the connection would fail.

Disconnect beyond a restart should definitely be a separate issue. I think we could just use the "ban" functionality for that though, but we'll discuss separately.

[kilrau]

So #315 is not adding inbound connections to the hosts table, correct? Meaning xuds connected to my IP+Port, but me not having a separate connection to their IP + Port are not added to hosts.

Opened a separte issue for disconnect: #336

[sangaman]

It does add inbound connections to the db if the handshake succeeds, it will only attempt to connect to that node in the future if the handshake advertised an external address.

[kilrau]

My thinking was to only add inbound connected peers if that advertised external address actually works (at least once)... I could easily spam the network with tons of advertised external fake addresses which you'll help me relay and the whole network will be busy trying to connect to these. Of course not perfect, but puts the bar higher.

[sangaman]

The same problem would exist if I connect to an outbound peer and he sends me a ton of fake listening addresses, or would you suggest trying each one? Not all of them might work at any given point in time, too. I think we can just maybe set some upper bound on the # of external addresses that can be shared.

[kilrau]

Yep, I was actually thinking about trying each one, but you are right "momentarily unavailable" will be an issue. @sangaman @michael1011

#311 is taking care to delete a host after a week of not being able to successfully connect. So at least we have a limit there and don't try to connect to fakehosts in perpetuity. Closing.