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

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);?

ACK 3ab545d.

I think all CAddrMan members should be guarded by cs, so LGTM as it is: https://github.com/bitcoin/bitcoin/blob/8335cb478183d800e274f6e96f9d7269ae584220/src/addrman.h#L184-L186

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;

That is what https://github.com/bitcoin/bitcoin/blob/8335cb478183d800e274f6e96f9d7269ae584220/src/addrman.h#L185 says. It seems public methods do lock cs.

@promag I'll investigate! :-)

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

utACK 3ab545d7f8e979e2cabeff6f5f6f1afe94a5c195. 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:

https://github.com/bitcoin/bitcoin/blob/8335cb478183d800e274f6e96f9d7269ae584220/src/addrman.h#L185 @theuni probably knows more

utACK 3ab545d7f8e979e2cabeff6f5f6f1afe94a5c195. 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.

utACK 3ab545d7f8e979e2cabeff6f5f6f1afe94a5c195