test: Verify that a message is not in rpc errors raised (follow-up 31451) #32174

1. 
   pablomartin4btc commented at 3:23 pm on March 31, 2025: member

   This follow-up was proposed in #31451 (review).

   Verify that an unwanted message is not among the errors raised by the rpc call, otherwise raise an exception with the details.

2. 
   DrahtBot commented at 3:23 pm on March 31, 2025: contributor

   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.

   Type	Reviewers
   Concept ACK	EniRox001

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31250 (wallet: Disable creating and loading legacy wallets by achow101)
   • #28710 (Remove the legacy wallet and BDB dependency by achow101)

   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.

3. DrahtBot added the label Tests on Mar 31, 2025
4. test, rpc: Verify message is not in rpc errors raised

   Check that the message passed in **kwds, named unwanted_message,
   is not among the errors raised by the rpc call.

   eff1c0dde4
5. pablomartin4btc force-pushed on Mar 31, 2025
6. DrahtBot added the label CI failed on Mar 31, 2025
7. 
   DrahtBot commented at 3:29 pm on March 31, 2025: contributor

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

   Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

   • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

   • An intermittent issue.

   Leave a comment here, if you need help tracking down a confusing failure.

8. DrahtBot removed the label CI failed on Mar 31, 2025
9. 
   EniRox001 commented at 10:08 pm on April 5, 2025: none

   @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!

10. 
    pablomartin4btc commented at 4:33 pm on April 7, 2025: member

    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.

     0--- a/src/wallet/wallet.cpp
     1+++ b/src/wallet/wallet.cpp
     2@@ -4610,7 +4610,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
     3         // Reload it into memory if the wallet was previously loaded.
     4         bilingual_str restore_error;
     5         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);
     6-        if (!restore_error.empty()) {
     7+        if (restore_error.empty()) {
     8             error += restore_error + _("\nUnable to restore backup of wallet.");
     9             return util::Error{error};
    10         }
    

11. 
    EniRox001 commented at 8:44 am on April 13, 2025: none

    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.

Contributors
pablomartin4btc DrahtBot EniRox001

Labels
Tests