scripted-diff: Remove redundant lock annotations in net processing #21188

Would be good to not redundantly copy the lock annotation from the class declaration to the member implementation. Otherwise it may not result in a compile failure if a new lock requirement is added to the member implementation, but not the class declaration.

Fixes #20942 (review)

Too bad the compiler can't error on those by itself

ACK fafddfadda0c77876ba764c5b65ee5fa8e53a5e0

Out of curiosity, is the code base is free from similar issues in other translation units?

I don't think so. This is all my hacky grep could find.

$ git grep --extended-regexp --ignore-case  ' [a-z0-9]+::[a-z0-9]+\([^()]*\).*LOCKS_'
src/net_processing.cpp:bool PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
src/net_processing.cpp:bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex, std::list<QueuedBlock>::iterator** pit) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
src/net_processing.cpp:void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
src/net_processing.cpp:bool PeerManagerImpl::TipMayBeStale() EXCLUSIVE_LOCKS_REQUIRED(cs_main)
src/net_processing.cpp:void PeerManagerImpl::FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<const CBlockIndex*>& vBlocks, NodeId& nodeStaller) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
src/net_processing.cpp:bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
src/net_processing.cpp:CTransactionRef PeerManagerImpl::FindTxForGetData(const CNode& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main)
src/net_processing.cpp:void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(!cs_main, peer.m_getdata_requests_mutex)
src/qt/transactiondesc.cpp:            strHTML += BitcoinUnits::formatHtmlWithUnit(unit, nUnmatured)+ " (" + tr("matures in %n more block(s)", "", status.blocks_to_maturity) + ")";

Light utACK fafddfadda0c77876ba764c5b65ee5fa8e53a5e0 verified that the removed annotations in the definitions correspond to those in their respective declarations

I don't understand the issue. How can I reproduce it?

I interpreted the issue to be that we would not get a compiler warning under the following circumstance:

• header declaration of function FooBar has annotation EXCLUSIVE_LOCKS_REQUIRED(lock1, lock2)
• FooBar definition has annotation EXCLUSIVE_LOCKS_REQUIRED(lock1)
• call site acquires lock1 then calls FooBar
• issue: we do not get a compiler warning that lock2 was not acquired before calling FooBar

I tried it out by doing the following:

• on master, TipMayBeStale() has EXCLUSIVE_LOCKS_REQUIRED(cs_main) on both the function declaration and the function definition
• I added a new mutex RecursiveMutex cs_other; and updated only the declaration of TipMayBeStale() to be EXCLUSIVE_LOCKS_REQUIRED(cs_main, cs_other);
• I did not update the annotations in the definition of TipMayBeStale() or the call site CheckForStaleTipAndEvictPeers.

full diff:

-    bool TipMayBeStale() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    RecursiveMutex cs_other;
+    bool TipMayBeStale() EXCLUSIVE_LOCKS_REQUIRED(cs_main, cs_other);

• I expected that the issue would be that we would not get a compiler warning even though the call site didn't take cs_other.

However, I did get a compiler warning: net_processing.cpp:4284:110: warning: calling function 'TipMayBeStale' requires holding mutex 'cs_other' exclusively [-Wthread-safety-analysis] So, I don't understand the context of what we are trying to prevent / fix.

Ah good catch. Looks like it is not shadowing. Though, I still think the patch is correct. You can test by adding cs_other to the TipMayBeStale implementation. Then move a function that calls TipMayBeStale to before its implementation. Compilation won't print any warning. The diff I used:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index c782498e14..bb5385f8de 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -778,7 +778,33 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) EXCLUS
     }
 }
 
-bool PeerManagerImpl::TipMayBeStale() EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
+{
+    LOCK(cs_main);
+
+    int64_t time_in_seconds = GetTime();
+
+    EvictExtraOutboundPeers(time_in_seconds);
+
+    if (time_in_seconds > m_stale_tip_check_time) {
+        // Check whether our tip is stale, and if so, allow using an extra
+        // outbound peer
+        if (!fImporting && !fReindex && m_connman.GetNetworkActive() && m_connman.GetUseAddrmanOutgoing() && TipMayBeStale()) {
+            LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", time_in_seconds - m_last_tip_update);
+            m_connman.SetTryNewOutboundPeer(true);
+        } else if (m_connman.GetTryNewOutboundPeer()) {
+            m_connman.SetTryNewOutboundPeer(false);
+        }
+        m_stale_tip_check_time = time_in_seconds + STALE_CHECK_INTERVAL;
+    }
+
+    if (!m_initial_sync_finished /*&& CanDirectFetch(m_chainparams.GetConsensus())*/) {
+        m_connman.StartExtraBlockRelayPeers();
+        m_initial_sync_finished = true;
+    }
+}
+
+bool PeerManagerImpl::TipMayBeStale() EXCLUSIVE_LOCKS_REQUIRED(m_recent_confirmed_transactions_mutex)
 {
     AssertLockHeld(cs_main);
     const Consensus::Params& consensusParams = m_chainparams.GetConsensus();
@@ -4269,32 +4295,6 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds)
     }
 }
 
-void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
-{
-    LOCK(cs_main);
-
-    int64_t time_in_seconds = GetTime();
-
-    EvictExtraOutboundPeers(time_in_seconds);
-
-    if (time_in_seconds > m_stale_tip_check_time) {
-        // Check whether our tip is stale, and if so, allow using an extra
-        // outbound peer
-        if (!fImporting && !fReindex && m_connman.GetNetworkActive() && m_connman.GetUseAddrmanOutgoing() && TipMayBeStale()) {
-            LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", time_in_seconds - m_last_tip_update);
-            m_connman.SetTryNewOutboundPeer(true);
-        } else if (m_connman.GetTryNewOutboundPeer()) {
-            m_connman.SetTryNewOutboundPeer(false);
-        }
-        m_stale_tip_check_time = time_in_seconds + STALE_CHECK_INTERVAL;
-    }
-
-    if (!m_initial_sync_finished && CanDirectFetch(m_chainparams.GetConsensus())) {
-        m_connman.StartExtraBlockRelayPeers();
-        m_initial_sync_finished = true;
-    }
-}
-
 namespace {
 class CompareInvMempoolOrder
 {

(Clarified OP and title a bit)

ok, I understand now- if the declaration annotates EXCLUSIVE_LOCKS_REQUIRED(lock1), then the caller invokes, then the implementation annotates EXCLUSIVE_LOCKS_REQUIRED(lock1, lock2), we won't get the compiler warnings about lock2. I was able to see this with your diff, @MarcoFalke. thanks!

ACK fafddfadda, confirmed that the annotations removed were all redundant. confirmed the claim of potential issue :)

I also checked to see if there were any other annotations with this issue. I didn't find any with the same redundancy problem. However, I found that CheckInputScripts has the annotation on the function definition instead of the declaration earlier in the file. I checked that this could lead to a similar issue, so we should change that as well.

I think this clarification is worth adding to the developer-notes, so I'll open a small PR right now. I can add the CheckInputScripts change there since this PR already has 3 ACKs.