Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Apr 25, 2021

Keeping the lock longer than needed is confusing to reviewers and thread analysis. For example, keeping the lock while appending tx-invs, which requires the mempool lock, will tell thread analysis tools an incorrect lock order of (1) m_block_inv_mutex, (2) pool.cs.

@DrahtBot
Copy link
Contributor

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.

@Crypt-iQ
Copy link
Contributor

Crypt-iQ commented May 1, 2021

crACK fac96d0

Makes sense, the lock wasn't being used for the whole scope. Nice improvement!

@jnewbery
Copy link
Contributor

jnewbery commented May 3, 2021

Concept ACK.

Do you mind de-indenting the lines 4455-4581 in a second commit?

@jnewbery
Copy link
Contributor

jnewbery commented May 3, 2021

utACK fac96d0

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Code-Review ACK fac96d0

@maflcko maflcko merged commit b859361 into bitcoin:master May 3, 2021
@maflcko maflcko deleted the 2104-netLockBlockInv branch May 3, 2021 09:15
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 3, 2021
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants