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

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

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.