tests: remove usage of LegacyScriptPubKeyMan from some wallet tests #23288

1. 
   achow101 commented at 7:29 pm on October 15, 2021: member

   Currently, various tests use LegacyScriptPubKeyMan because it was convenient for the refactor that introduced the ScriptPubKeyMan interface. However, with the legacy wallet slated to be removed, these tests should not continue to use LegacyScriptPubKeyMan as they are not testing any specific legacy wallet behavior. These tests are changed to use DescriptorScriptPubKeyMans.

   Some of the coin selection tests and benchmarks had a global testWallet, but this seemed to cause some issues with ensuring that descriptors were set up in that wallet for each test. Those have been restructured to not have any global variables that may be modified between tests.

   The tests which test specific legacy wallet behavior remain unchanged.

2. tests: Remove global vCoins and testWallet from coinselector_tests

   To avoid issues with test data leaking across tests cases, the global
   vCoins and testWallet are removed from coinselector_tests and all of the
   relevant functions reworked to not need them.

   a5595b1320
3. bench: remove global testWallet from CoinSelection benchmark 5e54aa9b90
4. bench: Use DescriptorScriptPubKeyMan for wallet things

   For wallet related benchmarks that need a ScriptPubKeyMan for operation,
   use a DescriptorScriptPubKeyMan

   9bf0243872
5. tests, gui: Use DescriptorScriptPubKeyMan in GUI tests 811319fea4
6. tests: Use DescriptorScriptPubKeyMan in coinselector_tests 4b1588c6bd
7. DrahtBot added the label GUI on Oct 15, 2021
8. DrahtBot added the label Wallet on Oct 15, 2021
9. 
   DrahtBot commented at 8:56 pm on October 15, 2021: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #22981 (doc: Fix incorrect C++ named args by MarcoFalke)
   • #22928 (refactor: Remove gArgs from wallet.h and wallet.cpp (2) by kiminuo)
   • #22805 (refactor: use CWallet const shared pointers in dump{privkey,wallet} by theStack)
   • #22787 (refactor: actual immutable pointing by kallewoof)
   • #22260 (Make bech32m the default for RPC, opt-in for GUI by Sjors)
   • #22019 (wallet: Introduce SelectionResult for encapsulating a coin selection solution by achow101)
   • #21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)

   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.

10. tests: Use descriptors in psbt_wallet_tests dcd6eeb64a
11. tests: Use legacy change type in subtract fee from outputs test

    The subtract fee from outputs assumes that the leftover input amount
    will be dropped to fees. However this only happens if that amount is
    less than the cost of change. In the event that it is higher than the
    cost of change, the leftover amount will actually become a change
    output. To avoid this scenario, force a change type which has a high
    cost of change.

    99516285b7
12. tests: Use Descriptor wallets for generic wallet tests

    For the generic wallet tests, make DescriptorScriptPubKeyMans. There are
    still some wallet tests that test legacy wallet things. Those remain
    unchanged.

    2d2edc1248
13. achow101 force-pushed on Oct 15, 2021
14. brunoerg approved
15. 
    brunoerg commented at 3:50 pm on October 19, 2021: member

    tACK 2d2edc1248a2e49636409b07448676e5bfe44956

    Ran benchs and tests successfully

16. 
    laanwj commented at 1:09 pm on October 22, 2021: member

    Code review ACK 2d2edc1248a2e49636409b07448676e5bfe44956
17. laanwj merged this on Oct 22, 2021
18. laanwj closed this on Oct 22, 2021

    ---

19. sidhujag referenced this in commit 6b394bdef2 on Oct 22, 2021
20. MarcoFalke referenced this in commit b9cf505bdf on Oct 25, 2021
21. sidhujag referenced this in commit 7b4431664b on Oct 25, 2021
22. 
    hebasto commented at 12:03 pm on October 31, 2021: member

    The dcd6eeb64adb2b532f5003cbb86ba65b3c08a87b commit introduced an intermittent failure in the psbt_wallet_tests/psbt_updater_test unit test.

    See:

    • #23368
    • #23403
23. in src/wallet/test/psbt_wallet_tests.cpp:69 in 2d2edc1248

    64@@ -71,7 +65,8 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test)
    65 
    66     // Try to sign the mutated input
    67     SignatureData sigdata;
    68-    BOOST_CHECK(spk_man->FillPSBT(psbtx, PrecomputePSBTData(psbtx), SIGHASH_ALL, true, true) != TransactionError::OK);
    69+    BOOST_CHECK(m_wallet.FillPSBT(psbtx, complete, SIGHASH_ALL, true, true) != TransactionError::OK);
    70+    //BOOST_CHECK(spk_man->FillPSBT(psbtx, PrecomputePSBTData(psbtx), SIGHASH_ALL, true, true) != TransactionError::OK);
    

    ---

    ---

    

    MarcoFalke commented at 2:15 pm on October 31, 2021:

    What is this comment for?

    ---

    

    jonatack commented at 8:13 pm on March 16, 2022:

    Removal proposed in #24592.
24. MarcoFalke referenced this in commit 5adc5c0280 on Nov 1, 2021
25. sidhujag referenced this in commit 0e193a7268 on Nov 1, 2021
26. Fabcien referenced this in commit f74630fe32 on Jan 25, 2022
27. achow101 referenced this in commit b8992f2d4a on Mar 16, 2022
28. sidhujag referenced this in commit e525e80ab1 on Mar 16, 2022
29. DrahtBot locked this on Mar 16, 2023

Contributors
achow101 DrahtBot brunoerg laanwj hebasto MarcoFalke jonatack

Labels
GUI Wallet