Skip to content

net: Add missing cs_vNodes lock #18458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 1 commit into from
Apr 6, 2020
Merged

net: Add missing cs_vNodes lock #18458

laanwj merged 1 commit into from
Apr 6, 2020

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Mar 28, 2020

Fixes #18457

@DrahtBot DrahtBot added the P2P label Mar 28, 2020
@laanwj laanwj added this to the 0.20.0 milestone Mar 28, 2020
laanwj
laanwj reviewed Mar 28, 2020
View reviewed changes
@practicalswift
Copy link
Contributor

practicalswift commented Mar 29, 2020

Concept ACK

Very nice to see a NO_THREAD_SAFETY_ANALYSIS annotation go :)

FWIW:

$ git grep -E "^[^/]*NO_THREAD_SAFETY_ANALYSIS" -- ":(exclude)src/leveldb/" ":(exclude)src/threadsafety.h"
src/net.h:    void Stop() NO_THREAD_SAFETY_ANALYSIS;
src/wallet/wallet.h:    CAmount GetAvailableCredit(bool fUseCache = true, const isminefilter& filter = ISMINE_SPENDABLE) const NO_THREAD_SAFETY_ANALYSIS;
src/wallet/wallet.h:    std::set<uint256> GetConflicts() const NO_THREAD_SAFETY_ANALYSIS;
src/wallet/wallet.h:    int GetDepthInMainChain() const NO_THREAD_SAFETY_ANALYSIS;

@maflcko
Copy link
Member Author

maflcko commented Mar 29, 2020

Fixed circular dependency as requested by @laanwj and @sipa

@maflcko maflcko force-pushed the 2003-netLock branch 2 times, most recently from fa75ce8 to fa2a6b6 Compare March 29, 2020 14:24
promag
promag reviewed Mar 29, 2020
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK.

How about split CConnman::Stop? In Shutdown(NodeContext& node) we would then have:

if (node.connman) {
    node.connman->BeforeStop();
    LOCK2(::cs_main, ::g_cs_orphans);
    node.connman->Stop();
}

You could also add annotation to Stop.

@maflcko
Copy link
Member Author

maflcko commented Mar 29, 2020

Took suggestion by @promag

You could also add annotation to Stop.

No, that would be incorrect. The locks are not needed for the correct functionality of the connection manager itself. They are only needed when the connection manager is used in the context of a full node that also runs net processing and a scheduler. I think init.cpp is the best place to document this, since that is the place where the full node gets pieced together.

@promag
Copy link
Contributor

promag commented Mar 29, 2020

Code review ACK fa36965.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 29, 2020 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced Mar 30, 2020
Closed
Closed
@maflcko
Copy link
Member Author

maflcko commented Apr 2, 2020

Would be nice to get one or two more ACKs on this

@promag
Copy link
Contributor

promag commented Apr 2, 2020

ACK ACK, you are welcome.

@laanwj
Copy link
Member

laanwj commented Apr 6, 2020

ACK fa36965

@laanwj laanwj merged commit c31bcaf into bitcoin:master Apr 6, 2020
@maflcko maflcko deleted the 2003-netLock branch April 6, 2020 19:42
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 8, 2020
@laanwj
Merge bitcoin#18458: net: Add missing cs_vNodes lock
81f3ca7 
fa36965 net: Add missing cs_vNodes lock (MarcoFalke)

Pull request description:

  Fixes bitcoin#18457

ACKs for top commit:
  promag:
    Code review ACK fa36965.
  laanwj:
    ACK fa36965

Tree-SHA512: 60d7000f2f3d480bb0953ce27a0020763e7102da16a0006b619e0a236cfc33cbd4f83d870e9f0546639711cd877c1f9808d419184bbc153bb328885417e0066c
This was referenced Apr 24, 2020
Closed
Merged
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 18, 2020
@Fabcien
net: Add missing cs_vNodes lock
781244d 
Summary: Backport of core [[bitcoin/bitcoin#18458 | PR18458]].

Test Plan:
With Clang as a compiler:
  cmake -GNinja .. -DCMAKE_BUILD_TYPE=Debug
  ninja all check-all

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D8443
@jnewbery jnewbery mentioned this pull request Apr 13, 2021
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

@sipa sipa sipa left review comments

+1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

0.20.0

Development

Successfully merging this pull request may close these issues.

net: Missing lock on cs_vNodes on shutdown

6 participants

@maflcko @practicalswift @promag @DrahtBot @laanwj @sipa