rpc: Remove cs_main lock from blockToJSON and blockheaderToJSON by promag · Pull Request #12151 · bitcoin/bitcoin

promag · 2018-01-11T13:19:03Z

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.

TheBlueMatt

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.

TheBlueMatt · 2018-01-11T17:46:47Z

src/rpc/blockchain.cpp

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

This way you get next of blockindex.

Ohoh, sorry, missed its use in blockheaderToJSON.

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

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

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

Renamed to ComputeNextBlockAndDepth as per @ryanofsky suggestion.

TheBlueMatt · 2018-01-11T17:48:15Z

src/rpc/blockchain.cpp

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 moves up tip variable declared below.

promag · 2018-01-11T18:19:24Z

Best reviewed commit by commit

src/rpc/blockchain.cpp

promag

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

src/rest.cpp

src/rpc/blockchain.cpp

promag · 2018-01-11T22:52:44Z

Fixed @TheBlueMatt and @jimpo comments, thanks.

ryanofsky

utACK f6d175ddc97a0d2b2614b44ddb9efbebf8d6eec5

ryanofsky · 2018-02-05T23:01:39Z

src/rpc/blockchain.cpp

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

promag · 2018-02-26T16:25:37Z

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

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

maflcko · 2018-03-19T01:13:33Z

Needs rebase if still relevant

src/rest.cpp

promag · 2018-03-19T13:53:05Z

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 · 2018-04-03T21:56:38Z

needs rebase

promag · 2018-04-03T22:13:52Z

Rebased.

ryanofsky

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

src/rest.cpp

ryanofsky

utACK e20f745d9b279de9e43994505731658f0f3582fa, just whitespace fix since last review

jimpo

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

src/rpc/blockchain.cpp

ryanofsky

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

maflcko · 2018-09-14T22:05:40Z

src/rpc/blockchain.cpp

-    {
-        return 1.0;
-    }
+    assert(blockindex);


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

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

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

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

DrahtBot · 2018-09-21T13:27:59Z

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

Conflicts

No conflicts as of last run.

maflcko · 2019-01-03T17:10:35Z

utACK b9f226b

…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

zcash: cherry picked from commit 54dc13b zcash: bitcoin/bitcoin#12151

zcash: cherry picked from commit 343b98c zcash: bitcoin/bitcoin#12151

zcash: cherry picked from commit b9f226b zcash: bitcoin/bitcoin#12151

zcash: cherry picked from commit 54dc13b zcash: bitcoin/bitcoin#12151

zcash: cherry picked from commit 343b98c zcash: bitcoin/bitcoin#12151

zcash: cherry picked from commit b9f226b zcash: bitcoin/bitcoin#12151

…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

promag force-pushed the 2018-01-blocktojson branch from a40e35a to b1950e4 Compare January 11, 2018 13:19

promag mentioned this pull request Jan 11, 2018

Avoid cs_main during ReadBlockFromDisk Calls #11913

Closed

promag force-pushed the 2018-01-blocktojson branch 3 times, most recently from 1aeee56 to f88df2e Compare January 11, 2018 14:22

promag mentioned this pull request Jan 11, 2018

Avoid permanent cs_main lock in getblockheader #12153

Merged

TheBlueMatt reviewed Jan 11, 2018

View reviewed changes

jimpo reviewed Jan 11, 2018

View reviewed changes

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 Show resolved Hide resolved

promag force-pushed the 2018-01-blocktojson branch from f88df2e to 8ca0981 Compare January 11, 2018 22:23

promag commented Jan 11, 2018

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

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

promag force-pushed the 2018-01-blocktojson branch from 8ca0981 to f6d175d Compare January 11, 2018 22:43

fanquake added the RPC/REST/ZMQ label Jan 13, 2018

ryanofsky reviewed Feb 5, 2018

View reviewed changes

promag force-pushed the 2018-01-blocktojson branch 2 times, most recently from 1e6f7f7 to 2925d3b Compare February 26, 2018 16:23

dcousens reviewed Mar 19, 2018

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

promag force-pushed the 2018-01-blocktojson branch from 2925d3b to 1a6b13c Compare March 19, 2018 13:50

promag force-pushed the 2018-01-blocktojson branch from 1a6b13c to 77d51a6 Compare April 3, 2018 22:13

ryanofsky reviewed Apr 4, 2018

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

promag force-pushed the 2018-01-blocktojson branch from 77d51a6 to e20f745 Compare April 4, 2018 22:38

ryanofsky reviewed Apr 16, 2018

View reviewed changes

jimpo reviewed May 17, 2018

View reviewed changes

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

promag force-pushed the 2018-01-blocktojson branch from e20f745 to 00755e5 Compare May 18, 2018 00:39

ryanofsky reviewed Sep 14, 2018

View reviewed changes

maflcko reviewed Sep 14, 2018

View reviewed changes

maflcko merged commit b9f226b into bitcoin:master Jan 4, 2019

promag deleted the 2018-01-blocktojson branch January 4, 2019 11:55

IntegralTeam mentioned this pull request May 22, 2019

[WIP] rpc: Serialize in getblock without cs_main dogecoin/dogecoin#1585

Closed

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 29, 2021

rpc: Fix SoftForkMajorityDesc and SoftForkDesc signatures

39a28fd

zcash: cherry picked from commit 54dc13b zcash: bitcoin/bitcoin#12151

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 29, 2021

rpc: Specify chain tip instead of chain in GetDifficulty

1c0aed8

zcash: cherry picked from commit 343b98c zcash: bitcoin/bitcoin#12151

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 29, 2021

rpc: Remove cs_main lock from blockToJSON and blockHeaderToJSON

7b71d3e

zcash: cherry picked from commit b9f226b zcash: bitcoin/bitcoin#12151

LarryRuane mentioned this pull request May 4, 2021

Bitcoin 0.18 locking PRs zcash/zcash#5050

Open

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Jun 1, 2021

rpc: Fix SoftForkMajorityDesc and SoftForkDesc signatures

e346ed3

zcash: cherry picked from commit 54dc13b zcash: bitcoin/bitcoin#12151

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Jun 1, 2021

rpc: Specify chain tip instead of chain in GetDifficulty

2bd3a2e

zcash: cherry picked from commit 343b98c zcash: bitcoin/bitcoin#12151

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Jun 1, 2021

rpc: Remove cs_main lock from blockToJSON and blockHeaderToJSON

6bcb6fd

zcash: cherry picked from commit b9f226b zcash: bitcoin/bitcoin#12151

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

promag commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Jan 11, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

promag commented Jan 11, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Mar 19, 2018

Uh oh!

Uh oh!

promag commented Mar 19, 2018

Uh oh!

jnewbery commented Apr 3, 2018

Uh oh!

promag commented Apr 3, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

promag commented Jan 11, 2018 •

edited

Loading

promag commented Feb 26, 2018 •

edited

Loading

DrahtBot commented Sep 21, 2018 •

edited

Loading