wallet: Fix issues when `walletdir` is root directory #21944

ghost commented at 4:04 PM on May 13, 2021: none

Remove one character less from wallet path
After testing lot of random strings with special chars in wallet_name, I found that the issue was not related to special characters in the name. Reviewing PR #21907 helped me resolve the issue.

Real issue: If the path mentioned in walletdir is a root directory, first character of the wallet name or path is removed

Solution: if statement to check walletdir is a root directory

Fixes: #21510 https://github.com/bitcoin/bitcoin/issues/21501 Related PR: #20080

Consider the wallet directories w1 and w2 saved in D:\. Run bitcoind.exe -walletdir=D:\, Results for bitcoin-cli.exe listwalletdir:

Before this PR:


{
  "wallets": [
    {
      "name": "1"
    },
    {
      "name": "2"
    }
  ]
}

After this PR:

  "wallets": [
    {
      "name": "w1"
    },
    {
      "name": "w2"
    }
  ]
}

DrahtBot added the label Wallet on May 13, 2021

Talkless commented at 6:34 PM on May 13, 2021: none

Welp, I did crazy thing and launched bitcoind as root, with:

sudo src/bitcoind -regtest -walletdir=/ -datadir=/tmp/datadir -server

Created wallet, and then listed:

{
  "wallets": [
    {
      "name": "est"
    },
    {
      "name": "ome/vincas/.bitcoin/regtest/wallets"
    },
    {
      "name": "ome/vincas/code/eclair/eclair_recovery_tool.git/eclair-core/target/integration-8198c6f7-2f0b-4394-a92a-d4d738994f33/datadir-bitcoin/regtest/wallets"
    },
    {
      "name": "ome/vincas/code/eclair/eclair_recovery_tool.git/eclair-core/target/integration-9063e6a5-c013-4127-a7dd-a10a3874ed18/datadir-bitcoin/regtest/wallets"
    },
    {
      "name": "ome/vincas/code/eclair/eclair_recovery_tool.git/eclair-core/target/integration-e9783f5b-51ab-458f-ab57-31976e38bff8/datadir-bitcoin/regtest/wallets"
    },
    {
      "name": "ome/vincas/code/eclair/eclair_recovery_tool.git/eclair-core/target/integration-f1c34eba-d63e-4379-a315-75056bbcc162/datadir-bitcoin/regtest/wallets"
    },
    {
      "name": "ome/vincas/code/eclair/eclair_recovery_tool.git/eclair-core/target/integration-d4096df3-5adb-40dd-9425-6f80fb6fd07b/datadir-bitcoin/regtest/wallets"
    },
    {
      "name": "ome/vincas/code/eclair/eclair_recovery_tool.git/eclair-core/target/integration-e6a27da9-a78e-48a5-a272-38a950ddfd41/datadir-bitcoin/regtest/wallets"
    },
    {
      "name": "ome/vincas/code/bitcoin/123-bitcoin-core-gui.git/test/functional/data/wallets/high_minversion"
    },
    {
      "name": "ome/vincas/code/bitcoin/149-bitcoin-core-gui.git/test/functional/data/wallets/high_minversion"
    }
  ]
}

Interesting to see bitcoind scan whole system for wallets :D (log even printed ListDatabases: Error scanning /sys/fs/bpf: boost::filesystem::status: Operation not permitted: "/sys/fs/bpf/wallet.dat").

Anyhow, issue is the same on Linux, and this PR did fix that too:

