Page MenuHomePhabricator

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

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

Details

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
arcpatch-D3021
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5896
Build 9852: Bitcoin ABC Buildbot (legacy)
Build 9851: arc lint + arc unit

Event Timeline

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.May 13 2019, 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.May 13 2019, 19:41
markblundeberg marked an inline comment as done.

rebase ; change casting style per comments

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

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
markblundeberg edited the summary of this revision. (Show Details)

rebased & separated out config removal into D3070

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

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.

jasonbcox added inline comments.
src/rpc/blockchain.cpp
1321

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

This revision is now accepted and ready to land.May 23 2019, 21:42
src/rpc/blockchain.cpp
1321

Yep --> D3101