Details

Reviewers

deadalnix
Fabien
jasonbcox

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rSTAGINGba606ae94f18: Merge #13481: doc: Rewrite some validation docs as lock annotations
rABCba606ae94f18: Merge #13481: doc: Rewrite some validation docs as lock annotations

Summary

fa324a8b15a4ef4138685b3427c895ec14faf3af doc: Rewrite some validation doc as lock annotations (MarcoFalke)

Pull request description:

#13402 added some lock annotations in comments. This pull removes them and adds clang-readable locking annotations instead.

Tree-SHA512: 2d392efa8ac4978830a9df08b2009e69d6f1ac031f62be2275ae8d7c7e483331c7f8d458d865443af907a7af27a592421c6cca6b2df3f2877e0f369b9198f383

Backport of Core PR13481
https://github.com/bitcoin/bitcoin/pull/13481/

Depends on D4029

Test Plan

../configure CXX=clang++ CC=clang
make check

And run team city build-werror

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

nakihito planned changes to this revision.Sep 23 2019, 23:09

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster failed remote builds in B7561: Diff 13100!Sep 23 2019, 23:20

Build Bitcoin-ABC / Diffs / Diff Testing failed.

nakihito requested review of this revision.Sep 24 2019, 00:45

nakihito added inline comments.

src/validation.cpp
207 ↗	(On Diff #13100)	This was added in D4094.
221 ↗	(On Diff #13100)	This was added in D4094.
225 ↗	(On Diff #13100)	This was added in D4094.

Clearly this patch wasn't adapted appropriately to our codebase.

src/validation.cpp
3164 ↗	(On Diff #13100)	It's a strong tell that UnwindBlock require annotations if that the only gad damn thing this function is doing and this function require the lock.
3168 ↗	(On Diff #13100)	This is also a tell that this guy needs it as well.

This revision now requires changes to proceed.Sep 24 2019, 14:17

Added lock annotations to UnwindBlock() and ParkBlock().

Build Bitcoin-ABC / Diffs / Diff Testing started.

nakihito planned changes to this revision.Sep 24 2019, 23:14

Fixed typo.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7580: Diff 13128.Sep 24 2019, 23:23

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7581: Diff 13129.Sep 24 2019, 23:37

Please go over the file and add proper annotations instead of simply doing ssh over phabricator.

This revision now requires changes to proceed.Sep 26 2019, 10:23

nakihito planned changes to this revision.Sep 26 2019, 18:49

Added locking annotations for all functions remaining in validation.cpp/h with AssertLockHeld(cs_main).

nakihito planned changes to this revision.Oct 6 2019, 21:20

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7699: Diff 13372.Oct 6 2019, 21:32

Build Bitcoin-ABC / Diffs / Diff Testing passed.

nakihito edited the summary of this revision. (Show Details)Oct 7 2019, 23:35

nakihito edited the summary of this revision. (Show Details)Oct 7 2019, 23:39

nakihito added a child revision: D4225: Merge #14444: Add compile time checking for cs_main locks which we assert at run time.Oct 7 2019, 23:51

nakihito requested review of this revision.Oct 7 2019, 23:51

nakihito added inline comments.

src/validation.h
405 ↗	(On Diff #13372)	This was part of PR14444.
647 ↗	(On Diff #13372)	This was part of PR14444.

nakihito mentioned this in D4225: Merge #14444: Add compile time checking for cs_main locks which we assert at run time.Oct 9 2019, 01:05

nakihito removed a child revision: D4225: Merge #14444: Add compile time checking for cs_main locks which we assert at run time.Oct 10 2019, 15:49

Removed changes from PR14444.

Build Bitcoin-ABC / Diffs / Diff Testing started.

nakihito edited the summary of this revision. (Show Details)Oct 16 2019, 01:54

Harbormaster completed remote builds in B7784: Diff 13570.Oct 16 2019, 02:04

Build Bitcoin-ABC / Diffs / Diff Testing passed.

deadalnix requested changes to this revision.Oct 17 2019, 01:41

deadalnix added inline comments.

src/validation.h
337 ↗	(On Diff #13570)	relayout
447 ↗	(On Diff #13570)	This doesn't seem to be part of the original PR, nor especially related to the code in question.
612 ↗	(On Diff #13570)	dito
686 ↗	(On Diff #13570)	Why is that necessary?
692 ↗	(On Diff #13570)	dito

This revision now requires changes to proceed.Oct 17 2019, 01:41

nakihito added inline comments.Oct 24 2019, 18:57

src/validation.h
447 ↗	(On Diff #13570)	Removed.
612 ↗	(On Diff #13570)	Also removed.
686 ↗	(On Diff #13570)	This and `IsBlockFinalized()` require the `cs_main` lock (see lines 3296 and 3301 in validation.cpp where they both `AssertLockHeld(cs_main)`).