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.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

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