doc: clarify loadwallet path loading for wallets

am-sq commented at 5:30 pm on June 18, 2024: none

Why this change?

#30269 describes a need for documentation improvement with the loadwallet RPC. Namely, some users have found the usage description confusing when it comes to loading wallets that are not in the normal case of being in the default wallet directory.

The default wallet directory, depending on the machine OS, has the base directory defined here: https://github.com/bitcoin/bitcoin/blob/9c5cdf07f30f816cd134e2cd2dca9c27ef7067a5/src/common/args.cpp#L699 which is then appended with /wallets. So for example, for MacOS, it would be ~/Library/Application Support/Bitcoin/wallets.

The changes implemented

Change the help text to indicate that the filename (or directory) passed in to loadwallet is relative to the base wallet directory
Adds additional examples to the help page showing how to fetch a wallet within a subdirectory of the base data directory for wallets, or from an absolute path

DrahtBot commented at 5:30 pm on June 18, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30302.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator, adamandrews1
Concept ACK	BrandonOdiwuor, pablomartin4btc, MarnixCroes
Stale ACK	marcofleon, murchandamus, tdb3

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Docs on Jun 18, 2024

in src/wallet/rpc/wallet.cpp:223 in 7325a49532 outdated

219@@ -220,7 +220,7 @@ static RPCHelpMan loadwallet()
220                 "\nNote that all wallet command-line options used when starting bitcoind will be"
221                 "\napplied to the new wallet.\n",
222                 {
223-                    {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The wallet directory or .dat file."},
224+                    {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "Filename relative to the wallet directory."},

maflcko commented at 5:50 pm on June 18, 2024:

Absolute paths are also allowed.

am-sq commented at 6:08 pm on June 18, 2024:

Thanks for the review. Is this description better? 

0You may pass in the absolute path of your wallet directory or .dat file. Otherwise, the interpreted path will be relative to the default wallet directory.

maflcko commented at 6:11 pm on June 18, 2024:

Do sqlite wallets allow .dat files to be passed, or is this only for BDB legacy wallets?

am-sq commented at 6:11 pm on June 18, 2024:

I can also add examples that load wallet using an absolute path.

am-sq commented at 7:05 pm on June 19, 2024:

Based on https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#sqlite-database-based-wallets, it appears .dat files can be passed to sqlite wallets as well.

am-sq commented at 8:42 pm on June 19, 2024:

Added additional examples

am-sq force-pushed on Jun 18, 2024

DrahtBot added the label CI failed on Jun 18, 2024

am-sq renamed this:
~~doc: loadwallet loads from relative walletdir~~
doc: clarify loadwallet path loading for wallets
on Jun 19, 2024

am-sq force-pushed on Jun 19, 2024

am-sq marked this as ready for review on Jun 19, 2024

in src/wallet/rpc/wallet.cpp:239 in dc4cbe2095 outdated

234@@ -235,7 +235,11 @@ static RPCHelpMan loadwallet()
235                 },
236                 RPCExamples{
237                     HelpExampleCli("loadwallet", "\"test.dat\"")
238+            + HelpExampleCli("loadwallet", "\"specialWallets/test2.dat\"")
239+            + HelpExampleCli("loadwallet", "\"/Users/joe/specialWallets\"")

hodlinator commented at 7:47 pm on June 19, 2024:

Tried createwallet to observe that the current version creates a directory with the given name, containing wallet.dat (and wallet.dat-journal while loaded).

Suggestion: put the normal/current/simplest operation first, and also add some descriptions to command examples:

0                    "\nLoad regular wallet with files under wallets/foo/:\n"
1                    + HelpExampleCli("loadwallet", "\"foo\"") +
2                    "\nLoad wallet using absolute path:\n"
3                    + HelpExampleCli("loadwallet", "\"/Users/joe/specialWallet/\"")                    
4                    "\nLoad legacy wallet.dat file directly under the wallets/ directory:\n"
5                    + HelpExampleCli("loadwallet", "\"legacy.dat\"")

hodlinator commented at 7:51 pm on June 19, 2024:

/Users/joe/specialWallets should be changed to /Users/joe/specialWallet/ here and in the RPC example since we only load one wallet at a time and to make clear that it is a directory.

am-sq commented at 8:41 pm on June 19, 2024:

Thanks for the suggestion - fixed.

am-sq commented at 8:41 pm on June 19, 2024:

Great suggestion- added!

hodlinator changes_requested

hodlinator commented at 7:55 pm on June 19, 2024: contributor

Approach NACK dc4cbe209508b2f95a748137b200056451405c9e

Commits need squashing together.

(Also had a look at https://github.com/am-sq/bitcoin/blob/2f6c4f30b0b937be4cacc8b62a767fb4b73b352f/src/wallet/wallet.cpp#L2942-L2944 to better understand the wallet loading logic).

I like the new filename parameter help text. 👍

am-sq force-pushed on Jun 19, 2024

hodlinator approved

hodlinator commented at 8:53 pm on June 19, 2024: contributor

Approach NACK c3bd87370a274adab9ba2d07c87508d3eded6be7

Needs small fix to compile, otherwise LGTM.

in src/wallet/rpc/wallet.cpp:240 in c3bd87370a outdated

233@@ -234,8 +234,18 @@ static RPCHelpMan loadwallet()
234                     }
235                 },
236                 RPCExamples{
237-                    HelpExampleCli("loadwallet", "\"test.dat\"")
238-            + HelpExampleRpc("loadwallet", "\"test.dat\"")
239+                    "\nLoad regular wallet with files under wallets/foo/:\n"
240+                    + HelpExampleCli("loadwallet", "\"foo\"")
241+                    + HelpExampleRpc("loadwallet", "\"foo\"")
242+                    "\nLoad regular wallet with files under wallets/foo/specialWallet/:\n"

hodlinator commented at 8:57 pm on June 19, 2024:

0                    + "\nLoad regular wallet with files under wallets/foo/specialWallet/:\n"

am-sq force-pushed on Jun 19, 2024

hodlinator approved

hodlinator commented at 9:49 pm on June 19, 2024: contributor

ACK ee15876ab33ad2487d7f1126ed653f9ae43d0514

Compiled and ran bitcoin-cli --regtest loadwallet without parameters. I like how you ended up alternating CLI/RPC examples.

Tested loadwallet "foo" just to make sure nothing funny happens with extra quotes.

DrahtBot removed the label CI failed on Jun 19, 2024

in src/wallet/rpc/wallet.cpp:245 in ee15876ab3 outdated

242+                    + "\nLoad regular wallet with files under wallets/foo/specialWallet/:\n"
243+                    + HelpExampleCli("loadwallet", "\"foo/specialWallet/\"")
244+                    + HelpExampleRpc("loadwallet", "\"foo/specialWallet/\"")
245+                    + "\nLoad wallet using absolute path:\n"
246+                    + HelpExampleCli("loadwallet", "\"/Users/joe/specialWallet/\"")
247+                    + HelpExampleRpc("loadwallet", "\"/Users/joe/specialWallet/\"")

luke-jr commented at 7:15 pm on June 21, 2024:

This should probably reflect the OS bitcoind was compiled for

am-sq commented at 2:14 am on June 22, 2024:

I made the change to reflect different path string depending on OS. Let me know if this is what you meant. I was also debating doing an alternative way where 2 example types are displayed, one MacOS and other Unix, so that web docs like https://developer.bitcoin.org/reference/rpc/loadwallet.html show both examples.

am-sq commented at 4:15 pm on June 28, 2024:

Went ahead with 2 example types displayed.

am-sq force-pushed on Jun 22, 2024

DrahtBot added the label CI failed on Jun 22, 2024

DrahtBot commented at 3:48 am on June 22, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/26542999697

am-sq force-pushed on Jun 22, 2024

DrahtBot removed the label CI failed on Jun 23, 2024

in src/wallet/rpc/wallet.cpp:230 in a0dffe6318 outdated

225+        if (pszHome == nullptr || strlen(pszHome) == 0)
226+            pathRet = fs::path("/");
227+        else
228+            pathRet = fs::path(pszHome);
229+        absoluteSpecialWalletPath = fs::PathToString(pathRet) + "/joe/specialWallet/";
230+    #endif

hodlinator commented at 2:22 pm on June 24, 2024:

Would have written it closer to something like:

 0    fs::path absolutePath;
 1#ifdef WIN32
 2    if (const char* homedrive = std::getenv("HOMEDRIVE"), * homepath = std::getenv("HOMEPATH");
 3        homedrive != nullptr && homedrive[0] != 0 && homepath != nullptr && homepath[0] != 0)
 4        absolutePath = fs::path(homedrive) / fs::path(homepath);
 5    else
 6        absolutePath = fs::path("C:\\Users\\Satoshi");
 7#else
 8    if (const char* home = std::getenv("HOME"); home != nullptr && home[0] != 0)
 9        absolutePath = fs::path(home);
10    else
11        absolutePath = fs::path("/home/satoshi");
12#endif
13    absolutePath /= fs::path("specialWallet") + fs::path::preferred_separator;

No old school Hungarian notation (pszHome -> home). (Also +const where possible).
See if char* is empty through checking first character instead of counting length of string (strlen()). Wish std::empty()had an overload for this but couldn’t find one.
Hopefully proper Windows path calculation (see Stackoverflow post).
Joe -> Satoshi. :)
absoluteSpecialWalletPath -> absolutePath (maybe absolutePathExample would be a good middle ground?), and type is fs::path all the way.
More common #preprocessor indentation.
/<name>/specialWallet/ -> /home/<name>/specialWallet/ when environment variable is nullptr.

Take from it what you want. Warning: other reviewers might disagree with me on something. If you prefer to keep your current version and others are fine with that, I will ACK too.

am-sq commented at 6:32 am on June 25, 2024:

Thank you for the suggestions and learning opportunity! Made all the suggested changes since they seem like improvements to me. Rebased as well.

hodlinator changes_requested

hodlinator commented at 2:29 pm on June 24, 2024: contributor

Concept ACK a0dffe6318d32b7b49b7d0be6d45b59ee3e0f4ec

git range-diff ee15876~1..ee15876 a0dffe6~1..a0dffe6 Agree that “regular” wording wasn’t needed.

am-sq force-pushed on Jun 25, 2024

DrahtBot added the label CI failed on Jun 25, 2024

DrahtBot commented at 7:54 am on June 25, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/26637054345

in src/wallet/rpc/wallet.cpp:233 in 164e97f46f outdated

228+        absolutePath = fs::path(home);
229+    else
230+        absolutePath = fs::path("/home/satoshi");
231+#endif
232+    absolutePath /= fs::path("specialWallet") + fs::path::preferred_separator;
233+

jonatack commented at 6:07 pm on June 25, 2024:

Didn’t verify, but could all of this be replaced by a call to fs::path GetDefaultDataDir()?

jonatack commented at 6:10 pm on June 25, 2024:

(It may also help with the Win CI errors)

am-sq commented at 7:40 pm on June 25, 2024:

It doesn’t quite work, because it would return:

0fs::path GetDefaultDataDir()
1{
2    // Windows:
3    //   old: C:\Users\Username\AppData\Roaming\Bitcoin
4    //   new: C:\Users\Username\AppData\Local\Bitcoin
5    // macOS: ~/Library/Application Support/Bitcoin
6    // Unix-like: ~/.bitcoin

but we are looking for:

0    // Windows: C:\Users\Satoshi\
1    // macOS: /Users/satoshi/
2    // Unix-like: /home/satoshi/

hodlinator commented at 7:54 pm on June 25, 2024:

It doesn’t quite work, because it would return: … // Unix-like: ~/.bitcoin

Don’t trust the comment, it gives me the absolute path. Tested on Linux with:

0const fs::path absolutePath = GetDefaultDataDir() / fs::path("specialWallet") + fs::path::preferred_separator;

am-sq commented at 1:56 am on June 26, 2024:

Made the change.

am-sq force-pushed on Jun 25, 2024

DrahtBot removed the label CI failed on Jun 25, 2024

in src/wallet/rpc/wallet.cpp:247 in 325152a2fd outdated

244+                    + "\nLoad wallet with files under wallets/foo/specialWallet/:\n"
245+                    + HelpExampleCli("loadwallet", "\"foo/specialWallet/\"")
246+                    + HelpExampleRpc("loadwallet", "\"foo/specialWallet/\"")
247+                    + "\nLoad wallet using absolute path:\n"
248+                    + HelpExampleCli("loadwallet", fs::PathToString(absolutePath))
249+                    + HelpExampleRpc("loadwallet", fs::PathToString(absolutePath))

hodlinator commented at 4:44 pm on June 26, 2024:

nit: Might be good to add quotes around the path like the other examples in case there are spaces in usernames or other part of the directory?

hodlinator approved

hodlinator commented at 4:45 pm on June 26, 2024: contributor

ACK 325152a2fd965dcccf71b0f599dc91cef0441862

git range-diff a0dffe6~1..a0dffe6 325152a~1..325152a ✅

 0$ bitcoin-cli -regtest loadwallet
 1error code: -1
 2error message:
 3loadwallet "filename" ( load_on_startup )
 4
 5Loads a wallet from a wallet file or directory.
 6Note that all wallet command-line options used when starting bitcoind will be
 7applied to the new wallet.
 8
 9Arguments:
101. filename           (string, required) You may pass in the absolute path of your wallet directory or .dat file. Otherwise, the interpreted path will be relative to the default wallet directory.
112. load_on_startup    (boolean, optional) Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged.
12
13Result:
14{                    (json object)
15  "name" : "str",    (string) The wallet name if loaded successfully.
16  "warnings" : [     (json array, optional) Warning messages, if any, related to loading the wallet.
17    "str",           (string)
18    ...
19  ]
20}
21
22Examples:
23
24Load wallet with files under wallets/foo/:
25> bitcoin-cli loadwallet "foo"
26> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": ["foo"]}' -H 'content-type: application/json' http://127.0.0.1:8332/
27
28Load wallet with files under wallets/foo/specialWallet/:
29> bitcoin-cli loadwallet "foo/specialWallet/"
30> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": ["foo/specialWallet/"]}' -H 'content-type: application/json' http://127.0.0.1:8332/
31
32Load wallet using absolute path:
33> bitcoin-cli loadwallet /home/hodlinator/.bitcoin/specialWallet/
34> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": [/home/hodlinator/.bitcoin/specialWallet/]}' -H 'content-type: application/json' http://127.0.0.1:8332/
35
36Load legacy wallet.dat file directly under the wallets/ directory:
37> bitcoin-cli loadwallet "legacy.dat"
38> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": ["legacy.dat"]}' -H 'content-type: application/json' http://127.0.0.1:8332/

in src/wallet/rpc/wallet.cpp:219 in 325152a2fd outdated

215@@ -215,12 +216,13 @@ static RPCHelpMan listwallets()
216 
217 static RPCHelpMan loadwallet()
218 {
219+    const fs::path absolutePath = GetDefaultDataDir() / fs::path("specialWallet") + fs::path::preferred_separator;

maflcko commented at 5:04 pm on June 26, 2024:

I don’t think the RPC documentation in the right place to inject and leak the name of the user that generated the docs. Among the obvious issues, this also makes them less deterministic.

I am sure a user can figure out what an absolute path means without having to see it in the docs.

If it was important to show this location, it can be returned by a dedicated RPC field?

hodlinator commented at 5:55 pm on June 26, 2024:

Hadn’t thought of the doc-generation aspect and leaks, good point.

hodlinator commented at 6:06 pm on June 26, 2024:

Just realized that in the console output in my latest test #30302#pullrequestreview-2142445372, the RPC --user is hardcoded to myusername:

0> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": [/home/hodlinator/.bitcoin/specialWallet/]}' -H 'content-type: application/json' http://127.0.0.1:8332/

So might as well use the same name if we want to go back to hardcoded platform-specific paths.

am-sq commented at 5:43 pm on June 27, 2024:

How about just hard-coding two examples- one for Unix and other for Windows? since I generally agree with

I am sure a user can figure out what an absolute path means without having to see it in the docs.

But there was a request here for showing the paths for different OS.

am-sq commented at 5:58 pm on June 27, 2024:

0...
1+ "\nLoad wallet using absolute path (Unix):\n"
2+ HelpExampleCli("loadwallet", "\"/home/myusername/specialWallet/\"")
3+ HelpExampleRpc("loadwallet", "\"/home/myusername/specialWallet/\"")
4+ "\nLoad wallet using absolute path (Windows):\n"
5+ HelpExampleCli("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
6+ HelpExampleRpc("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
7...

hodlinator commented at 8:54 pm on June 27, 2024:

Maybe go more general to increase MacOS/darwin compatibility:

0...
1+ "\nLoad wallet using absolute path (Unix):\n"
2+ HelpExampleCli("loadwallet", "\"/path/to/specialWallet/\"")
3+ HelpExampleRpc("loadwallet", "\"/path/to/specialWallet/\"")
4+ "\nLoad wallet using absolute path (Windows):\n"
5+ HelpExampleCli("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
6+ HelpExampleRpc("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
7...

am-sq commented at 4:17 pm on June 28, 2024:

Made this change.

am-sq force-pushed on Jun 28, 2024

in src/wallet/rpc/wallet.cpp:251 in 69bf58dc0e outdated

248+                    + "\nLoad wallet using absolute path (Windows):\n"
249+                    + HelpExampleCli("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
250+                    + HelpExampleRpc("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
251+                    + "\nLoad legacy wallet.dat file directly under the wallets/ directory:\n"
252+                    + HelpExampleCli("loadwallet", "\"legacy.dat\"")
253+                    + HelpExampleRpc("loadwallet", "\"legacy.dat\"")

hodlinator commented at 5:53 pm on June 28, 2024:

Could reduce it down to only one HelpExampleRpc if one would want to reduce the number of lines.

0                    + "\nLoad wallet with files under wallets/foo/ via RPC:\n"
1                    + HelpExampleRpc("loadwallet", "\"foo\"")
2                    + "\nLoad wallet with files under wallets/foo/specialWallet/:\n"
3                    + HelpExampleCli("loadwallet", "\"foo/specialWallet/\"")
4                    + "\nLoad wallet using absolute path (Unix):\n"
5                    + HelpExampleCli("loadwallet", "\"/path/to/specialWallet/\"")
6                    + "\nLoad wallet using absolute path (Windows):\n"
7                    + HelpExampleCli("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
8                    + "\nLoad legacy wallet.dat file directly under the wallets/ directory:\n"
9                    + HelpExampleCli("loadwallet", "\"legacy.dat\"")

am-sq commented at 3:36 pm on June 29, 2024:

Thank you, let’s see if others want this change too. I think it could be acceptable to have more example lines.

hodlinator commented at 7:38 pm on June 29, 2024:

Yeah, I could accept either way. You have shown good persistence with this issue @am-sq!

am-sq commented at 3:30 am on July 18, 2024:

Hi @hodlinator do you know if there is something I can do to get this merged?

hodlinator commented at 6:28 am on July 18, 2024:

Don’t know of a good way unfortunately. Maybe one of the other reviewers will react soon from our comment activity today.

Did a quick search for HelpExampleRpc now and there are a few other RPCs which have quite a few examples.

hodlinator approved

hodlinator commented at 5:54 pm on June 28, 2024: contributor

ACK 69bf58dc0e25897e9fde435c9823a921590a90dc

Built and tested bitcoin-cli -regtest loadwallet.

am-sq requested review from luke-jr on Jun 29, 2024

am-sq requested review from jonatack on Jun 29, 2024

am-sq requested review from maflcko on Jun 29, 2024

DrahtBot added the label CI failed on Aug 19, 2024

DrahtBot removed the label CI failed on Aug 22, 2024

BrandonOdiwuor commented at 11:27 am on September 10, 2024: contributor

Concept ACK

maflcko requested review from murchandamus on Sep 10, 2024

marcofleon commented at 3:11 pm on September 11, 2024: contributor

ACK 69bf58dc0e25897e9fde435c9823a921590a90dc

 0bcli -regtest loadwallet
 1error code: -1
 2error message:
 3loadwallet "filename" ( load_on_startup )
 4
 5Loads a wallet from a wallet file or directory.
 6Note that all wallet command-line options used when starting bitcoind will be
 7applied to the new wallet.
 8
 9Arguments:
101. filename           (string, required) You may pass in the absolute path of your wallet directory or .dat file. Otherwise, the interpreted path will be relative to the default wallet directory.
112. load_on_startup    (boolean, optional) Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged.
12
13Result:
14{                    (json object)
15  "name" : "str",    (string) The wallet name if loaded successfully.
16  "warnings" : [     (json array, optional) Warning messages, if any, related to loading the wallet.
17    "str",           (string)
18    ...
19  ]
20}
21
22Examples:
23
24Load wallet with files under wallets/foo/:
25> bitcoin-cli loadwallet "foo"
26> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": ["foo"]}' -H 'content-type: application/json' http://127.0.0.1:8332/
27
28Load wallet with files under wallets/foo/specialWallet/:
29> bitcoin-cli loadwallet "foo/specialWallet/"
30> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": ["foo/specialWallet/"]}' -H 'content-type: application/json' http://127.0.0.1:8332/
31
32Load wallet using absolute path (Unix):
33> bitcoin-cli loadwallet "/path/to/specialWallet/"
34> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": ["/path/to/specialWallet/"]}' -H 'content-type: application/json' http://127.0.0.1:8332/
35
36Load wallet using absolute path (Windows):
37> bitcoin-cli loadwallet "C:\Users\myusername\specialWallet\"
38> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": ["C:\Users\myusername\specialWallet\"]}' -H 'content-type: application/json' http://127.0.0.1:8332/
39
40Load legacy wallet.dat file directly under the wallets/ directory:
41> bitcoin-cli loadwallet "legacy.dat"
42> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": ["legacy.dat"]}' -H 'content-type: application/json' http://127.0.0.1:8332/

DrahtBot requested review from BrandonOdiwuor on Sep 11, 2024

in src/wallet/rpc/wallet.cpp:223 in 69bf58dc0e outdated

219@@ -220,7 +220,7 @@ static RPCHelpMan loadwallet()
220                 "\nNote that all wallet command-line options used when starting bitcoind will be"
221                 "\napplied to the new wallet.\n",
222                 {
223-                    {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The wallet directory or .dat file."},
224+                    {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "You may pass in the absolute path of your wallet directory or .dat file. Otherwise, the interpreted path will be relative to the default wallet directory."},

tdb3 commented at 5:46 pm on September 11, 2024:

This seems to discuss passing in the absolute path of the .dat file. Saw the following behavior:

(regtest=1 in bitcoin.conf)

 0dev@bdev01:~/bitcoin$ src/bitcoind -daemonwait
 1Bitcoin Core starting
 2dev@bdev01:~/bitcoin$ src/bitcoin-cli createwallet foo
 3{
 4  "name": "foo"
 5}
 6dev@bdev01:~/bitcoin$ src/bitcoin-cli stop
 7Bitcoin Core stopping
 8dev@bdev01:~/bitcoin$ src/bitcoind -daemonwait
 9Bitcoin Core starting
10dev@bdev01:~/bitcoin$ ls -l /home/dev/.bitcoin/regtest/wallets/foo/wallet.dat
11-rw------- 1 dev dev 24576 Sep 11 13:40 /home/dev/.bitcoin/regtest/wallets/foo/wallet.dat
12dev@bdev01:~/bitcoin$ src/bitcoin-cli loadwallet "/home/dev/.bitcoin/regtest/wallets/foo/wallet.dat"
13error code: -4
14error message:
15Wallet file verification failed. Invalid -wallet path '/home/dev/.bitcoin/regtest/wallets/foo/wallet.dat'. -wallet path should point to a directory where wallet.dat and database/log.?????????? files can be stored, a location where such a directory could be created, or (for backwards compatibility) the name of an existing data file in -walletdir ("/home/dev/.bitcoin/regtest/wallets")

hodlinator commented at 9:54 pm on September 11, 2024:

Might be clearer like this:

0                    {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "You may pass in the absolute path of your wallet directory (or .dat file for legacy wallets). Otherwise, the interpreted path will be relative to the default wallet directory."},

murchandamus commented at 7:13 pm on September 12, 2024:

I agree with @hodlinator, and this may be a bit verbose for something that is output to the console. How about:

0                    {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path of the wallet directory (or a legacy wallet’s .dat file). Takes an absolute path or a path relative to the default wallet directory."},

am-sq commented at 9:38 pm on September 12, 2024:

I have made this last suggested change ^.

tdb3 approved

tdb3 commented at 5:50 pm on September 11, 2024: contributor

ACK 69bf58dc0e25897e9fde435c9823a921590a90dc

This is definitely an improvement on the existing loadwallet help.

nit: I believe the RPC page listed in the commit message is actually derived from the help output of loadwallet. Technically this PR addresses the help output, and downstream things that use it would also benefit. I don’t really see a crucial need to change the commit message though, so feel free to ignore. If you happen to change something else, it might be good to also update the commit message.

murchandamus commented at 7:20 pm on September 12, 2024: contributor

utACK 69bf58dc0e25897e9fde435c9823a921590a90dc

Thanks for the additional examples and improving the documentation

am-sq force-pushed on Sep 12, 2024

am-sq commented at 9:38 pm on September 12, 2024: none

ACK 69bf58d

This is definitely an improvement on the existing loadwallet help.

nit: I believe the RPC page listed in the commit message is actually derived from the help output of loadwallet. Technically this PR addresses the help output, and downstream things that use it would also benefit. I don’t really see a crucial need to change the commit message though, so feel free to ignore. If you happen to change something else, it might be good to also update the commit message.

Fixed the commit message!

tdb3 approved

tdb3 commented at 10:54 am on September 13, 2024: contributor

ACK b9aded42252969cc81acf4122ca5e2d1e00ecf90

DrahtBot requested review from marcofleon on Sep 13, 2024

DrahtBot requested review from hodlinator on Sep 13, 2024

DrahtBot requested review from murchandamus on Sep 13, 2024

hodlinator approved

hodlinator commented at 1:28 pm on September 17, 2024: contributor

re-ACK b9aded42252969cc81acf4122ca5e2d1e00ecf90

Checked changes since last ACK with git range-diff master 69bf58d b9aded4.

Adjusted commit message to not mention indirect documentation generation.
Incorporated feedback around .dat-file paths being for legacy wallets.

in src/wallet/rpc/wallet.cpp:223 in b9aded4225 outdated

219@@ -220,7 +220,7 @@ static RPCHelpMan loadwallet()
220                 "\nNote that all wallet command-line options used when starting bitcoind will be"
221                 "\napplied to the new wallet.\n",
222                 {
223-                    {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The wallet directory or .dat file."},
224+                    {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path of the wallet directory (or a legacy wallet's .dat file). Takes an absolute path or a path relative to the default wallet directory."},

achow101 commented at 3:59 pm on September 20, 2024:

The relative path is relative to the wallet directory, not the default wallet directory. Starting with -walletdir=<dir> will change the wallet directory and specifying a wallet name in this argument will be interpreted to be relative to that walletdir.

pablomartin4btc commented at 11:18 pm on September 26, 2024:

We have already updated <filename> to <walletname> in a few places (bitcoin-cli.cpp and wallet/rpc/utils.cpp), shouldn’t be this the case as well? nit:

0                    {"walletname", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the wallet directory (absolute or relative to the wallets directory), or a legacy wallet's .dat file (within the wallets directory). The default wallets directory is the 'wallets' folder inside the data directory, unless the `-walletdir` option is specified."},

am-sq commented at 4:30 am on October 1, 2024:

Thanks for the feedback - made the change to indicate that if -walletdir is specified, loadwallet will interpret argument relative to that directory.

am-sq commented at 4:30 am on October 1, 2024:

Sounds fair to me - made this change 👍

maflcko commented at 5:10 am on October 1, 2024:

This is a breaking change and will cause the tests to fail, not a documentation change

pablomartin4btc commented at 1:18 pm on October 1, 2024:

My bad, my suggestion was more for discussion as I did it on top of @achow101’s one. The tests can be updated but I see we could be breaking things on users end if they pass named args (filename=) as in wallet_startup,py.

am-sq commented at 6:44 pm on October 1, 2024:

Perhaps that can be a separate change to this then? Fixed so that change is not breaking.

pablomartin4btc commented at 11:48 pm on September 26, 2024: member

Concept ACK

am-sq force-pushed on Oct 1, 2024

doc: loadwallet loads from relative walletdir

Improves the documentation of help output for loadwallet
to clarify that filename is relative to the default
wallet directory. Adds examples that get a wallet from
sub-directories.

221296c1f0

am-sq force-pushed on Oct 1, 2024

hodlinator approved

hodlinator commented at 7:00 pm on October 1, 2024: contributor

re-ACK 221296c1f0917bcf4da8fa0bbc5494a28dc6b697

git range-diff master b9aded4 221296c

Just improving precision of filename argument description.

DrahtBot requested review from pablomartin4btc on Oct 1, 2024

DrahtBot requested review from tdb3 on Oct 1, 2024

am-sq requested review from achow101 on Oct 2, 2024

adamandrews1 approved

adamandrews1 commented at 9:47 pm on October 16, 2024: none

ACK https://github.com/bitcoin/bitcoin/commit/221296c1f0917bcf4da8fa0bbc5494a28dc6b697

in src/wallet/rpc/wallet.cpp:238 in 221296c1f0

233@@ -234,8 +234,21 @@ static RPCHelpMan loadwallet()
234                     }
235                 },
236                 RPCExamples{
237-                    HelpExampleCli("loadwallet", "\"test.dat\"")
238-            + HelpExampleRpc("loadwallet", "\"test.dat\"")
239+                    "\nLoad wallet with files under wallets/foo/:\n"
240+                    + HelpExampleCli("loadwallet", "\"foo\"")

MarnixCroes commented at 3:20 pm on October 18, 2024:

0                    + HelpExampleCli("loadwallet", "\"walletname\"")

walletname is easier to understand than foo imo

in src/wallet/rpc/wallet.cpp:237 in 221296c1f0

233@@ -234,8 +234,21 @@ static RPCHelpMan loadwallet()
234                     }
235                 },
236                 RPCExamples{
237-                    HelpExampleCli("loadwallet", "\"test.dat\"")
238-            + HelpExampleRpc("loadwallet", "\"test.dat\"")
239+                    "\nLoad wallet with files under wallets/foo/:\n"

MarnixCroes commented at 3:22 pm on October 18, 2024:

0                    "\nLoad wallet from the wallet dir:\n"

this is easier to understand imo, and is correct for default cases and when -walletdir is specified

in src/wallet/rpc/wallet.cpp:240 in 221296c1f0

233@@ -234,8 +234,21 @@ static RPCHelpMan loadwallet()
234                     }
235                 },
236                 RPCExamples{
237-                    HelpExampleCli("loadwallet", "\"test.dat\"")
238-            + HelpExampleRpc("loadwallet", "\"test.dat\"")
239+                    "\nLoad wallet with files under wallets/foo/:\n"
240+                    + HelpExampleCli("loadwallet", "\"foo\"")
241+                    + HelpExampleRpc("loadwallet", "\"foo\"")
242+                    + "\nLoad wallet with files under wallets/foo/specialWallet/:\n"

MarnixCroes commented at 3:37 pm on October 18, 2024:

in what case would a user have this? seems non standard, and not worth having an example for imo.

in src/wallet/rpc/wallet.cpp:244 in 221296c1f0

241+                    + HelpExampleRpc("loadwallet", "\"foo\"")
242+                    + "\nLoad wallet with files under wallets/foo/specialWallet/:\n"
243+                    + HelpExampleCli("loadwallet", "\"foo/specialWallet/\"")
244+                    + HelpExampleRpc("loadwallet", "\"foo/specialWallet/\"")
245+                    + "\nLoad wallet using absolute path (Unix):\n"
246+                    + HelpExampleCli("loadwallet", "\"/path/to/specialWallet/\"")

MarnixCroes commented at 3:44 pm on October 18, 2024:

0                    + HelpExampleCli("loadwallet", "\"/path/to/walletname/\"")

more clear imo same for other ones

MarnixCroes commented at 3:48 pm on October 18, 2024: contributor

cACK good clarification

in src/wallet/rpc/wallet.cpp:223 in 221296c1f0

219@@ -220,7 +220,7 @@ static RPCHelpMan loadwallet()
220                 "\nNote that all wallet command-line options used when starting bitcoind will be"
221                 "\napplied to the new wallet.\n",
222                 {
223-                    {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The wallet directory or .dat file."},
224+                    {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the wallet directory (absolute or relative to the wallets directory), or a legacy wallet's .dat file (within the wallets directory). The default wallets directory is the 'wallets' folder inside the data directory, unless the `-walletdir` option is specified."},

achow101 commented at 4:39 pm on October 23, 2024:

“The default wallets directory is the ‘wallets’ folder inside the data directory, unless the -walletdir option is specified.”

This sentence is a little bit confusing as it suggests -walletdir changes the default somehow. I would rephrase this as “The wallets directory is set by the -walletdir option and defaults to the ‘wallets’ folder within the data directory.”

ralyodio approved

ralyodio commented at 4:10 pm on November 1, 2024: none

LGtm

doc: clarify loadwallet path loading for wallets #30302

Why this change?

The changes implemented

Code Coverage & Benchmarks

Reviews

Conflicts