[MOVEONLY] Move some static functions out of wallet.h/cpp #10976

ryanofsky · 2017-08-02T12:06:22Z

This just moves some static wallet fee and init functions out of wallet/wallet.cpp and into new wallet/fees.cpp and wallet/init.cpp source files. There is one commit updating declarations and callers, followed by two MOVEONLY commits actually moving the function bodies.

This change is desirable because wallet.h/cpp are monolithic and hard to navigate, so pulling things out and grouping together pieces of related functionality should improve the organization.

Another motivation is the wallet process separation work in #10973, where (at least initially) parameter parsing and fee estimation are still done in the main process rather than the wallet process, and having functions that run in different processes scrambled up throughout wallet.cpp is unnecessarily confusing.

ryanofsky · 2017-08-02T12:14:14Z

@laanwj, if you think this change is a good idea, it might be worth merging before 0.15 is branched, since it moves some functions around. I don't think it conflicts with other tagged prs, but if it does I'd be happy to rebase this on top of them.

jnewbery · 2017-08-02T16:59:50Z

Seems to be largely the same as #10767 ?

ryanofsky · 2017-08-02T17:32:47Z

Seems to be largely the same as #10767 ?

This covers fee code as well as init code, and I think the cleanups in #10767 are good but also that it's nice to start with a PR that's 100% MOVEONLY (or as close as possible). This way both the cleanup changes and moveonly changes are simpler to evaluate, and the MOVEONLY changes have (hopefully) a chance of getting merged before the branch.

jnewbery

ACK 4298d90.

I'd prefer to name the new files walletinit.cpp and walletinit.h just to avoid any ambiguity with src/init.cpp

jnewbery · 2017-08-02T21:44:43Z

src/wallet/rpcwallet.cpp

 #include "wallet/wallet.h"
 #include "wallet/walletdb.h"

+#include <init.h>  // For StartShutdown


This is needed to pull in src/init.h instead of src/wallet/init.h. Probably all of our code should be using <> instead of "" because we don't generally include relative to current directory.

jonasschnelli · 2017-08-03T09:36:22Z

Concept ACK

promag

The only static function staying is CreateWalletFromFile() which is a factory for CWallet and requires access to private members, so makes sense to keep it there.

Agree with @jnewbery regarding file names, but to be consistent to rpcwallet.*, initwallet.* sounds better.

ACK 4298d90.

promag · 2017-08-06T13:58:53Z

src/wallet/init.h

+//  being loaded (CWallet::ParameterInteraction forbids -salvagewallet, -zapwallettxes or -upgradewallet with multiwallet).
+bool WalletVerify();
+
+//! Load wallet databases


Nit, new comment so add missing period? The other comments without periods are move-only.

Added in aee0e0f

jnewbery · 2017-08-06T22:25:21Z

@laanwj - as @ryanofsky says, this is another move/refactor PR that would be good to get in before branching v0.15, to help with backporting. But maybe there are already too many of those competing for your attention!

(I'd still like the files to be renamed initwallet.{cpp,h} before this gets merged)

maflcko · 2017-08-06T22:53:28Z

We can always backport the commits right after release of 0.15.0 to the 0.15 branch. No need to rush, imo.

This commit just moves a few function declarations and updates callers. Function bodies are moved in two followup MOVEONLY commits. This change is desirable because wallet.h/cpp are monolithic and hard to navigate, so pulling things out and grouping together pieces of related functionality should improve the organization. Another proximate motivation is the wallet process separation work in bitcoin#10973, where (at least initially) parameter parsing and fee estimation are still done in the main process rather than the wallet process, and having functions that run in different processes scrambled up throughout wallet.cpp is unnecessarily confusing.

ryanofsky

Rebased 4298d90 -> 5461f69 (pr/wmove.2 -> pr/wmove.3) because of conflicts with gArgs PR.
Added 1 commit 5461f69 -> aee0e0f (pr/wmove.3 -> pr/wmove.4, compare)
Squashed aee0e0f -> f01103c (pr/wmove.4 -> pr/wmove.5)

I'd prefer to name the new files walletinit.cpp and walletinit.h just to avoid any ambiguity with src/init.cpp

to be consistent to rpcwallet.*, initwallet.* sounds better.

I don't think this is a good rationale for renaming. Including relative to current directory is generally bad practice in c and c++, because it makes code harder to understand, and can lead to accidental breakages or strange requirements like filenames throughout the directory tree having to be globally unique. In bitcoin, we mostly avoid including relative to current directory (hence #include "policy/fees.h" and #include "wallet/feebumper.h" instead of #include "fees.h" and #include "feebumper.h"). In other large codebases, including relative to current directory is forbidden outright.

In the src/wallet directory, there are currently 7 header files. 5 of them have no redundant wallet prefix or suffix, 1 of them has a redundant wallet prefix, and 1 has a redundant wallet suffix. In other bitcoin source directories like src/policy, src/primitives, and src/rpc, there are no redundant prefixes or suffixes at all. I would prefer also not to add redundant prefixes or suffixes here.

ryanofsky · 2017-08-14T19:44:40Z

src/wallet/rpcwallet.cpp

 #include "wallet/wallet.h"
 #include "wallet/walletdb.h"

+#include <init.h>  // For StartShutdown


This is needed to pull in src/init.h instead of src/wallet/init.h. Probably all of our code should be using <> instead of "" because we don't generally include relative to current directory.

ryanofsky · 2017-08-14T20:21:46Z

src/wallet/init.h

+//  being loaded (CWallet::ParameterInteraction forbids -salvagewallet, -zapwallettxes or -upgradewallet with multiwallet).
+bool WalletVerify();
+
+//! Load wallet databases


Added in aee0e0f

jnewbery · 2017-08-14T21:01:10Z

it makes code harder to understand

Can you explain this?

In fact, there are only a couple of places where non-test files are not named uniquely within the directory tree:

→ find -name *cpp -printf '%f\n' | sort | uniq -c | sort
...
      1 zmqpublishnotifier.cpp
      2 base58.cpp
      2 crypto_tests.cpp
      2 lockedpool.cpp
      2 net.cpp
      2 protocol.cpp
→ find -name base58.cpp
./src/base58.cpp
./src/bench/base58.cpp
→ find -name crypto_tests.cpp
./src/wallet/test/crypto_tests.cpp
./src/test/crypto_tests.cpp
→ find -name lockedpool.cpp
./src/support/lockedpool.cpp
./src/bench/lockedpool.cpp
→ find -name net.cpp
./src/net.cpp
./src/rpc/net.cpp
→ find -name protocol.cpp
./src/rpc/protocol.cpp
./src/protocol.cpp

I prefer walletinit.cpp, but I'll defer to yours and others' C++ expertise on this.

ryanofsky · 2017-08-14T21:10:23Z

it makes code harder to understand

Can you explain this?

#include "policy/fees.h" is easier to understand than #include "fees.h" because it provides more context.

In fact, there are only a couple of places where non-test files are not named uniquely within the directory tree:

I've never worked on any project that required source filenames to be globally unique, so I'm not surprised that you found source filenames are not globally unique in bitcoin.

laanwj · 2017-08-15T07:33:28Z

I don't think this is a good rationale for renaming. Including relative to current directory is generally bad practice in c and c++, because it makes code harder to understand,

Unfortunately, relative inclusion is what we do in bitcoin by using #include "" instead of #include <> everywhere. Heck, there have been well-meaning PRs that change to #include "" for "consistency" (see #10752).

I agree it's generally a bad idea. It can lead to confusion, as well as makes the compiler do a lot of 'probing' where files are (so slows down compilation). But not something we're going to get rid of from one moment to another. On the longer run I hope we do that.

So: right now if you were to include "init.h" in "wallet.h", it gets "wallet/init.h" instead of the main one (which is what happens in rpcwallet). Yes, this is confusing. I'd also prefer unique naming of header files for now, for that reason. Another vote to rename it to walletinit.h.

promag · 2017-08-15T09:45:22Z

I also prefer <> includes with full project path instead of prefixes or whatever.

laanwj · 2017-08-15T10:03:56Z

I also prefer <> includes with full project path instead of prefixes or whatever.

The funniest thing is that that's how we're using #include "". I would guess a script-diff that replaces "..." in #include lines with <...> would build with only few changes (maybe some exceptions such as including generated test data). We hardly, if ever use "" for actual relative include.

Edit: working on this

ryanofsky · 2017-08-25T18:57:16Z

Could this be merged? It has 2 code review acks and a concept ack and is mostly moveonly.

#11053 in it's current form or one of the variations proposed in #11053 (comment) should moot whatever concerns there might be about includes going forward.

jnewbery · 2017-08-25T19:07:22Z

I withdraw my objection to the filename init.cpp. utACK

f01103c MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (Russell Yanofsky) e7fe320 MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (Russell Yanofsky) d97fe20 Move some static functions out of wallet.h/cpp (Russell Yanofsky) Pull request description: This just moves some static wallet fee and init functions out of `wallet/wallet.cpp` and into new `wallet/fees.cpp` and `wallet/init.cpp` source files. There is one commit updating declarations and callers, followed by two MOVEONLY commits actually moving the function bodies. This change is desirable because wallet.h/cpp are monolithic and hard to navigate, so pulling things out and grouping together pieces of related functionality should improve the organization. Another motivation is the wallet process separation work in #10973, where (at least initially) parameter parsing and fee estimation are still done in the main process rather than the wallet process, and having functions that run in different processes scrambled up throughout wallet.cpp is unnecessarily confusing. Tree-SHA512: 6e6982ff82b2ab4e681c043907e2b1801ceb9513394730070f16c46ad338278a863f5b3759aa13db76a259b268b1c919c81f4e339f0796a3cfb990161e8c316d

maflcko · 2017-08-25T20:15:43Z

src/wallet/init.h

+
+//! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
+//  This function will perform salvage on the wallet if requested, as long as only one wallet is
+//  being loaded (CWallet::ParameterInteraction forbids -salvagewallet, -zapwallettxes or -upgradewallet with multiwallet).


nit: doc string needs fixup

maflcko · 2017-08-25T20:21:57Z

Post merge utACK f01103c. Might want to address the nit sometime

…let.h/cpp bitcoin#10976 Move some static functions out of wallet.h/cpp This commit just moves a few function declarations and updates callers. Function bodies are moved in two followup MOVEONLY commits. This change is desirable because wallet.h/cpp are monolithic and hard to navigate, so pulling things out and grouping together pieces of related functionality should improve the organization. Another proximate motivation is the wallet process separation work in bitcoin#10973, where (at least initially) parameter parsing and fee estimation are still done in the main process rather than the wallet process, and having functions that run in different processes scrambled up throughout wallet.cpp is unnecessarily confusing. MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp make it actual move only Signed-off-by: Pasta <[email protected]> MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp make it actual move only Signed-off-by: Pasta <[email protected]> add keepass include Signed-off-by: Pasta <[email protected]>

d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider) 3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider) dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli) 20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky) e411b70 [wallet] Fix leak in CDB constructor (João Barbosa) f15aeea Change getmininginfo errors field to warnings (Andrew Chow) c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra) 1d966ce Add warnings field to getblockchaininfo (Andrew Chow) ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra) e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra) e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra) 2188c3e Move some static functions out of wallet.h/cpp (random-zebra) f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra) 8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra) 900bbfa Reject invalid wallet files (João Barbosa) a1f4e2a Reject duplicate wallet filenames (random-zebra) ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra) 37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra) 3955ee9 [Doc] Update release notes (random-zebra) 4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery) 1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery) fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery) cf4a90b Remove factor of 3 from definition of dust. (random-zebra) a1c56fd [Policy] Introduce -dustrelayfee (random-zebra) 9fb29cc [Doc] Update multiwallet release notes (random-zebra) 379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra) 808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra) f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery) 2e02006 Rename -usewallet to -rpcwallet (Alex Morcos) a64440b Select wallet based on the given endpoint (Jonas Schnelli) 5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra) b0fe92f Fix test_pivx circular dependency issue (random-zebra) 6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) 7dd3916 Register wallet endpoint (Jonas Schnelli) 5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos) 41a7335 Remove unused variables (practicalswift) 5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky) e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra) 54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra) 494ba64 [test] Add test for getmemoryinfo (random-zebra) 2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders) 7d977ac Remove unused C++ code not covered by unit tests (random-zebra) 60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar) 3633d75 Initialize nRelockTime (Patrick Strateman) 3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra) f219be9 Add safe flag to listunspent result (NicolasDorier) 0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra) 75c8c6d Disallow copy of CReserveKeys (Gregory Sanders) 8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra) Pull request description: I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us. I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files). This changeset is based on top of - [x] #2337 as it keeps going with the multiwallet implementation, adding: - RPC endpoint support - `listwallets` RPC - wallet name in `getwalletinfo` RPC - `wallet_multiwallet.py` functional test As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code. List of upstream PRs backported/adapted: - bitcoin#9906 : Disallow copy constructor CReserveKeys - bitcoin#9830 : Add safe flag to listunspent result - bitcoin#9993 : Initialize nRelockTime - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value - bitcoin#10075 : Remove unused C++ code not covered by unit tests - bitcoin#10257 : Add test for getmemoryinfo - bitcoin#10295 : Move some WalletModel functions into CWallet - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings - bitcoin#10522 : Remove unused variables - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet - bitcoin#10849 : Multiwallet: simplest endpoint support - bitcoin#9380 : Separate different uses of minimum fees (only dustRelayFee) - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit) - bitcoin#10883 : Rename -usewallet to -rpcwallet - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests - bitcoin#10893 : Avoid running multiwallet.py twice - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets - bitcoin#10931 : Fix misleading "method not found" multiwallet errors - bitcoin#10885 : Reject invalid wallets - bitcoin#11022 : Basic keypool topup - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp - bitcoin#11310 : Test listwallets RPC - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs - bitcoin#11492 : Fix leak in CDB constructor - bitcoin#11476 : Avoid opening copied wallet databases simultaneously - bitcoin#11590 : always show help-line of wallet encryption calls - bitcoin#11289 : Add wallet backup text to import* and add* RPCs ACKs for top commit: furszy: re-re-utACK d5526bd. Fuzzbawls: ACK d5526bd Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2

…let.h/cpp bitcoin#10976 Move some static functions out of wallet.h/cpp This commit just moves a few function declarations and updates callers. Function bodies are moved in two followup MOVEONLY commits. This change is desirable because wallet.h/cpp are monolithic and hard to navigate, so pulling things out and grouping together pieces of related functionality should improve the organization. Another proximate motivation is the wallet process separation work in bitcoin#10973, where (at least initially) parameter parsing and fee estimation are still done in the main process rather than the wallet process, and having functions that run in different processes scrambled up throughout wallet.cpp is unnecessarily confusing. MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp make it actual move only Signed-off-by: Pasta <[email protected]> MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp make it actual move only Signed-off-by: Pasta <[email protected]> add keepass include Signed-off-by: Pasta <[email protected]>

ryanofsky force-pushed the pr/wmove branch from 5a64499 to 4298d90 Compare August 2, 2017 12:17

maflcko added Refactoring Wallet labels Aug 2, 2017

jnewbery reviewed Aug 2, 2017

View reviewed changes

promag mentioned this pull request Aug 6, 2017

scripted-diff: stop using the gArgs wrappers #10607

Closed

promag reviewed Aug 6, 2017

View reviewed changes

ryanofsky added 3 commits August 14, 2017 11:19

MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp

e7fe320

MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp

f01103c

jnewbery mentioned this pull request Aug 14, 2017

[wallet] Clarify wallet initialization / destruction interface #10767

Merged

ryanofsky commented Aug 14, 2017

View reviewed changes

ryanofsky force-pushed the pr/wmove branch from 4298d90 to f01103c Compare August 14, 2017 20:28

laanwj mentioned this pull request Aug 15, 2017

Use quoted form when including primitives/transaction.h and wallet/wallet.h #10752

Closed

laanwj mentioned this pull request Aug 15, 2017

refactor: Make all #includes relative to project root #11053

Closed

laanwj merged commit f01103c into bitcoin:master Aug 25, 2017

maflcko added this to the 0.15.1 milestone Aug 25, 2017

maflcko added the Needs backport label Aug 25, 2017

maflcko reviewed Aug 25, 2017

View reviewed changes

maflcko removed the Needs backport label Oct 4, 2017

maflcko removed this from the 0.15.1 milestone Oct 4, 2017

random-zebra mentioned this pull request Apr 28, 2021

[Wallet] Laggard wallet-related backports from btc 0.15 PIVX-Project/PIVX#2351

Merged

1 task

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

[MOVEONLY] Move some static functions out of wallet.h/cpp #10976

[MOVEONLY] Move some static functions out of wallet.h/cpp #10976

Uh oh!

Conversation

ryanofsky commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Aug 2, 2017

Uh oh!

jnewbery commented Aug 2, 2017

Uh oh!

ryanofsky commented Aug 2, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Aug 2, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 3, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Aug 6, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Aug 6, 2017

Uh oh!

maflcko commented Aug 6, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Aug 14, 2017

Uh oh!

ryanofsky commented Aug 14, 2017

Uh oh!

laanwj commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Aug 15, 2017

Uh oh!

laanwj commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Aug 25, 2017

Uh oh!

jnewbery commented Aug 25, 2017

Uh oh!

maflcko Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ryanofsky commented Aug 2, 2017 •

edited

Loading

laanwj commented Aug 15, 2017 •

edited

Loading

laanwj commented Aug 15, 2017 •

edited

Loading