[refactor] interfaces: Add missing LockAnnotation for cs_main by maflcko · Pull Request #15855 · bitcoin/bitcoin

maflcko · 2019-04-19T16:40:53Z

This adds missing LockAnnotation lock(::cs_main); to src/interfaces/chain.cpp (as well as tests and benchmarks)

DrahtBot · 2019-04-19T17:43:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
#15921 (Tidy up ValidationState interface by jnewbery)

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.

promag

Concept ACK.

src/wallet/test/wallet_tests.cpp

src/test/txvalidationcache_tests.cpp

maflcko · 2019-04-22T13:08:55Z

Addressed feedback by @promag

practicalswift · 2019-05-07T16:30:36Z

Concept ACK

promag · 2019-05-13T15:04:24Z

utACK face05a080c081fc426ff34ecb39ac072710034b.

src/wallet/test/wallet_tests.cpp

practicalswift · 2019-05-13T18:30:31Z

@MarcoFalke The added comment makes it easier to understand for human reviewers. That is good, but I still think we would be better off if we used different annotations for the "opt-out of analysis" and "give the compiler a locking guarantee when it fails to understand our code" use cases:

When analysing locks using static analysis etc I want to be able to let LockAnnotation imply the stronger "AssertLockHeld(…)" (instead of the weaker NO_THREAD_SAFETY_ANALYSIS).

This change breaks that implication.

Perhaps --enable-debug builds should do AssertLockHeld(…) as part of the LockAnnotationctor to make sure the LockAnnotation guarantees hold up over time? The opt-out variant shouldn't have that (obviously) :-)

maflcko · 2019-05-13T18:47:31Z

Perhaps --enable-debug builds should do AssertLockHeld(…) as part of the LockAnnotationctor to make sure the LockAnnotation guarantees hold up over time

Sounds good. I have addressed your concerns, but this suggestion should go into a follow-up pull request?

ryanofsky

utACK fa3c651

practicalswift · 2019-05-14T05:25:09Z

utACK fa3c651

…s_main fa3c651 [refactor] interfaces: Add missing LockAnnotation for cs_main (MarcoFalke) Pull request description: This adds missing `LockAnnotation lock(::cs_main);` to `src/interfaces/chain.cpp` (as well as tests and benchmarks) ACKs for commit fa3c65: practicalswift: utACK fa3c651 ryanofsky: utACK fa3c651 Tree-SHA512: b67082fe3718c94b4addf7f2530593915225c25080f20c3ffa4ff7e08f1f49548f255fb285f89a8feff84be3f6c91e1792495ced9f6bf396732396d1356d597a

…r cs_main Summary: bitcoin/bitcoin@fa3c651 --- Depends on D6260 Backport of Core [[bitcoin/bitcoin#15855 | PR15855]] Test Plan: cmake .. -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DENABLE_WERROR=ON -DENABLE_SANITIZERS=thread ninja check-all ninja ./src/bench/bitcoin-bench Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6259

* merge bitcoin#15855: Add missing LockAnnotation for cs_main * mutex: update cs_main locks, assertions and annotations This commit is a squash between 8c98823 and 90d0535

…nnotation for cs_main fa3c651 [refactor] interfaces: Add missing LockAnnotation for cs_main (MarcoFalke) Pull request description: This adds missing `LockAnnotation lock(::cs_main);` to `src/interfaces/chain.cpp` (as well as tests and benchmarks) ACKs for commit fa3c65: practicalswift: utACK fa3c651 ryanofsky: utACK fa3c651 Tree-SHA512: b67082fe3718c94b4addf7f2530593915225c25080f20c3ffa4ff7e08f1f49548f255fb285f89a8feff84be3f6c91e1792495ced9f6bf396732396d1356d597a

…4530) * merge bitcoin#15855: Add missing LockAnnotation for cs_main * mutex: update cs_main locks, assertions and annotations This commit is a squash between 8c98823 and 90d0535

…nnotation for cs_main fa3c651 [refactor] interfaces: Add missing LockAnnotation for cs_main (MarcoFalke) Pull request description: This adds missing `LockAnnotation lock(::cs_main);` to `src/interfaces/chain.cpp` (as well as tests and benchmarks) ACKs for commit fa3c65: practicalswift: utACK fa3c651 ryanofsky: utACK fa3c651 Tree-SHA512: b67082fe3718c94b4addf7f2530593915225c25080f20c3ffa4ff7e08f1f49548f255fb285f89a8feff84be3f6c91e1792495ced9f6bf396732396d1356d597a

maflcko added the Refactoring label Apr 19, 2019

maflcko force-pushed the 1904-interfacesLock branch from fad8bc2 to fa0a3ba Compare April 19, 2019 17:18

maflcko force-pushed the 1904-interfacesLock branch from fa0a3ba to fa90b1a Compare April 19, 2019 17:46

promag reviewed Apr 21, 2019

View reviewed changes

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved

src/test/txvalidationcache_tests.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 1904-interfacesLock branch from fa90b1a to fa7a61d Compare April 22, 2019 13:06

maflcko force-pushed the 1904-interfacesLock branch 2 times, most recently from bab981d to fab495f Compare May 1, 2019 19:10

DrahtBot added the Needs rebase label May 7, 2019

maflcko force-pushed the 1904-interfacesLock branch from fab495f to fa2633e Compare May 7, 2019 16:29

maflcko force-pushed the 1904-interfacesLock branch from fa2633e to fa58deb Compare May 7, 2019 16:31

DrahtBot removed the Needs rebase label May 7, 2019

maflcko force-pushed the 1904-interfacesLock branch 2 times, most recently from 5b0d796 to face05a Compare May 10, 2019 20:48

practicalswift reviewed May 13, 2019

View reviewed changes

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 1904-interfacesLock branch from face05a to fad1d26 Compare May 13, 2019 17:33

[refactor] interfaces: Add missing LockAnnotation for cs_main

fa3c651

maflcko force-pushed the 1904-interfacesLock branch from fad1d26 to fa3c651 Compare May 13, 2019 18:47

ryanofsky approved these changes May 13, 2019

View reviewed changes

maflcko merged commit fa3c651 into bitcoin:master May 14, 2019

maflcko deleted the 1904-interfacesLock branch May 14, 2019 12:27

UdjinM6 mentioned this pull request Jan 22, 2020

Backports 0.16 pr5 dashpay/dash#3300

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Oct 24, 2021

merge bitcoin#15855: Add missing LockAnnotation for cs_main

07ee416

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] interfaces: Add missing LockAnnotation for cs_main#15855

[refactor] interfaces: Add missing LockAnnotation for cs_main#15855
maflcko merged 1 commit intobitcoin:masterfrom
maflcko:1904-interfacesLock

maflcko commented Apr 19, 2019

Uh oh!

DrahtBot commented Apr 19, 2019 •

edited

Loading

Uh oh!

promag left a comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Apr 22, 2019

Uh oh!

practicalswift commented May 7, 2019

Uh oh!

promag commented May 13, 2019

Uh oh!

Uh oh!

practicalswift commented May 13, 2019 •

edited

Loading

Uh oh!

maflcko commented May 13, 2019

Uh oh!

ryanofsky left a comment

Uh oh!

practicalswift commented May 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

maflcko commented Apr 19, 2019

Uh oh!

DrahtBot commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Apr 22, 2019

Uh oh!

practicalswift commented May 7, 2019

Uh oh!

promag commented May 13, 2019

Uh oh!

Uh oh!

practicalswift commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 13, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented May 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DrahtBot commented Apr 19, 2019 •

edited

Loading

practicalswift commented May 13, 2019 •

edited

Loading