nMaxInbound might very well be 0 or -1, if the user prefers to keep a small number of maxconnections.
Note: nMaxInbound of -1 means that the user set maxconnections to 8 or less, but we still want to keep an additional slot for the feeler connection.
Closes #9007
nMaxInbound might very well be 0 or -1, if the user prefers to keep
a small number of maxconnections.
Note: nMaxInbound of -1 means that the user set maxconnections
to 8 or less, but we still want to keep an additional slot for
the feeler connection.
Isn't there some behaviour we want when the actual value (based on number of connections, not maximum number) <= 0, like cancelling the attempt?
Isn't there some behaviour we want when the actual value (based on number of connections, not maximum number) <= 0, like cancelling the attempt?
I've checked the code below it and I think there is no danger from this value reaching <0. The only thing that actually uses nMaxInBound is:
if (nInbound >= nMaxInbound)
{
if (!AttemptToEvictConnection()) {
// No connection to evict, disconnect the new connection
LogPrint("net", "failed to find an eviction candidate - connection dropped (full)\n");
CloseSocket(hSocket);
return;
}
}
So it will try to evict all connections, just like when nMaxInbound would be 0, which I think makes sense. No need for special-casing.
utACK https://github.com/bitcoin/bitcoin/pull/9008/commits/fa1c3c2eb0a1853ed0e0662fc2bdbca51e05ccf5
969 | @@ -970,7 +970,6 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
970 | CAddress addr;
971 | int nInbound = 0;
972 | int nMaxInbound = nMaxConnections - (nMaxOutbound + nMaxFeeler);
Maybe it would be wise to cap this to 0 explicitly, as no one will be expecting this to be negative in future maintenance, and they may add something again that crashes if it is.
int nMaxInbound = std::max(nMaxConnections - (nMaxOutbound + nMaxFeeler), 0);
I disagree at just blindly capping this at 0. There are only two ways imo:
I don't think a negative value for something called nMaxOutbound makes sense. There should never be a difference in behavior between no outgoing connections, or negative outgoing connections. But ok, this needs a different solution anyway at a higher level.
Theoretically we could stop listening for inbound connections completely if this is <=0, however, whitelisted connections are probably an exception? There are some potential traps there that can create additional unexpected behavior changes. I think this pull is safe and we can do that later.
Theoretically we could stop listening for inbound connections completely if this is <=0, however, whitelisted connections are probably an exception?
Probably the logic should be changed further in the future, if you set maxconnections to 5 and -listen=1 -connect=gateway.host I would expect you to end up with 5 connections, not 1.
But ACK getting rid of the assert, good move for now and telling people to set maxconnections to a small number used to be common advice for people concerned with memory and bandwidth (we now use less memory per peer, so less reason)... I also checked that the negative is fine with the current code.
Milestone
0.13.1