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

1. 
   S3RK commented at 3:33 am on October 20, 2020: member

   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: 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:

   • #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. 
   achow101 commented at 6:52 pm on October 20, 2020: member

   ACK 07d2fffea9ea3589060632fdfdbf44f9d31caab6
6. 
   instagibbs commented at 8:09 am on January 8, 2021: member

   concept ACK
7. 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
8. S3RK force-pushed on Jan 14, 2021
9. achow101 approved
10. 
    achow101 commented at 6:58 pm on January 15, 2021: member

    re-ACK c9a4e0d8e2846e7eb03e8d5fd3ee18dba6f3c4a1

    Only change since last review is a comment.

11. 
    meshcollider commented at 9:53 am on January 21, 2021: contributor

    utACK c9a4e0d8e2846e7eb03e8d5fd3ee18dba6f3c4a1
12. DrahtBot added the label Needs rebase on Feb 23, 2021
13. S3RK force-pushed on Feb 24, 2021
14. DrahtBot removed the label Needs rebase on Feb 24, 2021
15. S3RK requested review from meshcollider on May 19, 2021
16. S3RK requested review from achow101 on May 19, 2021
17. DrahtBot added the label Needs rebase on Jun 24, 2021
18. S3RK force-pushed on Jun 29, 2021
19. DrahtBot removed the label Needs rebase on Jun 29, 2021
20. achow101 approved
21. 
    achow101 commented at 6:38 pm on June 29, 2021: member

    ACK 7e329e281042051fd85acd2f9dd8f893cbefcb8d
22. 
    fanquake commented at 1:29 am on June 30, 2021: member

    @meshcollider @instagibbs
23. 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
24. instagibbs approved
25. 
    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.

26. 
    jb55 commented at 2:49 am on June 30, 2021: member

    ACK c94e3da57e85812ad391e4f7173321781419b9c0
27. 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
28. S3RK force-pushed on Jun 30, 2021
29. 
    instagibbs commented at 1:17 pm on June 30, 2021: member

    reACK https://github.com/bitcoin/bitcoin/pull/20191/commits/181181019c5baa3e2d5b675d1843a45aa028781c
30. 
    achow101 commented at 5:29 pm on June 30, 2021: member

    reACK 181181019c5baa3e2d5b675d1843a45aa028781c
31. fanquake merged this on Jul 1, 2021
32. fanquake closed this on Jul 1, 2021

    ---

33. 
    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?
34. 
    achow101 commented at 5:25 am on July 1, 2021: member

    @fanquake Fix in #22379
35. sidhujag referenced this in commit 96137a1642 on Jul 1, 2021
36. gwillen referenced this in commit 59bf2ee204 on Jun 1, 2022
37. DrahtBot locked this on Aug 18, 2022

Contributors
S3RK DrahtBot achow101 instagibbs meshcollider fanquake jb55

Review Requested
meshcollider

Labels
Wallet