A discussion that has come up several times at the Localhost office over the past several weeks has been whether it makes sense to load wallets by paths. In particular, supporting wallets by path has been the source of a couple issues, and has also had some involvement in the recent wallet deletion issue.
For some background, the name of a wallet can always be interpreted as a path. If it is not an absolute path, then it is a path relative to the wallets directory, e.g. a wallet named mywallet is always located at <walletsdir>/mywallet. But when referring to wallets named by relative path, I mean wallets which actually include / in their name which turns the name into a path specifier, rather than a logical name that happens to map to a path relative to the walletsdir. Otherwise, the path is an absolute path. An important note is that with the exception of BDB files stored directly in the walletsdir, the file actually opened is always <path>/wallet.dat.
For absolute paths, this seems mostly ok, if just a bit odd. This has the potential to be problematic when mixing wallet names with filenames and paths. However, I don’t recall any specific issues with absolute paths off the top of my head.
On the other hand, relative paths that include additional directories (e.g. <walletdir>/<subdir>/mywallet or ../<more path>/mywallet) have been a major source of headaches in wallet filesystem code:
- #32273 pointed out a flaw in how wallet names and paths were being combined in relative paths to produce nonsensical paths
- The root issue in #31409 is searching subdirectories for additional wallets to have their name by relative path
Furthermore, from a user perspective, relative path wallets are really weird in that the path is relative to the walletsdir, which itself is not always consistent relative to the datadir (excluding when set explicitly). Given that the actual location of the walletsdir is basically never surfaced to the user, I don’t really see any situation where a user would want to name a wallet ../<path>. I can see <sub>/mywallet, but that really doesn’t need to be a path, and the wallet could actually be in a directory named Edit: <sub>/mywallet where the / is escaped so it doesn’t become a path./ can’t be escaped.
I propose that we should remove support for wallets with by relative path, but retain support for wallets by absolute path.
But I’d like to get some feedback from contributors and users before moving forward with this.
There has also been the suggestion that we should go all the way and just remove support for both relative and absolute paths. I push back on this a bit as I think absolute paths can still be useful, e.g. if a user has wallets both in the walletsdir and on external media, they could want to load wallets from both locations at the same time without needing to change the walletsdir by specifying wallets by absolute path. However, I am open to being convinced that remove absolute path support could be a good idea.
