Fix AddAddress cs_mapaddresses/db transaction deadlock #500

pull gavinandresen wants to merge 1 commits into bitcoin:master from gavinandresen:txndeadlock changing 1 files +21 −15

gavinandresen commented at 5:45 PM on September 6, 2011: contributor

Thanks to Pieter for the detective work finding this.

Bug was a deadlock when the process-messages-thread and the IRC thread were adding peer addresses at the same time.

I inspected the code for any other cases where starting a database transaction might trigger this type of bug, and all other cases are OK. I'll keep thinking about how to get the DEBUG_LOCKORDER code to detect this in the future.

Tested by running bitcoind, letting it connect and startup (and join an IRC channel), then shutting it down repeatedly (10 times in a row) and observing no deadlocks.
TheBlueMatt commented at 6:13 PM on September 6, 2011: member

ACK
Fix AddAddress cs_mapaddresses/db transaction deadlock 9406696578
in src/net.cpp:None in 55b8833696 outdated
```
 442 | @@ -443,6 +443,10 @@ bool AddAddress(CAddress addr, int64 nTimePenalty, CAddrDB *pAddrDB)
 443 |      if (addr.ip == addrLocalHost.ip)
 444 |          return false;
 445 |      addr.nTime = max((int64)0, (int64)addr.nTime - nTimePenalty);
 446 | +    bool fUpdated = false;
 447 | +    bool fNew = false;
 448 | +    CAddress& addrFound = addr;
```
sipa commented at 6:22 PM on September 6, 2011:

I think this should not be a reference, as it would make the test on line 464 always fail.

gavinandresen commented at 11:44 PM on September 6, 2011:

Huh? The reference gets reassigned on line 463...

sipa commented at 12:19 AM on September 7, 2011:

You can't change what a reference refers to. The assignment operator (except for initialization) only overwrites the value referred to. Maybe you want a pointer, which gives you control over what is referenced.

gavinandresen commented at 12:25 AM on September 7, 2011:

Right you are, thanks for telling me I'm an idiot gently. Fixed (changed addrFound to a plain-old CAddress).
sipa commented at 8:35 AM on September 7, 2011: member

ACK
gavinandresen referenced this in commit f92f022eda on Sep 7, 2011
gavinandresen merged this on Sep 7, 2011
gavinandresen closed this on Sep 7, 2011
coblee referenced this in commit bdbe1819ce on Jul 17, 2012
ptschip referenced this in commit 9a8cbaba99 on May 2, 2017
MarcoFalke referenced this in commit 6899ef3f0e on Jun 11, 2019
sidhujag referenced this in commit 0d7bb5013b on Jun 11, 2019
kallewoof referenced this in commit 1dfef27322 on Oct 4, 2019
rajarshimaitra referenced this in commit f040632e53 on Aug 5, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

gavinandresen

TheBlueMatt

sipa

Linked (view graph)

#16143 tests: Mark unit test blockfilter_index_initial_sync as non-deterministic