wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off

MarcoFalke commented at 1:12 pm on May 9, 2020: member

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

MarcoFalke commented at 1:13 pm on May 9, 2020: member

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

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

MarcoFalke added the label Refactoring on May 9, 2020

MarcoFalke added the label Wallet on May 9, 2020

MarcoFalke force-pushed on May 9, 2020

DrahtBot commented at 3:16 am on May 10, 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:

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

MarcoFalke force-pushed on May 11, 2020

DrahtBot added the label Needs rebase on May 22, 2020

MarcoFalke force-pushed on May 22, 2020

DrahtBot removed the label Needs rebase on May 22, 2020

in src/interfaces/wallet.cpp:494 in faccdaba58 outdated

490@@ -491,7 +491,7 @@ class WalletClientImpl : public ChainClient
491     }
492     bool verify() override { return VerifyWallets(m_chain, m_wallet_filenames); }
493     bool load() override { return LoadWallets(m_chain, m_wallet_filenames); }
494-    void start(CScheduler& scheduler) override { return StartWallets(scheduler); }
495+    void start(CScheduler& scheduler, const ArgsManager& argsman) override { return StartWallets(scheduler, argsman); }

ryanofsky commented at 11:49 am on May 29, 2020:

In commit “wallet: Pass unused argsman to StartWallets()” (faccdaba583288273399481b1f7892a910080fda)

Not a big deal and I can fix it later, but this is a bit broken with #10102 because it is trying to pass a whole argsman object from the node to wallet process just to pass a single bool. (See “Interface method parameter and return types should either be serializable or be other interface classes” examples in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines)

Recommended fix for this would be to pass -flushwallet bool setting to StartWallets the same way the -wallet list setting gets passed to LoadWallets above: first read in WalletInit::Construct and passed as an argument to MakeWalletClient, stored in the WalletClientImpl instance, and used as needed

MarcoFalke commented at 12:49 pm on May 29, 2020:

Thanks, tried to implement your suggestion in branch 9ddddd2d927b3a752e4698c4771fc68cdc2d9698

ryanofsky commented at 1:33 pm on May 29, 2020:

Thanks, tried to implement your suggestion in branch 9ddddd2

Branch looks good. Current version is fine too in case you prefer that or the suggestion doesn’t work.

I was thinking maybe the best another approach after #19096 would be to add an ArgsManager pointer to the WalletContext struct, and initializing it from a new MakeWalletClient ArgsManager& argument so the wallet filenames and periodic flush arguments and temporary storage could be dropped

MarcoFalke commented at 5:21 pm on June 5, 2020:

add an ArgsManager pointer to the WalletContext struct

thx, done

ryanofsky approved

ryanofsky commented at 11:57 am on May 29, 2020: member

Code review ACK fa6195467a3533fcdcc001d559555a86717cd5d3

MarcoFalke force-pushed on May 29, 2020

ryanofsky approved

ryanofsky commented at 1:40 pm on May 29, 2020: member

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

DrahtBot added the label Needs rebase on Jun 5, 2020

MarcoFalke force-pushed on Jun 5, 2020

DrahtBot removed the label Needs rebase on Jun 5, 2020

MarcoFalke force-pushed on Jun 5, 2020

in src/wallet/test/init_test_fixture.h:21 in fa00c37cba outdated

17@@ -18,7 +18,6 @@ struct InitWalletDirTestingSetup: public BasicTestingSetup {
18     fs::path m_datadir;
19     fs::path m_cwd;
20     std::map<std::string, fs::path> m_walletdir_path_cases;
21-    NodeContext m_node;

ryanofsky commented at 6:27 pm on June 5, 2020:

In commit “test: Avoid overwriting the NodeContext member of the testing setup” (fa00c37cba98794c1c12c4b77e885d831d9bf183)

Wow, good catches. I wonder if there was a compiler warning we could enable that would have caught these.

in src/util/check.h:47 in fa38755d2f outdated

41@@ -42,4 +42,12 @@ class NonFatalCheckError : public std::runtime_error
42         }                                                         \
43     } while (false)
44 
45+/** Assert that the passed pointer is not nullptr and return a reference to the pointed object */
46+template <typename PtrT>
47+typename std::remove_reference<decltype(*PtrT{})>::type& Ensure(const PtrT& ptr)

ryanofsky commented at 6:46 pm on June 5, 2020:

In commit “wallet: Pass unused args to WalletClientImpl” (fa38755d2fc444589ae506e4edd8597ab25e55e9)

Slight preference for calling this AssertNotNull instead of Ensure just because it could wind up being used widely, and it’d be nice for it to be obvious what it does.

Also for code review purposes, C++ programmers are already pretty trained to know that *ptr is dangerous and can kill the program, while Ensure(ptr) might look more innocuous if it’s part of a larger expression. And since the name doesn’t contain “Assert” or “Abort”, it might actually be clearer if this returned a pointer so the caller is forced to write *. Could also go more generic and provide

0template<typename T> T&& Assert(T&& value) { assert(value); return std::forward<T>(value); }

ryanofsky approved

ryanofsky commented at 6:51 pm on June 5, 2020: member

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

fanquake commented at 6:06 am on June 6, 2020: member

Concept ACK

MarcoFalke force-pushed on Jun 8, 2020

MarcoFalke commented at 12:42 pm on June 8, 2020: member

commit id 00000d4 was lost in a rebase

ryanofsky approved

ryanofsky commented at 4:45 pm on June 9, 2020: member

Code review ACK faf18d6c86cbb3e90070a971ee341cc76fb1a8f5. Changes since previous review were replacing Ensure with Assert, and adding new checks to ensure the NDEBUG is disabled and that assert isn’t called from RPC code. First two commits are new, previous shadow fix commit became #19188, and the other commits are basically unchanged except being divided differently and replacing assert with ensure. Long list. Nice cleanups, though

in src/util/check.h:51 in fa54cf1073 outdated

45@@ -46,4 +46,12 @@ class NonFatalCheckError : public std::runtime_error
46 #error "Cannot compile without assertions!"
47 #endif
48 
49+/** Identity function. Abort if the value compares equal to zero */
50+template <typename T>
51+T&& Assert(T&& value)

ryanofsky commented at 5:15 pm on June 9, 2020:

In commit “util: Add Assert identity function” (fa54cf107319609ff44de960b987bd09b46945d0)

Thinking about this, I think it would be better to change signature to T Assert(T&& value) to avoid a possibility of returning a reference to a temporary. Contrived example but I think p would be an invalid reference here returning T&& instead of T:

0std::unique_ptr<int>&& p = Assert(MakeUnique<int>(5));

No other change should be needed to fix this other than removing && from return type.

MarcoFalke force-pushed on Jun 9, 2020

MarcoFalke commented at 6:50 pm on June 14, 2020: member

I’ve split out the first commits into #19277

ryanofsky approved

ryanofsky commented at 7:53 pm on June 23, 2020: member

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

DrahtBot added the label Needs rebase on Jul 1, 2020

MarcoFalke force-pushed on Jul 4, 2020

DrahtBot removed the label Needs rebase on Jul 4, 2020

MarcoFalke commented at 6:01 pm on July 4, 2020: member

Rebased

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

gui tests: Limit life-time of dummy testing setup

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.

fa6c186436

wallet: Pass unused args to StartWallets

This refactor does not change behavior

faf8401c19

wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off fa7b164d62

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

MarcoFalke force-pushed on Jul 9, 2020

in src/wallet/bdb.cpp:627 in fa73493930

622@@ -623,8 +623,8 @@ bool BerkeleyDatabase::PeriodicFlush()
623     if (!lockDb) return false;
624 
625     // Don't flush if any databases are in use
626-    for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); it++) {
627-        if ((*it).second > 0) return false;
628+    for (const auto& use_count : env->mapFileUseCount) {
629+        if (use_count.second > 0) return false;

promag commented at 11:14 am on July 9, 2020:

fa73493930e35850e877725167dc9d42e47015c8

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

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

And drop .erase(it).

MarcoFalke commented at 11:21 am on July 9, 2020:

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

in test/functional/wallet_dump.py:206 in fa73493930

201@@ -202,5 +202,10 @@ def run_test(self):
202         result = self.nodes[0].getaddressinfo(multisig_addr)
203         assert result['ismine']
204 
205+        self.log.info('Check that wallet is flushed')
206+        with self.nodes[0].assert_debug_log(['Flushing wallet.dat'], timeout=20):

jnewbery commented at 11:45 am on July 9, 2020:

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.

MarcoFalke commented at 12:16 pm on July 9, 2020:

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 ;)

jnewbery commented at 12:45 pm on July 9, 2020:

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

jnewbery commented at 12:02 pm on July 9, 2020: member

utACK fa73493930e35850e877725167dc9d42e47015c8

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

meshcollider commented at 10:51 am on July 11, 2020: contributor

utACK fa73493930e35850e877725167dc9d42e47015c8

ryanofsky approved

ryanofsky commented at 11:08 am on July 11, 2020: member

Code review ACK fa73493930e35850e877725167dc9d42e47015c8. Just rebased since last review

meshcollider merged this on Jul 11, 2020

meshcollider closed this on Jul 11, 2020

MarcoFalke deleted the branch on Jul 11, 2020

sidhujag referenced this in commit c96248b149 on Jul 11, 2020

Fabcien referenced this in commit 912bd27d4b on Dec 9, 2020

DrahtBot locked this on Feb 15, 2022

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

Conflicts