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/32174.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/39708397356}

@pablomartin4btc Based on the commit, I see that an unwanted message variable was added to test/functional/test_framework/util.py, along with an assertion to raise an error if that message is encountered. In the test_failed_migration_cleanup test case, this unwanted message is now passed to the assertion method.

I was trying to verify the fix by triggering the error on the master branch (before this change), but I’m not sure how to reproduce it. Could you give me a quick heads-up on how to get the error to appear, so I can confirm that the changes properly prevent it?

Sorry if this is a basic question— I want to make sure I’m testing it correctly. Appreciate the help!

Thanks @enirox001 for reviewing this PR!

Sorry if this is a basic question— I want to make sure I’m testing it correctly. Appreciate the help!

No problem. This was a bug that was already fixed in master, but we could verify that this change works by detecting "hidden" error messages withing the failure response. Not sure if this is the best way to test it but you could just change the line after the RestoreWallet call in order to concatenate the "unwanted" error message.

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4610,7 +4610,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
         // Reload it into memory if the wallet was previously loaded.
         bilingual_str restore_error;
         const auto& ptr_wallet = RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/was_loaded);
-        if (!restore_error.empty()) {
+        if (restore_error.empty()) {
             error += restore_error + _("\nUnable to restore backup of wallet.");
             return util::Error{error};
         }

ACK. I tested this by modifying the line immediately following RestoreWallet, which triggered the hidden failure message and revealed the unwanted error. This fix successfully ensures that the unwanted message is no longer included in the RPC error outputs.

This PR does not seem to have attracted much attention from reviewers. As such, it does not seem important enough right now to keep it sitting idle in the list of open PRs.

Closing due to lack of interest.