Fixes #15980.
Hold cs_main when reading chainActive (via ::ChainActive()) in RewindBlockIndex.
validation: Hold cs_main when reading chainActive in RewindBlockIndex #16015
pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:missing-cs_main-in-RewindBlockIndex changing 1 files +1 −0-
practicalswift commented at 7:55 PM on May 12, 2019: contributor
- fanquake added the label Validation on May 12, 2019
-
jamesob commented at 3:11 AM on May 15, 2019: member
utACK https://github.com/bitcoin/bitcoin/pull/16015/commits/7232a23949e7dc934f2b1d6dce888f4f51c1ce93
Is there going to be a wider effort to find all instances of unguarded
chainActiveuse? -
practicalswift commented at 5:04 AM on May 15, 2019: contributor
@jamesob Yes, I'll work on that :-)
-
MarcoFalke commented at 11:41 AM on May 15, 2019: member
Why are you using
WITH_LOCK?FlushStateToDiskrequires cs_main anyway, so might as well take the lock for the reminder of the function? -
validation: Hold cs_main when reading chainActive in RewindBlockIndex 1609809fb2
- practicalswift force-pushed on May 15, 2019
-
practicalswift commented at 1:00 PM on May 15, 2019: contributor
@MarcoFalke OK, no harm in holding
cs_mainfor the remainder of the function: updated accordingly. Please re-review :-) -
promag commented at 1:27 PM on May 15, 2019: member
Why not add the annotation in
::ChainActive()? -
jamesob commented at 1:59 PM on May 15, 2019: member
Why not add the annotation in ::ChainActive()?
I think because as noted earlier, there're various existing violations of that that need to be fixed first.
-
practicalswift commented at 2:19 PM on May 15, 2019: contributor
@promag The end goal is
::ChainActive() EXCLUSIVE_LOCKS_REQUIRED(cs_main), but there are a few violations that must be addressed first. -
promag commented at 2:27 PM on May 15, 2019: member
So why this change in particularly? Are those violations more than adding the lock? If so then those should be submitted first IMO.
utACK anyway.
-
practicalswift commented at 2:47 PM on May 15, 2019: contributor
@promag This is in validation code so it needs extra care in reviewing. I'll take the non-critical cases separately.
-
MarcoFalke commented at 2:49 PM on May 15, 2019: member
The other cases are also validation related, IIRC
-
practicalswift commented at 3:03 PM on May 15, 2019: contributor
@MarcoFalke It's a mixed bag IIRC. Do I have your utACK on this one?
-
MarcoFalke commented at 3:23 PM on May 15, 2019: member
utACK 1609809fb2a4c2ec15b7c26dc328e2e666bd5d57
- MarcoFalke referenced this in commit 1c177c3a00 on May 22, 2019
- MarcoFalke merged this on May 22, 2019
- MarcoFalke closed this on May 22, 2019
- sidhujag referenced this in commit 438cefda5f on May 24, 2019
- practicalswift deleted the branch on Apr 10, 2021
- DrahtBot locked this on Aug 16, 2022
