wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag #20191

1. 
   S3RK commented at 3:33 AM on October 20, 2020: contributor

   Rationale: improve consistency between CWallet and DescriptorScriptPubKeyMan; simplify ScriptPubKeyMan interface.

   Descriptor in itself is neither internal or external. It's responsibility of a wallet to assign and manage descriptors for a specific purpose. Duplicating information about internalness of a descriptor could lead to inconsistencies and unexpected behaviour (for example misreporting keypool size).

2. fanquake added the label Wallet on Oct 20, 2020
3. fanquake requested review from achow101 on Oct 20, 2020
4. 
   DrahtBot commented at 7:45 AM on October 20, 2020: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #22364 by achow101
   • #22337 by achow101
   • #19602 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.

5. DrahtBot cross-referenced this on Oct 20, 2020 from issue wallet: Migrate legacy wallets to descriptor wallets by achow101
6. DrahtBot cross-referenced this on Oct 20, 2020 from issue External signer support - Wallet Box edition by Sjors
7. 
   achow101 commented at 6:52 PM on October 20, 2020: member

   ACK 07d2fffea9ea3589060632fdfdbf44f9d31caab6

8. 
   instagibbs commented at 8:09 AM on January 8, 2021: member

   concept ACK

9. in src/wallet/scriptpubkeyman.cpp:1591 in 07d2fffea9 outdated

   1589 | -
   1590 |  bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
   1591 |  {
   1592 |      // Returns true if this descriptor supports getting new addresses. Conditions where we may be unable to fetch them (e.g. locked) are caught later
   1593 | -    if (!CanGetAddresses(m_internal)) {
   1594 | +    if (!CanGetAddresses(true)) {
   

   ---

   ---

   

   instagibbs commented at 8:10 AM on January 8, 2021:

   Please annotate the boolean arg

   ---

   

   S3RK commented at 10:05 AM on January 14, 2021:

   Thanks. Annotated and added comment that the arg is not used

10. S3RK force-pushed on Jan 14, 2021
11. achow101 approved
12. 
    achow101 commented at 6:58 PM on January 15, 2021: member

    re-ACK c9a4e0d8e2846e7eb03e8d5fd3ee18dba6f3c4a1

    Only change since last review is a comment.

13. 
    meshcollider commented at 9:53 AM on January 21, 2021: contributor

    utACK c9a4e0d8e2846e7eb03e8d5fd3ee18dba6f3c4a1

14. DrahtBot added the label Needs rebase on Feb 23, 2021
15. S3RK force-pushed on Feb 24, 2021
16. DrahtBot removed the label Needs rebase on Feb 24, 2021
17. S3RK requested review from meshcollider on May 19, 2021
18. S3RK requested review from achow101 on May 19, 2021
19. DrahtBot cross-referenced this on Jun 5, 2021 from issue Add OutputType::BECH32M and related wallet support for fetching bech32m addresses by achow101
20. DrahtBot cross-referenced this on Jun 16, 2021 from issue Make bech32m the default for RPC, opt-in for GUI by Sjors
21. DrahtBot added the label Needs rebase on Jun 24, 2021
22. S3RK force-pushed on Jun 29, 2021
23. DrahtBot removed the label Needs rebase on Jun 29, 2021
24. DrahtBot cross-referenced this on Jun 29, 2021 from issue wallet: Make a tr() descriptor by default by achow101
25. DrahtBot cross-referenced this on Jun 29, 2021 from issue wallet: Use bilingual_str for errors by achow101
26. achow101 approved
27. 
    achow101 commented at 6:38 PM on June 29, 2021: member

    ACK 7e329e281042051fd85acd2f9dd8f893cbefcb8d

28. 
    fanquake commented at 1:29 AM on June 30, 2021: member

    @meshcollider @instagibbs

29. in src/wallet/scriptpubkeyman.cpp:1619 in c94e3da57e outdated

    1617 | -
    1618 |  bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
    1619 |  {
    1620 |      // Returns true if this descriptor supports getting new addresses. Conditions where we may be unable to fetch them (e.g. locked) are caught later
    1621 | -    if (!CanGetAddresses(m_internal)) {
    1622 | +    if (!CanGetAddresses(/* internal= */ true)) { // internal arg is not used for descriptor spk managers
    

    ---

    ---

    

    instagibbs commented at 2:22 AM on June 30, 2021:

    Seems less confusing to just leave the optional argument out

    ---

    

    S3RK commented at 6:38 AM on June 30, 2021:

    I don't know how I missed that. Thanks

30. instagibbs approved
31. 
    instagibbs commented at 2:40 AM on June 30, 2021: member

    utACK

    Idea for follow-up:

    Extract the desc_str creation from DescriptorScriptPubKeyMan::SetupDescriptorGeneration, then I think the logic/flow between ExternalSignerScriptPubKeyMan and DescriptorScriptPubKeyMan becomes a lot closer, could reduce amount of required duplicate code.

    It would also mean that DescriptorSPKM never have to know anything about internal-ness, just like External ones.

32. 
    jb55 commented at 2:49 AM on June 30, 2021: contributor

    ACK c94e3da57e85812ad391e4f7173321781419b9c0

33. refactor: remove m_internal from DescriptorSPKman

    Descriptor in itself is neither internal or external.
    It's responsibility of a wallet to assign and manage descriptors
    for a specific purpose. Duplicating such information could lead to
    inconsistencies and unexpected behaviour.

    181181019c
34. S3RK force-pushed on Jun 30, 2021
35. 
    instagibbs commented at 1:17 PM on June 30, 2021: member

    reACK https://github.com/bitcoin/bitcoin/pull/20191/commits/181181019c5baa3e2d5b675d1843a45aa028781c

36. 
    achow101 commented at 5:29 PM on June 30, 2021: member

    reACK 181181019c5baa3e2d5b675d1843a45aa028781c

37. fanquake merged this on Jul 1, 2021
38. fanquake closed this on Jul 1, 2021

    ---

39. 
    fanquake commented at 4:42 AM on July 1, 2021: member

    Looks like there are now some CI failures for the wallet_importdescriptors.py --descriptors test. An incompatiblity between this and #19651?

40. 
    achow101 commented at 5:25 AM on July 1, 2021: member

    @fanquake Fix in #22379

41. S3RK cross-referenced this on Jul 1, 2021 from issue wallet: erase spkmans rather than setting to nullptr by achow101
42. sidhujag referenced this in commit 96137a1642 on Jul 1, 2021
43. gwillen referenced this in commit 59bf2ee204 on Jun 1, 2022
44. bitcoin locked this on Aug 18, 2022

Contributors

S3RKDrahtBotachow101instagibbsmeshcolliderfanquakejb55

Review Requested
meshcollider