Large locking change

A while back I started looking at bitcoin, and I was disappointed with the code quality wrt locking.

I tried to fix the code to be clang -Wthread-safety clean, but I don't know the code enough to understand some of the entanglements that are currently present.

I've added lots of annotations that specifies the locking requirements on functions and data.  This might seem a little overzealous, but I believe that this is necessary to "get back on track" and make the locking strategy clear and consistent.

Even with this patch there are lots of locking issues left - probably benign like things happening during startup and shutdown.   There are also false positives from me not getting clang to grok TRY_LOCK correctly.

Below is a long note I wrote while looking through the source code a while back.  Maybe you guys agree or maybe not - at least it explains the whats and the whys.

---

For an object that is accessed by multiple threads we have, for every field:
- It is const, constant after initialization, or guarded by a mutex.

- Re-using an object is a *bad idea*.  It should never be done. If we
  do, we are mutating the constant-after-initialization fields.  This
  leads to exessive locking and overcomplicated code.  It is a lot
  simpler to discard the object and re-create it.  This works, because
  the object is owned by some other object, and in that other object,
  it is a "guarded by mutex" field.  Thus by re-creating the object,
  we lift the responsibility for locking into the parent.

- A result of the above is that "constructors" must be separate from
  other functions in the class.  Constructors that are not strictly
  C++ constructors, such as deserializing should be static functions
  if possible.  This makes it impossible for them to to be used to
  mutate constant-after-initialization fields.

- Locks should not be exported outside of a class.  Java did this
  mistake and it is considered an anti-pattern.

- Locks should never serialize code, they should protect data from
  being accessed while invariants on that data are broken.

- Locks should be textually close to the fields.  Given a lock, it
  should be easy to understand what it guards, and given a field, it
  should be easy to see which locks guard that field.
  Thus, locks should be private members of a class.

  A good way to obfuscate this is to put a lock in a parent class, as
  done in keystore.h where CKeyStore.  It is hard to know what that
  lock guards.  It is a lot cleaner to duplicate that lock in the data
  section of the subclasses.

- Recursive locks should never be used.  They lead to bugs, and are
  *only* necessary in order to write sloppy code.  If some functionality
  is both a transaction that ensures that the invariants of the data are
  obeyed, and part of larger transactions, then it is much clearer to
  separate these conserns into two functions:

  void DoFoo() {
     LOCK(cs);
     DoFooUnlocked();
  }
  void DoFooUnlocked() { ... }

- Using locks to protect simple setters and getters is often buggy
  and/or complex.  In most cases, mutating some state is part of a
  larger transaction, so the setter is often protected by some other
  lock external to the class.  In these cases, re-building a new
  object is usually the best approach, but otherwise, explicitly
  marking setters as SetFooUnlocked() and not lock is clearer.  It
  makes it explicit that you can't just mutate randomly without doing
  some external locking.

- Do not hold locks over callbacks out of an object.  This means that
  objects should *signal* some state change, and not guarantee which
  state the object is in when a callback happens.  Sometimes, it makes
  sense to break this, but then the callback will have to be treated
  like a signal handler, or irq handler - i.e. it can only do simple
  things and not call out to other random code.

- Every time you call TRY_LOCK, a kitten dies.  The reason why this is
  bad is that it makes reasoning, and testing the code incredibly
  hard.  TRY_LOCK is basically this:

    if (rand() % 1) {
      DoA();
    } else {
      DoB();
    }

  It is simply unnecessary randomness. The TRY_LOCK macro should be
  removed, it kills kittens.

  If something like TRY_LOCK is truly needed, then at least it should
  be abstracted out of the actual functions, so that the functions
  themselves are testable.  Thus in the above case, DoA() would be a
  separate, testable, function.

  In some really large systems (millions of large tps), sharding work
  queues by lock is a reasonable design to reduce resource usage by
  threads (so you do cs_someLock.DoWorkNowOrWhenUnlocked(&DoA), and it
  is more efficient than TRY_LOCK, but for bitcoin - this is just
  overkill.

- Having lots of semantically different locks in a class is not ideal.
  For example CNode has 4 locks.  This means that there are 4 sets of
  invariants that can be established independently of each other in
  this class.  To put it another way, when you call a function on this
  object, it should work on a subset of the state, or it should hold
  all 4 locks.  This suggests that either the class is a composite of
  multiple objects, and should be broken up, or the number of locks
  should be reduced.

- We cannot compose multiple critical sections which all take a lock
  into a large critical section.  After we have released locks, all
  checks that we have done are invalidated.  Most of CWallet is just
  plain wrong and based on this false assumption.  Locking is done in
  CCryptoKeyStore instead of in CWallet.  See above "Locks should be
  textually close to the fields" for a reason why people confuse
  themselves.

- There is a lot of convoluted code like this:
        bool result;
        {
            LOCK(cs_KeyStore);
            result = (mapKeys.count(address) > 0);
        }
        return result;

   which is equivalent to this:

        LOCK(cs_KeyStore);
        return (mapKeys.count(address) > 0);

   The destructor is guaranteed to run after the expression is
   evaluated.  There is no need to over-engineer.

   Similarly, there is a lot of code like this:

    void SomeFunction(...)
    {
        something();
        {
            LOCK(cs_something);
            ...
        }
    }

    or this:
    void SomeFunction(...)
    {
        {
            LOCK(cs_something);
            .. rest of function ...
        }
        return false;
    }

   There is no point in all the excessive blocks.  What are they for?