Fix: While fiddling with wallets, I noticed that if datadir == walltetsdir an unsynced node can become quite unresponsive.
if no “wallets” dir is in datadir ( i.e. exiting old node ) we would have iterate trough our own node files to find wallets, that consumes time and could cause an unresponsive node.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
No strong opinion regarding excluding some paths. Current patch needs to be updated (format) and maybe change implementation to use a list of ignore/exclude paths.
This can improve GUI by reducing freezes but it’s not the right fix for that. When ListWalletDir is triggered from the GUI, it should be called on a background thread. Alternatively we could consider caching ListWalletDir result and invalidate it when wallets are created or loaded.
No strong opinion regarding excluding some paths. Current patch needs to be updated (format) and maybe change implementation to use a list of ignore/exclude paths.
This can improve GUI by reducing freezes but it’s not the right fix for that. When
ListWalletDiris triggered from the GUI, it should be called on a background thread. Alternatively we could consider cachingListWalletDirresult and invalidate it when wallets are created or loaded.
yup, ugly quick fix and background thread seams reasonable.. but fix proves the point and lets be honest why should we waste time and risk locks, when traverse trough known node dir?.. . also other GUI freezes should be tackled. Before moon scale there is a long list of things in my backlog, that need to be fixed before bam.. . There might be soon real users who use the gui, and lets be real, gui is bitcoin for them.
59@@ -60,33 +60,69 @@ std::vector<fs::path> ListWalletDir()
60 const size_t offset = wallet_dir.string().size() + 1;
61 std::vector<fs::path> paths;
62 boost::system::error_code ec;
63+ fs::path sub_path = GetDataDir();
64+
65+ bool we_have_wallets_dir = (wallet_dir != sub_path);
66+ const fs::path fblocks = GetBlocksDir();
const std::vector ignore_paths{ GetBlocksDir(), sub_path / "testnet3", ... } and then compare against this.
84- // Get wallet path relative to walletdir by removing walletdir from the wallet path.
85- // This can be replaced by boost::filesystem::lexically_relative once boost is bumped to 1.60.
86- const fs::path path = it->path().string().substr(offset);
87+ // We dont want to iterate trough those special node dirs
88+ if (it->status().type() == fs::directory_file) {
89+ skip_path = false;
better to define skip_path here
Note to other reviewers, the enum file_type: https://www.boost.org/doc/libs/1_66_0/libs/filesystem/doc/reference.html#Enum-file_type
87+ // We dont want to iterate trough those special node dirs
88+ if (it->status().type() == fs::directory_file) {
89+ skip_path = false;
90+ for(const auto& fpath: ignore_paths) {
91+ if (!skip_path && it->path().string().find(fpath.string()) == 0) {
92+ skip_path = true;
could use break; so you don’t need to check !skip_path && (and in fact, don’t need it at all)
You can also do it.disable_recursion_pending(); here
(and in fact, don’t need it at all)
hmm… If we allow wallets in those dir yes, but i would choose to touch the dir’s not at all, Please view fixup I will post before i squash. I guess we will need the skip But sure about that we need some vote. :+1:
94+ }
95+ if (skip_path) {
96+ it.disable_recursion_pending();
97+ continue;
98+ }
99+ } else it.disable_recursion_pending();
nit: just use brackets and newline
Also, I think we should follow symlinks? symlink_file
Concept ACK
This seems orthogonal to using a background thread. We shouldn’t be looking for wallets inside the blocks and chainstate dir even in the background.
86- const fs::path path = it->path().string().substr(offset);
87+ // We dont want to iterate trough those special node dirs
88+ if (it->status().type() == fs::directory_file) {
89+ skip_path = false;
90+ for(const auto& fpath: ignore_paths) {
91+ if (!skip_path && it->path().string().find(fpath.string()) == 0) {
ignore_paths, can’t we use an exact match? (And then ignore_paths can become an unordered_set and just use .count here…)
70+ blocks_dir,
71+ data_dir / "testnet3",
72+ data_dir / "regtest",
73+ data_dir / "chainstate",
74+ data_dir / "database",
75+ data_dir / "indexes",
files.md)
const std::vector<fs::path> ignore_paths = {
blocks_dir,
data_dir / "blktree",100
101 if (it->status().type() == fs::directory_file && IsBerkeleyBtree(it->path() / "wallet.dat")) {
102 // Found a directory which contains wallet.dat btree file, add it as a wallet.
103- paths.emplace_back(path);
104+ // Get wallet path relative to walletdir by removing walletdir from the wallet path.
105+ // This can be replaced by boost::filesystem::lexically_relative once boost is bumped to 1.60.
std::filesystem::path::lexically_relative (C++17) :)
93+ }
94+ }
95+ if (skip_path) {
96+ it.disable_recursion_pending();
97+ continue;
98+ }
0 if (ignore_paths.count(it->path()) {
1 it.disable_recursion_pending();
2 continue;
3 }
if (std::count(ignore_paths.begin(), ignore_paths.end(), it->path())) {
not sure ignore_paths has a count?
unordered_set as suggested
101- // opened in a shared database environment for backwards compatibility.
102- // Add it to the list of available wallets.
103- paths.emplace_back(path);
104+ // We dont want to iterate trough those special node dirs
105+ if (std::count(ignore_paths.begin(), ignore_paths.end(), it->path())) {
106+ it.disable_recursion_pending();
disable_recursion_pending is a new name introduced in Boost 1.72. We support much older. Renaming this to no_push (the old name) should just work.
How about?
Suggestion:
0diff --git a/src/fs.h b/src/fs.h
1index dfbecc18e..390c38790 100644
2--- a/src/fs.h
3+++ b/src/fs.h
4@@ -17,6 +17,10 @@
5 /** Filesystem operations and types */
6 namespace fs = boost::filesystem;
7
8+#ifndef NAMESPACE_BOOST
9+#define NAMESPACE_BOOST
10+#endif
11+
12 /** Bridge operations to C stdio */
13 namespace fsbridge {
14 FILE *fopen(const fs::path& p, const char *mode);
15diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp
16index a1387111e..806818a36 100644
17--- a/src/wallet/walletutil.cpp
18+++ b/src/wallet/walletutil.cpp
19@@ -85,7 +85,13 @@ std::vector<fs::path> ListWalletDir()
20
21 // We dont want to iterate trough those special node dirs
22 if (std::count(ignore_paths.begin(), ignore_paths.end(), it->path())) {
23- it.disable_recursion_pending();
24+ #ifdef NAMESPACE_BOOST
25+ // BOOST iterator
26+ it.no_push();
27+ #else
28+ // We have >= c++17
29+ it.disable_recursion_pending();
30+ #endif
31 continue;
32 }
33
72+ data_dir / "chainstate",
73+ data_dir / "coins",
74+ data_dir / "database",
75+ data_dir / "indexes",
76+ data_dir / "regtest",
77+ data_dir / "testnet3"
63 const size_t offset = wallet_dir.string().size() + 1;
64 std::vector<fs::path> paths;
65 boost::system::error_code ec;
66
67+ // Here we place the top level dirs we want to skip in case walletdir is datadir or blocksdir
68+ // Those directory's are referenced in doc/files.md
nit:
0 // Those directories are referenced in doc/files.md
99- // Found top-level btree file not called wallet.dat. Current bitcoin
100- // software will never create these files but will allow them to be
101- // opened in a shared database environment for backwards compatibility.
102- // Add it to the list of available wallets.
103- paths.emplace_back(path);
104+ // We dont want to iterate trough those special node dirs
nit:
0 // We don't want to iterate through those special node dirs
124+ // opened in a shared database environment for backwards compatibility.
125+ // Add it to the list of available wallets.
126+ paths.emplace_back(it->path().filename());
127+ }
128 }
129+ } catch (const std::exception& e) {
When WalletDir == DataDir we would have iterate trough our own node files
to find wallets, that consumes time and could cause an unresponsive node.
101- // software will never create these files but will allow them to be
102- // opened in a shared database environment for backwards compatibility.
103- // Add it to the list of available wallets.
104- paths.emplace_back(path);
105+ // We don't want to iterate through those special node dirs
106+ if (std::count(ignore_paths.begin(), ignore_paths.end(), it->path())) {
yup. no insist here from my side, but nit too if “not” const time could that sidechannel reveal where the wallet is or else?
Btw> since u review here and in #18095, 18095 does fix some problems, but not the wallet of death, could we transfer attention of reviewers to this here and fix some bugs fast in one sweep, before adding there new ones? I must admit my PR headlines look often a bit harmless and just like minor nits, but when I way in and insist, the hut is burning. just saying .
std::count guaranties const time to prevent sidechannel reveal where the wallet is or else. But I’m not an expert in this field. Feel free to mark this comment as resolved.
Approach ACK da6eb3c2f8ed5c8e2a5acb878a1abcb9d6f1c02b.
Mind adding #include <algorithm> in separate header group?
Designed to be used on ranges of elements.
122+ } else {
123+ // Found top-level btree file not called wallet.dat. Current bitcoin
124+ // software will never create these files but will allow them to be
125+ // opened in a shared database environment for backwards compatibility.
126+ // Add it to the list of available wallets.
127+ paths.emplace_back(it->path().filename());
4@@ -5,6 +5,7 @@
5 #ifndef BITCOIN_WALLET_WALLETUTIL_H
6 #define BITCOIN_WALLET_WALLETUTIL_H
7
8+#include <algorithm>
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
Rebased and addressed https://github.com/bitcoin/bitcoin/commit/5a8f19eb331acbb1f7e2f52fe786ed21b30297b6 https://github.com/bitcoin/bitcoin/commit/4fb4fb716cd222ca8ae988d039328a66eb6e1f3b an comments, changed by this a lot so pls review again @luke-jr could not use ur code by merge and lost by fixup ur commit entrys, i hope u don;t mind.
https://github.com/Saibato/bitcoin/tree/wallet_351
Since this is closed here the rebase and changes are just in my tree, since github does not update closed pr tyi
to find wallets, that consumes time and could cause an unresponsive node. @laanwj “… cause an unresponsive node” in commit message was a hint, this PR is not about to fix something and must not be merged anyway but to point to some lines of code and its implications to gave others a chance to have a look in this and guess by themselfs. So my question is now, will have we have a fix and sure its not done if we just skip the boost traversing?
in #19502 and #18095 we see some progress, but will that be enough and why have I to write this even, if it is to u so obvious?
@Saibato GitHub won’t let anyone reopen this since your branch changed. Can you git push ??? 3f9cc0cd736e79c563dee3573f0dab01dac10622:wallet_351 -f temporarily (you can re-push your new version after it’s reopened; put your repo name in place of ??? of course).
Don’t forget to rebase!
Also: You need to include <set> instead of <algorithm> ;)