chain: Remove CBlockIndex::SetNull helper #17162

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:1910-docChainLocks changing 1 files +23 −49

MarcoFalke commented at 5:22 PM on October 16, 2019: member

The first commit removes the SetNull helper and inlines the member initialization (C++11). See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures for rationale.

<strike>The second commit adds the cs_main lock annotation to RaiseValidity. See also #17161.</strike>
chain: Set all CBlockIndex members to null, remove SetNull helper fa0467326f
MarcoFalke added the label Refactoring on Oct 16, 2019
MarcoFalke added the label Validation on Oct 16, 2019
DrahtBot commented at 5:57 PM on October 16, 2019: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #13875 ([doc] nChainTx needs to become a 64-bit earlier due to SegWit by Sjors)
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.
practicalswift commented at 6:01 PM on October 16, 2019: contributor

ACK faa2cdc58204823fb275905870f7dfee337f80ab -- diff looks correct
in src/chain.h:297 in faa2cdc582 outdated
```
 293 | @@ -317,7 +294,7 @@ class CBlockIndex
 294 |  
 295 |      //! Raise the validity level of this block index entry.
 296 |      //! Returns true if the validity was changed.
 297 | -    bool RaiseValidity(enum BlockStatus nUpTo)
 298 | +    bool RaiseValidity(enum BlockStatus nUpTo) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
```
promag commented at 9:47 AM on October 17, 2019:

faa2cdc58204823fb275905870f7dfee337f80ab

Move this to validation.cpp instead?

static bool RaiseValidity(const CBlockIndex* pindex, enum BlockStatus nUpTo) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
MarcoFalke commented at 12:32 PM on October 17, 2019:

I'll leave it here for now, because someone might want to make nStatus private at some point.

promag commented at 12:42 PM on October 17, 2019:

I think it's preferable to deal with that later and avoid cs_main here now.

MarcoFalke commented at 2:20 PM on October 17, 2019:

This is under cs_main one way or another. You mean it is preferable to not include the sync header here?

promag commented at 2:39 PM on October 17, 2019:

I mean this is only used in validation.cpp. Also, this kind of adds a circular dependency chain.h <-> validation.h (it's a forward declaration but still). I also don't think "this might be private some day" is valid argument. And by moving to validation.cpp you don't need to include <sync.h>.

It was a suggestion, either way looks like cs_main is indeed held.

MarcoFalke commented at 3:13 PM on October 17, 2019:

Removed commit instead. This can be addressed later.
promag commented at 9:49 AM on October 17, 2019: member

Concept ACK. Nice 1st commit, I think next step is to make some member immutable - this requires further refactor in txdb.cpp.
MarcoFalke force-pushed on Oct 17, 2019
MarcoFalke renamed this:
~~chain: Remove CBlockIndex::SetNull helper, add RaiseValidity lock annotation~~
chain: Remove CBlockIndex::SetNull helper
on Oct 17, 2019
MarcoFalke commented at 3:14 PM on October 17, 2019: member

Removed second commit due to controversy
promag commented at 3:19 PM on October 17, 2019: member

Code review ACK fa0467326ff004a0a20c6af9c8f6adcc74a0c8af.
practicalswift commented at 3:25 PM on October 17, 2019: contributor

ACK fa0467326ff004a0a20c6af9c8f6adcc74a0c8af -- diff still looks correct :)
laanwj commented at 6:42 PM on October 17, 2019: member

ACK fa0467326ff004a0a20c6af9c8f6adcc74a0c8af, this makes it easy to see that all fields are initialized.
fanquake referenced this in commit 4daadce36c on Oct 17, 2019
fanquake merged this on Oct 17, 2019
fanquake closed this on Oct 17, 2019
Fabcien referenced this in commit ca7abed8bb on Dec 24, 2020
PastaPastaPasta referenced this in commit 1d2f2b0cc8 on Jun 27, 2021
PastaPastaPasta referenced this in commit 26822f5e68 on Jun 28, 2021
PastaPastaPasta referenced this in commit 328a7fd7be on Jun 29, 2021
PastaPastaPasta referenced this in commit a9d7580bc7 on Jul 1, 2021
PastaPastaPasta referenced this in commit fae0bfdfa8 on Jul 1, 2021
PastaPastaPasta referenced this in commit d577147f4f on Jul 12, 2021
PastaPastaPasta referenced this in commit bf8935119c on Jul 13, 2021
PastaPastaPasta referenced this in commit b485210d68 on Jul 13, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Refactoring Validation

Linked (view graph)

#17161 CBlockIndex::nStatus race when pruning

chain: Remove CBlockIndex::SetNull helper #17162

Conflicts