Merge #13423: [net] Thread safety annotations in net_processing

1e3bcd2517 [net_processing] Add thread safety annotations (Jesse Cohen)
f393a533be Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety analysis (Jesse Cohen)

Pull request description:

  (note that this depends on #13417)
  This commit fully annotates all globals in net_processing with clang thread safety annotations. Subsequent commits will begin transitioning some of this data away from cs_main into locks that are local to net_processing. Static thread safety analysis should it easier to verify correctness of that process.

Tree-SHA512: b47aa410cb9ada21072370176aea9a74c575643fa1ee8cf1d43c8e28675eef17f33e5242ac422f840e8178e132ecb58412034c6334b68f1b57c686df80d4e8e2
This commit is contained in:
Wladimir J. van der Laan 2018-07-26 17:19:07 +02:00
commit f58674a20a
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
3 changed files with 50 additions and 47 deletions

View file

@ -76,7 +76,7 @@ std::map<uint256, COrphanTx> mapOrphanTransactions GUARDED_BY(g_cs_orphans);
void EraseOrphansFor(NodeId peer); void EraseOrphansFor(NodeId peer);
/** Increase a node's misbehavior score. */ /** Increase a node's misbehavior score. */
void Misbehaving(NodeId nodeid, int howmuch, const std::string& message=""); void Misbehaving(NodeId nodeid, int howmuch, const std::string& message="") EXCLUSIVE_LOCKS_REQUIRED(cs_main);
/** Average delay between local address broadcasts in seconds. */ /** Average delay between local address broadcasts in seconds. */
static constexpr unsigned int AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL = 24 * 60 * 60; static constexpr unsigned int AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL = 24 * 60 * 60;
@ -96,22 +96,20 @@ static constexpr unsigned int MAX_FEEFILTER_CHANGE_DELAY = 5 * 60;
// Internal stuff // Internal stuff
namespace { namespace {
/** Number of nodes with fSyncStarted. */ /** Number of nodes with fSyncStarted. */
int nSyncStarted = 0; int nSyncStarted GUARDED_BY(cs_main) = 0;
/** /**
* Sources of received blocks, saved to be able to send them reject * Sources of received blocks, saved to be able to send them reject
* messages or ban them when processing happens afterwards. Protected by * messages or ban them when processing happens afterwards.
* cs_main.
* Set mapBlockSource[hash].second to false if the node should not be * Set mapBlockSource[hash].second to false if the node should not be
* punished if the block is invalid. * punished if the block is invalid.
*/ */
std::map<uint256, std::pair<NodeId, bool>> mapBlockSource; std::map<uint256, std::pair<NodeId, bool>> mapBlockSource GUARDED_BY(cs_main);
/** /**
* Filter for transactions that were recently rejected by * Filter for transactions that were recently rejected by
* AcceptToMemoryPool. These are not rerequested until the chain tip * AcceptToMemoryPool. These are not rerequested until the chain tip
* changes, at which point the entire filter is reset. Protected by * changes, at which point the entire filter is reset.
* cs_main.
* *
* Without this filter we'd be re-requesting txs from each of our peers, * Without this filter we'd be re-requesting txs from each of our peers,
* increasing bandwidth consumption considerably. For instance, with 100 * increasing bandwidth consumption considerably. For instance, with 100
@ -127,38 +125,38 @@ namespace {
* *
* Memory used: 1.3 MB * Memory used: 1.3 MB
*/ */
std::unique_ptr<CRollingBloomFilter> recentRejects; std::unique_ptr<CRollingBloomFilter> recentRejects GUARDED_BY(cs_main);
uint256 hashRecentRejectsChainTip; uint256 hashRecentRejectsChainTip GUARDED_BY(cs_main);
/** Blocks that are in flight, and that are in the queue to be downloaded. Protected by cs_main. */ /** Blocks that are in flight, and that are in the queue to be downloaded. */
struct QueuedBlock { struct QueuedBlock {
uint256 hash; uint256 hash;
const CBlockIndex* pindex; //!< Optional. const CBlockIndex* pindex; //!< Optional.
bool fValidatedHeaders; //!< Whether this block has validated headers at the time of request. bool fValidatedHeaders; //!< Whether this block has validated headers at the time of request.
std::unique_ptr<PartiallyDownloadedBlock> partialBlock; //!< Optional, used for CMPCTBLOCK downloads std::unique_ptr<PartiallyDownloadedBlock> partialBlock; //!< Optional, used for CMPCTBLOCK downloads
}; };
std::map<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator> > mapBlocksInFlight; std::map<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator> > mapBlocksInFlight GUARDED_BY(cs_main);
/** Stack of nodes which we have set to announce using compact blocks */ /** Stack of nodes which we have set to announce using compact blocks */
std::list<NodeId> lNodesAnnouncingHeaderAndIDs; std::list<NodeId> lNodesAnnouncingHeaderAndIDs GUARDED_BY(cs_main);
/** Number of preferable block download peers. */ /** Number of preferable block download peers. */
int nPreferredDownload = 0; int nPreferredDownload GUARDED_BY(cs_main) = 0;
/** Number of peers from which we're downloading blocks. */ /** Number of peers from which we're downloading blocks. */
int nPeersWithValidatedDownloads = 0; int nPeersWithValidatedDownloads GUARDED_BY(cs_main) = 0;
/** Number of outbound peers with m_chain_sync.m_protect. */ /** Number of outbound peers with m_chain_sync.m_protect. */
int g_outbound_peers_with_protect_from_disconnect = 0; int g_outbound_peers_with_protect_from_disconnect GUARDED_BY(cs_main) = 0;
/** When our tip was last updated. */ /** When our tip was last updated. */
std::atomic<int64_t> g_last_tip_update(0); std::atomic<int64_t> g_last_tip_update(0);
/** Relay map, protected by cs_main. */ /** Relay map */
typedef std::map<uint256, CTransactionRef> MapRelay; typedef std::map<uint256, CTransactionRef> MapRelay;
MapRelay mapRelay; MapRelay mapRelay GUARDED_BY(cs_main);
/** Expiration-time ordered list of (expire time, relay map entry) pairs, protected by cs_main). */ /** Expiration-time ordered list of (expire time, relay map entry) pairs. */
std::deque<std::pair<int64_t, MapRelay::iterator>> vRelayExpiration; std::deque<std::pair<int64_t, MapRelay::iterator>> vRelayExpiration GUARDED_BY(cs_main);
std::atomic<int64_t> nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block std::atomic<int64_t> nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block
@ -302,18 +300,17 @@ struct CNodeState {
} }
}; };
/** Map maintaining per-node state. Requires cs_main. */ /** Map maintaining per-node state. */
static std::map<NodeId, CNodeState> mapNodeState; static std::map<NodeId, CNodeState> mapNodeState GUARDED_BY(cs_main);
// Requires cs_main. static CNodeState *State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
static CNodeState *State(NodeId pnode) {
std::map<NodeId, CNodeState>::iterator it = mapNodeState.find(pnode); std::map<NodeId, CNodeState>::iterator it = mapNodeState.find(pnode);
if (it == mapNodeState.end()) if (it == mapNodeState.end())
return nullptr; return nullptr;
return &it->second; return &it->second;
} }
static void UpdatePreferredDownload(CNode* node, CNodeState* state) static void UpdatePreferredDownload(CNode* node, CNodeState* state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{ {
nPreferredDownload -= state->fPreferredDownload; nPreferredDownload -= state->fPreferredDownload;
@ -344,10 +341,9 @@ static void PushNodeVersion(CNode *pnode, CConnman* connman, int64_t nTime)
} }
} }
// Requires cs_main.
// Returns a bool indicating whether we requested this block. // Returns a bool indicating whether we requested this block.
// Also used if a block was /not/ received and timed out or started with another peer // Also used if a block was /not/ received and timed out or started with another peer
static bool MarkBlockAsReceived(const uint256& hash) { static bool MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
std::map<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash); std::map<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash);
if (itInFlight != mapBlocksInFlight.end()) { if (itInFlight != mapBlocksInFlight.end()) {
CNodeState *state = State(itInFlight->second.first); CNodeState *state = State(itInFlight->second.first);
@ -370,10 +366,9 @@ static bool MarkBlockAsReceived(const uint256& hash) {
return false; return false;
} }
// Requires cs_main.
// returns false, still setting pit, if the block was already in flight from the same peer // returns false, still setting pit, if the block was already in flight from the same peer
// pit will only be valid as long as the same cs_main lock is being held // pit will only be valid as long as the same cs_main lock is being held
static bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex = nullptr, std::list<QueuedBlock>::iterator** pit = nullptr) { static bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex = nullptr, std::list<QueuedBlock>::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
CNodeState *state = State(nodeid); CNodeState *state = State(nodeid);
assert(state != nullptr); assert(state != nullptr);
@ -407,7 +402,7 @@ static bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlock
} }
/** Check whether the last unknown block a peer advertised is not yet known. */ /** Check whether the last unknown block a peer advertised is not yet known. */
static void ProcessBlockAvailability(NodeId nodeid) { static void ProcessBlockAvailability(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
CNodeState *state = State(nodeid); CNodeState *state = State(nodeid);
assert(state != nullptr); assert(state != nullptr);
@ -423,7 +418,7 @@ static void ProcessBlockAvailability(NodeId nodeid) {
} }
/** Update tracking information about which blocks a peer is assumed to have. */ /** Update tracking information about which blocks a peer is assumed to have. */
static void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) { static void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
CNodeState *state = State(nodeid); CNodeState *state = State(nodeid);
assert(state != nullptr); assert(state != nullptr);
@ -447,7 +442,8 @@ static void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) {
* lNodesAnnouncingHeaderAndIDs, and keeping that list under a certain size by * lNodesAnnouncingHeaderAndIDs, and keeping that list under a certain size by
* removing the first element if necessary. * removing the first element if necessary.
*/ */
static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman* connman) { static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman* connman)
{
AssertLockHeld(cs_main); AssertLockHeld(cs_main);
CNodeState* nodestate = State(nodeid); CNodeState* nodestate = State(nodeid);
if (!nodestate || !nodestate->fSupportsDesiredCmpctVersion) { if (!nodestate || !nodestate->fSupportsDesiredCmpctVersion) {
@ -463,11 +459,13 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman* connma
} }
} }
connman->ForNode(nodeid, [connman](CNode* pfrom){ connman->ForNode(nodeid, [connman](CNode* pfrom){
AssertLockHeld(cs_main);
uint64_t nCMPCTBLOCKVersion = (pfrom->GetLocalServices() & NODE_WITNESS) ? 2 : 1; uint64_t nCMPCTBLOCKVersion = (pfrom->GetLocalServices() & NODE_WITNESS) ? 2 : 1;
if (lNodesAnnouncingHeaderAndIDs.size() >= 3) { if (lNodesAnnouncingHeaderAndIDs.size() >= 3) {
// As per BIP152, we only get 3 of our peers to announce // As per BIP152, we only get 3 of our peers to announce
// blocks using compact encodings. // blocks using compact encodings.
connman->ForNode(lNodesAnnouncingHeaderAndIDs.front(), [connman, nCMPCTBLOCKVersion](CNode* pnodeStop){ connman->ForNode(lNodesAnnouncingHeaderAndIDs.front(), [connman, nCMPCTBLOCKVersion](CNode* pnodeStop){
AssertLockHeld(cs_main);
connman->PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion)); connman->PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion));
return true; return true;
}); });
@ -480,7 +478,7 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman* connma
} }
} }
static bool TipMayBeStale(const Consensus::Params &consensusParams) static bool TipMayBeStale(const Consensus::Params &consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{ {
AssertLockHeld(cs_main); AssertLockHeld(cs_main);
if (g_last_tip_update == 0) { if (g_last_tip_update == 0) {
@ -489,14 +487,12 @@ static bool TipMayBeStale(const Consensus::Params &consensusParams)
return g_last_tip_update < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty(); return g_last_tip_update < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty();
} }
// Requires cs_main static bool CanDirectFetch(const Consensus::Params &consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
static bool CanDirectFetch(const Consensus::Params &consensusParams)
{ {
return chainActive.Tip()->GetBlockTime() > GetAdjustedTime() - consensusParams.nPowTargetSpacing * 20; return chainActive.Tip()->GetBlockTime() > GetAdjustedTime() - consensusParams.nPowTargetSpacing * 20;
} }
// Requires cs_main static bool PeerHasHeader(CNodeState *state, const CBlockIndex *pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
static bool PeerHasHeader(CNodeState *state, const CBlockIndex *pindex)
{ {
if (state->pindexBestKnownBlock && pindex == state->pindexBestKnownBlock->GetAncestor(pindex->nHeight)) if (state->pindexBestKnownBlock && pindex == state->pindexBestKnownBlock->GetAncestor(pindex->nHeight))
return true; return true;
@ -507,7 +503,8 @@ static bool PeerHasHeader(CNodeState *state, const CBlockIndex *pindex)
/** Update pindexLastCommonBlock and add not-in-flight missing successors to vBlocks, until it has /** Update pindexLastCommonBlock and add not-in-flight missing successors to vBlocks, until it has
* at most count entries. */ * at most count entries. */
static void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<const CBlockIndex*>& vBlocks, NodeId& nodeStaller, const Consensus::Params& consensusParams) { static void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<const CBlockIndex*>& vBlocks, NodeId& nodeStaller, const Consensus::Params& consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
if (count == 0) if (count == 0)
return; return;
@ -797,10 +794,8 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans)
/** /**
* Mark a misbehaving peer to be banned depending upon the value of `-banscore`. * Mark a misbehaving peer to be banned depending upon the value of `-banscore`.
*
* Requires cs_main.
*/ */
void Misbehaving(NodeId pnode, int howmuch, const std::string& message) void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{ {
if (howmuch == 0) if (howmuch == 0)
return; return;
@ -898,10 +893,10 @@ void PeerLogicValidation::BlockConnected(const std::shared_ptr<const CBlock>& pb
// All of the following cache a recent block, and are protected by cs_most_recent_block // All of the following cache a recent block, and are protected by cs_most_recent_block
static CCriticalSection cs_most_recent_block; static CCriticalSection cs_most_recent_block;
static std::shared_ptr<const CBlock> most_recent_block; static std::shared_ptr<const CBlock> most_recent_block GUARDED_BY(cs_most_recent_block);
static std::shared_ptr<const CBlockHeaderAndShortTxIDs> most_recent_compact_block; static std::shared_ptr<const CBlockHeaderAndShortTxIDs> most_recent_compact_block GUARDED_BY(cs_most_recent_block);
static uint256 most_recent_block_hash; static uint256 most_recent_block_hash GUARDED_BY(cs_most_recent_block);
static bool fWitnessesPresentInMostRecentCompactBlock; static bool fWitnessesPresentInMostRecentCompactBlock GUARDED_BY(cs_most_recent_block);
/** /**
* Maintain state about the best-seen block and fast-announce a compact block * Maintain state about the best-seen block and fast-announce a compact block
@ -930,6 +925,8 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std:
} }
connman->ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) { connman->ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) {
AssertLockHeld(cs_main);
// TODO: Avoid the repeated-serialization here // TODO: Avoid the repeated-serialization here
if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect) if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect)
return; return;
@ -1327,7 +1324,7 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm
} }
} }
static uint32_t GetFetchFlags(CNode* pfrom) { static uint32_t GetFetchFlags(CNode* pfrom) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
uint32_t nFetchFlags = 0; uint32_t nFetchFlags = 0;
if ((pfrom->GetLocalServices() & NODE_WITNESS) && State(pfrom->GetId())->fHaveWitness) { if ((pfrom->GetLocalServices() & NODE_WITNESS) && State(pfrom->GetId())->fHaveWitness) {
nFetchFlags |= MSG_WITNESS_FLAG; nFetchFlags |= MSG_WITNESS_FLAG;
@ -3160,6 +3157,8 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds)
LOCK(cs_main); LOCK(cs_main);
connman->ForEachNode([&](CNode* pnode) { connman->ForEachNode([&](CNode* pnode) {
AssertLockHeld(cs_main);
// Ignore non-outbound peers, or nodes marked for disconnect already // Ignore non-outbound peers, or nodes marked for disconnect already
if (!IsOutboundDisconnectionCandidate(pnode) || pnode->fDisconnect) return; if (!IsOutboundDisconnectionCandidate(pnode) || pnode->fDisconnect) return;
CNodeState *state = State(pnode->GetId()); CNodeState *state = State(pnode->GetId());
@ -3173,6 +3172,8 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds)
}); });
if (worst_peer != -1) { if (worst_peer != -1) {
bool disconnected = connman->ForNode(worst_peer, [&](CNode *pnode) { bool disconnected = connman->ForNode(worst_peer, [&](CNode *pnode) {
AssertLockHeld(cs_main);
// Only disconnect a peer that has been connected to us for // Only disconnect a peer that has been connected to us for
// some reasonable fraction of our check-frequency, to give // some reasonable fraction of our check-frequency, to give
// it time for new information to have arrived. // it time for new information to have arrived.

View file

@ -74,13 +74,13 @@ public:
void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false); void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
void LeaveCritical(); void LeaveCritical();
std::string LocksHeld(); std::string LocksHeld();
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs); void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs);
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs); void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
void DeleteLock(void* cs); void DeleteLock(void* cs);
#else #else
void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {} void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
void static inline LeaveCritical() {} void static inline LeaveCritical() {}
void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {} void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
void static inline AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {} void static inline AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
void static inline DeleteLock(void* cs) {} void static inline DeleteLock(void* cs) {}
#endif #endif

View file

@ -31,6 +31,7 @@
#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__))) #define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
#define SHARED_LOCKS_REQUIRED(...) __attribute__((shared_locks_required(__VA_ARGS__))) #define SHARED_LOCKS_REQUIRED(...) __attribute__((shared_locks_required(__VA_ARGS__)))
#define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis)) #define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis))
#define ASSERT_EXCLUSIVE_LOCK(...) __attribute((assert_exclusive_lock(__VA_ARGS__)))
#else #else
#define LOCKABLE #define LOCKABLE
#define SCOPED_LOCKABLE #define SCOPED_LOCKABLE
@ -50,6 +51,7 @@
#define EXCLUSIVE_LOCKS_REQUIRED(...) #define EXCLUSIVE_LOCKS_REQUIRED(...)
#define SHARED_LOCKS_REQUIRED(...) #define SHARED_LOCKS_REQUIRED(...)
#define NO_THREAD_SAFETY_ANALYSIS #define NO_THREAD_SAFETY_ANALYSIS
#define ASSERT_EXCLUSIVE_LOCK(...)
#endif // __GNUC__ #endif // __GNUC__
#endif // BITCOIN_THREADSAFETY_H #endif // BITCOIN_THREADSAFETY_H