Store remote features in PeersDb
#2978
Conversation
We only store information about our peers when we succeed in making an outgoing connection to them. The only informaiton we stored was the address that we used when connecting. We now also store the features supported by our peer when we last connected to them. It's important to note that this means that we currently don't store peers that always connect to us (e.g. mobile wallet users that never have a stable address): we will address that in the next commit.
Once we have a channel with a peer that connected to us, we store their details in our DB. We don't store the address they're connecting from, because we don't know if we will be able to connect to them using this address, but we store their features.
t-bast
force-pushed
the
store-remote-peer-features
branch
from
2092ead to
0c9b78c
Compare
| statement.setBytes(3, encodedFeatures) | ||
| statement.executeUpdate() | ||
| } | ||
| case None => |
There was a problem hiding this comment.
From the name of the function, I would guess that addOrUpdatePeer(_, NodeInfo(features, None)) would remove the previously stored address. You should document it. Maybe there should even be two functions, one for updating only the features and one for updating both the features and the address.
There was a problem hiding this comment.
I agree, this was confusing, thanks for pointing it out. I extracted it to a different function in ad482dc.
| case Some(remoteFeatures) => Some(remoteFeatures) | ||
| case None => nodeParams.db.peers.getPeer(remoteNodeId).map(nodeInfo => LastRemoteFeatures(nodeInfo.features, written = true)) | ||
| } | ||
| replyTo ! PeerInfo(self, remoteNodeId, stateName, remoteFeatures_opt.map(_.features), None, d.channels.values.toSet) |
There was a problem hiding this comment.
Why not give the address too?
There was a problem hiding this comment.
In the disconnected case, we don't store the peer's address (in DisconnectedData). We may have one if we connected and then disconnected, or we may not. We could start storing this (optional) remote address in the disconnected case, but I don't know if we would gain much: the scope of this PR is rather to store the remote features, so it's something we'd do in a different PR if we think it's useful?
| // If this is an incoming connection, we only store the peer details in our DB if we have channels with them. | ||
| // Otherwise nodes could DoS by simply connecting to us to force us to store data in our DB. | ||
| // We don't update the remote address, we don't know if we would successfully connect using the current one. | ||
| nodeParams.db.peers.addOrUpdatePeer(remoteNodeId, NodeInfo(connectionReady.remoteInit.features, None)) |
There was a problem hiding this comment.
Why do we need a write here? Isn't it enough to write when the channel is ready?
There was a problem hiding this comment.
Good point, I removed it in ad482dc
- Use separate function when updating features only - Remove DB write before channel ready for incoming connection
We only store information about our peers when we succeed in making an outgoing connection to them. The only information we stored was the address that we used when connecting. We now also store the features supported by our peer when we last connected to them.
It's important to note that this means that we previously didn't store peers that always connect to us (e.g. mobile wallet users that never have a stable address): we now store their features in our DB once we have a channel with them. For private peers that always connect to us, we will never store an address: it's their responsibility to connect back to us on disconnection.