Page MenuHomePhabricator

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

Authored by nakihito on Sep 10 2019, 00:26.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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
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.

deadalnix requested changes to this revision.Sep 24 2019, 14:17

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
nakihito updated this revision to Diff 13128.Sep 24 2019, 23:11

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

nakihito planned changes to this revision.Sep 24 2019, 23:14
nakihito updated this revision to Diff 13129.Sep 24 2019, 23:22

Fixed typo.

deadalnix requested changes to this revision.Sep 26 2019, 10:23

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
nakihito updated this revision to Diff 13372.Oct 6 2019, 21:20

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
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 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 updated this revision to Diff 13570.Wed, Oct 16, 01:52

Removed changes from PR14444.

nakihito edited the summary of this revision. (Show Details)Wed, Oct 16, 01:54
deadalnix requested changes to this revision.Thu, Oct 17, 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.Thu, Oct 17, 01:41
nakihito added inline comments.Thu, Oct 24, 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)).

nakihito updated this revision to Diff 13692.Thu, Oct 24, 18:58

Rebased and removed some annotations that Core does not have.

nakihito planned changes to this revision.Thu, Oct 24, 18:58
nakihito requested review of this revision.Thu, Oct 24, 19:15
nakihito edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Fri, Oct 25, 01:55

nits, but lgtm

src/validation.h
355 ↗(On Diff #13692)

relayout

649 ↗(On Diff #13692)

dito

This revision now requires changes to proceed.Fri, Oct 25, 01:55
nakihito updated this revision to Diff 13699.Fri, Oct 25, 02:02

Relayed out comments.

Fabien accepted this revision.Fri, Oct 25, 06:05
deadalnix accepted this revision.Fri, Oct 25, 14:19
This revision is now accepted and ready to land.Fri, Oct 25, 14:19
nakihito updated this revision to Diff 13709.Fri, Oct 25, 17:39

Resynchronizing commit with phab.