Page MenuHomePhabricator

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

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

Details

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 7373
Build 12789: Bitcoin ABC Buildbot (legacy)
Build 12788: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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

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

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

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

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.

Removed changes from PR14444.

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
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)).

Rebased and removed some annotations that Core does not have.

nakihito edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Oct 25 2019, 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.Oct 25 2019, 01:55
This revision is now accepted and ready to land.Oct 25 2019, 14:19

Resynchronizing commit with phab.