Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: let Listwalletdir do not iterate through our blocksdata. #19419

Closed
wants to merge 2 commits into from

Conversation

Saibato
Copy link
Contributor

@Saibato Saibato commented Jun 30, 2020

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.

@Saibato Saibato force-pushed the wallet_351 branch 2 times, most recently from 530c3a2 to 24d2044 Compare Jun 30, 2020
@Saibato Saibato changed the title wallet: Listwallets do not iterate trough our blocksdata. wallet: Listwalletdir do not iterate trough our blocksdata. Jun 30, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 30, 2020

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

Conflicts

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.

@promag
Copy link
Member

promag commented Jul 1, 2020

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.

@Saibato
Copy link
Contributor Author

Saibato commented Jul 1, 2020

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.

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.

@Saibato Saibato changed the title wallet: Listwalletdir do not iterate trough our blocksdata. [WIP} wallet: Listwalletdir do not iterate trough our blocksdata. Jul 1, 2020
@Saibato Saibato changed the title [WIP} wallet: Listwalletdir do not iterate trough our blocksdata. [WIP] wallet: Listwalletdir do not iterate trough our blocksdata. Jul 1, 2020
@Saibato Saibato force-pushed the wallet_351 branch 3 times, most recently from 55e5460 to 6e5b87a Compare Jul 1, 2020
@Saibato Saibato force-pushed the wallet_351 branch 3 times, most recently from 0a58f3f to 3d69e4c Compare Jul 1, 2020
fs::path sub_path = GetDataDir();

bool we_have_wallets_dir = (wallet_dir != sub_path);
const fs::path fblocks = GetBlocksDir();
Copy link
Member

@promag promag Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of multiple variables, you can declare something like const std::vector ignore_paths{ GetBlocksDir(), sub_path / "testnet3", ... } and then compare against this.

Copy link
Contributor Author

@Saibato Saibato Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup 👍 , btw strange how long those bare strings survived refactor, still no global #define BLOCKS_DIR or TESTNET_STR etc.

@Saibato Saibato force-pushed the wallet_351 branch 2 times, most recently from c49d922 to 4aa7bc5 Compare Jul 2, 2020
@Saibato Saibato changed the title [WIP] wallet: Listwalletdir do not iterate trough our blocksdata. wallet: let Listwalletdir do not iterate trough our blocksdata. Jul 2, 2020
Copy link
Member

@Sjors Sjors left a comment

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.

const fs::path path = it->path().string().substr(offset);
// We dont want to iterate trough those special node dirs
if (it->status().type() == fs::directory_file) {
skip_path = false;
Copy link
Member

@Sjors Sjors Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/wallet/walletutil.cpp Outdated Show resolved Hide resolved
it.disable_recursion_pending();
continue;
}
} else it.disable_recursion_pending();
Copy link
Member

@Sjors Sjors Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: just use brackets and newline

Also, I think we should follow symlinks? symlink_file

Copy link
Member

@luke-jr luke-jr Jul 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following symlinks could create an infinite loop.

src/wallet/walletutil.cpp Outdated Show resolved Hide resolved
src/wallet/walletutil.cpp Outdated Show resolved Hide resolved
src/wallet/walletutil.cpp Outdated Show resolved Hide resolved
src/wallet/walletutil.cpp Outdated Show resolved Hide resolved
skip_path = false;
for(const auto& fpath: ignore_paths) {
if (!skip_path && it->path().string().find(fpath.string()) == 0) {
skip_path = true;
}
}
if (skip_path) {
it.disable_recursion_pending();
continue;
}
Copy link
Member

@luke-jr luke-jr Jul 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
skip_path = false;
for(const auto& fpath: ignore_paths) {
if (!skip_path && it->path().string().find(fpath.string()) == 0) {
skip_path = true;
}
}
if (skip_path) {
it.disable_recursion_pending();
continue;
}
if (ignore_paths.count(it->path()) {
it.disable_recursion_pending();
continue;
}

Copy link
Contributor Author

@Saibato Saibato Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Though I choose
if (std::count(ignore_paths.begin(), ignore_paths.end(), it->path())) {
not sure ignore_paths has a count?

Copy link
Member

@luke-jr luke-jr Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will once changed to an unordered_set as suggested

@meshcollider
Copy link
Contributor

meshcollider commented Aug 13, 2020

Concept ACK, looks good to me

@achow101
Copy link
Member

achow101 commented Aug 14, 2020

Concept ACK, please squash your commits.

src/wallet/walletutil.cpp Outdated Show resolved Hide resolved
src/wallet/walletutil.cpp Outdated Show resolved Hide resolved
}
} catch (const std::exception& e) {
Copy link
Member

@hebasto hebasto Oct 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally prefer "throwing" versions of the Boost.Filesystem API. But I don't think that combining "throwing" and "non-throwing" versions of the Boost.Filesystem API in the same function is a good design.

Copy link
Contributor Author

@Saibato Saibato Oct 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with catch boost::exception
and dynamic_cast<std.exception i run with a recursive path in an non catched
EXCEPTION: N5boost10filesystem16filesystem_errorE
so unless u have a hint, i leave it that way, for now?

When WalletDir == DataDir we would have iterate trough our own node files
to find wallets, that consumes time and could cause an unresponsive node.
@Saibato
Copy link
Contributor Author

Saibato commented Oct 8, 2020

Rebased da6eb3c and addressed reviews.

@Saibato Saibato changed the title wallet: let Listwalletdir do not iterate trough our blocksdata. wallet: let Listwalletdir do not iterate through our blocksdata. Oct 8, 2020
Copy link
Member

@hebasto hebasto left a comment

Approach ACK da6eb3c.

Mind adding #include <algorithm> in separate header group?

src/wallet/walletutil.cpp Show resolved Hide resolved
Designed to be used on ranges of elements.
@Saibato
Copy link
Contributor Author

Saibato commented Oct 12, 2020

Add #include <algorithm> to header.

Tyi @hebasto while dev this i encountered and ugly wallet and filesystem edge case. that is only partly fixed by #18095 but with fewer changes and final here and in #19502 u might reach out to @luke-jr or @laanwj for more info.

// software will never create these files but will allow them to be
// opened in a shared database environment for backwards compatibility.
// Add it to the list of available wallets.
paths.emplace_back(it->path().filename());
Copy link
Member

@luke-jr luke-jr Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this?

Copy link
Contributor Author

@Saibato Saibato Oct 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its not changed just the correct indent.

Copy link
Contributor Author

@Saibato Saibato Oct 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If u mean the whole logic itself, its because there where an other wallet access issue not mentioned here fixed also, could be no longer valid since we had some grave changes in master, i have to check, but since the PR its closed yet, it makes sense to let all as is and open two new and or wait until its reopened and then help out with the review.

@@ -5,6 +5,7 @@
#ifndef BITCOIN_WALLET_WALLETUTIL_H
#define BITCOIN_WALLET_WALLETUTIL_H

#include <algorithm>
Copy link
Member

@luke-jr luke-jr Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add this here? If it's needed in the .cpp file, put it there...

Copy link
Contributor Author

@Saibato Saibato Oct 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

;was keen to see some compiler warnings, i though that was the hint from @hebasto to add <algorithm> addressed and changed back to .cpp that other bug we can fix later.

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Oct 13, 2020
When WalletDir == DataDir we would have iterate trough our own node files
to find wallets, that consumes time and could cause an unresponsive node.

Diff-minimised

Github-Pull: bitcoin#19419
Rebased-From: da6eb3c
@luke-jr
Copy link
Member

luke-jr commented Oct 13, 2020

Please make these changes:

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 15, 2020

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

@laanwj
Copy link
Member

laanwj commented Oct 16, 2020

Marking this as up for grabs. The concept to not iterate needlessly into directories that should not contain wallets definitely makes sense, but I think this makes unnecessary changes that are not well motivated.

@laanwj laanwj closed this Oct 16, 2020
@Saibato
Copy link
Contributor Author

Saibato commented Oct 17, 2020

Rebased and addressed 5a8f19e 4fb4fb7 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

@Saibato
Copy link
Contributor Author

Saibato commented Oct 25, 2020

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?

@luke-jr
Copy link
Member

luke-jr commented Nov 13, 2020

@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> ;)

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Nov 13, 2020
When WalletDir == DataDir we would have iterate trough our own node files
to find wallets, that consumes time and could cause an unresponsive node.

Github-Pull: bitcoin#19419
Rebased-From: c730c7a
@luke-jr
Copy link
Member

luke-jr commented Nov 13, 2020

Here's a rebase for later: master...luke-jr:listwalletdir_skip_data

@Saibato
Copy link
Contributor Author

Saibato commented Nov 27, 2020

@luke-jr tyi was the last week just counting my sats and had not looked in this, thx. for the reopen hint.
btw look now why some wallets are missing when using the 0.21 rc1 tag, maybe that was the reason i changed also the scan for wallets logic here , but just forgot to mention that sggh...

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021
When WalletDir == DataDir we would have iterate trough our own node files
to find wallets, that consumes time and could cause an unresponsive node.

Github-Pull: bitcoin#19419
Rebased-From: c730c7a
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants