wallet: Remove boost from PeriodicFlush

MarcoFalke commented at 0:31 am on April 28, 2020: member

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 #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 (...))

MarcoFalke added the label Refactoring on Apr 28, 2020

MarcoFalke commented at 0:33 am on April 28, 2020: member

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

hebasto commented at 0:41 am on April 28, 2020: member

Concept ACK.

practicalswift commented at 6:18 am on April 28, 2020: contributor

Concept ACK

MarcoFalke added this to the "In progress" column in a project

laanwj commented at 10:35 am on April 30, 2020: member

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.

MarcoFalke force-pushed on Apr 30, 2020

MarcoFalke commented at 11:05 am on April 30, 2020: member

I do not know if this is a problem

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

laanwj commented at 11:48 am on April 30, 2020: member

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

MarcoFalke force-pushed on Apr 30, 2020

MarcoFalke commented at 3:35 pm on April 30, 2020: member

Thanks, fixed.

MarcoFalke force-pushed on May 4, 2020

MarcoFalke commented at 5:56 pm on May 4, 2020: member

Rebased after silent merge conflict

DrahtBot commented at 4:34 am on May 14, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

in src/wallet/wallet.cpp:436 in fa223c318b outdated

432@@ -433,6 +433,8 @@ void CWallet::Flush(bool shutdown)
433     database->Flush(shutdown);
434 }
435 
436+std::atomic<bool> CWallet::m_end_flush{false};

jnewbery commented at 6:14 pm on May 25, 2020:

Why not use default member initialization in the header file?

MarcoFalke commented at 10:28 am on May 26, 2020:

This wouldn’t compile. Thanks C++

jnewbery commented at 6:20 pm on May 25, 2020: member

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

You say here: #18758#issue-408624391 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.

MarcoFalke commented at 10:28 am on May 26, 2020: member

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

Done

MarcoFalke commented at 10:49 am on May 26, 2020: member

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.

MarcoFalke force-pushed on May 26, 2020

MarcoFalke commented at 12:07 pm on May 26, 2020: member

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

jnewbery commented at 6:46 pm on May 26, 2020: member

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 commented at 8:08 pm on May 26, 2020: member

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.

MarcoFalke commented at 11:29 pm on May 26, 2020: member

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

MarcoFalke commented at 11:46 pm on May 26, 2020: member

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 306f71b4eb4a0fd8e64f47dc008bc235b80b13d9 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.

MarcoFalke commented at 11:54 pm on May 26, 2020: member

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

wallet: Make PeriodicFlush uninterruptible 5555d978b0

jnewbery commented at 3:32 pm on May 27, 2020: member

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 commented at 3:54 pm on May 27, 2020: member

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.

MarcoFalke force-pushed on May 27, 2020

walletdb: Remove unsed boost/thread fa7b885f51

MarcoFalke force-pushed on May 27, 2020

MarcoFalke commented at 5:48 pm on May 27, 2020: member

Simplified pull request to only remove code :rocket:

jnewbery commented at 7:59 pm on May 27, 2020: member

concept ACK just removing the boost interruption stuff. It’s essentially unused for the reason you give. We’d only ever interrupt a flush that we’re doing between two other flushes.

utACK fa7b885f51ff848d3f913bc6e15d24528300c210

MarcoFalke commented at 11:25 am on May 28, 2020: member

@laanwj @hebasto Mind taking another look here?

MarcoFalke added the label Wallet on May 28, 2020

MarcoFalke removed the label Refactoring on May 28, 2020

fanquake changes_requested

fanquake commented at 9:42 am on June 2, 2020: member

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

https://github.com/bitcoin/bitcoin/blob/44307449f758b239fca0ad6b722bc36795ac6727/src/wallet/walletdb.cpp#L890-L893

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 fa7b885f51ff848d3f913bc6e15d24528300c210.

wallet: Remove unused boost::thread_interrupted

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

fa1c74fd03

MarcoFalke commented at 1:14 pm on June 2, 2020: member

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

fanquake approved

fanquake commented at 2:08 pm on June 2, 2020: member

ACK fa1c74fd0342b74d44cc4e41fff3890c1434e8f7

fanquake merged this on Jun 2, 2020

fanquake closed this on Jun 2, 2020

MarcoFalke deleted the branch on Jun 2, 2020

sidhujag referenced this in commit e2b11597e6 on Jun 2, 2020

ComputerCraftr referenced this in commit 48199f2dc7 on Jun 16, 2020

deadalnix referenced this in commit 4a00dfee75 on Dec 7, 2020

fanquake moved this from the "In progress" to the "Done" column in a project

kittywhiskers referenced this in commit 04af982450 on Jul 13, 2021

kittywhiskers referenced this in commit f7d037ef66 on Jul 13, 2021

kittywhiskers referenced this in commit d3460a1996 on Jul 13, 2021

kittywhiskers referenced this in commit dfcec5c8be on Jul 13, 2021

kittywhiskers referenced this in commit 4406f8433f on Jul 15, 2021

kittywhiskers referenced this in commit 910c967a78 on Jul 15, 2021

kittywhiskers referenced this in commit 52d7f810f1 on Jul 15, 2021

kittywhiskers referenced this in commit 1c6c1279cb on Jul 15, 2021

kittywhiskers referenced this in commit c0084d934c on Jul 15, 2021

kittywhiskers referenced this in commit db248e82a0 on Jul 16, 2021

UdjinM6 referenced this in commit 730a89f8ed on Jul 16, 2021

DrahtBot locked this on Feb 15, 2022

wallet: Remove boost from PeriodicFlush #18792

Conflicts