As introduced by (my suggestion, sorry, in) #11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we’re not queuing these up forever after failed resolves.
Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it.
Maybe @sipa can shed a light on what the original intention was.
1951@@ -1953,29 +1952,29 @@ void CConnman::ThreadOpenAddedConnections()
1952 }
1953
1954 // if successful, this moves the passed grant to the constructed node
1955-bool CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool fOneShot, bool fFeeler, bool manual_connection)
1956+void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool fOneShot, bool fFeeler, bool manual_connection)
Yes, this is actually a really big help for the libevent code. This needs to be a one-way function so that it can be run async, with failures optionally triggering callbacks.
The oneshot reinsertion was the only thing preventing making that change trivially.
Edit: See here for that change on the libevent PR: https://github.com/bitcoin/bitcoin/pull/11227/commits/03e73a734ceac8a2fc7ed1a21a752e3580b47569. Notice the only thing the failed-connect callback does: https://github.com/bitcoin/bitcoin/commit/03e73a734ceac8a2fc7ed1a21a752e3580b47569#diff-9a82240fe7dfe86564178691cc57f2f1R2013
@sipa mentioned on IRC that these were originally retried because they may be tor seed attempts, which can only try one address, as opposed to normal seed resolves which may return a long list.
So by not retrying these, if using tor, you only get seednode_count chances to make a connection and learn some new addresses.
To avoid that, I think we should just re-run the entire ThreadDNSAddressSeed() as necessary until we have the desired connections.
@theuni To avoid that, I think we should just re-run the entire ThreadDNSAddressSeed() as necessary until we have the desired connections.
Are you going to do that in this PR or leave it for later?
utACK https://github.com/bitcoin/bitcoin/pull/12329/commits/660f5f19ae74cc81b83540fcb95a33ec437834c8
Are you going to do that in this PR or leave it for later?
Let’s leave that for a future PR, and take this fix for rc2.