scripted-diff: Remove redundant lock annotations in net processing by maflcko · Pull Request #21188 · bitcoin/bitcoin

maflcko · 2021-02-15T12:43:54Z

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.

Can be reviewed with --word-diff-regex=. -BEGIN VERIFY SCRIPT- sed -i --regexp-extended 's/(PeerManagerImpl::.*\)).*LOCKS_.*\)/\1/g' ./src/net_processing.cpp -END VERIFY SCRIPT-

maflcko · 2021-02-15T12:44:34Z

Fixes #20942 (comment)

maflcko · 2021-02-15T12:45:22Z

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

hebasto

ACK fafddfa

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

maflcko · 2021-02-15T15:19:01Z

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) + ")";

jonatack

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

amitiuttarwar · 2021-02-16T00:22:17Z

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.

maflcko · 2021-02-16T07:27:00Z

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
 {

maflcko · 2021-02-16T07:32:14Z

(Clarified OP and title a bit)

amitiuttarwar · 2021-02-16T19:29:03Z

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.

… in net processing fafddfa scripted-diff: Remove shadowing lock annotations (MarcoFalke) Pull request description: 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. ACKs for top commit: amitiuttarwar: ACK `fafddfadda`, confirmed that the annotations removed were all redundant. confirmed the claim of potential issue :) hebasto: ACK fafddfa jonatack: Light utACK fafddfa verified that the removed annotations in the definitions correspond to those in their respective declarations Tree-SHA512: ea095c6d4e0bedd70d4e2d8a42b06cfd90c161ebfcaac13558c5dc065601a732e5f812f332104b7daa087aa57b8b0242b177799d22eef7628d77d4d87f443bf2

25c57d6 [doc] Add a note about where lock annotations should go. (Amiti Uttarwar) ad5f01b [validation] Move the lock annotation from function definition to declaration (Amiti Uttarwar) Pull request description: Based on reviewing #21188 the first commit switches the lock annotations on `CheckInputScripts` to be on the function declaration instead of on the function definition. this ensures that all call sites are checked, not just ones that come after the definition. the second commit adds a note to the developer-notes section to clarify where the annotations should be applied. ACKs for top commit: MarcoFalke: ACK 25c57d6 🥘 promag: Code review ACK 25c57d6. Tree-SHA512: 61b6ef856bf6c6016d535fbdd19daf57b9e59fe54a1f30d47282a071b9b9d60b2466b044ee57929e0320cb1bdef52e7a1687cacaa27031bbc43d058ffffe22ba

Summary: This errors arise with some clang version and -Wthread-safety-analysis. Introduced in D11436. Note: the annotations are also needlessely duplicated in the function definition and should be removed by backporting bitcoin/bitcoin#21188. Test Plan: With a recent Clang: ninja all check Check it raises no warning. Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D11442

Summary: This is a backport of [[bitcoin/bitcoin#21188 | core#21188]] Bitcoin ABC has one additional occurence of duplicated lock annotations (D9044 for `ProcessOrphanTx`) and two occurences that were already deduplicated (D11435 for `TipMayBeStale` and D11437 for `MaybeSetPeerAsAnnouncingHeaderAndIDs`). Test Plan: With clang: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11443

scripted-diff: Remove shadowing lock annotations

fafddfa

Can be reviewed with --word-diff-regex=. -BEGIN VERIFY SCRIPT- sed -i --regexp-extended 's/(PeerManagerImpl::.*\)).*LOCKS_.*\)/\1/g' ./src/net_processing.cpp -END VERIFY SCRIPT-

DrahtBot added P2P Refactoring labels Feb 15, 2021

hebasto approved these changes Feb 15, 2021

View reviewed changes

jonatack reviewed Feb 15, 2021

View reviewed changes

maflcko changed the title ~~scripted-diff: Remove shadowing lock annotations in net processing~~ scripted-diff: Remove redundant lock annotations in net processing Feb 16, 2021

amitiuttarwar mentioned this pull request Feb 16, 2021

[validation] Two small clang lock annotation improvements #21202

Merged

maflcko merged commit 8639c44 into bitcoin:master Feb 17, 2021

maflcko deleted the 2102-netProcNoShadow branch February 17, 2021 09:01

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

scripted-diff: Remove redundant lock annotations in net processing#21188
maflcko merged 1 commit intobitcoin:masterfrom
maflcko:2102-netProcNoShadow

maflcko commented Feb 15, 2021 •

edited

Loading

Uh oh!

maflcko commented Feb 15, 2021

Uh oh!

maflcko commented Feb 15, 2021

Uh oh!

hebasto left a comment

Uh oh!

maflcko commented Feb 15, 2021

Uh oh!

jonatack left a comment

Uh oh!

amitiuttarwar commented Feb 16, 2021 •

edited

Loading

Uh oh!

maflcko commented Feb 16, 2021

Uh oh!

maflcko commented Feb 16, 2021

Uh oh!

amitiuttarwar commented Feb 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

maflcko commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Feb 15, 2021

Uh oh!

maflcko commented Feb 15, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 15, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

amitiuttarwar commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Feb 16, 2021

Uh oh!

maflcko commented Feb 16, 2021

Uh oh!

amitiuttarwar commented Feb 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maflcko commented Feb 15, 2021 •

edited

Loading

amitiuttarwar commented Feb 16, 2021 •

edited

Loading