validation: Make VerifyDB level 4 interruptible #19142

maflcko · 2020-06-02T17:41:52Z

level 0,1,2, and 3 are already interruptible, so make level 4 also interruptible

maflcko · 2020-06-02T18:26:36Z

(intentionally left empty)

fanquake · 2020-06-03T08:32:51Z

Concept ACK

In fa6e9b84ef25b266729dd03337ae00c0ab44e52a:

ActivateBestChain is only called in the "msghand" or one of the RPC
threads, neither of which is a boost::thread.

Is this correct? ActivateBestChain() is also called by ThreadImport(), both directly as well via LoadExternalBlockFile. ThreadImport is run in the boost::thread_group:

bitcoin/src/init.cpp

Lines 1847 to 1849 in 657b82c

    
           threadGroup.create_thread([=, &chainman] { ThreadImport(chainman, vImportFiles); });

VerifyDB is only called in the main thread ("init") or one of the RPC
threads, neither of which is a boost::thread.

Checked that the calls to CVerifyDB().VerifyDB() are in the init thread and verifychain RPC call.

maflcko · 2020-06-03T09:53:06Z

Is this correct? ActivateBestChain() is also called by ThreadImport(), both directly as well via LoadExternalBlockFile. ThreadImport is run in the boost::thread_group:

Good catch, you are correct. However, after #18786 (init: Remove boost from ThreadImport), it should be possible to simply switch ThreadImport to a std::thread. Also see that in line 2929 there is another non-boost breakpoint: if (ShutdownRequested()) break;.

I will adjust the commit message accordingly.

ActivateBestChain (ABC) is only called in the "msghand" or one of the RPC threads, neither of which is a boost::thread. However, ABC is also called in ThreadImport (which currently happens to be a boost::thread). In all cases, the interruption_point is redundant with the breakpoint in ABC that triggers when ShutdownRequested() VerifyDB is only called in the main thread ("init") or one of the RPC threads, neither of which is a boost::thread.

maflcko · 2020-06-03T10:07:58Z

Done. Force pushed a commit message change:

$ git range-diff bitcoin-core/master  faed13ea16 fa3b4f9b8e
1:  fa6e9b84ef ! 1:  fa1d5800d9 validation: Remove unused boost interruption_point
    @@ Metadata
      ## Commit message ##
         validation: Remove unused boost interruption_point
     
    -    ActivateBestChain is only called in the "msghand" or one of the RPC
    -    threads, neither of which is a boost::thread.
    +    ActivateBestChain (ABC) is only called in the "msghand" or one of the
    +    RPC threads, neither of which is a boost::thread. However, ABC is also
    +    called in ThreadImport (which currently happens to be a boost::thread).
    +    In all cases, the interruption_point is redundant with the breakpoint in
    +    ABC that triggers when ShutdownRequested()
     
         VerifyDB is only called in the main thread ("init") or one of the RPC
         threads, neither of which is a boost::thread.
2:  faed13ea16 = 2:  fa3b4f9b8e validation: Make VerifyDB level 4 interruptible

DrahtBot · 2020-06-04T12:28:44Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Make CCheckQueue RAII-styled #18731 (refactor: Make CCheckQueue RAII-styled by hebasto)
Add local thread pool to CCheckQueue #18710 (Add local thread pool to CCheckQueue by hebasto)

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.

laanwj · 2020-06-04T12:48:48Z

Code review ACK fa3b4f9
Thank you for cleaning up more boost interruption point usage.

fanquake

ACK fa3b4f9

I will adjust the commit message accordingly.

Thanks.

Rechecked that a call to verifychain is interrupted:

diff --git a/src/validation.cpp b/src/validation.cpp
index 1ce440dc0..6e813e00a 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4337,7 +4337,7 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview,
                 return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
             if (!::ChainstateActive().ConnectBlock(block, state, pindex, coins, chainparams))
                 return error("VerifyDB(): *** found unconnectable block at %d, hash=%s (%s)", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString());
-            if (ShutdownRequested()) return true;
+            //if (ShutdownRequested()) return true;
         }
     }

2020-06-04T13:04:33.970347Z [msghand] New outbound peer connected: version: 70015, blocks=633028, peer=17 (block-relay)
2020-06-04T13:04:47.234325Z [httpworker.0] Verifying last 100 blocks at level 4
2020-06-04T13:04:47.234376Z [httpworker.0] [0%]...[10%]...[20%]...[30%]...[40%]...[50%]...[60%]...tor: Thread interrupt
2020-06-04T13:05:11.967041Z [init] Shutdown: In progress...
2020-06-04T13:05:11.967992Z [addcon] addcon thread exit
2020-06-04T13:05:11.971026Z [torcontrol] torcontrol thread exit
2020-06-04T13:05:11.978165Z [opencon] opencon thread exit
2020-06-04T13:05:11.979705Z [net] net thread exit
2020-06-04T13:05:13.444961Z [httpworker.0] [70%]...[80%]...[90%]...[DONE].
2020-06-04T13:05:21.697665Z [httpworker.0] No coin database inconsistencies in last 100 blocks (187923 transactions)
2020-06-04T13:05:21.786008Z [msghand] msghand thread exit
2020-06-04T13:05:21.948084Z [scheduler] scheduler thread exit
2020-06-04T13:05:21.988007Z [shutoff] Writing 0 unbroadcast transactions to disk.
2020-06-04T13:05:22.000826Z [shutoff] Dumped mempool: 0.001229s to copy, 0.024559s to dump
2020-06-04T13:05:22.032434Z [shutoff] FlushStateToDisk: write coins cache to disk (272667 coins, 38381kB) started
2020-06-04T13:05:22.189725Z [shutoff] FlushStateToDisk: write coins cache to disk (272667 coins, 38381kB) completed (0.16s)
2020-06-04T13:05:22.227430Z [shutoff] FlushStateToDisk: write coins cache to disk (0 coins, 3292kB) started
2020-06-04T13:05:22.227563Z [shutoff] FlushStateToDisk: write coins cache to disk (0 coins, 3292kB) completed (0.00s)
2020-06-04T13:05:22.289343Z [shutoff] [default wallet] Releasing wallet
2020-06-04T13:05:22.294110Z [shutoff] Shutdown: done

This PR

2020-06-04T13:26:17.699492Z [msghand] New outbound peer connected: version: 70015, blocks=633028, peer=8 (full-relay)
2020-06-04T13:26:39.372328Z [httpworker.0] Verifying last 100 blocks at level 4
2020-06-04T13:26:39.372380Z [httpworker.0] [0%]...[10%]...[20%]...[30%]...[40%]...[50%]...[60%]...tor: Thread interrupt
2020-06-04T13:27:24.895974Z [addcon] addcon thread exit
2020-06-04T13:27:24.896071Z [torcontrol] torcontrol thread exit
2020-06-04T13:27:24.896148Z [init] Shutdown: In progress...
2020-06-04T13:27:24.905837Z [net] net thread exit
2020-06-04T13:27:24.969283Z [msghand] msghand thread exit
2020-06-04T13:27:24.969452Z [opencon] opencon thread exit
2020-06-04T13:27:25.134754Z [scheduler] scheduler thread exit
2020-06-04T13:27:25.174710Z [shutoff] Writing 0 unbroadcast transactions to disk.
2020-06-04T13:27:25.186320Z [shutoff] Dumped mempool: 0.001838s to copy, 0.02701s to dump
2020-06-04T13:27:25.217315Z [shutoff] FlushStateToDisk: write coins cache to disk (273951 coins, 38546kB) started
2020-06-04T13:27:25.389811Z [shutoff] FlushStateToDisk: write coins cache to disk (273951 coins, 38546kB) completed (0.17s)
2020-06-04T13:27:25.424695Z [shutoff] FlushStateToDisk: write coins cache to disk (0 coins, 3292kB) started
2020-06-04T13:27:25.424895Z [shutoff] FlushStateToDisk: write coins cache to disk (0 coins, 3292kB) completed (0.00s)
2020-06-04T13:27:25.492179Z [shutoff] [default wallet] Releasing wallet
2020-06-04T13:27:25.497838Z [shutoff] Shutdown: done

fa3b4f9 validation: Make VerifyDB level 4 interruptible (MarcoFalke) fa1d580 validation: Remove unused boost interruption_point (MarcoFalke) Pull request description: level 0,1,2, and 3 are already interruptible, so make level 4 also interruptible ACKs for top commit: laanwj: Code review ACK fa3b4f9 fanquake: ACK fa3b4f9 Tree-SHA512: d302c84a17add1b5993dd78339c88670d27eee45ce208c4d046ae188b50be9843ee5a9584739d5d25453b54ae08fd1cb6eeee8cb1307d84c05cde8a54a7c445b

Github-Pull: bitcoin#19142 Rebased-From: fa3b4f9

Mentioned in bitcoin#19142, which removed the boost::interruption_point() in ThreadImport().

83fd3a6 init: use std::thread for ThreadImport() (fanquake) Pull request description: [Mentioned](#19142 (comment)) in #19142, which removed the `boost::interruption_point()` in `ThreadImport()`. ACKs for top commit: hebasto: ACK 83fd3a6, I have reviewed the code and it looks OK, I agree it can be merged. donaloconnor: ACK 83fd3a6 laanwj: Code review ACK 83fd3a6 MarcoFalke: ACK 83fd3a6 Tree-SHA512: 0644947d669feb61eed3a944012dad1bd3dd75cf994aa2630013043c213a335b162b63e20aa37e0997740d8e3a3ec367b660b5196007a09e13f0ac455b36c821

83fd3a6 init: use std::thread for ThreadImport() (fanquake) Pull request description: [Mentioned](bitcoin#19142 (comment)) in bitcoin#19142, which removed the `boost::interruption_point()` in `ThreadImport()`. ACKs for top commit: hebasto: ACK 83fd3a6, I have reviewed the code and it looks OK, I agree it can be merged. donaloconnor: ACK 83fd3a6 laanwj: Code review ACK 83fd3a6 MarcoFalke: ACK 83fd3a6 Tree-SHA512: 0644947d669feb61eed3a944012dad1bd3dd75cf994aa2630013043c213a335b162b63e20aa37e0997740d8e3a3ec367b660b5196007a09e13f0ac455b36c821

Summary: fa1d5800d9c5e33942b76f6667839a818723dee9 > validation: Remove unused boost interruption_point > > ActivateBestChain (ABC) is only called in the "msghand" or one of the > RPC threads, neither of which is a boost::thread. However, ABC is also > called in ThreadImport (which currently happens to be a boost::thread). > In all cases, the interruption_point is redundant with the breakpoint in > ABC that triggers when ShutdownRequested() > > VerifyDB is only called in the main thread ("init") or one of the RPC > threads, neither of which is a boost::thread. fa3b4f9b8e54ec07aeb2e5e2333da3e784f7be12 > validation: Make VerifyDB level 4 interruptible This is a backport of [[bitcoin/bitcoin#19142 | core#19142]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9269

Summary: Mentioned in [[bitcoin/bitcoin#19142 | core#19142]], which removed the boost::interruption_point() in ThreadImport(). This is a backport of [[bitcoin/bitcoin#19197 | core#19197]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D9833

maflcko added the Validation label Jun 2, 2020

maflcko mentioned this pull request Jun 2, 2020

Remove unused boost/thread #18758

Merged

MarcoFalke added 2 commits June 3, 2020 06:06

validation: Make VerifyDB level 4 interruptible

fa3b4f9

maflcko force-pushed the 2006-valVerifyDbInterrupt4 branch from faed13e to fa3b4f9 Compare June 3, 2020 10:07

fanquake approved these changes Jun 4, 2020

View reviewed changes

fanquake merged commit 584170a into bitcoin:master Jun 4, 2020

maflcko deleted the 2006-valVerifyDbInterrupt4 branch June 4, 2020 14:00

This was referenced Jun 4, 2020

refactor: Make CCheckQueue RAII-styled #18731

Closed

Add local thread pool to CCheckQueue #18710

Merged

fanquake mentioned this pull request Jun 7, 2020

init: use std::thread for ThreadImport() #19197

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020

validation: Make VerifyDB level 4 interruptible

c9254a2

Github-Pull: bitcoin#19142 Rebased-From: fa3b4f9

fanquake added a commit to fanquake/bitcoin that referenced this pull request Jun 12, 2020

init: use std::thread for ThreadImport()

83fd3a6

Mentioned in bitcoin#19142, which removed the boost::interruption_point() in ThreadImport().

Bushstar mentioned this pull request Sep 9, 2021

Boost usage reduction DeFiCh/ain#714

Closed

Bushstar mentioned this pull request Sep 10, 2021

C++17 and reduce Boost usage DeFiCh/ain#701

Closed

str4d mentioned this pull request Dec 6, 2021

Backport upstream PRs that remove Boost usage zcash/zcash#4819

Open

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

validation: Make VerifyDB level 4 interruptible #19142

validation: Make VerifyDB level 4 interruptible #19142

Uh oh!

maflcko commented Jun 2, 2020

Uh oh!

maflcko commented Jun 2, 2020 •

edited

Loading

Uh oh!

fanquake commented Jun 3, 2020

Uh oh!

maflcko commented Jun 3, 2020

Uh oh!

maflcko commented Jun 3, 2020

Uh oh!

DrahtBot commented Jun 4, 2020

Uh oh!

laanwj commented Jun 4, 2020

Uh oh!

fanquake left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

validation: Make VerifyDB level 4 interruptible #19142

validation: Make VerifyDB level 4 interruptible #19142

Uh oh!

Conversation

maflcko commented Jun 2, 2020

Uh oh!

maflcko commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jun 3, 2020

Uh oh!

maflcko commented Jun 3, 2020

Uh oh!

maflcko commented Jun 3, 2020

Uh oh!

DrahtBot commented Jun 4, 2020

Conflicts

Uh oh!

laanwj commented Jun 4, 2020

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maflcko commented Jun 2, 2020 •

edited

Loading