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

Branch

pr13481

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 7784
Build 13607: Bitcoin ABC Buildbot (legacy)
Build 13606: arc lint + arc unit

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	relayout
447	This doesn't seem to be part of the original PR, nor especially related to the code in question.
612	dito
686	Why is that necessary?
692	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	Removed.
612	Also removed.
686	This and `IsBlockFinalized()` require the `cs_main` lock (see lines 3296 and 3301 in validation.cpp where they both `AssertLockHeld(cs_main)`).