Page MenuHomePhabricator
Differential D13037

Require IsBlockPruned() to hold mutex cs_main
ClosedPublic
Actions

Authored by PiRK on Jan 24 2023, 07:19.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCbd5235a5a860: Require IsBlockPruned() to hold mutex cs_main
Summary

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

This is a partial backport of core#22932
https://github.com/bitcoin/bitcoin/pull/22932/commits/eaeeb88768db529b5241ccd42f1e87579908b4df

Depends on D13036

Test Plan

With clang and DEBUG:

ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK created this revision.Jan 24 2023, 07:19
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 24 2023, 07:19
PiRK requested review of this revision.Jan 24 2023, 07:19
teamcity edited the summary of this revision. (Show Details)Jan 24 2023, 07:19
PiRK added a child revision: D13038: Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main.Jan 24 2023, 07:22
Harbormaster completed remote builds in B21830: Diff 37655.Jan 24 2023, 08:10
Fabien accepted this revision.Jan 24 2023, 08:55
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/rpc/blockchain.cpp
177 ↗(On Diff #37655)

You should move the call to UndoReadFromDisk out of the lock, because it creates recursive locking.
cs_main is a recursive lock so this is not an actual issue but better avoid double locks if possible. Not blocking on this because it's not wrong, but can you please check if it's been done by core and eventually backport the change as a follow up ?

1077 ↗(On Diff #37655)

Do you need the lock for this one ?

1097 ↗(On Diff #37655)

dito

This revision is now accepted and ready to land.Jan 24 2023, 08:55
PiRK added inline comments.Jan 24 2023, 09:11
src/rpc/blockchain.cpp
177 ↗(On Diff #37655)

Good remark. It is fixed in core#26308

1077 ↗(On Diff #37655)

No. The lock scope is reduced in https://github.com/bitcoin/bitcoin/pull/26308/commits

Closed by commit rABCbd5235a5a860: Require IsBlockPruned() to hold mutex cs_main (authored by Jon Atack <jon@atack.com>, committed by PiRK). · Explain WhyJan 24 2023, 09:21
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABCbd5235a5a860: Require IsBlockPruned() to hold mutex cs_main.

Revision Contents
Changeset List

PathSize
src/
node/
3 lines
1 line
rpc/
13 lines

Diff 37666

View Options

src/node/blockstorage.h

Loading...
View Options

src/node/blockstorage.cpp

Loading...
View Options

src/rpc/blockchain.cpp

Loading...