wallet: Reject whitespace-only wallet names #35768

Summary

This PR trims wallet names using util::TrimStringView before the empty check, across all call sites that currently guard against empty wallet names

Motivation: Although there is a check against empty wallet names, it does not reject wallet names consisting solely of whitespace. As a result, the RPC command below succeeds and creates a wallet with a whitespace-only name. Such names are not meaningful and are effectively indistinguishable from nameless directories to users, so they should be rejected in the same way as empty wallet names.

bitcoin-cli createwallet "    "

util::TrimStringView; has been preferred as opposed to util::TrimString; to prevent mutating the user input but guarding against space-only names. This means that the wallet name can have leading and trailing white space unless requested otherwise.

Testing

Before:

ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli createwallet "                            "
{
  "name": "                            "
}
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ls ~/.bitcoin/regtest/wallets/
 '                        '   '                           '   '                            '
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ 

After:

ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ ./bitcoin-cli createwallet "                    "
error code: -8
error message:
Wallet name cannot be empty
ratedg@0xratedg:~/Desktop/bitcoin/build/bin$

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #35833 (rpc,wallet: prevent user input injecting fake log lines by l0rinc)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task lint: https://github.com/bitcoin/bitcoin/actions/runs/29873844736/job/88871531263} _{LLM reason (✨ experimental): CI failed because the Python lint checks reported trailing/blank-line whitespace in test/functional/tool_wallet.py (ruff W293 / trailing_whitespace), causing py_lint to exit non-zero.}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly 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.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

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

</details>

What is the benefit here? Seems highly risky, because it could mean that a pre-existing wallet name that is nested in whitespace could possibly not be loaded/restored/dumped/etc.

So ~0 for a risky change without any benefits.

What is the benefit here? Seems highly risky, because it could mean that a pre-existing wallet name that is nested in whitespace could possibly not be loaded/restored/dumped/etc.

So NACK for a risky change without any benefits.

Thanks for the feedback. My motivation was to prevent wallet names that consist solely of whitespace, since they can be confusing to users and difficult to distinguish in the UI or when passed through the RPC interface. For instance, a user creates a wallet with 2 spaces , 3 spaces , and 4 spaces , which results in a very confusing directory structure. That is why I approached it with TrimStringView instead of TrimString. I would humbly like to understand whether there are use cases where a wallet name consisting only of whitespace would be intentionally created or relied upon.

Please note that the PR is not trimming the user inputs

ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ ./bitcoin-cli createwallet "               testallet               "
{
  "name": "               testallet               "
}

the above is totally valid, and it is saved as such. The one below is, however, rejected because it contains only spaces

ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ ./bitcoin-cli createwallet "                              "
error code: -8
error message:
Wallet name cannot be empty
ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ 

NB: I doubt whether someone would resort to the latter.

IMO, I do not see the difference between and

I would humbly like to understand whether there are use cases where a wallet name consisting only of whitespace would be intentionally created or relied upon.

I don't know if a user exists with a space-only wallet, but breaking the UX for them for no reason seems pointless?

I don't know if a user exists with a space-only wallet, but breaking the UX for them for no reason seems pointless?

Does it break the UX? The check was there already name.empty() only this time TrimStringView(name).empty()). I'm genuinely trying to understand how this change could negatively impact the user experience. I'd really appreciate any examples or scenarios I may be overlooking

It's reasonable to disallow such names when creating a new wallet, but still allow existing wallets to be loaded.

It's reasonable to disallow such names when creating a new wallet, but still allow existing wallets to be loaded.

I have restored the src/wallet/wallet.cpp:458::RestoreWallet. I have left the src/wallet/dump.cpp:124:bool CreateFromDump however, meaning that a wallet will have to be imported into a properly named wallet. If otherwise, I can revert it too.

Are non-ASCII covered by TrimStringView? Or should they even matter in this case?

Are non-ASCII covered by TrimStringView? Or should they even matter in this case?

TrimStringView() (util/string.h:L162C103) checks for whitespace patterns (6 whitespace characters to be specific).

I think if we are doing this we should disable any white-space beginning or ending a wallet name.

Valid cases:

• "this is a valid wallet name"

Invalid cases:

• " "
• " wallet name"
• "wallet name "
• " wallet name "

But this restriction must only be applied in wallet creation to keep backwards compatibility. Also the error message should not be "empty wallet name" as that is not the case. Functional tests should be added for each case. And a release note will be necessary for this, affecting the GUI and the RPC interface. I think the motivation for this should be avoid not being able to load the wallet as you cannot easily identify the number of white-spaces that the name has.

I think if we are doing this we should disable any white-space beginning or ending a wallet name.

Valid cases:

• "this is a valid wallet name"

Invalid cases:

• " "
• " wallet name"
• "wallet name "
• " wallet name "

This is very helpful @polespinasa . Checking before accepting instead of accepting then trimming without users' knowledge, for your last three example cases i.e. I will work on tests and the release notes.

I think the motivation for this should be avoid not being able to load the wallet as you cannot easily identify the number of white-spaces that the name has.

Sorry for bike-shedding this, but if this was the motivation, what about:

• wallet a name
• wallet aaaaaaaaaaaaaaaaaaaaaaaaaaa name

Should they be invalid as well? Again, no objection. I am just curios what the real motivation here is. Also, this currently code-conflicts with #35833.

Will take a look to #35833.

About motivation, yeah it is tricky because there can always be weird cases, idk where the line is on what should be able to be done and what not. I only specified white-spaces at the begging and at the end bc they are more likely to be "invisible" in some desktop interfaces. But if we care about > 1 white-spaces in between also, we could just forbid more than one white-spaces together if between other characters.

About the "... aaaaaaaaaaaaaaaa ..." I think that is fine as those are "visible" characters, but that is just an opinion, obviously still ugly.

I see it more as a "mistake" prevention, in case a user by mistake sets an empty space at the beginning and then he is unable to load the wallet.

My motivation was to prevent space only wallet names by adding more validation. For backward compatibility achow recommended leaving the loading logic for now. I guess it will be done eventually, hahah. IMO I can't seem to find any justification for allowing such names.