addrman: Add missing lock in Clear() (CAddrMan)

[practicalswift]

Add missing lock in Clear() (CAddrMan).

The variable vRandom is guarded by the mutex cs.

Note to reviewers: Does this look correct? Should the lock cover the entire scope of the method, or should it be limited to cover only std::vector<int>().swap(vRandom);?

[promag]

ACK 3ab545d.

I think all CAddrMan members should be guarded by cs, so LGTM as it is:

bitcoin/src/addrman.h

Lines 184 to 186 in 8335cb4

	private:
	//! critical section to protect the inner data structures
	mutable CCriticalSection cs;

[practicalswift]

Thanks for the quick review @promag!

@promag, you mean that all of the following should be guarded by cs? If so I think we might be missing some additional locks. Let me know and I'll investigate.

    //! last used nId
    int nIdCount;

    //! table with information about all nIds
    std::map<int, CAddrInfo> mapInfo;

    //! find an nId based on its network address
    std::map<CNetAddr, int> mapAddr;

    //! randomly-ordered vector of all nIds
    std::vector<int> vRandom;

    // number of "tried" entries
    int nTried;

    //! list of "tried" buckets
    int vvTried[ADDRMAN_TRIED_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE];

    //! number of (unique) "new" entries
    int nNew;

    //! list of "new" buckets
    int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE];

    //! last time Good was called (memory only)
    int64_t nLastGood;

protected:
    //! secret key to randomize bucket select with
    uint256 nKey;

    //! Source of random numbers for randomization in inner loops
    FastRandomContext insecure_rand;

[promag]

That is what

bitcoin/src/addrman.h

Line 185 in 8335cb4

//! critical section to protect the inner data structures

says. It seems public methods do lock cs.

[practicalswift]

@promag I'll investigate! :-)

[practicalswift]

@promag You're right! The locking seems to be correct for the other CAddrMan members. Thanks!

[ryanofsky]

utACK 3ab545d. CAddrMan::Clear is only called during initialization (during construction and deserialization) so it seems safe to add this lock. It would be good if the critical section comment was clearer about whether a lock is needed to access all class members or just some:

bitcoin/src/addrman.h

Line 185 in 8335cb4

//! critical section to protect the inner data structures

@theuni probably knows more

[theuni]

utACK 3ab545d. I believe the intention is for all public methods to lock the cs, and the privates ones don't need to. That would imply that all vars should be covered under the cs.

[TheBlueMatt]

utACK 3ab545d