Assert cs_main is held when retrieving node state #11515

pull promag wants to merge 1 commits into bitcoin:master from promag:201710-node-state-guard changing 1 files +5 −4

promag commented at 2:10 PM on October 17, 2017: member

Replaces a comment with the actual assertion.
fanquake added the label P2P on Oct 17, 2017
promag force-pushed on Oct 17, 2017
practicalswift commented at 2:42 PM on October 17, 2017: contributor

@promag This change is already in https://github.com/bitcoin/bitcoin/pull/11226/commits/4c8604e6607114fd06b9be210a25b208e9d64821 which is part of #11226 :-)
promag force-pushed on Oct 18, 2017
promag commented at 9:49 AM on October 18, 2017: member

@practicalswift removed GUARDED_BY, but I'm keeping the AssertLockHeld.
promag force-pushed on Oct 18, 2017
TheBlueMatt commented at 6:57 PM on November 6, 2017: member

Fails travis as DoS_Tests calls State() without cs_main.
MarcoFalke commented at 5:56 PM on November 7, 2017: member

@promag ping
practicalswift commented at 6:03 PM on November 7, 2017: contributor

Locking added to tests in #11623 :-)
MarcoFalke commented at 5:39 PM on November 10, 2017: member

utACK afd5cd588a994ad067862652beaebe78a8d35381
TheBlueMatt commented at 5:44 PM on November 10, 2017: member

I dont know if we want to be adding new AssertLockHelds, actually. We should be adding GUARDED_BY annotations as we go, and I think just doing that here may be nice (cause merging them all in one huge #11226 probably isn't gonna happen), but AssertLockHelds dont add much of anything if we're already checking it at compile/travis-time.
laanwj commented at 9:01 AM on December 12, 2017: member

@promag Can you add the annotation please?
promag force-pushed on Dec 12, 2017
promag force-pushed on Dec 12, 2017
promag commented at 12:13 PM on December 12, 2017: member

Rebased and updated.
practicalswift commented at 3:38 PM on December 12, 2017: contributor

utACK 8f2616cf99d1e0b573f647cd45ee87fba1c969e9

Yay for annotations!
promag commented at 4:47 PM on December 12, 2017: member

Adding the annotations breaks the build, which #11226 already takes care of. Rebasing on top of that results only in the assert, which is what was initially. @laanwj wdyt?
TheBlueMatt commented at 7:00 PM on December 12, 2017: member

@promag the annotations are required to be applied recursively. Any function that calls State() must either LOCK(cs_main) prior to the call, or be marked with EXCLUSIVE_LOCKS_REQUIRED(cs_main) itself (and then its callers must LOCK(cs_main)). That's the magic of the new annotations - you get notified by travis for (the simple cases of) missing locks!
Assert cs_main is held when retrieving node state e9762dd245
promag force-pushed on Apr 27, 2018
promag closed this on May 10, 2018
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels