Skip to content

Wallet fixes for Knots 29.3#249

Open
luke-jr wants to merge 58 commits intobitcoinknots:29.x-knotsfrom
luke-jr:knots29.3_walletfixes
Open

Wallet fixes for Knots 29.3#249
luke-jr wants to merge 58 commits intobitcoinknots:29.x-knotsfrom
luke-jr:knots29.3_walletfixes

Conversation

@luke-jr
Copy link
Copy Markdown
Collaborator

@luke-jr luke-jr commented Jan 22, 2026

This is a cherry-picking of multiple fix branches (including Core 29.3 backports) for review purposes

furszy and others added 30 commits January 22, 2026 14:42
Track what RestoreWallet creates so only those files and directories
are removed during a failure and nothing else. Preexisting paths
must be left untouched.

Note:
Using fs::remove_all() instead of fs::remove() in RestoreWallet does
not cause any problems currently, but the change is necessary for the
next commit which extends RestoreWallet to work with existing directories,
which may contain files that must not be deleted.

Github-Pull: bitcoin#34156
Rebased-From: 4ed0693
When migrating any legacy unnamed wallet, a failed migration would
cause the cleanup logic to remove its parent directory. Since this
type of legacy wallet lives directly in the main '/wallets/' folder,
this resulted in unintentionally erasing all wallets, including the
backup file.

To be fully safe, we will no longer call `fs::remove_all`. Instead,
we only erase the individual db files we have created, leaving
everything else intact. The created wallets parent directories are
erased only if they are empty.
As part of this last change, `RestoreWallet` was modified to allow
an existing directory as the destination, since we no longer remove
the original wallet directory (we only remove the files we created
inside it). This also fixes the restore of top-level default wallets
during failures, which were failing due to the directory existence
check that always returns true for the /wallets/ directory.

This bug started after:
bitcoin@f6ee59b
Previously, the `fs::copy_file` call was failing for top-level wallets,
which prevented the `fs::remove_all` call from being reached.

Github-Pull: bitcoin#34156
Rebased-From: f4c7e28
Verifies that a failed migration of the unnamed (default) wallet
does not erase the main /wallets/ directory, and also that the
backup file exists.

Github-Pull: bitcoin#34156
Rebased-From: 36093bd
…rune failure

The first test verifies that restoring into an existing empty directory
or a directory with no .dat db files succeeds, while restoring into a
dir with a .dat file fails.

The second test covers restoring into the default unnamed wallet
(wallet.dat), which also implicitly exercises the recovery path used
after a failed migration.

The third test covers failure during restore on a prune node. When
the wallet last sync was beyond the pruning height.

Github-Pull: bitcoin#34156
Rebased-From: f011e0f
Right now, after migration the last message users see is "migration completed",
but the migration isn't actually finished yet. We still need to load the new wallets
to ensure consistency, and if that fails, the migration will be rolled back. This
can be confusing for users.

This change logs the post-migration loading step and if a wallet fails to load and
the migration will be rolled back.

Github-Pull: bitcoin#34156
Rebased-From: d70b159
Because the default wallet has no name, the watch-only and solvables
wallets created during migration end up having no name either.

This fixes it by applying the same prefix name we use for the backup
file for an unnamed default wallet.

Before: watch-only wallet named "_watchonly"
After:  watch-only wallet named "default_wallet_watchonly"

Github-Pull: bitcoin#34156
Rebased-From: 82caa81
Refactor a common way to perform the failed migration test that exists
for default wallets, and add relative-path wallets and absolute-path
wallets.

Github-Pull: 34226
Rebased-From: eeaf28d
This check ensures that when migrating a legacy wallet with a direct
filename, the backup file is named as expected.

Co-authored-by: Ava Chow <github@achow101.com>

Github-Pull: bitcoin#32273
Rebased-From: e22c359
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

Github-Pull: bitcoin#32273
Rebased-From: 70f1c99
…allet

If any other files exist in the directory, we cannot assume the sharable files are exclusively for this wallet.
But if they are, this also cleans up other log.* files
…ils, log the correct wallet name in error message
…eason

Since we no longer delete the wallet directory, there's no need to vacate it
The moving only served to risk errors by crossing filesystem boundaries (which fs::rename can't handle)
…ating from non-directory

While 30.x+ keep backup files in walletdir, 29.x places them in the migrated wallet directory
luke-jr and others added 9 commits January 22, 2026 14:49
Wallet creation/opening errors turn into RPC_MISC_ERROR without this
…legacy wallet

Currently, the migration process creates a brand-new descriptor wallet with no
connection to the user's legacy wallet when the legacy wallet lacks key material
and contains only watch-only scripts. This behavior is not aligned with user
expectations. If the legacy wallet contains only watch-only scripts, the migration
process should only generate a watch-only wallet instead.

Github-Pull: bitcoin#31423
Rebased-From: b789907
After a wallet is migrated and we are trying to load it, if it could not be
loaded, don't try to set the wallet name.

Github-Pull: bitcoin#32984
Rebased-From: 8a4cfdd
- Comment updates
- Use fs::remove when deleting empty migration target
- Adapt tests to only having watchonly migration output where applicable
- break out of wallet reload loop when any fail

Originally-From: f4c7e28 and d70b159 and 82caa81 (bitcoin#34156)
@luke-jr luke-jr added the bug label Jan 22, 2026
@luke-jr luke-jr added this to the 29.3 milestone Jan 22, 2026
…atabase" subdirectory

Otherwise, there's a race condition if another program/instance opens a wallet before we delete it
…tdown

It's hypothetically possible, albeit difficult to arrange, that other non-loaded wallets are still relying on it
…vironment::Close()

This reverts commit 8a06c8cf55f67934a4d86732217e38e42ac5198e.
@luke-jr luke-jr force-pushed the knots29.3_walletfixes branch from b8561d2 to cf43a63 Compare January 23, 2026 20:49
Comment thread src/wallet/sqlite.h
std::vector<fs::path> Files() override
{
std::vector<fs::path> files;
files.emplace_back(m_dir_path / fs::PathFromString(m_file_path));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

m_file_path is already the full absolute path (used directly for sqlite3_open_v2). The / operator with an absolute RHS ignores the LHS. Consider using fs::PathFromString(m_file_path) directly.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Where is m_file_path guaranteed to be absolute?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

m_file_path is always absolute in practice, since it comes from SQLiteDataFile(path) which returns a full path. So the m_dir_path / is silently ignored by operator/. The code works but is misleading.

Using fs::PathFromString(m_file_path) directly would be correct either way.

Comment thread src/wallet/bdb.cpp
};
try {
if (build_files_list(files, env, m_filename)) return files;
} catch (...) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Catching all exceptions silently could mask filesystem errors and lead to incomplete cleanup. Consider logging when falling back to the smaller file list.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Might be a good idea, but I don't see a good way to do it. We build the file list even if we don't delete them later.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The fallback (line 391) just returns the single wallet file, so the failure mode is conservative (fewer files deleted, not more). Logging would be noisy since Files() is called even when no deletion follows.

Comment thread src/wallet/wallet.cpp
Comment thread src/wallet/wallet.cpp
Comment thread src/wallet/wallet.cpp
@luke-jr luke-jr added the merged label Feb 10, 2026
@luke-jr
Copy link
Copy Markdown
Collaborator Author

luke-jr commented Feb 10, 2026

Merged in v29.3.knots20260210

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements wallet fixes for Knots 29.3, cherry-picking multiple fix branches including Core 29.3 backports. The changes focus on wallet migration improvements, directory permission checks, safer cleanup during failed operations, and more robust error handling.

Changes:

  • Adds write-permission checks for wallet directories before opening databases, and improves RestoreWallet to handle pre-existing directories without overwriting database files.
  • Refactors wallet migration to properly handle watch-only wallets (empty spendable wallets are discarded), relative/absolute paths, and uses safer file-level cleanup instead of fs::remove_all during failure recovery.
  • Adds a Files() virtual method to WalletDatabase for precise tracking of database files, changes BDB CheckpointLSN to return success/failure, and upgrades incompatible BDB warning to FATAL_ERROR.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/wallet/wallet.cpp Major migration logic refactor: backup naming, empty wallet handling, safer cleanup, path resolution
src/wallet/bdb.cpp Add Files() impl, error handling for CheckpointLSN, writable dir check, catch exceptions in MakeBerkeleyDatabase
src/wallet/bdb.h CheckpointLSN returns bool, add Files() declaration
src/wallet/sqlite.cpp Writable directory check, type change for m_dir_path
src/wallet/sqlite.h m_dir_path changed to fs::path, add Files() impl
src/wallet/db.h Add pure virtual Files() method
src/wallet/dump.cpp Use Files() for targeted cleanup instead of remove_all
src/wallet/salvage.cpp Add Files() to DummyDatabase
src/wallet/migrate.h Add Files() to BerkeleyRODatabase
src/wallet/test/util.h Add Files() to MockableDatabase
src/util/fs_helpers.h / .cpp Add IsDirWritable() utility function
src/qt/walletcontroller.cpp Use original wallet name in migration success message
CMakeLists.txt Upgrade incompatible BDB warning to FATAL_ERROR
test/functional/wallet_migration.py Extensive new tests for migration edge cases
test/functional/wallet_backup.py Tests for restore into existing dirs and unnamed wallets
test/functional/wallet_startup.py Test loading unwritable wallet
test/functional/wallet_createwallet.py Test creating wallet in unwritable directory
test/functional/wallet_multiwallet.py Simplify error code expectation
test/functional/tool_wallet.py Tests for createfromdump with unnamed wallet
test/functional/test_framework/util.py Add is_dir_writable() helper

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/wallet/sqlite.h
Comment on lines +173 to +174
files.emplace_back(m_dir_path / fs::PathFromString(m_file_path));
files.emplace_back(m_dir_path / fs::PathFromString(m_file_path + "-journal"));
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

m_file_path is a full absolute file path string (set via fs::PathToString(file_path) where file_path is an absolute path from the constructor). Joining m_dir_path / fs::PathFromString(m_file_path) will produce an incorrect path because you're appending an absolute path to another path. This should use just the filename component, e.g., fs::PathFromString(m_file_path).filename(), or simply use fs::PathFromString(m_file_path) directly without prepending m_dir_path.

Suggested change
files.emplace_back(m_dir_path / fs::PathFromString(m_file_path));
files.emplace_back(m_dir_path / fs::PathFromString(m_file_path + "-journal"));
files.emplace_back(fs::PathFromString(m_file_path));
files.emplace_back(fs::PathFromString(m_file_path + "-journal"));

Copilot uses AI. Check for mistakes.
Comment thread src/wallet/wallet.cpp
fs::path this_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path();
fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : wallet_name), GetTime()));
const std::string backup_prefix = wallet_name.empty() ? MigrationPrefixName(*local_wallet) : [&] {
// fs::weakly_canonical resolves relative specifiers and remove trailing slashes.
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The comment on line 4668 says "remove trailing slashes" (singular verb form). It should read "removes trailing slashes" to be grammatically correct.

Suggested change
// fs::weakly_canonical resolves relative specifiers and remove trailing slashes.
// fs::weakly_canonical resolves relative specifiers and removes trailing slashes.

Copilot uses AI. Check for mistakes.
shutil.copytree(self.old_node.wallets_path / wallet_name, self.master_node.wallets_path / wallet_name)
src = os.path.abspath(self.old_node.wallets_path / wallet_name)
dst = os.path.abspath(self.master_node.wallets_path / wallet_name)
if src != dst :
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

There is a trailing space before the colon. Remove the extra space to follow standard Python style conventions.

Suggested change
if src != dst :
if src != dst:

Copilot uses AI. Check for mistakes.
assert not (self.master_node.wallets_path / f"{wallet_name}_watchonly").exists()
# Since the file in failed_solvables is one that we put there, migration shouldn't touch it
assert os.path.exists(solvables_path)
new_shasum = sha256sum_file(os.path.join(solvables_path , "wallet.dat"))
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

There is an extra space before the comma in os.path.join(solvables_path , "wallet.dat"). Remove it for consistent formatting.

Suggested change
new_shasum = sha256sum_file(os.path.join(solvables_path , "wallet.dat"))
new_shasum = sha256sum_file(os.path.join(solvables_path, "wallet.dat"))

Copilot uses AI. Check for mistakes.
self.start_node(self.old_node.index)
self.connect_nodes(1, 0)

def unsynced_wallet_on_pruned_node_fails(self):
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

This method name does not follow the test_ prefix convention used by all other test methods in this class (e.g., test_loading_failure_after_migration, test_failed_migration_cleanup). It should be renamed to test_unsynced_wallet_on_pruned_node_fails for consistency. The call site at line 1785 would also need to be updated.

Copilot uses AI. Check for mistakes.
Comment thread src/wallet/bdb.cpp
err = strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory())));
if (ret == DB_RUNRECOVERY) {
err += Untranslated(" ") + _("This error could occur if this wallet was not shutdown cleanly and was last loaded using a build with a newer version of Berkeley DB. If so, please use the software that last loaded this wallet");
err += Untranslated(" ") + _("This error could occur if this wallet was last loaded using a build with a newer version of Berkeley DB.");
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The original message provided the user with guidance about what to do ("please use the software that last loaded this wallet"). The updated message only states a potential cause without offering any actionable advice for the user.

Suggested change
err += Untranslated(" ") + _("This error could occur if this wallet was last loaded using a build with a newer version of Berkeley DB.");
err += Untranslated(" ") + _("This error could occur if this wallet was last loaded using a build with a newer version of Berkeley DB. Please use the software that last loaded this wallet.");

Copilot uses AI. Check for mistakes.
Comment thread CMakeLists.txt
if(WARN_INCOMPATIBLE_BDB)
message(WARNING "If this is intended, pass \"-DWARN_INCOMPATIBLE_BDB=OFF\".\n"
message(FATAL_ERROR "If this is intended, pass \"-DWARN_INCOMPATIBLE_BDB=OFF\".\n"
"Passing \"-DWITH_BDB=OFF\" will suppress this warning."
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The message text still says "will suppress this warning" but the severity has been changed from WARNING to FATAL_ERROR. The text should be updated to say something like "will suppress this error" to match the new behavior.

Suggested change
"Passing \"-DWITH_BDB=OFF\" will suppress this warning."
"Passing \"-DWITH_BDB=OFF\" will suppress this error."

Copilot uses AI. Check for mistakes.
Comment on lines +746 to +752
open(survive_path, "wb").close()
assert survive_path.exists()
if migrating_file_to_dir:
survive2_path = master_path.parent / "survive"
else:
survive2_path = master_path / "survive"
open(survive2_path, "wb").close()
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

Using open(...).close() without a with statement is a common anti-pattern. Consider using survive_path.touch() (from pathlib.Path) instead, which is both cleaner and already used elsewhere in the test suite (e.g., extra_file.touch() in wallet_backup.py). This also applies to line 752.

Suggested change
open(survive_path, "wb").close()
assert survive_path.exists()
if migrating_file_to_dir:
survive2_path = master_path.parent / "survive"
else:
survive2_path = master_path / "survive"
open(survive2_path, "wb").close()
survive_path.touch()
assert survive_path.exists()
if migrating_file_to_dir:
survive2_path = master_path.parent / "survive"
else:
survive2_path = master_path / "survive"
survive2_path.touch()

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants