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

ghost · 2021-05-13T16:04:01Z

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 wallet: Do not iterate a directory if having an error while accessing it #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 #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"
    }
  ]
}

Talkless · 2021-05-13T18:34:29Z

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

src/wallet/db.cpp

ghost · 2021-05-14T00:25:29Z

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.

promag

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

promag · 2021-05-15T09:43:37Z

src/wallet/db.cpp

+            fs::path path = wallet_dir.string();
+
+            if (path == path.root_path()) { //If path is a 'root directory' for Windows
+               path = it->path().string().substr(offset - 1);


nit, wrong indent

DrahtBot · 2021-05-17T16:00:34Z

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

Conflicts

No conflicts as of last run.

Talkless · 2021-05-19T17:42:13Z

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 · 2021-05-20T16:34:21Z

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

Cc: @MarcoFalke @meshcollider

ryanofsky · 2021-05-28T00:13:56Z

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 c5ee096:

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:"])

+ Remove one character less from wallet path if root directory

ghost · 2021-06-01T15:08:40Z

Thanks for the review @ryanofsky

Made changes in d44a261

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

Agree

ryanofsky

Code review ACK d44a261

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

utACK d44a261

…ctory d44a261 Fix issues when `walletdir` is root directory (unknown) Pull request description: + 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 bitcoin#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: bitcoin#21510 bitcoin#21501 Related PR: bitcoin#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" } ] } ``` ACKs for top commit: ryanofsky: Code review ACK d44a261 meshcollider: utACK d44a261 Tree-SHA512: b09b00f727407e3771c8694861dae1bfd29d97a0d51ddcb5d9c0111dc618b3fff2f75829cbb4361c54457ee564e94fcefd9e2928262a1c918a2b6bbad724eb55

+ Remove one character less from wallet path if root directory Github-Pull: bitcoin#21944 Rebased-From: d44a261

DrahtBot added the Wallet label May 13, 2021

Talkless reviewed May 13, 2021

View reviewed changes

src/wallet/db.cpp Outdated Show resolved Hide resolved

ghost changed the title ~~wallet: Fix issues when walletdir is root directory(Windows)~~ wallet: Fix issues when walletdir is root directory May 14, 2021

promag reviewed May 15, 2021

View reviewed changes

DrahtBot mentioned this pull request May 17, 2021

Use std::filesystem. Remove Boost Filesystem & System #20744

Merged

Fix issues when walletdir is root directory

d44a261

+ Remove one character less from wallet path if root directory

ryanofsky approved these changes Jun 2, 2021

View reviewed changes

meshcollider reviewed Jun 9, 2021

View reviewed changes

meshcollider merged commit 69577a2 into bitcoin:master Jun 9, 2021

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

Fix issues when walletdir is root directory

632e444

+ Remove one character less from wallet path if root directory Github-Pull: bitcoin#21944 Rebased-From: d44a261

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 69577a2 into merged_master (Bitcoin PR bitcoin/bitcoin#21944)

29a1e0d

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

ghost commented May 13, 2021 •

edited by ghost

Loading

Uh oh!

Talkless commented May 13, 2021

Uh oh!

Uh oh!

ghost commented May 14, 2021 •

edited by ghost

Loading

Uh oh!

promag left a comment

Uh oh!

promag May 15, 2021

Uh oh!

DrahtBot commented May 17, 2021 •

edited

Loading

Uh oh!

Talkless commented May 19, 2021

Uh oh!

ghost commented May 20, 2021

Uh oh!

ryanofsky commented May 28, 2021 •

edited

Loading

Uh oh!

ghost commented Jun 1, 2021

Uh oh!

ryanofsky left a comment

Uh oh!

meshcollider left a comment

Uh oh!

Uh oh!

wallet: Fix issues when walletdir is root directory #21944

wallet: Fix issues when walletdir is root directory #21944

Uh oh!

Conversation

ghost commented May 13, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Talkless commented May 13, 2021

Uh oh!

Uh oh!

ghost commented May 14, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag May 15, 2021

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Talkless commented May 19, 2021

Uh oh!

ghost commented May 20, 2021

Uh oh!

ryanofsky commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 1, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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

ghost commented May 13, 2021 •

edited by ghost

Loading

ghost commented May 14, 2021 •

edited by ghost

Loading

DrahtBot commented May 17, 2021 •

edited

Loading

ryanofsky commented May 28, 2021 •

edited

Loading