net: don't lock cs_main while reading blocks in net processing

andrewtoth commented at 3:34 pm on October 17, 2022: contributor

Inspired by #11913 and #26308.

cs_main doesn’t need to be locked while reading blocks. This removes the locks in net_processing.

DrahtBot added the label P2P on Oct 17, 2022

andrewtoth force-pushed on Oct 17, 2022

DrahtBot commented at 6:24 pm on October 17, 2022: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sr-gi, furszy, mzumsande, TheCharlatan, achow101
Concept ACK	dergoegge, pablomartin4btc
Approach ACK	hernanmarino

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

dergoegge commented at 2:42 pm on October 18, 2022: member

Concept ACK

I would suggest that you rebase this PR on top of #26316 (if there is a dependency) and then maybe also mark this PR as a draft until the base is merged.

andrewtoth force-pushed on Oct 19, 2022

andrewtoth marked this as a draft on Oct 19, 2022

andrewtoth marked this as ready for review on Nov 18, 2022

andrewtoth commented at 4:11 pm on November 18, 2022: contributor

I don’t think #26316 is necessary anymore. We can simply remove files that are still on disk like in #26533.

andrewtoth force-pushed on Nov 18, 2022

andrewtoth force-pushed on Nov 24, 2022

in src/net_processing.cpp:3872 in 351e5873ab outdated

3874+            is_above_blocktxn_depth = pindex->nHeight >= m_chainman.ActiveChain().Height() - MAX_BLOCKTXN_DEPTH;
3875+        }
3876+        if (is_above_blocktxn_depth) {
3877+            CBlock block;
3878+            if (!ReadBlockFromDisk(block, pindex, m_chainparams.GetConsensus())) {
3879+                LogPrint(BCLog::NET, "Cannot load block from disk. It was likely pruned before we could read it.\n");

maflcko commented at 10:12 am on December 8, 2022:

I don’t think this can happen. MAX_BLOCKTXN_DEPTH is 10, so could keep the assert or at least make it a error log category + Assume(false)?

andrewtoth commented at 1:45 pm on December 8, 2022:

Could it not happen in regtest or unit tests?

maflcko commented at 2:01 pm on December 8, 2022:

Not sure, but it seems fine to crash in tests if they violate assumptions

maflcko commented at 3:01 pm on December 8, 2022:

See MIN_BLOCKS_TO_KEEP, which is also enforced in regtest

andrewtoth commented at 3:50 am on December 19, 2022:

Updated this to keep the assert. Also changed it to take FlatFilePos in ReadBlockFromDisk so it doesn’t have to reacquire the lock again after releasing it.

in src/node/blockstorage.h:214 in 351e5873ab outdated

210@@ -211,7 +211,7 @@ void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune);
211 /** Functions for disk access for blocks */
212 bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams);
213 bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams);
214-bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, const CMessageHeader::MessageStartChars& message_start);
215+bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start);

maflcko commented at 10:14 am on December 8, 2022:

0bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex& block, const CMessageHeader::MessageStartChars& message_start);

shouldn’t the * be a &, like before? (nullptr is not accepted)

andrewtoth commented at 3:49 am on December 19, 2022:

Removed touching this file.

jamesob commented at 4:11 pm on December 8, 2022: member

Cool. Can someone add a “needs benchmark” label?

fanquake added the label Needs Benchmark on Dec 8, 2022

andrewtoth commented at 5:14 pm on December 8, 2022: contributor

Can someone add a “needs benchmark” label?

What kind of benchmark would be appropriate here? AFAICT message processing is done on a single thread, so this patch wouldn’t speed up handling requests. It would free up other threads that are waiting on the lock as the message processing thread does lengthy disk IO.

hernanmarino commented at 6:59 pm on December 16, 2022: contributor

Approach ACK. Run all tests, everything run fine.

pablomartin4btc commented at 10:45 pm on December 16, 2022: member

cr ACK. I like the idea and I think this change is very useful for performance purposes. I ran unit tests and functional ones with no errors or warnings. Pending of benchmarking if I manage to find the way, otherwise when someone posts here I’m happy to re-evaluate.

andrewtoth commented at 3:47 am on December 19, 2022: contributor

I wrote a tool to allow benchmarking the affected code paths https://github.com/andrewtoth/spam_block_reqs. I ran it on block 768022 using 5000 requests for each of the four paths on both this branch and master. For example:

0cargo run --release -- -r block-transactions -b 000000000000000000064b1c6e9606714fd4a96d6ff877e4a93b170d86361e28 -n 5000

I did not benchmark the MSG_FILTERED_BLOCK path because it isn’t supported in rust-bitcoin, however there isn’t anything in that path that requires taking the lock other than the shared code for the other paths.

Results show no regression:

Request Type	master (cb32328d1b80d0ccd6eb9532bd8fe4e0a4de385e)	branch
witness-block	22.6s	22.3s
compact-block	60.9s	59.1s
block-transactions	58.5s	59.0s
legacy-block	77.7s	77.4s

However, while running the spam-blocks tool using a very high number of requests so it would not complete, I ran a benchmark with ApacheBench requesting 2000 headers from block 750k 1000 times on a single core using REST.

0ab -n 1000 -c 1 "http://127.0.0.1:8332/rest/headers/0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e.bin?count=2000"

The mean response times for fetching headers while running the spam-blocks tool for each request-type is below:

Request Type	master (cb32328d1b80d0ccd6eb9532bd8fe4e0a4de385e)	branch
witness-block	14ms	1ms
compact-block	19ms	1ms
block-transactions	19ms	1ms
legacy-block	42ms	1ms

So it seems like this definitely improves responsiveness for any other threads that need to acquire a lock to cs_main.

andrewtoth force-pushed on Dec 19, 2022

andrewtoth commented at 2:30 pm on December 19, 2022: contributor

Moved all whitespace only scope changes to separate commit to make it easier to review.

andrewtoth force-pushed on Dec 20, 2022

in src/net_processing.cpp:4412 in b630e5dc16 outdated

3878         }
3879 
3880+        if (!block_pos.IsNull()) {
3881+            CBlock block;
3882+            bool ret = ReadBlockFromDisk(block, block_pos, m_chainparams.GetConsensus());
3883+            assert(ret);

furszy commented at 1:16 pm on February 28, 2023:

What about this assertion? Couldn’t ReadBlockFromDisk return false if the block is not on disk anymore?

andrewtoth commented at 2:55 pm on February 28, 2023:

See #26326 (review). Blocks below MAX_BLOCKTXN_DEPTH can’t be pruned, so we should retain the current behavior if this read fails.

DrahtBot added the label Needs rebase on May 11, 2023

andrewtoth force-pushed on Aug 2, 2023

DrahtBot removed the label Needs rebase on Aug 3, 2023

DrahtBot added the label CI failed on Aug 3, 2023

DrahtBot removed the label CI failed on Aug 3, 2023

in src/net_processing.cpp:2296 in 3e93e6b756 outdated

2209@@ -2205,23 +2210,29 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
2210         if (!(pindex->nStatus & BLOCK_HAVE_DATA)) {
2211             return;
2212         }
2213+        can_direct_fetch = CanDirectFetch();
2214+        block_pos = pindex->GetBlockPos();
2215+    }
2216+    const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());

furszy commented at 4:25 pm on August 3, 2023:

tiny nit: would be good to add a jump line here.

in src/net_processing.cpp:2171 in 3e93e6b756 outdated

2171@@ -2172,16 +2172,20 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
2172         }
2173     }
2174 
2175+    const CBlockIndex* pindex;
2176+    const CBlockIndex* tip;
2177+    bool can_direct_fetch;

furszy commented at 4:29 pm on August 3, 2023:

tiny nit: would be good to initialize the variable to nullptr and false.

furszy commented at 4:36 pm on August 3, 2023: member

Code ACK 3e93e6b7

DrahtBot added the label Needs rebase on Aug 7, 2023

andrewtoth force-pushed on Aug 7, 2023

DrahtBot removed the label Needs rebase on Aug 7, 2023

furszy commented at 2:36 pm on August 7, 2023: member

re-ACK 144dd46. No changes since the last review.

andrewtoth commented at 2:56 pm on August 7, 2023: contributor

@furszy thank you for the review. Sorry I forgot to address your nits on this rebase. Will do them if I have to rebase again.

DrahtBot added the label CI failed on Aug 7, 2023

andrewtoth force-pushed on Aug 17, 2023

DrahtBot removed the label CI failed on Aug 18, 2023

furszy commented at 9:25 pm on August 30, 2023: member

reACK e8357eb after rebase.

DrahtBot added the label Needs rebase on Oct 16, 2023

andrewtoth force-pushed on Oct 23, 2023

DrahtBot removed the label Needs rebase on Oct 23, 2023

DrahtBot added the label CI failed on Oct 23, 2023

furszy commented at 1:34 pm on October 23, 2023: member

diff-ACK fae1fd61

Have to admit that I’m not fan of the first commit. It forces me to remember where the indentation changes were located when I’m reviewing the other commits.

DrahtBot requested review from hernanmarino on Oct 23, 2023

DrahtBot requested review from dergoegge on Oct 23, 2023

DrahtBot requested review from pablomartin4btc on Oct 23, 2023

in src/net_processing.cpp:2494 in fae1fd6180 outdated

2314         std::shared_ptr<CBlock> pblockRead = std::make_shared<CBlock>();
2315-        if (!m_chainman.m_blockman.ReadBlockFromDisk(*pblockRead, *pindex)) {
2316-            assert(!"cannot load block from disk");
2317+        if (!m_chainman.m_blockman.ReadBlockFromDisk(*pblockRead, block_pos)) {
2318+            LogPrint(BCLog::NET, "Cannot load block from disk. It was likely pruned before we could read it.\n");
2319+            return;

maflcko commented at 8:58 am on October 25, 2023:

Last commit: Not sure. This looks like a p2p protocol change?

furszy commented at 1:53 am on October 27, 2023:

Last commit: Not sure. This looks like a p2p protocol change?

For the local node, this change means that the node will continue functional instead of crashing. Which I think is good. The node shouldn’t crash because it received a p2p message that cannot answer.

For the remote peer, this means that the connection will remain active for ~10 more minutes until the block request times out and the peer closes the socket. Which isn’t the best, but it is how we currently handle unanswered block requests.

Are you seeing something else?

maflcko commented at 8:11 am on October 27, 2023:

For the local node, this change means that the node will continue functional instead of crashing.

The node wouldn’t crash here, because cs_main was being held, so no pruning could happen in-between.

My point is that it would be good to explain behavior changes, especially if they are p2p protocol changes. Hiding this under a “reduce cs_main” commit, which appears like a refactor doesn’t seem ideal.

furszy commented at 1:45 pm on October 27, 2023:

For the local node, this change means that the node will continue functional instead of crashing.

The node wouldn’t crash here, because cs_main was being held, so no pruning could happen in-between.

Could also be that the requested block data is not on disk or not accessible for the time this happen. Something external to our software.

Still, removing the external cause from the equation. The only diff here is that we allow a tiny extra pruning window in-between the index lookup and the block data read. Because, we currently return early if the block is pruned here, without logging anything. And after this PR, we could either return early on the no logging line (if the block is already pruned) or log “Cannot load block from disk. It was likely pruned before we could read it” if the block was pruned right after the index lookup.

But this is all internal to the node, I’m not seeing any p2p protocol change.

Maybe @andrewtoth could take this to the PR description?

maflcko commented at 1:59 pm on October 27, 2023:

For the local node, this change means that the node will continue functional instead of crashing.

The node wouldn’t crash here, because cs_main was being held, so no pruning could happen in-between.

Could also be that the requested block data is not on disk or not accessible for the time this happen. Something external to our software.

If your datadir was corrupted from outside, I think a crash is acceptable. You likely wouldn’t be able to re-org or otherwise stay in consensus anyway.

I’m not seeing any p2p protocol change.

Maybe “protocol change” was the wrong word? I guess “behavior change” may be better?

furszy commented at 2:32 pm on October 27, 2023:

If your datadir was corrupted from outside, I think a crash is acceptable. You likely wouldn’t be able to re-org or otherwise stay in consensus anyway.

You know; if the block in conflict is old enough, then the node should be “mostly” ok. The problem would be on new peers performing IBD, which would disconnect from the node for the lack of response.

I’m not seeing any p2p protocol change.

Maybe “protocol change” was the wrong word? I guess “behavior change” may be better?

Sounds good.

andrewtoth commented at 6:22 pm on October 29, 2023:

If a block is pruned before we lock cs_main, this function returns early. This behavior is not changed in this PR.

If a block is not pruned before we lock cs_main, previously we are guaranteed to send the block to the peer.

This PR changes this to have a short window after unlocking cs_main but before opening the fd that if the block is pruned and unlinked from another thread, then this function also returns early.

I think we all agree this is not a p2p protocol change? I also believe I documented this behavior change in the PR description and in the debug logs.

andrewtoth commented at 6:35 pm on October 29, 2023:

So the other behavior change is that if we fail to read a block for reasons other than being pruned, previously the node would crash. Now we will log the failure and continue.

If your datadir was corrupted from outside, I think a crash is acceptable. You likely wouldn’t be able to re-org or otherwise stay in consensus anyway.

I suppose if the read fails, we can retake cs_main and check if the block was pruned out from under us. If so, we log and continue, otherwise we can keep crashing on an assert? Do we really want to keep crashing here if we fail to read a block? I’m not sure.

andrewtoth commented at 6:53 pm on October 29, 2023:

if the read fails, we can retake cs_main and check if the block was pruned out from under us. If so, we log and continue, otherwise we can keep crashing on an assert @maflcko I updated the code to do this. Thus I don’t think there is any behavior change. If the block requested is pruned, we return early before and after this PR. If the block requested is unpruned and we fail to read it, we will crash before and after this PR.

maflcko commented at 8:05 am on October 30, 2023:

Ah sorry for being unclear. I am not asking to add back the assert. Just adding the comment from #26326 (review) to the commit description would be great.

However, I wonder what the point is of forcing the remote peer into a 10 minute delay+disconnect. Why not disconnect immediately? Also, if the remote has set a permission flag to avoid the disconnect (not sure if this exists), I wonder what the correct behavior would be.

furszy commented at 2:31 pm on November 3, 2023:

I am not asking to add back the assert.

+1 on not re-adding the assert.

However, I wonder what the point is of forcing the remote peer into a 10 minute delay+disconnect. Why not disconnect immediately? Also, if the remote has set a permission flag to avoid the disconnect (not sure if this exists), I wonder what the correct behavior would be.

If the remote peer has a permission flag to avoid the disconnection, maybe it could respond with a NOTFOUND message? Same as we do with tx getdata requests. But probably better to discuss this in a follow-up or in an issue?

andrewtoth commented at 0:12 am on November 29, 2023:

Updated to log and return instead of hitting an assert. Also updated the commit description to clarify this behavior.

However, I wonder what the point is of forcing the remote peer into a 10 minute delay+disconnect. Why not disconnect immediately?

We could, but I think this is very unlikely to occur in practice. We already disconnect above on line 2293 if we are past the NODE_NETWORK_LIMITED_MIN_BLOCKS threshold, which is MIN_BLOCKS_TO_KEEP (288) so we won’t prune below that anyways.

DrahtBot removed review request from hernanmarino on Oct 25, 2023

DrahtBot requested review from hernanmarino on Oct 25, 2023