Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Remove cs_main lock from blockToJSON and blockheaderToJSON #12151

Merged
merged 3 commits into from Jan 4, 2019

Conversation

@promag
Copy link
Member

commented Jan 11, 2018

Motivated by #11913 (comment), 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.

@promag promag force-pushed the promag:2018-01-blocktojson branch Jan 11, 2018

@promag promag force-pushed the promag:2018-01-blocktojson branch 3 times, most recently Jan 11, 2018

@TheBlueMatt
Copy link
Contributor

left a comment

Generally wouldn't bother too much reducing cs_main scope when its really a trivial amount of time running with cs_main held, though I agree in the context of #11913 it makes sense to not lock cs_main and then unlock, then re-lock it there.

src/rpc/blockchain.cpp Outdated
{
return GetDifficulty(chainActive, blockindex);
next = tip->GetAncestor(blockindex->nHeight + 1);
if (next && next->pprev == blockindex) {

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 11, 2018

Contributor

Why? You can just do a GetAncestor for blockindex->nHeight here, no?

This comment has been minimized.

Copy link
@promag

promag Jan 11, 2018

Author Member

This way you get next of blockindex.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 11, 2018

Contributor

Ohoh, sorry, missed its use in blockheaderToJSON.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 5, 2018

Contributor

This confused me for a minute too. Maybe could rename function to something like ComputeNextBlockAndDepth to mention the next part.

This comment has been minimized.

Copy link
@promag

promag Feb 5, 2018

Author Member

Can do, WDYT about returning std::pair<int, const CBlockIndex*>?

This comment has been minimized.

Copy link
@promag

promag Feb 5, 2018

Author Member

I can add a comment there too explaning the height + 1 and ->pprev == blockindex.

This comment has been minimized.

Copy link
@promag

promag Feb 26, 2018

Author Member

Renamed to ComputeNextBlockAndDepth as per @ryanofsky suggestion.

@@ -1185,20 +1182,21 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)

LOCK(cs_main);

const CBlockIndex* tip = chainActive.Tip();

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 11, 2018

Contributor

No need to change these unless you're gonna actually kill the cs_main held for the whole time, I'd think, no (and little reason to, its not like its being held for an extended period)?

This comment has been minimized.

Copy link
@promag

promag Jan 11, 2018

Author Member

This moves up tip variable declared below.

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2018

Best reviewed commit by commit

Show resolved Hide resolved src/rpc/blockchain.cpp Outdated
Show resolved Hide resolved src/rpc/blockchain.cpp Outdated
Show resolved Hide resolved src/rpc/blockchain.cpp Outdated
Show resolved Hide resolved src/rpc/blockchain.cpp Outdated

@promag promag force-pushed the promag:2018-01-blocktojson branch Jan 11, 2018

@promag
Copy link
Member Author

left a comment

Changed the order in the signature and forgot to change callers.

Show resolved Hide resolved src/rest.cpp Outdated
Show resolved Hide resolved src/rpc/blockchain.cpp Outdated

@promag promag force-pushed the promag:2018-01-blocktojson branch Jan 11, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2018

Fixed @TheBlueMatt and @jimpo comments, thanks.

@ryanofsky
Copy link
Contributor

left a comment

utACK f6d175ddc97a0d2b2614b44ddb9efbebf8d6eec5

src/rpc/blockchain.cpp Outdated
{
return GetDifficulty(chainActive, blockindex);
next = tip->GetAncestor(blockindex->nHeight + 1);
if (next && next->pprev == blockindex) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 5, 2018

Contributor

This confused me for a minute too. Maybe could rename function to something like ComputeNextBlockAndDepth to mention the next part.

@promag promag force-pushed the promag:2018-01-blocktojson branch 2 times, most recently Feb 26, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2018

Rebased mainly due to recent change from push_back(Pair()) to pushKV().

Also reworded to replace prefix [rpc] to rpc: as per @laanwj suggestion.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

Needs rebase if still relevant

Show resolved Hide resolved src/rest.cpp Outdated

@promag promag force-pushed the promag:2018-01-blocktojson branch Mar 19, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2018

Rebased.

@MarcoFalke like @TheBlueMatt said above:

in the context of #11913 it makes sense to not lock cs_main and then unlock, then re-lock it there.

I'd say at least let's wait for that.

@jnewbery

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

needs rebase

@promag

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2018

Rebased.

@promag promag force-pushed the promag:2018-01-blocktojson branch Apr 3, 2018

@ryanofsky
Copy link
Contributor

left a comment

utACK 77d51a68108e282856d9894d41e8a600dba78dd8. No changes since last review other than rebase and removing std::move.

Show resolved Hide resolved src/rest.cpp Outdated

@promag promag force-pushed the promag:2018-01-blocktojson branch Apr 4, 2018

@ryanofsky
Copy link
Contributor

left a comment

utACK e20f745d9b279de9e43994505731658f0f3582fa, just whitespace fix since last review

@jimpo
Copy link
Contributor

left a comment

I think you can stop passing tip to GetDifficulty in most places.

Show resolved Hide resolved src/rpc/blockchain.cpp Outdated

@promag promag force-pushed the promag:2018-01-blocktojson branch May 18, 2018

Show resolved Hide resolved src/rpc/blockchain.h Outdated

@promag promag force-pushed the promag:2018-01-blocktojson branch Jul 8, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2018

Now related to #13903.

@promag promag force-pushed the promag:2018-01-blocktojson branch to b9f226b Sep 9, 2018

@DrahtBot DrahtBot removed the Needs rebase label Sep 9, 2018

@DrahtBot DrahtBot referenced this pull request Sep 10, 2018

Closed

[RPC] decodeblock #14191

@ryanofsky
Copy link
Contributor

left a comment

utACK b9f226b only change since previous review is simplifying GetDifficulty in the second commit.

{
return 1.0;
}
assert(blockindex);

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 14, 2018

Member

In rpc code, assert should be replaced with throw JSONRPCError?

This comment has been minimized.

Copy link
@promag

promag Jan 3, 2019

Author Member

I don't think this is a usage error, should be a programatic error?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jan 3, 2019

Member

I guess you could avoid the assert by passing in a const reference?

This comment has been minimized.

Copy link
@promag

promag Jan 3, 2019

Author Member

Sure but out of scope here and I'm happy to submit that refactor in a separate PR.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

utACK b9f226b

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jan 4, 2019

Merge bitcoin#12151: rpc: Remove cs_main lock from blockToJSON and bl…
…ockheaderToJSON

b9f226b rpc: Remove cs_main lock from blockToJSON and blockHeaderToJSON (João Barbosa)
343b98c rpc: Specify chain tip instead of chain in GetDifficulty (João Barbosa)
54dc13b rpc: Fix SoftForkMajorityDesc and SoftForkDesc signatures (João Barbosa)

Pull request description:

  Motivated by bitcoin#11913 (comment), 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.

Tree-SHA512: a6720ace0182c19033bbed1a404f729d793574db8ab16e0966ffe412145611e32c30aaab02975d225df6d439d7b9ef2070e732b16137a902b0293c8cddfeb85f

@MarcoFalke MarcoFalke merged commit b9f226b into bitcoin:master Jan 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@promag promag deleted the promag:2018-01-blocktojson branch Jan 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.