This issue was found by TSAN:
https://build.bitcoinabc.org/viewLog.html?buildTypeId=BitcoinABC_ResourceIntensiveBuilds_BitcoinAbcMasterTsan&buildId=135843&tab=buildLog&logTab=tree&filter=debug&expand=all&_focus=1665&guest=1
What happened here is a race on the `m_max_outbound_full_relay` or
`m_max_outbound_block_relay` conman members. There is a cirular
dependency betwen `CConman` and `PeerLogicValidation`, which is "solved"
by the use of an `Init()` function to pass the peer logic validation to
the conman. The racey members are initialized at this time.
But the peer logic validation starts a thread at construction time which
in turns needs access to these members. Under certain timing
circumstances, the conman initialization and the peer logic validation
can concurrently set/read these members, causing the race.
This behavior can only occur once at initialization time (either in the
main app or during tests) and only when certain timing conditions are
met, which happens under heavy load. This is why TSAN triggered on CI:
the unit tests running in parallel are very resources intensive,
especially since we have the radix insert stress test consuming 100% CPU
for quite some time and requiring over 16GB of memory. This is also the
reason why core won't notice this issue: they don't have this test.
There are several solutions to this problem, and the one submitted in
this diff makes us take the less code ownership: simply add a new mutex
to prevent the race. Since the scope is very narrow the performance
impact is negligible.
Other solutions are to start the scheduler after the conman
initialization, which will cause large updates of the init sequence, or
breaking the circular dependency which would require a lot of
refactoring and completely take ownership of this code.