wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off #18923

maflcko · 2020-05-09T13:12:04Z

User-facing, this is a refactor. Internally, the scheduler does not have to call a mostly empty function every half a second.

maflcko · 2020-05-09T13:13:37Z

Can be tested by applying the following diff and observing a failing test:

diff --git a/test/functional/wallet_dump.py b/test/functional/wallet_dump.py
index 6bfb468823..f30589761a 100755
--- a/test/functional/wallet_dump.py
+++ b/test/functional/wallet_dump.py
@@ -190,7 +190,7 @@ class WalletDumpTest(BitcoinTestFramework):
         assert_raises_rpc_error(-8, "already exists", lambda: self.nodes[0].dumpwallet(wallet_enc_dump))
 
         # Restart node with new wallet, and test importwallet
-        self.restart_node(0, ['-wallet=w2'])
+        self.restart_node(0, ['-wallet=w2', '-flushwallet=0'])
 
         # Make sure the address is not IsMine before import
         result = self.nodes[0].getaddressinfo(multisig_addr)

DrahtBot · 2020-05-10T03:16:39Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

multiprocess: Add bitcoin-gui -ipcconnect option #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
multiprocess: Add bitcoin-wallet -ipcconnect option #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335 (wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch by achow101)
refactor: remove ::vpwallets and related global variables #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
refactor: Move wallet methods out of chain.h and node.h #19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
test: Remove duplicate NodeContext hacks #19098 (test: Remove duplicate NodeContext hacks by ryanofsky)
Don't call lsn_reset in periodic flush #18904 (Don't call lsn_reset in periodic flush by bvbfan)
Use shared pointers only in validation interface #18354 (Use shared pointers only in validation interface by bvbfan)
Add loadwallet and createwallet load_on_startup options #15937 (Add loadwallet and createwallet load_on_startup options by ryanofsky)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

ryanofsky

Code review ACK fa6195467a3533fcdcc001d559555a86717cd5d3

src/interfaces/wallet.cpp

ryanofsky

Code review ACK 9ddddd2d927b3a752e4698c4771fc68cdc2d9698. SInce last review was changed to pass periodic flush option from WalletInit::Construct

ryanofsky

Code review ACK 00000d48588689f7576a8f4cbd9eb8edee3b53b0. No important suggestions. First two commits are the same since last review, last commit is basically the same, middle commits switch back to passing ArgsManager instead of the bool, but going through WalletContext struct from #19096

src/wallet/test/init_test_fixture.h

src/util/check.h

fanquake · 2020-06-06T06:06:49Z

Concept ACK

ryanofsky

Would be good to merge #19277 first, but code review ACK fac173681bfcd0debd7f203aeb4eb6910407cce8. Only change since last review is suggested change to assert return type

maflcko · 2020-07-04T18:01:11Z

Rebased

Its only purpose is to create a directory. So instead of keeping it alive, use it only to get the path of the directory and then create it explicitly.

This refactor does not change behavior

promag · 2020-07-09T11:14:28Z

src/wallet/bdb.cpp

-    for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); it++) {
-        if ((*it).second > 0) return false;
+    for (const auto& use_count : env->mapFileUseCount) {
+        if (use_count.second > 0) return false;


fa73493

nit, if you feel like cleaning it a bit more maybe do this below:

if (env->mapFileUseCount.erase(strFile) == 0) return false

And drop .erase(it).

Thanks, but I'll leave that for a follow-up

jnewbery

utACK fa73493

One comment on the new test. Feel free to ignore.

jnewbery · 2020-07-09T11:45:53Z

test/functional/wallet_dump.py

        assert result['ismine']

+        self.log.info('Check that wallet is flushed')
+        with self.nodes[0].assert_debug_log(['Flushing wallet.dat'], timeout=20):


I know you'll disagree, but I think it's always better to test the result of an action rather than whether it was logged. In this case, I think you could test the file modification time before and after.

I think both can be done at the same time, but asserting the modification time increases requires a time.sleep(1), since os.path.getmtime returns seconds. I don't like sleeps longer than 200 ms in tests, so I'll leave that for a follow-up ;)

No need for a follow-up. Fast tests are nice too!

meshcollider

utACK fa73493

ryanofsky

Code review ACK fa73493. Just rebased since last review

… -flushwallet is off fa73493 refactor: Use C++11 range-based for loop (MarcoFalke) fa7b164 wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off (MarcoFalke) faf8401 wallet: Pass unused args to StartWallets (MarcoFalke) fa6c186 gui tests: Limit life-time of dummy testing setup (MarcoFalke) fa28a61 test: Add smoke test to check that wallets are flushed by default (MarcoFalke) Pull request description: User-facing, this is a refactor. Internally, the scheduler does not have to call a mostly empty function every half a second. ACKs for top commit: jnewbery: utACK fa73493 meshcollider: utACK fa73493 ryanofsky: Code review ACK fa73493. Just rebased since last review Tree-SHA512: 99e1fe1b2c22a3f4b19de3e566241d38693f4fd8d5a68ba1838d86740aa6c08e3325c11a072e30fd262a8861af4278bed52eb9374c85179b8f536477f528247c

Summary: ``` User-facing, this is a refactor. Internally, the scheduler does not have to call a mostly empty function every half a second. ``` Backport of core [[bitcoin/bitcoin#18923 | PR18923]]. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8638

maflcko added Refactoring Wallet labels May 9, 2020

maflcko force-pushed the 2005-walletNoFlush branch 4 times, most recently from fa5be60 to fabdbbf Compare May 9, 2020 18:32

DrahtBot mentioned this pull request May 10, 2020

wallet: Keep inactive seeds after sethdseed and derive keys from them as needed #17681

Merged

maflcko force-pushed the 2005-walletNoFlush branch from fabdbbf to fa4dca2 Compare May 11, 2020 15:13

DrahtBot added the Needs rebase label May 22, 2020

maflcko force-pushed the 2005-walletNoFlush branch 2 times, most recently from fa07b86 to fa61954 Compare May 22, 2020 12:14

DrahtBot removed the Needs rebase label May 22, 2020

DrahtBot mentioned this pull request May 28, 2020

Remove g_rpc_chain global #19096

Merged

maflcko mentioned this pull request May 28, 2020

test: Remove duplicate NodeContext hacks #19098

Merged

This was referenced May 29, 2020

refactor: remove ::vpwallets and related global variables #19101

Merged

refactor: Move wallet methods out of chain.h and node.h #19099

Merged

ryanofsky approved these changes May 29, 2020

View reviewed changes

src/interfaces/wallet.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2005-walletNoFlush branch from fa61954 to 9ddddd2 Compare May 29, 2020 12:48

ryanofsky approved these changes May 29, 2020

View reviewed changes

This was referenced May 29, 2020

Use shared pointers only in validation interface #18354

Closed

Multiprocess bitcoin #10102

Draft

DrahtBot added the Needs rebase label Jun 5, 2020

maflcko force-pushed the 2005-walletNoFlush branch from 9ddddd2 to fa4689f Compare June 5, 2020 13:20

DrahtBot removed the Needs rebase label Jun 5, 2020

maflcko force-pushed the 2005-walletNoFlush branch from fa4689f to 00000d4 Compare June 5, 2020 17:08

ryanofsky approved these changes Jun 5, 2020

View reviewed changes

src/wallet/test/init_test_fixture.h Outdated Show resolved Hide resolved

src/util/check.h Outdated Show resolved Hide resolved

maflcko force-pushed the 2005-walletNoFlush branch from 00000d4 to faf18d6 Compare June 8, 2020 12:39

ryanofsky approved these changes Jun 23, 2020

View reviewed changes

This was referenced Jun 25, 2020

Remove the automatic creation and loading of the default wallet #15454

Merged

refactor: Drop g_orphan_list global #19374

Closed

Add loadwallet and createwallet load_on_startup options #15937

Merged

DrahtBot added the Needs rebase label Jul 1, 2020

maflcko force-pushed the 2005-walletNoFlush branch from fac1736 to fafc7d4 Compare July 4, 2020 13:08

DrahtBot removed the Needs rebase label Jul 4, 2020

This was referenced Jul 7, 2020

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

MarcoFalke added 5 commits July 9, 2020 13:07

test: Add smoke test to check that wallets are flushed by default

fa28a61

gui tests: Limit life-time of dummy testing setup

fa6c186

Its only purpose is to create a directory. So instead of keeping it alive, use it only to get the path of the directory and then create it explicitly.

wallet: Pass unused args to StartWallets

faf8401

This refactor does not change behavior

wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off

fa7b164

refactor: Use C++11 range-based for loop

fa73493

maflcko mentioned this pull request Jul 9, 2020

Refactor: clean up PeriodicFlush() #19085

Merged

maflcko force-pushed the 2005-walletNoFlush branch from fafc7d4 to fa73493 Compare July 9, 2020 11:09

promag reviewed Jul 9, 2020

View reviewed changes

jnewbery reviewed Jul 9, 2020

View reviewed changes

This was referenced Jul 9, 2020

Don't call lsn_reset in periodic flush #18904

Closed

wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335

Merged

meshcollider reviewed Jul 11, 2020

View reviewed changes

ryanofsky approved these changes Jul 11, 2020

View reviewed changes

meshcollider merged commit 5f96bce into bitcoin:master Jul 11, 2020

maflcko deleted the 2005-walletNoFlush branch July 11, 2020 11:31

hebasto mentioned this pull request Jul 24, 2020

util: Make default arg values more specific #19471

Closed

Brightside56 mentioned this pull request Apr 22, 2021

Add -flushwalletinterval command line arg #21760

Closed

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

wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off #18923

wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off #18923

Uh oh!

Conversation

maflcko commented May 9, 2020

Uh oh!

maflcko commented May 9, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fanquake commented Jun 6, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 4, 2020

Uh oh!

promag Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented May 10, 2020 •

edited

Loading

ryanofsky left a comment •

edited

Loading