Paths

Table of Contentst

Merge #12151: rpc: Remove cs_main lock from blockToJSON and blockheaderToJSON
ClosedPublic
Actions

Authored by markblundeberg on May 10 2019, 16:01.

Details

Reviewers

jasonbcox
Fabien
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGINGec204539c2ad: Merge #12151: rpc: Remove cs_main lock from blockToJSON and blockheaderToJSON
rABCec204539c2ad: Merge #12151: rpc: Remove cs_main lock from blockToJSON and blockheaderToJSON

Summary

backport PR12151 https://github.com/bitcoin/bitcoin/pull/12151/files
b9f226b41f rpc: Remove cs_main lock from blockToJSON and blockHeaderToJSON (João Barbosa)
343b98cbcd rpc: Specify chain tip instead of chain in GetDifficulty (João Barbosa)
54dc13b6a2 rpc: Fix SoftForkMajorityDesc and SoftForkDesc signatures (João Barbosa)

Pull request description:

Motivated by https://github.com/bitcoin/bitcoin/pull/11913#discussion_r157798157, this pull makes `blockToJSON` and `blockheaderToJSON` free of `cs_main` locks.

Locking `cs_main` was required to access `chainActive` in order to check if the block was in the chain and to retrieve the next block index.

With the this approach, `CBlockIndex::GetAncestor()` is used in a way to check if the block belongs to the specified chain tip and, at the same time, get the next block index.

Backport note: The modification of get_difficulty_for_null_tip cannot be included in this due to divergent and conflicting histories; this situation will be amended in D3012.

Depends on D3070

Test Plan

make check
test_runner.py

Diff Detail

Repository

rABC Bitcoin ABC

Branch

patch2

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5804
Build 9670: Bitcoin ABC Buildbot (legacy)
Build 9669: arc lint + arc unit

Event Timeline

markblundeberg created this revision.May 10 2019, 16:01

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 10 2019, 16:01

Herald added a reviewer: deadalnix. · View Herald Transcript

markblundeberg edited the summary of this revision. (Show Details)May 10 2019, 16:02

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

markblundeberg added a child revision: D3012: Backport current GetDifficulty logic (& tests) from Core.May 10 2019, 16:10

Due to divergent histories, the blockchain_tests affected in original PR do not exist yet in ABC; this will be fixed in child diff (D3012)

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5804: Diff 8598.May 10 2019, 16:16

jasonbcox requested changes to this revision.May 13 2019, 19:41

jasonbcox added inline comments.

src/rpc/blockchain.cpp
1319	keep the `cast(...)` style (treat the case like a function call).

This revision now requires changes to proceed.May 13 2019, 19:41

rebase ; change casting style per comments

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B5838: Diff 8643.May 13 2019, 20:14

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Fabien accepted this revision.May 14 2019, 12:03

jasonbcox requested changes to this revision.May 14 2019, 15:58

jasonbcox added inline comments.

src/rpc/blockchain.cpp
912 ↗	(On Diff #8643)	config should not be removed here. how does this even compile?

This revision now requires changes to proceed.May 14 2019, 15:58

markblundeberg added inline comments.May 14 2019, 16:11

src/rpc/blockchain.cpp
912 ↗	(On Diff #8643)	Yeah, I removed the config param from blockToJSON definition since it doesn't get used at all, and this makes the call signature the same as Core. I can separate it out as another diff but I see it as kind of a trivial "compiler-tested" change.
912 ↗	(On Diff #8643)	see "ABC note" in Diff description

markblundeberg requested review of this revision.May 14 2019, 17:18

deadalnix requested changes to this revision.May 16 2019, 15:52

deadalnix added inline comments.

src/rpc/blockchain.cpp
912 ↗	(On Diff #8643)	This belongs in its own patch for the very reason that this discussion exists.

This revision now requires changes to proceed.May 16 2019, 15:52

rebased & separated out config removal into D3070

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

markblundeberg marked 2 inline comments as done.May 19 2019, 17:38

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5896: Diff 8734.May 19 2019, 17:52

markblundeberg mentioned this in D3073: [RPC] Serialize in getblock without cs_main.May 20 2019, 13:51

It seems like this would depend on whatever added the get_difficulty_for_null_tip test.

In D3021#72499, @deadalnix wrote:

It seems like this would depend on whatever added the get_difficulty_for_null_tip test.

Yes, apologies for not pointing it out in this diff as well, but we have a divergent history from core -- see D3012 description.

markblundeberg edited the summary of this revision. (Show Details)May 20 2019, 17:40

deadalnix accepted this revision.May 22 2019, 00:13

jasonbcox accepted this revision.May 23 2019, 21:42

jasonbcox added inline comments.

src/rpc/blockchain.cpp
1321 ↗	(On Diff #8734)	I'll leave it to you to verify that there was an out-of-order backport or a backport soon to be completed regarding the missing `obj.pushKV("initialblockdownload", IsInitialBlockDownload());`
912 ↗	(On Diff #8643)	For posterity: D3070