Skip to content

Commit

Permalink
Merge bitcoin#13423: [net] Thread safety annotations in net_processing
Browse files Browse the repository at this point in the history 
1e3bcd2 [net_processing] Add thread safety annotations (Jesse Cohen)
f393a53 Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety analysis (Jesse Cohen)

Pull request description:

  (note that this depends on bitcoin#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
  • Loading branch information
@laanwj @PastaPastaPasta
laanwj authored and PastaPastaPasta committed Apr 15, 2020
1 parent 4e0c627 commit 614a0bb
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 43 deletions.
86 changes: 45 additions & 41 deletions src/net_processing.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,22 +121,20 @@ void EraseOrphansFor(NodeId peer);
// Internal stuff
namespace {
/** 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
* messages or ban them when processing happens afterwards. Protected by
* cs_main.
* messages or ban them when processing happens afterwards.
* Set mapBlockSource[hash].second to false if the node should not be
* 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
* AcceptToMemoryPool. These are not rerequested until the chain tip
* changes, at which point the entire filter is reset. Protected by
* cs_main.
* changes, at which point the entire filter is reset.
*
* Without this filter we'd be re-requesting txs from each of our peers,
* increasing bandwidth consumption considerably. For instance, with 100
Expand All @@ -152,38 +150,38 @@ namespace {
*
* Memory used: 1.3MB
*/
std::unique_ptr<CRollingBloomFilter> recentRejects;
uint256 hashRecentRejectsChainTip;
std::unique_ptr<CRollingBloomFilter> recentRejects GUARDED_BY(cs_main);
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 {
uint256 hash;
const CBlockIndex* pindex; //!< Optional.
bool fValidatedHeaders; //!< Whether this block has validated headers at the time of request.
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 */
std::list<NodeId> lNodesAnnouncingHeaderAndIDs;
std::list<NodeId> lNodesAnnouncingHeaderAndIDs GUARDED_BY(cs_main);

/** 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. */
int nPeersWithValidatedDownloads = 0;
int nPeersWithValidatedDownloads GUARDED_BY(cs_main) = 0;

/** 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. */
std::atomic<int64_t> g_last_tip_update(0);

/** Relay map, protected by cs_main. */
/** Relay map */
typedef std::map<uint256, CTransactionRef> MapRelay;
MapRelay mapRelay;
/** Expiration-time ordered list of (expire time, relay map entry) pairs, protected by cs_main). */
std::deque<std::pair<int64_t, MapRelay::iterator>> vRelayExpiration;
MapRelay mapRelay GUARDED_BY(cs_main);
/** Expiration-time ordered list of (expire time, relay map entry) pairs. */
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

Expand Down Expand Up @@ -384,18 +382,17 @@ struct CNodeState {
unordered_limitedmap<uint256, std::chrono::microseconds, StaticSaltedHasher> g_already_asked_for(MAX_INV_SZ, MAX_INV_SZ * 2);
unordered_limitedmap<uint256, std::chrono::microseconds, StaticSaltedHasher> g_erased_object_requests(MAX_INV_SZ, MAX_INV_SZ * 2);

/** Map maintaining per-node state. Requires cs_main. */
static std::map<NodeId, CNodeState> mapNodeState;
/** Map maintaining per-node state. */
static std::map<NodeId, CNodeState> mapNodeState GUARDED_BY(cs_main);

// Requires cs_main.
static CNodeState *State(NodeId pnode) {
static CNodeState *State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
std::map<NodeId, CNodeState>::iterator it = mapNodeState.find(pnode);
if (it == mapNodeState.end())
return nullptr;
return &it->second;
}

void UpdatePreferredDownload(CNode* node, CNodeState* state)
void UpdatePreferredDownload(CNode* node, CNodeState* state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
nPreferredDownload -= state->fPreferredDownload;

Expand Down Expand Up @@ -440,10 +437,9 @@ void PushNodeVersion(CNode *pnode, CConnman* connman, int64_t nTime)
}
}

// Requires cs_main.
// 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
bool MarkBlockAsReceived(const uint256& hash) {
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);
if (itInFlight != mapBlocksInFlight.end()) {
CNodeState *state = State(itInFlight->second.first);
Expand All @@ -466,10 +462,9 @@ bool MarkBlockAsReceived(const uint256& hash) {
return false;
}

// Requires cs_main.
// 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
bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex *pindex = nullptr, std::list<QueuedBlock>::iterator **pit = nullptr) {
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);
assert(state != nullptr);

Expand Down Expand Up @@ -503,7 +498,7 @@ bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex *
}

/** Check whether the last unknown block a peer advertised is not yet known. */
void ProcessBlockAvailability(NodeId nodeid) {
void ProcessBlockAvailability(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
CNodeState *state = State(nodeid);
assert(state != nullptr);

Expand All @@ -518,7 +513,7 @@ void ProcessBlockAvailability(NodeId nodeid) {
}

/** Update tracking information about which blocks a peer is assumed to have. */
void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) {
void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
CNodeState *state = State(nodeid);
assert(state != nullptr);

Expand All @@ -535,7 +530,8 @@ void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) {
}
}

void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman* connman) {
void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman* connman)
{
AssertLockHeld(cs_main);
CNodeState* nodestate = State(nodeid);
if (!nodestate || !nodestate->fSupportsDesiredCmpctVersion) {
Expand All @@ -551,11 +547,13 @@ void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman* connman) {
}
}
connman->ForNode(nodeid, [connman](CNode* pfrom){
AssertLockHeld(cs_main);
uint64_t nCMPCTBLOCKVersion = 1;
if (lNodesAnnouncingHeaderAndIDs.size() >= 3) {
// As per BIP152, we only get 3 of our peers to announce
// blocks using compact encodings.
connman->ForNode(lNodesAnnouncingHeaderAndIDs.front(), [connman, nCMPCTBLOCKVersion](CNode* pnodeStop){
AssertLockHeld(cs_main);
connman->PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion));
return true;
});
Expand All @@ -568,7 +566,7 @@ void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman* connman) {
}
}

