test: remove explicit descriptors=True for createwallet RPC calls #24551

1. 
   prusnak commented at 6:53 pm on March 13, 2022: contributor

   because descriptors wallets are created by default since #23002

   Fixes https://github.com/bitcoin/bitcoin/issues/24550

2. DrahtBot added the label Tests on Mar 13, 2022
3. 
   michaelfolkson commented at 8:14 pm on March 13, 2022: contributor

   Concept ACK

   Some failures on the CI. Will run the tests locally and attempt to debug tomorrow.

4. test: remove explicit descriptors=True for createwallet RPC calls

   because descriptors wallets are created by default since #23002

   e334426816
5. prusnak force-pushed on Mar 14, 2022
6. 
   achow101 commented at 10:35 am on March 14, 2022: member

   NACK

   Most of these are tests that require that the wallet must be a descriptor wallet. Removing the explicit parameter makes the type depend on the presence of --descriptors. If --legacy-wallet were provided, the type of wallet created would incorrectly be legacy. Hence the explicit option must remain.

7. 
   michaelfolkson commented at 12:45 pm on March 14, 2022: contributor

   Thanks for this @achow101. This PR and accompanying issue should be closed then it sounds like. The below is just for my own understanding.

   If –legacy-wallet were provided, the type of wallet created would incorrectly be legacy

   And when you say “incorrectly be legacy” you mean be legacy-bdb rather than legacy-sqlite? As you say in #20160 you need the explicit parameter to switch between descriptor-sqlite (descriptors=True) and legacy-bdb (descriptors=False).

   I’m not sure why there aren’t just two explicit parameters: descriptors(True/False for whether it uses descriptors or not) and sqlite (True/False for the database choice) rather than descriptors=False implicitly flipping the database choice from sqlite to bdb.

   edit: Ok I do understand. The user wasn’t expected to specify a database choice (sqlite, bdb) in the RPC and hence if they previously used the descriptor flag (before it was made default) it would choose sqlite implicitly for them.

8. 
   theStack commented at 1:50 pm on March 14, 2022: member

   NACK

   Most of these are tests that require that the wallet must be a descriptor wallet. Removing the explicit parameter makes the type depend on the presence of --descriptors. If --legacy-wallet were provided, the type of wallet created would incorrectly be legacy. Hence the explicit option must remain.

   Right, I completely forgot about that! In the test framework calling node.createwallet doesn’t immediately invoke the RPC, but passes it through an overload wrapper first: https://github.com/bitcoin/bitcoin/blob/25d045a9ecc24a2752685d176c28f8f75bb100f6/test/functional/test_framework/test_node.py#L749-L752

   So agreed, this PR can be closed. Sorry for creating confusion!

9. prusnak closed this on Mar 14, 2022

   ---

10. prusnak deleted the branch on Mar 14, 2022
11. 
    prusnak commented at 1:55 pm on March 14, 2022: contributor

    So agreed, this PR can be closed. Sorry for creating confusion!

    Closing 👍

12. DrahtBot locked this on Mar 14, 2023

Contributors
prusnak michaelfolkson achow101 theStack

Labels
Tests