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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet #20715 (util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet by MarcoFalke)
• rpc: Add RPCContext #20017 (rpc: Add RPCContext by promag)

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.

I'm able to reproduce appveyor failure on linux with

make -C src test/test_bitcoin && src/test/test_bitcoin -l test_suite -t init_tests -t wallet_tests/CreateWallet --random=2

To reproduce, it may be necessary to vary the random seed to get CreateWallet test to run after the init_tests as happens on appveyor.

The problem seems to be that init_tests force set a -walletdir that GetWalletDir rejects and turns into an empty path, which gets rejected by the assert because it is not absolute.

Probably a minimal fix is possible in init_tests. Backtrace at assert fail looks like

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff59b38b1 in __GI_abort () at abort.c:79
#2  0x00007ffff59a342a in __assert_fail_base (fmt=0x7ffff5b2aa38 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=assertion@entry=0x5555566fc54f "base.is_absolute()", file=file@entry=0x5555566fc548 "fs.cpp", line=line@entry=36, 
    function=function@entry=0x5555566fc600 <fsbridge::AbsPathJoin(boost::filesystem::path const&, boost::filesystem::path const&)::__PRETTY_FUNCTION__> "boost::filesystem::path fsbridge::AbsPathJoin(const boost::filesystem::path&, const boost::filesystem::path&)") at assert.c:92
#3  0x00007ffff59a34a2 in __GI___assert_fail (assertion=0x5555566fc54f "base.is_absolute()", file=0x5555566fc548 "fs.cpp", line=36, 
    function=0x5555566fc600 <fsbridge::AbsPathJoin(boost::filesystem::path const&, boost::filesystem::path const&)::__PRETTY_FUNCTION__> "boost::filesystem::path fsbridge::AbsPathJoin(const boost::filesystem::path&, const boost::filesystem::path&)") at assert.c:101
#4  0x000055555620de2f in fsbridge::AbsPathJoin (base=..., p=...) at fs.cpp:36
#5  0x00005555563d5298 in MakeWalletDatabase (name="", options=..., status=@0x7fffffffb57c: 21845, error_string=...) at wallet/wallet.cpp:3783
#6  0x0000555555cd0d59 in wallet_tests::TestLoadWallet (chain=...) at wallet/test/wallet_tests.cpp:46
#7  0x0000555555ce03f2 in wallet_tests::CreateWallet::test_method (this=0x7fffffffc9c0) at wallet/test/wallet_tests.cpp:699
#8  0x0000555555cdf585 in wallet_tests::CreateWallet_invoker () at wallet/test/wallet_tests.cpp:696

Following fix seems to work:

--- a/src/wallet/test/init_test_fixture.cpp
+++ b/src/wallet/test/init_test_fixture.cpp
@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <fs.h>
+#include <univalue.h>
 #include <util/check.h>
 #include <util/system.h>
 
@@ -37,6 +38,9 @@ InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainNam
 
 InitWalletDirTestingSetup::~InitWalletDirTestingSetup()
 {
+    gArgs.LockSettings([&](util::Settings& settings) {
+        settings.forced_settings.erase("walletdir");
+    });
     fs::current_path(m_cwd);
 }
 

Would suggest commit message like:

test: Clear forced -walletdir setting after wallet init_tests

Leaving this value set interfered with the CreateWallet test if it happened to execute later in the test ordering. Specifically it would cause CreateWallet test to write data to the current directory instead of temporary test directory.

re: #20932 (comment)

This PR is based on @ryanofsky suggestion here #20744 (review) and it should help make #20744 diff smaller in the long run.

Would suggest writing a self-contained PR description that doesn't require reading a comment in another issue to understand. Something like:

refactor: Replace fs::absolute calls with AbsPathJoin calls

This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.

This PR doesn't change behavior aside from adding an assert and fixing a test bug.

Code review ACK da9caa1 only doxygen improvements since my last review per git diff d867d7a da9caa1

Thanks for updating.

review ACK da9caa1 📯

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

review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 📯
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgRdQv8DzNpEk8/k2bFwErjQdb2bW93pR3nNfNIarBpkEE+QHpmP7vSlMXz+04q
JlOLmdAPEVv2jWA+d2lEdvoMapyCEBbUvii7jSgxBN/Qb6ONyXddjRTjDEVXmJZW
UCjgJ9hfo59NxuA49LcAVPMVatdkA7PtgKkYZ78sG14BlJ+15csFYA8R+FzMW+hh
kD/j8tYTQQQ1KN11Vmyw08rLcUU/XFavbCf/0++FlDGKbZOROYVtfhqxybyn4YzN
Ibt/AJq+dGIOBB3LY9cN0Muwb8sxVZQrM4GWxNUwHzkH1VHZL7vdVGGWJBx+rU2c
5mICi+9n60wmeRUahHH5hAT2+hr5XDgBo1i5mZUpZYmTZXp+kO2a83LI/oNGpkK5
MTWy4K+5koCzgLPQNDRGRiVM/aB81F0l4LsaY9/q5ANDQg5LP6YWwyHnuzbLujHs
pHTn2rUe+DnLmEM94a6p05Ht0NsfyWusw/NpTkAYl9ZkvNjqixRmIbkwIm7sYC65
UrqATWdd
=3DWR
-----END PGP SIGNATURE-----