Because we (finally) have C++11, there is no excuse for using ints/flags/anything concurrently.
#9695 (which this is based on) got most of them, but the last two (addrName and addrLocal) were left out because they might need more discussion. Here I opted for giving them a wrapper which does locked access, though that does mean creating copies of them during use.
Concept ACK. Lightly tested ack. Getting these cleaned up makes it much easier to use tools that catch serious bugs.
light utACK 74b46ed
ACK
598 | + return addrName; 599 | +} 600 | + 601 | +void CNode::MaybeSetAddrName(const std::string& addrNameIn) { 602 | + LOCK(cs_addrName); 603 | + if (addrName == "") {
nit: .empty() please
Fixed.
I added clang annotations for everything in net (I think) that's guarded by a mutex, and built with -Wthread-safety: https://github.com/theuni/bitcoin/commit/df50d4fabc71a175c3faa0fcf74ded856ea35709
This reveals that nearly all is good, with these exceptions: https://github.com/theuni/bitcoin/commit/d185ca0840f8107b2cf4307f51d703e849532928
The only one that we really need to worry about for 0.14 is the cs_filter locking. However, because cs_filter is locked from net_processing with a wide scope in a few places, I'm not confident that the LOCK(cs_filter) in AttemptToEvictConnection() can't deadlock due to ordering.
So, options are:
I'm fine with any of the above.
Heh, title change comes from IRC discussion. The race mentioned above is not new, and not likely to cause issues. It's not worth holding up 0.14 for another set of ACKs or another PR to review.
If anyone strongly disagrees I'll PR a fix, but I'm ok with leaving it alone.
ACK a51ecf7ce00096bf607d15227ff1e1e39a3f6803
What should hold up the release are critical issues, e.g. those that can (or do) cause crashes, and corruption. Fixing every little thing static and dynamic analyzers can complain about is not part of that, and can be done later.
utACK https://github.com/bitcoin/bitcoin/commit/a51ecf7ce00096bf607d15227ff1e1e39a3f6803
These are (afaik) all long-standing races or concurrent accesses. Going
forward, we can clean these up so that they're not all individual atomic
accesses.
- Reintroduce cs_vRecv to guard receive-specific vars
- Lock vRecv/vSend for CNodeStats
- Make some vars atomic.
- Only set the connection time in CNode's constructor so that it doesn't change
Rebased to fix a trivial merge conflict.
re-ACK db2dc7a58cb0a3df58188b748df8e0d04ba76f00
ACK db2dc7a58cb0a3df58188b748df8e0d04ba76f00. Running a node with master + this PR, with many connections, compiled with -fsanitize=thread -fsanitize=undefined. Only these cases are detected:
Milestone
0.14.0