wallet: Make -walletdir network only #17447

With this PR bitcoind -regtest doesn't run if bitcoin.conf has

walletdir=/mnt/mydisk/wallets

But works with

[regtest]
walletdir=/mnt/mydisk/wallets

Doesn't change mainnet behavior.

Closes #15630.

Seeking concept ACK.

How will it work if walletdir is specified somewhere out of the datadir?

@hebasto same as before, only change is you must have -walletdir after the network, like after [regtest].

In other words: This is a test-only change.

In other words: This is a test-only change.

No? bitcoind won't run if bitcoin.conf has

walletdir=/mnt/mydisk/wallets

But works with

[main]
walletdir=/mnt/mydisk/wallets

No. See https://dev.visucore.com/bitcoin/doxygen/system_8cpp_source.html#l00248

This boots up fine for me:

$ cat /tmp/a
uacomment=Alice
datadir=/tmp
rpcport=33333
$ ./src/qt/bitcoin-qt -conf=/tmp/a

Right, thanks for the correction, updated OP.

ACK 3c2c439dcd8797019ac6d6614775d5c20ee41c36

Maybe could use release notes like:

It is now an error to use an unqualified walletdir=path setting in the config file if bitcoin is running on testnet or regtest networks. The setting now needs to be qualified as test.walletdir=path, regtest.walletdir=path, or main.walletdir=path or placed in the appropriate [test] [regtest] or [main] section.

When I first looked at this I thought there might be tenuous use-case for not making -walletdir a network-specific setting: that you could specify a relative path like walletdir=russwallets have bitcoin automatically use appropriate ~/.bitcoin/russwallets, ~/.bitcoin/testnet3/russwallets, etc directories depending on the network. But actually there appears to be another bug where the directory isn't even interpreted relative to datadir.

https://github.com/bitcoin/bitcoin/blob/cd6cb9745e13a62e130b11f78a13bcc1d424b05e/src/wallet/load.cpp#L21

It probably would be better to change this to:

    fs::path canonical_wallet_dir = fs::canonical(wallet_dir, GetDataDir(), error);

Something for another PR, though.

Maybe say what benefit of the change is in the PR description. It should decrease the likelihood of opening a wallet file from the wrong chain when walletdir is used, helping with #12805 a little.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

It should decrease the likelihood of opening a wallet file from the wrong chain when walletdir is used, helping with #12805 a little.

Oof. Concept ACK.

Needs release notes for "test-only changes"?

ACK 3c2c439dcd8797019ac6d6614775d5c20ee41c36 🍈

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 3c2c439dcd8797019ac6d6614775d5c20ee41c36 🍈
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiFywwAnEqaW7Ys2ds9z8FV8RZd1M9W5DU+i2lLve7mukLeQwbccDKRNWiYdpN/
uOdEv19H3pF5brA1GKeHkrtMDmDuhUgIDUfWOTyGO1GAMCsKXzS0x5kw+c2+i01e
wz/ZMp1lXKGiVhwOPVszNRXuF4b+uICUtMfjYuSRXbMGzJh6aEgD1JqO/vYpn/xJ
hNdwQ/wZ0FFAud1qRctlDbLuR2zyq/H+TAPdklKtiMa1+xe5qtAXAEG2YfK5PrSL
6EOSKES7QK4b4wy2SVChpDF9Uc2YXk+bNqghJKJTZZqKfdTiZ3QiLNPDcmOruTyD
WHsdpCPPt5om7J2Fun2DrYZu+9UX7xLJfAj6MXTeVbSkssZcuORgHWTfGQsdE/KC
1C7S0ltwm03cVusMAXhOzl4BHUZrYBxQPQUDBy3xQSRVF8XXPQxAdRgf1QFN4GMh
+dtEXEyIHxv4E3/XfJ3aIr/hlkQtMIjNGLDglhORcGkmrnWnxaKUGpfGX41XT4Mf
llSvIM0A
=BTf0
-----END PGP SIGNATURE-----

Timestamp of file with hash 81fdb0207d259a48252e116f61bb427aa236eefd85d66edb01ea5281abfa70fd -

</details>

Needs release notes for "test-only changes"?

When you are saying test-only do you mean testnet-only? I'd expect changes affecting testnet or manual testing in general could have release notes, but changes just affecting automated c++/python test cases wouldn't typically get release notes.

Any case the release note I wrote up #17447 (comment) was also meant to provide a better description / motivation for the PR.

When you are saying test-only do you mean testnet-only?

testnet and regtest, or "low level test changes", see here: https://github.com/bitcoin/bitcoin/blob/fec230edcc2ae0ec4fd3811185ad3b1802a5c90b/doc/release-notes/release-notes-0.19.0.md#tests

Tested ACK 3c2c439dcd8797019ac6d6614775d5c20ee41c36

It probably would be better to change this to: @ryanofsky I think that's on purpose - it's the case where -walletdir is explicitly set and should be absolute? In this case maybe the following help could say it must be an absolute path: https://github.com/bitcoin/bitcoin/blob/3d6752779f0504e6435fe682f0c06c60b5c4c33b/src/wallet/init.cpp#L63

re: #17447 (comment), #17447 (comment)

It probably would be better to change this to:

@ryanofsky I think that's on purpose - it's the case where -walletdir is explicitly set and should be absolute? In this case maybe the following help could say it must be an absolute path

Yes I didn't see the check below which requires the path to be absolute. It is pointless to call fs::canonical at all if the path has to be absolute. The change I was suggesting would allow it to be relative to the datadir, but updating the help would be another option.