Test calling addnode more than once (and MAX_ADDNODE_CONNECTIONS) #28635

1. 
   Sjors commented at 8:56 am on October 11, 2023: member

   While reviewing #28331 we noticed a crash that wasn’t caught by the test suite, but would very quickly happen on a real node. See #28331 (review)

   I was able to reproduce it by calling addnode twice and having the test suite wait a little bit. But this also involved hacking ThreadOpenAddedConnections to wait 1 second instead of a full minute. See https://github.com/Sjors/bitcoin/commit/bc3995c66300fb3139c5360e8f6ee807b02ea86e

   If the test suite could mock the delay in ThreadOpenAddedConnections we can add a regular test for this.

   It would also be a good idea to test MAX_ADDNODE_CONNECTIONS, which should be a simple matter of spinning up ~10 test nodes.

2. 
   vasild commented at 9:45 am on October 11, 2023: contributor

   One area to explore for a possible solution to this is to use a unit test instead of a functional test. The reason being that in the unit test we have greater control of the internals of the program. This would allow us to create a CConnman object with mocked interruptNet member which sleeps less than requested and call its ThreadOpenAddedConnections() method. This would also be useful in fuzz tests like #28584 where the sleep is only slowing down the execution.
3. 
   jonatack commented at 11:53 pm on October 28, 2023: contributor

   Just saw this. I’ve begun writing unit tests like this in #28749.

   Will add a test for MAX_ADDNODE_CONNECTIONS to it now. Edit: never mind, will review #26812 and #28584 first (after finishing updating #28248).

Contributors
Sjors vasild jonatack