bool TipMayBeStale(const Consensus::Params &consensusParams)
bool TipMayBeStale(const Consensus::Params &consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
AssertLockHeld(cs_main);
if (g_last_tip_update == 0) {
Expand All @@ -577,14 +575,12 @@ bool TipMayBeStale(const Consensus::Params &consensusParams)
return g_last_tip_update < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty();
}

// Requires cs_main
bool CanDirectFetch(const Consensus::Params &consensusParams)
bool CanDirectFetch(const Consensus::Params &consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
return chainActive.Tip()->GetBlockTime() > GetAdjustedTime() - consensusParams.nPowTargetSpacing * 20;
}

// Requires cs_main
bool PeerHasHeader(CNodeState *state, const CBlockIndex *pindex)
bool PeerHasHeader(CNodeState *state, const CBlockIndex *pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
if (state->pindexBestKnownBlock && pindex == state->pindexBestKnownBlock->GetAncestor(pindex->nHeight))
return true;
Expand All @@ -595,7 +591,8 @@ bool PeerHasHeader(CNodeState *state, const CBlockIndex *pindex)

/** Update pindexLastCommonBlock and add not-in-flight missing successors to vBlocks, until it has
* at most count entries. */
void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<const CBlockIndex*>& vBlocks, NodeId& nodeStaller, const Consensus::Params& consensusParams) {
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)
return;

Expand Down Expand Up @@ -1025,8 +1022,10 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphansSize)

void static ProcessOrphanTx(CConnman* connman, std::set<uint256>& orphan_work_set) EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans);

// Requires cs_main.
void Misbehaving(NodeId pnode, int howmuch, const std::string& message)
/**
* Mark a misbehaving peer to be banned depending upon the value of `-banscore`.
*/
void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
if (howmuch == 0)
return;
Expand Down Expand Up @@ -1145,9 +1144,9 @@ 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
static CCriticalSection cs_most_recent_block;
static std::shared_ptr<const CBlock> most_recent_block;
static std::shared_ptr<const CBlockHeaderAndShortTxIDs> most_recent_compact_block;
static uint256 most_recent_block_hash;
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 GUARDED_BY(cs_most_recent_block);
static uint256 most_recent_block_hash GUARDED_BY(cs_most_recent_block);

void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) {
std::shared_ptr<const CBlockHeaderAndShortTxIDs> pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs> (*pblock);
Expand All @@ -1170,6 +1169,7 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std:
}

connman->ForEachNode([this, &pcmpctblock, pindex, &msgMaker, &hashBlock](CNode* pnode) {
AssertLockHeld(cs_main);
// TODO: Avoid the repeated-serialization here
if (pnode->fDisconnect)
return;
Expand Down Expand Up @@ -3739,6 +3739,8 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds)
LOCK(cs_main);

connman->ForEachNode([&](CNode* pnode) {
AssertLockHeld(cs_main);

// Don't disconnect masternodes just because they were slow in block announcement
if (pnode->fMasternode) return;
// Ignore non-outbound peers, or nodes marked for disconnect already
Expand All @@ -3754,6 +3756,8 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds)
});
if (worst_peer != -1) {
bool disconnected = connman->ForNode(worst_peer, [&](CNode *pnode) {
AssertLockHeld(cs_main);

// Only disconnect a peer that has been connected to us for
// some reasonable fraction of our check-frequency, to give
// it time for new information to have arrived.
Expand Down
4 changes: 2 additions & 2 deletions src/sync.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,13 @@ class LOCKABLE AnnotatedMixin : public PARENT
void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
void LeaveCritical();
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 DeleteLock(void* cs);
#else
void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
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 DeleteLock(void* cs) {}
#endif
Expand Down
2 changes: 2 additions & 0 deletions src/threadsafety.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_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 ASSERT_EXCLUSIVE_LOCK(...) __attribute((assert_exclusive_lock(__VA_ARGS__)))
#else
#define LOCKABLE
#define SCOPED_LOCKABLE
Expand All @@ -50,6 +51,7 @@
#define EXCLUSIVE_LOCKS_REQUIRED(...)
#define SHARED_LOCKS_REQUIRED(...)
#define NO_THREAD_SAFETY_ANALYSIS
#define ASSERT_EXCLUSIVE_LOCK(...)
#endif // __GNUC__

#endif // BITCOIN_THREADSAFETY_H

0 comments on commit 614a0bb

Please sign in to comment.