wallet: Remove boost from PeriodicFlush #18792

maflcko · 2020-04-28T00:31:45Z

The boost::this_thread::interruption_point() in the code base currently block the replacement of boost::thread with std::thread. [1]

Remove them from the wallet because they are either unused or useless.

The feature to interrupt a periodic flush is useless because all wallets have just been flushed

bitcoin/src/init.cpp

Line 194 in 9ccaee1

client->flush();

and another flush should be a noop. Also, they will be flushed again shortly after

bitcoin/src/init.cpp

Line 285 in 9ccaee1

client->stop();

, so even if repeated flushes weren't a noop, doing 3 instead of 2 shouldn't matter too much at this point. Also, the wallet is flushed every two seconds in the worst case, so if this is an expensive operation, that period should be readjusted. (Or bdb should be removed altogether #18916)

[1] Replacement of boost::thread with std::thread should happen because:

The boost thread dependency is slow to compile
Boost thread is less maintained than the standard lib
Boost thread is mostly redundant to the standard lib
Global interruption points via exceptions are hard to keep track of during review and easy to get wrong during runtime (e.g. accidental catch (...))

maflcko · 2020-04-28T00:33:19Z

Not sure how to test manually, but I have included a unit test

hebasto · 2020-04-28T00:41:11Z

Concept ACK.

practicalswift · 2020-04-28T06:18:58Z

Concept ACK

laanwj · 2020-04-30T10:35:50Z

The main thing to review here is the change in behavior: interruption_point raises an exception that escalates all the way to the thread main level, while the new code returns false, which is handled differently. For example it will still continue iterating over for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {. so dbh.nLastSeen and dbh.nLastWalletUpdate will be updated for all of them. I do not know if this is a problem.

maflcko · 2020-04-30T11:05:10Z

I do not know if this is a problem

Me neither, but it seems avoidable by adding one more if-check. Done that.

laanwj · 2020-04-30T11:48:43Z

Shouldn't the check be at the beginning of the for loop instead of of MaybeCompactWalletDB in that case?

maflcko · 2020-04-30T15:35:29Z

Thanks, fixed.

maflcko · 2020-05-04T17:56:50Z

Rebased after silent merge conflict

DrahtBot · 2020-05-14T04:34: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:

wallettool: Add dump and createfromdump commands #19137 (wallettool: Add dump and createfromdump commands by achow101)
wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102 (wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101)
wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
wallet: Refactor the classes in wallet/db.{cpp/h} #18971 (wallet: Refactor the classes in wallet/db.{cpp/h} by achow101)
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.

jnewbery

Concept ACK. Can you add a description for this PR?

You say here: #18758 (comment) that these interruption points can't be used. Were they used at some point in the past and were broken? If so, have you been able to determine which commit broke them? If not, I don't think it's worth adding the extra shutdown handling in this PR. It could potentially be added in a future PR.

You can remove the #include <boost/thread.hpp> from walletdb.cpp and db.cpp.

jnewbery · 2020-05-25T18:14:23Z

src/wallet/wallet.cpp

Why not use default member initialization in the header file?

This wouldn't compile. Thanks C++

maflcko · 2020-05-26T10:28:49Z

Concept ACK. Can you add a description for this PR?

Done

maflcko · 2020-05-26T10:49:24Z

You say here: #18758 (comment) that these interruption points can't be used.

Sorry, that pull is still marked as draft and is not ready for review. The decision boils down to "Is the interruption point run in a std::thread? If yes, the interruption point is unused".

Here, the scheduler is (still) a boost::thread, so the point should not be unused.

maflcko · 2020-05-26T12:07:13Z

Force pushed to address feedback by @jnewbery (remove boost include from db.cpp)

jnewbery · 2020-05-26T18:46:21Z

Force pushed to address feedback by @jnewbery (remove boost include from db.cpp)

I also suggested removing it from walletdb.cpp. Is that not possible?

jnewbery · 2020-05-26T20:08:01Z

the scheduler is (still) a boost::thread, so the point should not be unused.

From a quick read of https://www.boost.org/doc/libs/1_53_0/doc/html/thread/thread_management.html#thread.thread_management.tutorial.interruption, it seems to me that the boost::thread::interruption_point only does anything if that thread's interrupt() function is called.

The only place that happens is the threadGroup.interrupt_all() call in Shutdown(), which (since #18234) happens after the scheduler thread is stopped.

I haven't looked deeply enough into #18234 to figure out whether that's a regression. If not, I think we can just remove all of these interrupt points that no longer do anything. If it is a regression, then perhaps this PR should explicitly state that it's a bug fix.

maflcko · 2020-05-26T23:29:18Z

walletdb.cpp is still using it https://github.com/bitcoin/bitcoin/blob/fabd1c91b4c715f0b0cfa28457cf766246568449/src/wallet/walletdb.cpp#L742

maflcko · 2020-05-26T23:46:55Z

The only place that happens is the threadGroup.interrupt_all() call in Shutdown(), which (since #18234) happens after the scheduler thread is stopped.

It looks like 306f71b tells the scheduler to stop after the current task is executed or before the execution starts (i.e. the scheduler is waiting for the next flush). The call is non-blocking when a task is currently executed. Consequently, the next line will interrupt all threads in the thread group, which will also interrupt the periodic flush action in case it is currently running. So I don't think the behaviour has changed in that commit, at least not in regard to flushing? Correct me if I am wrong.

maflcko · 2020-05-26T23:54:37Z

I start to wonder if it is worth to abort the periodic flush when flush(true) is called shortly after in StopWallets.

jnewbery · 2020-05-27T15:32:23Z

walletdb.cpp is still using it ...

That's a catch in WalletBatch::LoadWallet(), which I think is only called in the main thread or rpc thread.

jnewbery · 2020-05-27T15:54:39Z

It looks like 306f71b tells the scheduler to stop after the current task is executed or before the execution starts (i.e. the scheduler is waiting for the next flush). The call is non-blocking when a task is currently executed. Consequently, the next line will interrupt all threads in the thread group, which will also interrupt the periodic flush action in case it is currently running. So I don't think the behaviour has changed in that commit, at least not in regard to flushing? Correct me if I am wrong.

No, you're right. I misunderstood what stop() was doing.

I start to wonder if it is worth to abort the periodic flush when flush(true) is called shortly after in StopWallets.

Yes, it does seem unnecessary. We call flush() on all wallets before interrupting the scheduler thread:

https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L194

then we interrupt the scheduler thread (which may cause us to interrupt a periodic flush), and then we flush again on shutdown:

https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L285

Making that middle flush interruptible does seem unnecessary.

maflcko · 2020-05-28T11:25:08Z

@laanwj @hebasto Mind taking another look here?

fanquake

ACK the change - however there's another boost::thread_interrupted& in walletdb.cpp:

bitcoin/src/wallet/walletdb.cpp

Lines 890 to 893 in 4430744

    
           } 
        
           catch (const boost::thread_interrupted&) { 
        
               throw; 
        
           }

I think it can also be removed, but if not, I'm not sure how we can drop the <boost/thread.hpp> include as unused in fa7b885.

FindWalletTx is only called by zapwallet, which is never called in a boost::thread

maflcko · 2020-06-02T13:14:48Z

@fanquake Good catch. Added a commit with rationale to remove that as well.

fanquake

ACK fa1c74f

fa1c74f wallet: Remove unused boost::thread_interrupted (MarcoFalke) fa7b885 walletdb: Remove unsed boost/thread (MarcoFalke) 5555d97 wallet: Make PeriodicFlush uninterruptible (MarcoFalke) Pull request description: The `boost::this_thread::interruption_point()` in the code base currently block the replacement of `boost::thread` with `std::thread`. [1] Remove them from the wallet because they are either unused or useless. The feature to interrupt a periodic flush is useless because all wallets have just been flushed https://github.com/bitcoin/bitcoin/blob/9ccaee1d5e2e4b79b0a7c29aadb41b97e4741332/src/init.cpp#L194 and another flush should be a noop. Also, they will be flushed again shortly after https://github.com/bitcoin/bitcoin/blob/9ccaee1d5e2e4b79b0a7c29aadb41b97e4741332/src/init.cpp#L285, so even if repeated flushes weren't a noop, doing 3 instead of 2 shouldn't matter too much at this point. Also, the wallet is flushed every two seconds in the worst case, so if this is an expensive operation, that period should be readjusted. (Or bdb should be removed altogether bitcoin#18916) [1] Replacement of `boost::thread` with `std::thread` should happen because: * The boost thread dependency is slow to compile * Boost thread is less maintained than the standard lib * Boost thread is mostly redundant to the standard lib * Global interruption points via exceptions are hard to keep track of during review and easy to get wrong during runtime (e.g. accidental `catch (...)`) ACKs for top commit: fanquake: ACK fa1c74f Tree-SHA512: b166619256de2ef4325480fa1367f68bc9371ad785ec503aed61eab41ba61f1a9807aab25451a24efda3db64855c9ba0025645b98bc58557bc3ec56c5b3297d0

Summary: ``` The boost::this_thread::interruption_point() in the code base currently block the replacement of boost::thread with std::thread. [1] Remove them from the wallet because they are either unused or useless. ``` Backport of core [[bitcoin/bitcoin#18792 | PR18792]]. Depends on D8615. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8616

Co-authored-by: UdjinM6 <[email protected]>

merge bitcoin#13671, bitcoin#17405, bitcoin#18786, bitcoin#18792, bitcoin#20067: Deboostification

maflcko added the Refactoring label Apr 28, 2020

maflcko mentioned this pull request Apr 28, 2020

Remove unused boost/thread #18758

Merged

maflcko force-pushed the 2004-walletNoBoostPeriodicFlush branch from fae3bd8 to fa900e6 Compare April 30, 2020 11:01

maflcko force-pushed the 2004-walletNoBoostPeriodicFlush branch from fa900e6 to faddf20 Compare April 30, 2020 11:58

maflcko force-pushed the 2004-walletNoBoostPeriodicFlush branch from faddf20 to fa223c3 Compare May 4, 2020 17:54

DrahtBot mentioned this pull request May 14, 2020

wallet: Refactor the classes in wallet/db.{cpp/h} #18971

Closed

8 tasks

jnewbery reviewed May 25, 2020

View reviewed changes

maflcko force-pushed the 2004-walletNoBoostPeriodicFlush branch from fa223c3 to fabd1c9 Compare May 26, 2020 10:51

wallet: Make PeriodicFlush uninterruptible

5555d97

maflcko force-pushed the 2004-walletNoBoostPeriodicFlush branch from fabd1c9 to e8a8e37 Compare May 27, 2020 16:28

walletdb: Remove unsed boost/thread

fa7b885

jnewbery mentioned this pull request May 28, 2020

Refactor: clean up PeriodicFlush() #19085

Merged

maflcko added Wallet and removed Refactoring labels May 28, 2020

This was referenced May 29, 2020

wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102

Merged

wallettool: Add dump and createfromdump commands #19137

Merged

fanquake requested changes Jun 2, 2020

View reviewed changes

wallet: Remove unused boost::thread_interrupted

fa1c74f

FindWalletTx is only called by zapwallet, which is never called in a boost::thread

fanquake approved these changes Jun 2, 2020

View reviewed changes

fanquake merged commit 5879bfa into bitcoin:master Jun 2, 2020

maflcko deleted the 2004-walletNoBoostPeriodicFlush branch June 2, 2020 14:38

kwvg added a commit to kwvg/dash that referenced this pull request Jul 15, 2021

merge bitcoin#18792: Remove boost from PeriodicFlush

52d7f81

Co-authored-by: UdjinM6 <[email protected]>

kwvg added a commit to kwvg/dash that referenced this pull request Jul 16, 2021

merge bitcoin#18792: Remove boost from PeriodicFlush

db248e8

Co-authored-by: UdjinM6 <[email protected]>

UdjinM6 added a commit to dashpay/dash that referenced this pull request Jul 16, 2021

Merge pull request #4246 from kittywhiskers/deboostification

730a89f

merge bitcoin#13671, bitcoin#17405, bitcoin#18786, bitcoin#18792, bitcoin#20067: Deboostification

This was referenced Sep 9, 2021

Boost usage reduction DeFiCh/ain#714

Closed

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

wallet: Remove boost from PeriodicFlush #18792

wallet: Remove boost from PeriodicFlush #18792

Uh oh!

Conversation

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

Uh oh!

Uh oh!

maflcko commented Apr 28, 2020

Uh oh!

hebasto commented Apr 28, 2020

Uh oh!

practicalswift commented Apr 28, 2020

Uh oh!

laanwj commented Apr 30, 2020

Uh oh!

maflcko commented Apr 30, 2020

Uh oh!

laanwj commented Apr 30, 2020

Uh oh!

maflcko commented Apr 30, 2020

Uh oh!

maflcko commented May 4, 2020

Uh oh!

DrahtBot commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery May 25, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko May 26, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 26, 2020

Uh oh!

maflcko commented May 26, 2020

Uh oh!

maflcko commented May 26, 2020

Uh oh!

jnewbery commented May 26, 2020

Uh oh!

jnewbery commented May 26, 2020

Uh oh!

maflcko commented May 26, 2020

Uh oh!

maflcko commented May 26, 2020

Uh oh!

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

Uh oh!

Uh oh!

jnewbery commented May 27, 2020

Uh oh!

jnewbery commented May 27, 2020

Uh oh!

maflcko commented May 28, 2020

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 2, 2020

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

maflcko commented Apr 28, 2020 •

edited

Loading

DrahtBot commented May 14, 2020 •

edited

Loading

maflcko commented May 26, 2020 •

edited

Loading