test: importdescriptors is not available for non-descriptor wallets #31609

1. 
   brunoerg commented at 10:40 am on January 6, 2025: contributor

   This PR adds test coverage for the error thrown when trying to import descriptors into a non-descriptor wallet.
2. 
   DrahtBot commented at 10:40 am on January 6, 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/31609.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	danielabrozzoni

   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.

3. DrahtBot added the label Tests on Jan 6, 2025
4. danielabrozzoni approved
5. 
   danielabrozzoni commented at 3:19 pm on January 7, 2025: contributor

   Nice!

   ACK 6b6b559d0f80246024a5b2fc8bcf357b86171472

6. 
   maflcko commented at 3:35 pm on January 7, 2025: member

   No objection, but this will just create a conflict with the bdb removal, at which point the test will be deleted as well?
7. test: importdescriptors is not available for non-descriptor wallets f9dc45680f
8. brunoerg force-pushed on Jan 7, 2025
9. 
   brunoerg commented at 4:36 pm on January 7, 2025: contributor

   No objection, but this will just create a conflict with the bdb removal, at which point the test will be deleted as well?

   The check will still exist in importdescriptors. But you’re right, the legacy wallet iteration in this functional test will be removed, so it’s better to move this check to wallet_migration test.

10. brunoerg requested review from danielabrozzoni on Jan 8, 2025
11. danielabrozzoni approved
12. 
    danielabrozzoni commented at 11:09 am on January 10, 2025: contributor

    My understanding of the legacy removal PRs is limited, but it appears that the importdescriptor function will still include the check for legacy wallets.

    utACK f9dc45680fa958bc89335fa50f46052e6bb29b37

13. 
    maflcko commented at 12:14 pm on January 10, 2025: member

    Huh? Now you are adding a test for a previous release, which does not increase the test coverage, see also https://corecheck.dev/bitcoin/bitcoin/pulls/31609 .

    If you want to test a previous release, it would be better to submit the test against the previous release branch. But I am not sure if this is worth it.

    Also, I don’t understand why the check should be kept after the bdb removal. Why would one want to guard against bdb at runtime when it is impossible to load bdb?

14. 
    brunoerg commented at 12:36 pm on January 10, 2025: contributor

    Huh? Now you are adding a test for a previous release, which does not increase the test coverage, see also https://corecheck.dev/bitcoin/bitcoin/pulls/31609 .

    Nevermind, my bad, I did it quickly and did not notice it. I’m gonna close it because I believe this check could be removed within bdb removal.

15. brunoerg closed this on Jan 10, 2025

    ---

Contributors
brunoerg DrahtBot danielabrozzoni maflcko

Labels
Tests