Migrating a legacy wallet that holds no spendable material fails whenever the
wallet contains transactions. migratewallet returns
std::bad_function_call (-1) and the wallet is left unmigrated.
Two defects are involved, and the first one hides the second.
1. The migration check reads in-memory state that has not been updated yet
When the wallet being migrated has no spendable material,
CWallet::ApplyMigrationData() asserts that nothing is left behind in it:
if (!m_address_book.empty()) return util::Error{_("Error: Not all address book records were migrated")};
if (!mapWallet.empty()) return util::Error{_("Error: Not all transaction records were migrated")};
The two checks look symmetric but are not. DelAddressBookWithDB() erases from
m_address_book straight away, so the first check sees current state.
RemoveTxs() only erases the transaction records from disk; it defers the
mapWallet removal to the db txn's on_commit listener, which does not run
until the txn is committed — after ApplyMigrationData() has returned.
mapWallet is therefore never empty at that point and the check always fails.
2. Aborting the txn calls a callback that was never set
DbTxnListener declares on_commit and on_abort as plain std::function
members and callers may set only the one they need. RemoveTxs() does exactly
that: on_commit is set, on_abort is left empty.
TxnCommit() and TxnAbort() each invoked their listener's callback
unconditionally, so the abort triggered by the failed check above threw
std::bad_function_call. The exception escaped the wallet entirely, replacing
the real error with an unhelpful one and skipping the caller's cleanup path.
Fix
The first commit skips callbacks that are not set, and documents that they are
optional. The second discounts the transactions already queued for deletion
instead of requiring mapWallet to be empty.
They are kept separate because the second defect masks the first: with only the
listener fix applied, the migration surfaces the real error
Error: Not all transaction records were migrated (-4) instead of
std::bad_function_call (-1).
Testing
src/wallet/test/db_tests.cpp gains a regression test covering a listener with
only one callback set, for both commit and abort. It fails on master with
std::bad_function_call and passes with the fix.
test/functional/wallet_migration.py gains a case migrating a blank legacy
wallet with a watch-only address that has received a transaction. The test is
taken from #36483.
To reproduce on master, apply the functional test alone and run
wallet_migration.py.
Fixes #36483
