Locking fix for AlreadyHave() #1094

Access to mapTransactions[] must be guarded by cs_mapTransactions lock.

Also, reformat long lines to make the switch statement more readable.

It is unfortunate that AlreadyHave() is called in a loop, implying a lot of lock/release in a row (even if it is likely cached and uncontended).

If LOCK() is moved outside AlreadyHave() and above its callers' loops, that reduces lock/release to one, but holds the lock for MSG_BLOCK in addition to MSG_TX.

How long do the loops run on average/worst-case? Is the runtime of the loops that enclose AlreadyHave dependent on I/O such as network/database?

It looks like AlreadyHave runs very fast (just some hash lookups), however the enclosing loops contains network calls such as PushMessage. I suppose it is undesirable to hold the lock during those?

In that case the current solution is best. It is also the best encapsulation.

ACK. If there is a performance problem arises for the processing of "inv" messages, we can always move the lock to a pre-processing step that filters the received inv list before further processing.

ACK, agreed, safety comes first

Edit: Btw, what's the reason that mapBlockIndex / mapOrphanBlocks don't require a mutex? Are they only ever accessed from one thread?

Actually, I'd prefer the txdb.ContainsTx() call to be outside of the lock, as the db lookup could be slow.

updated