Fix data races triggered by functional tests.

[gabriel-bjg]

Thread sanitizer triggers data races on functional tests. Steps to reproduce:

./configure --enable-debug --prefix=`pwd`/depends/x86_64-pc-linux-gnu/ --with-sanitizers=thread
make clean
make -j8
python3 test/functional/test_runner.py -j 4

Fixes:

• Function CWallet::KeepKey requires locking as it has concurrent access to database and member nKeysLeftSinceAutoBackup.
• Avoid data race when reading setInventoryTxToSend size by locking the read. If locking happens after the read, the size may change.
• Lock cs_mnauth when reading verifiedProRegTxHash.
• Make fRPCRunning atomic as it can be read/written from different threads simultaneously.
• Make m_masternode_iqr_connection atomic as it can be read/written from different threads simultaneously.
• Use a recursive mutex to synchronize concurrent access to quorumVvec.
• Make m_masternode_connection atomic as it can be read/written from different threads simultaneously.
• Make m_masternode_probe_connection atomic as it can be read/written from different threads simultaneously.
• Use a recursive mutex for synchronized access to activeMasterNode.

[PastaPastaPasta]

utACK

[UdjinM6]

Thanks for finding and fixing these issues! 👍

Few notes:

• it's better to avoid holding locks for too long to lower the risk of introducing deadlocks at some point (d876781)
• we should always lock quorumVvecCs whenever we access quorumVvec now that it's GUARDED_BY(quorumVvecCs) (099909a)
• running p2p_quorum_data.py revealed a similar issue on my machine, this time for skShare (da44228)

[PastaPastaPasta]

re-utACK

[PastaPastaPasta]

Needs rebase

[UdjinM6]

utACK

more fixes coming?

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

This pull request has conflicts, please rebase.

[gabriel-bjg]

more fixes coming?

yes, i finished all fixes now, waiting for the builds.

[PastaPastaPasta]

There was any additional data race / deadlock I detected on a run of feature_block.py which I forwarded to @gabriel-bjg

My understanding is he's planning to fix that and push

[github-actions]

This pull request has conflicts, please rebase.

[gabriel-bjg]

There was any additional data race / deadlock I detected on a run of feature_block.py which I forwarded to @gabriel-bjg

My understanding is he's planning to fix that and push

Regarding the data races, they are all triggered by zmq and libzmq. According to bitcoin tsan exceptions, zmq and libzmq data races are all suppressed (https://github.com/bitcoin/bitcoin/blob/master/test/sanitizer_suppressions/tsan). As agreed with @PastaPastaPasta we will ignore them for now and won't fix anything related to them in this PR.

The possible deadlock triggered by tsan is also a false positive. I present below the scenario which triggers the lock order inversion:

Thread19:
    LOCK2(cs_main, mempool.cs) (miner.cpp:235) => scriptcheckqueue->m_control_mutex (validation.cpp:2160) => cs_main, cgovernancemanager.cs (governance/governance.cpp:352)

Thread18:
    LOCK(cs_main) (validation.cpp:984) => LOCK(mempool.cs) (txmempool.cpp:1232)

Basically what tsan detects is T19 locking: cs_main (first locking), mempool.cs, m_control_mutex, cs_main (second locking as it's already locked, but it's a recursive mutex), cgovernancemanager.cs while T18 is locking: cs_main, mempool.cs. The possible deadlock detection comes from the fact that tsan is not able to detect that cs_main has been already locked before locking mempool.cs on thread19. So it sees thread19 locking mempool.cs, m_control_mutex, cs_main while it sees thread18 locking cs_main and mempool.cs afterwards.

[UdjinM6]

utACK

[PastaPastaPasta]

utACK for squash merge