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

ryanofsky commented at 12:06 pm on August 2, 2017: member

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 commented at 12:14 pm on August 2, 2017: member

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

ryanofsky force-pushed on Aug 2, 2017

MarcoFalke added the label Refactoring on Aug 2, 2017

MarcoFalke added the label Wallet on Aug 2, 2017

jnewbery commented at 4:59 pm on August 2, 2017: member

Seems to be largely the same as #10767 ?

ryanofsky commented at 5:32 pm on August 2, 2017: member

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.

in src/wallet/rpcwallet.cpp:29 in 4298d90a9e outdated

25@@ -27,6 +26,8 @@
26 #include "wallet/wallet.h"
27 #include "wallet/walletdb.h"
28 
29+#include <init.h>  // For StartShutdown

jnewbery commented at 9:44 pm on August 2, 2017:

why?

ryanofsky commented at 7:44 pm on August 14, 2017:

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.

jnewbery commented at 9:48 pm on August 2, 2017: member

ACK 4298d90a9e3ea04e74d85ee995bf58e6aa8a0587.

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

jonasschnelli commented at 9:36 am on August 3, 2017: contributor

Concept ACK

in src/wallet/init.h:22 in 4298d90a9e outdated

17+//! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
18+//  This function will perform salvage on the wallet if requested, as long as only one wallet is
19+//  being loaded (CWallet::ParameterInteraction forbids -salvagewallet, -zapwallettxes or -upgradewallet with multiwallet).
20+bool WalletVerify();
21+
22+//! Load wallet databases

promag commented at 1:58 pm on August 6, 2017:

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

ryanofsky commented at 8:21 pm on August 14, 2017:

Added in aee0e0f6857a18d0e93825a2bb475c55446cfa85

promag commented at 2:46 pm on August 6, 2017: member

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.

jnewbery commented at 10:25 pm on August 6, 2017: member

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

MarcoFalke commented at 10:53 pm on August 6, 2017: member

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

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
https://github.com/bitcoin/bitcoin/pull/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.

d97fe2016c

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

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

ryanofsky commented at 8:28 pm on August 14, 2017: member

Rebased 4298d90a9e3ea04e74d85ee995bf58e6aa8a0587 -> 5461f69fe9e503be734c33d7b57035214bafba53 (pr/wmove.2 -> pr/wmove.3) because of conflicts with gArgs PR. Added 1 commit 5461f69fe9e503be734c33d7b57035214bafba53 -> aee0e0f6857a18d0e93825a2bb475c55446cfa85 (pr/wmove.3 -> pr/wmove.4, compare) Squashed aee0e0f6857a18d0e93825a2bb475c55446cfa85 -> f01103c1e0a204fc7f40a06755f6c3adb5480cf8 (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 force-pushed on Aug 14, 2017

jnewbery commented at 9:01 pm on August 14, 2017: member

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:

 0→ find -name *cpp -printf '%f\n' | sort | uniq -c | sort
 1...
 2      1 zmqpublishnotifier.cpp
 3      2 base58.cpp
 4      2 crypto_tests.cpp
 5      2 lockedpool.cpp
 6      2 net.cpp
 7      2 protocol.cpp
 8→ find -name base58.cpp
 9./src/base58.cpp
10./src/bench/base58.cpp
11→ find -name crypto_tests.cpp
12./src/wallet/test/crypto_tests.cpp
13./src/test/crypto_tests.cpp
14→ find -name lockedpool.cpp
15./src/support/lockedpool.cpp
16./src/bench/lockedpool.cpp
17→ find -name net.cpp
18./src/net.cpp
19./src/rpc/net.cpp
20→ find -name protocol.cpp
21./src/rpc/protocol.cpp
22./src/protocol.cpp

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

ryanofsky commented at 9:10 pm on August 14, 2017: member

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 commented at 7:33 am on August 15, 2017: member

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 commented at 9:45 am on August 15, 2017: member

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

laanwj commented at 10:03 am on August 15, 2017: member

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 commented at 6:57 pm on August 25, 2017: member

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 commented at 7:07 pm on August 25, 2017: member

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

laanwj merged this on Aug 25, 2017

laanwj closed this on Aug 25, 2017

laanwj referenced this in commit 07c92b98e2 on Aug 25, 2017

MarcoFalke added this to the milestone 0.15.1 on Aug 25, 2017

MarcoFalke added the label Needs backport on Aug 25, 2017

in src/wallet/init.h:19 in d97fe2016c outdated

14+//! Wallets parameter interaction
15+bool WalletParameterInteraction();
16+
17+//! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
18+//  This function will perform salvage on the wallet if requested, as long as only one wallet is
19+//  being loaded (CWallet::ParameterInteraction forbids -salvagewallet, -zapwallettxes or -upgradewallet with multiwallet).

MarcoFalke commented at 8:15 pm on August 25, 2017:

nit: doc string needs fixup

MarcoFalke commented at 8:21 pm on August 25, 2017: member

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

MarcoFalke removed the label Needs backport on Oct 4, 2017

MarcoFalke removed this from the milestone 0.15.1 on Oct 4, 2017

PastaPastaPasta referenced this in commit 30eec06098 on Sep 19, 2019

PastaPastaPasta referenced this in commit 9efa568833 on Dec 22, 2019

PastaPastaPasta referenced this in commit d7df373a0b on Jan 2, 2020

PastaPastaPasta referenced this in commit 1f29aae2eb on Jan 4, 2020

PastaPastaPasta referenced this in commit ac94b9b4a8 on Jan 4, 2020

PastaPastaPasta referenced this in commit 05ccd98663 on Jan 10, 2020

PastaPastaPasta referenced this in commit c43d581347 on Jan 10, 2020

PastaPastaPasta referenced this in commit fc4ab83c83 on Jan 10, 2020

PastaPastaPasta referenced this in commit 20e8ce81db on Jan 12, 2020

ckti referenced this in commit 052ebdb7fd on Mar 28, 2021

random-zebra referenced this in commit 2d50b6e3b9 on Jun 9, 2021

gades referenced this in commit d4a6c7a3c6 on Jun 30, 2021

gades referenced this in commit 2580656097 on Feb 5, 2022

DrahtBot locked this on Feb 15, 2022

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