Avoid cs_main during ReadBlockFromDisk Calls #11913

TheBlueMatt · 2017-12-15T21:03:57Z

Built on #11281 and one commit from #11824, this removes almost all cs_main holds in ReadBlockFromDisk calls in net_processing/RPC/REST. Only real worry here is if something gets pruned out from under us during reading, so some previously-asserts in net_processing are now LogPrints.

laanwj · 2017-12-16T04:28:47Z

Concept ACK. Good to have less contention on the cs_main lock, it seems a good idea to release it during I/O.

promag

utACK 571f4ad.

promag · 2017-12-19T16:07:22Z

src/rpc/blockchain.cpp

@@ -144,6 +138,12 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool tx

    if (blockindex->pprev)
        result.push_back(Pair("previousblockhash", blockindex->pprev->GetBlockHash().GetHex()));
+    LOCK(cs_main);
+    int confirmations = -1;


Avoid lock here by receiving the pindex tip? (with the cost of jumping thru pprev).

That still wouldn't solve it for the chainActive.Next() call, so you'd need cs_main either way. Less code diff to just do it as-is.

Something along this should work:

const CBlockIndex* pindex = tip; const CBlockIndex* pnext = nullptr; int confirmations = 0; while (pindex && pindex != blockindex && pindex->nHeight >= blockindex->nHeight) { pnext = pindex; pindex = pindex->pprev; confirmations++; } if (pindex != blockindex) confirmations = -1;

This approach avoids accessing chainActive, blockToJSON just needs to receive the tip. This sounds more correct to me because a RPC can take a snapshot of the tip (and then leave cs_main) and all the remaining execution is consistent with that tip.

Submitted suggestion as #12151.

jonasschnelli · 2018-01-10T18:47:41Z

Needs rebase on rebased #11281
Concept ACK

promag

Needs rebase.

promag · 2018-01-11T13:21:57Z

src/rpc/blockchain.cpp

@@ -144,6 +138,12 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool tx

    if (blockindex->pprev)
        result.push_back(Pair("previousblockhash", blockindex->pprev->GetBlockHash().GetHex()));
+    LOCK(cs_main);
+    int confirmations = -1;


Submitted suggestion as #12151.

TheBlueMatt · 2018-01-11T18:00:56Z

Awaiting re-rebase of #11281. Will probably just wait for that to get merged.

jimpo

The concurrent access to data on disk makes me a little nervous. Is it worth having a separate read-write lock around reading/writing block files?

jimpo · 2018-01-11T20:08:46Z

src/net_processing.cpp

@@ -1044,6 +1044,8 @@ bool static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
        fWitnessesPresentInARecentCompactBlock = fWitnessesPresentInMostRecentCompactBlock;
    }

+    const CBlockIndex *pindex = nullptr;
+    {


I find the indentation here confusing. I assume it's not indented to help reviewers, so maybe indent in a separate commit?

jimpo · 2018-01-11T20:09:37Z

src/net_processing.cpp

@@ -1085,16 +1089,20 @@ bool static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
    }
    // Pruned nodes may have deleted the block, so check whether
    // it's available before trying to send.
-    if (send && (mi->second->nStatus & BLOCK_HAVE_DATA))
+    }


Add a // releases cs_main if there is no indentation.

jimpo · 2018-01-11T20:09:54Z

src/net_processing.cpp

@@ -1085,16 +1089,20 @@ bool static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
    }
    // Pruned nodes may have deleted the block, so check whether


This comment should get moved up to line 1066.

TheBlueMatt · 2018-01-11T21:45:29Z

@jimpo are you aware of any modern systems that have an issue reading from the middle of a file while it is being appended to? That sounds like it should be considered a critical kernel bug if anything, no?

jimpo · 2018-01-11T22:05:10Z

I say that mostly because of the pruning race.

TheBlueMatt · 2018-01-11T22:13:04Z

Pruning should be atomic still, no? Either we opened the file, then any unlink operations on the underlying file will result in us being able to finish reading, or we didn't open the file, so we can return false. I'm skeptical it can ever result in corrupt data, or am I missing some way Windows operates? It's definitely worth auditing that ReadBlockFromDisk returning false is handled properly everywhere!

promag · 2018-02-03T14:55:18Z

Needs rebase.

TheBlueMatt · 2018-03-27T20:22:36Z

Rebased.

TheBlueMatt · 2018-04-28T01:32:59Z

Rebased. In addition to RPC latency in some calls, this should also now make a nontrivial difference to -txindex nodes restart resync after an unclean shutdown (which, at least, should be useful to me).

promag · 2018-05-03T13:40:36Z

Needs rebase.

…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

…erToJSON 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 bitcoin/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. 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` Reviewers: jasonbcox, Fabien, #bitcoin_abc, deadalnix Reviewed By: jasonbcox, Fabien, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D3021

…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

fanquake added the Refactoring label Dec 15, 2017

TheBlueMatt force-pushed the 2017-12-no-readblockfromdisk-csmain branch from c20ff7d to 571f4ad Compare December 15, 2017 23:43

laanwj added the Block storage label Dec 16, 2017

promag reviewed Dec 19, 2017

View reviewed changes

promag mentioned this pull request Jan 11, 2018

rpc: Remove cs_main lock from blockToJSON and blockheaderToJSON #12151

Merged

promag reviewed Jan 11, 2018

View reviewed changes

jimpo reviewed Jan 11, 2018

View reviewed changes

TheBlueMatt force-pushed the 2017-12-no-readblockfromdisk-csmain branch from 571f4ad to 030c233 Compare March 27, 2018 20:22

TheBlueMatt force-pushed the 2017-12-no-readblockfromdisk-csmain branch from 030c233 to 7f538c1 Compare March 28, 2018 18:24

TheBlueMatt force-pushed the 2017-12-no-readblockfromdisk-csmain branch from 7f538c1 to 44d93b3 Compare April 28, 2018 01:32

TheBlueMatt added 2 commits April 27, 2018 22:00

Significantly reduce GetTransaction cs_main locking

613758d

[net_processing] Avoid holding cs_main during ReadBlockFromDisk

62a4d71

TheBlueMatt force-pushed the 2017-12-no-readblockfromdisk-csmain branch from 44d93b3 to b49fa7c Compare April 28, 2018 02:00

Avoid uneccessary cs_main during block reading in rest/rpc/zmq

cb14810

TheBlueMatt force-pushed the 2017-12-no-readblockfromdisk-csmain branch from b49fa7c to cb14810 Compare April 28, 2018 02:19

maflcko added the Needs rebase label Jun 6, 2018

TheBlueMatt closed this Jun 14, 2018

maflcko mentioned this pull request Aug 7, 2018

Significantly reduce GetTransaction cs_main locking (TheBlueMatt) #13903

Closed

promag mentioned this pull request Jan 8, 2019

Avoid permanent cs_main lock in getblockheader #12153

Merged

laanwj removed the Needs rebase label Oct 24, 2019

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

		@@ -1085,16 +1089,20 @@ bool static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
		}
		// Pruned nodes may have deleted the block, so check whether

Avoid cs_main during ReadBlockFromDisk Calls #11913

Avoid cs_main during ReadBlockFromDisk Calls #11913

Uh oh!

Conversation

TheBlueMatt commented Dec 15, 2017

Uh oh!

laanwj commented Dec 16, 2017

Uh oh!

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

jonasschnelli commented Jan 10, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jan 11, 2018

Uh oh!

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

TheBlueMatt commented Jan 11, 2018

Uh oh!

jimpo commented Jan 11, 2018

Uh oh!

TheBlueMatt commented Jan 11, 2018

Uh oh!

promag commented Feb 3, 2018

Uh oh!

TheBlueMatt commented Mar 27, 2018

Uh oh!

TheBlueMatt commented Apr 28, 2018

Uh oh!

promag commented May 3, 2018

Uh oh!

Uh oh!