Page MenuHomePhabricator

Merge #12151: rpc: Remove cs_main lock from blockToJSON and blockheaderToJSON
Needs RevisionPublic

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

Details

Reviewers
jasonbcox
Fabien
deadalnix
Group Reviewers
Restricted Project
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.

ABC note: this also removes needless config param from blockToJSON

Depends on D3014

Test Plan

make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D3021
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5838
Build 9738: Bitcoin ABC Teamcity Staging
Build 9737: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Fri, May 10, 16:01
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, May 10, 16:01
markblundeberg edited the summary of this revision. (Show Details)Fri, May 10, 16:02

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)

jasonbcox requested changes to this revision.Mon, May 13, 19:41
jasonbcox added inline comments.
src/rpc/blockchain.cpp
1319 ↗(On Diff #8598)

keep the cast(...) style (treat the case like a function call).

This revision now requires changes to proceed.Mon, May 13, 19:41
markblundeberg marked an inline comment as done.

rebase ; change casting style per comments

Fabien accepted this revision.Tue, May 14, 12:03
jasonbcox requested changes to this revision.Tue, May 14, 15:58
jasonbcox added inline comments.
src/rpc/blockchain.cpp
912

config should not be removed here. how does this even compile?

This revision now requires changes to proceed.Tue, May 14, 15:58
markblundeberg added inline comments.Tue, May 14, 16:11
src/rpc/blockchain.cpp
912

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

see "ABC note" in Diff description

markblundeberg requested review of this revision.Tue, May 14, 17:18
deadalnix requested changes to this revision.Thu, May 16, 15:52
deadalnix added inline comments.
src/rpc/blockchain.cpp
912

This belongs in its own patch for the very reason that this discussion exists.

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