Skip to content

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Jan 6, 2026

@fanquake fanquake added this to the 30.2 milestone Jan 6, 2026
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 6, 2026

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34209.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK willcl-ark, janb84, w0xlt, marcofleon, darosior

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

furszy added 7 commits January 7, 2026 11:18
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
Copy link
Member

@willcl-ark willcl-ark left a comment

Choose a reason for hiding this comment

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

tACK 483d158

Backports are clean (no conflicts) and mentioned in release notes correctly.

Version and manpage commits appear correct, but I didn't check them against the full release process.

Copy link
Contributor

@janb84 janb84 left a comment

Choose a reason for hiding this comment

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

ACK 483d158

PR contains commits form #34156 and #34215 (checked)
Updates the version to v30.2.0rc1

Details
$ build_dev_mode/bin/bitcoin --version
Bitcoin Core version v30.2.0rc1

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

ACK 483d158

Copy link
Contributor

@marcofleon marcofleon left a comment

Choose a reason for hiding this comment

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

lgtm ACK 483d158

@darosior
Copy link
Member

darosior commented Jan 7, 2026

ACK 483d158

@fanquake fanquake merged commit abb6ae2 into bitcoin:30.x Jan 7, 2026
20 checks passed
@fanquake fanquake deleted the 30_2_rc1 branch January 7, 2026 15:45
@fanquake fanquake mentioned this pull request Jan 9, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants