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

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.

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

Seems to be largely the same as #10767 ?

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.

ACK 4298d90a9e3ea04e74d85ee995bf58e6aa8a0587.

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

Concept ACK

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.

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

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

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.

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.

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.

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.

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

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

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.

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

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