{
  "wallets": [
    {
      "name": "test"
    },
...same...

in src/wallet/db.cpp:28 in 48a73833c2 outdated

  24 | @@ -25,7 +25,13 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
  25 |          try {
  26 |              // Get wallet path relative to walletdir by removing walletdir from the wallet path.
  27 |              // This can be replaced by boost::filesystem::lexically_relative once boost is bumped to 1.60.
  28 | -            const fs::path path = it->path().string().substr(offset);
  29 | +            fs::path path = wallet_dir.string();

Talkless commented at 6:50 PM on May 13, 2021:

You could still keep path const as before, by utilizing immediately-invoked lambda, as suggested in ES.28: Use lambdas for complex initialization, especially of const variables guideline, with something like this (also avoiding re-initializing path variable):

            const fs::path path = [&wallet_dir, &it, offset]() {

                if (wallet_dir == wallet_dir.root_path()) {
                   return it->path().string().substr(offset - 1);
                }

                return it->path().string().substr(offset);
            }();

Not sure if this is the best way to do that overall, as I don't have basically any experience with boost/stl filesystem...

unknown commented at 12:26 AM on May 14, 2021:

Will try this approach

ghost commented at 12:25 AM on May 14, 2021: none

Interesting to see bitcoind scan whole system for wallets :D (log even printed ListDatabases: Error scanning /sys/fs/bpf: boost::filesystem::status: Operation not permitted: "/sys/fs/bpf/wallet.dat").

Yes it recursively iterates path mentioned. How much time did this take?

Anyhow, issue is the same on Linux, and this PR did fix that too

Interesting. Thanks for testing this on Linux. I will change name, description, commit message and comment after testing few more things.

Although I think it's less likely someone will save wallet in root on Linux. But it's normal to save different folders in root of different partitions on Windows.

unknown renamed this:
~~wallet: Fix issues when `walletdir` is root directory(Windows)~~
wallet: Fix issues when `walletdir` is root directory
on May 14, 2021

in src/wallet/db.cpp:31 in 48a73833c2 outdated

  24 | @@ -25,7 +25,13 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
  25 |          try {
  26 |              // Get wallet path relative to walletdir by removing walletdir from the wallet path.
  27 |              // This can be replaced by boost::filesystem::lexically_relative once boost is bumped to 1.60.
  28 | -            const fs::path path = it->path().string().substr(offset);
  29 | +            fs::path path = wallet_dir.string();
  30 | +
  31 | +            if (path == path.root_path()) { //If path is a 'root directory' for Windows
  32 | +               path = it->path().string().substr(offset - 1);

promag commented at 9:43 AM on May 15, 2021:

nit, wrong indent

promag commented at 9:59 AM on May 15, 2021: member

Not sure it's worth adding these cases considering #20744 unless we want to backport the fix.

DrahtBot commented at 4:00 PM on May 17, 2021: member

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

Conflicts

No conflicts as of last run.

Talkless commented at 5:42 PM on May 19, 2021: none

Yes it recursively iterates path mentioned. How much time did this take?

About 19s in a VM running on i7 laptop with SATA SSD.

ghost commented at 4:34 PM on May 20, 2021: none

Not sure it's worth adding these cases considering #20744 unless we want to backport the fix.

Cc: @MarcoFalke @meshcollider

ryanofsky commented at 12:13 AM on May 28, 2021: member

Definitely good to have a fix for this issue. Would welcome simplifying the PR though. I think the following would be a simpler short term fix than c5ee096d596766b5f9dd6e3f8d81c8e87c0e9c3a:

diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp
index cd49baeb786..993dd09b8b9 100644
--- a/src/wallet/db.cpp
+++ b/src/wallet/db.cpp
@@ -12,7 +12,7 @@
 
 std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
 {
-    const size_t offset = wallet_dir.string().size() + 1;
+    const size_t offset = wallet_dir.string().size() + (wallet_dir == wallet_dir.root_name() ? 0 : 1);
     std::vector<fs::path> paths;
     boost::system::error_code ec;

This:

avoids adding and then subtracting 1 unnecessarily
avoids doing the same comparison in a loop
avoids using an unnecessary lambda
uses root_name instead of root_path which should avoid triggering for / on linux according to https://www.boost.org/doc/libs/1_69_0/libs/filesystem/doc/reference.html#path-decomposition-table

In long term after we switch from boost (#20744) or update boost (#19667), a more robust fix also suggested #21501 (comment) would be:

--- a/src/wallet/db.cpp
+++ b/src/wallet/db.cpp
@@ -12,7 +12,6 @@
 
 std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
 {
-    const size_t offset = wallet_dir.string().size() + 1;
     std::vector<fs::path> paths;
     boost::system::error_code ec;
 
@@ -24,8 +23,7 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
 
         try {
             // Get wallet path relative to walletdir by removing walletdir from the wallet path.
-            // This can be replaced by boost::filesystem::lexically_relative once boost is bumped to 1.60.
-            const fs::path path = it->path().string().substr(offset);
+            const fs::path path = fs::lexically_relative(it->path(), wallet_dir);
 
             if (it->status().type() == fs::directory_file &&
                 (IsBDBFile(BDBDataFile(it->path())) || IsSQLiteFile(SQLiteDataFile(it->path())))) {

If we wanted to be really ambitious maybe we could write a unit test for the bug using subst subprocess.run(f"subst {tempdir} x:"), node.start(["-walletdir=x:"])

Fix issues when `walletdir` is root directory

+ Remove one character less from wallet path if root directory

d44a261acf

ghost commented at 3:08 PM on June 1, 2021: none

Thanks for the review @ryanofsky

Made changes in https://github.com/bitcoin/bitcoin/commit/d44a261acff40c1c8727d3cc0106bde65a6416d0

Would welcome simplifying the PR though. I think the following would be a simpler short term fix than c5ee096

Agree

ryanofsky approved

ryanofsky commented at 7:55 PM on June 2, 2021: member

Code review ACK d44a261acff40c1c8727d3cc0106bde65a6416d0

For background on this issue: the problem of wallet name truncation would have started happening for X: style wallet dirs after #14561 and would have started happening for X:\ style wallet dirs after #20080, IIUC.

Long term fix should be switching lexically_relative as described #21944 (comment)

meshcollider commented at 2:06 AM on June 9, 2021: contributor

utACK d44a261acff40c1c8727d3cc0106bde65a6416d0

meshcollider merged this on Jun 9, 2021

meshcollider closed this on Jun 9, 2021

sidhujag referenced this in commit a150948159 on Jun 9, 2021

luke-jr referenced this in commit 632e444470 on Jun 27, 2021

gwillen referenced this in commit 29a1e0dbe8 on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022