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 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.May 10 2019, 16:01
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 10 2019, 16:01
markblundeberg edited the summary of this revision. (Show Details)May 10 2019, 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.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

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

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

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

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 updated this revision to Diff 8734.EditedMay 19 2019, 17:37
markblundeberg edited the summary of this revision. (Show Details)

rebased & separated out config removal into D3070

markblundeberg marked 2 inline comments as done.May 19 2019, 17:38
deadalnix added a comment.EditedMay 20 2019, 17:20

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.

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

For posterity: D3070

This revision is now accepted and ready to land.May 23 2019, 21:42
markblundeberg added inline comments.May 24 2019, 01:28
src/rpc/blockchain.cpp
1321 ↗(On Diff #8734)

Yep --> D3101