Avoid unnecessary signing provider copies on descriptor expansion #16116

1. 
   Empact commented at 5:39 am on May 29, 2019: member

   Currently descriptor expansion unnecessarily copies the signing provider data once per expansion. Avoid this work by making it a member of the class and doing the merge in-place.

   Current master https://github.com/Empact/bitcoin/commit/604606d4e5e5a12976fd752e6e6c80c0e9ad6aac
   ExpandDescriptor, 5, 6, 1.22676, 0.0392286, 0.0428134, 0.0403623 This PR https://github.com/bitcoin/bitcoin/pull/16116/commits/7748ecf90126cc388cd35af101f8aebef719a98b
   ExpandDescriptor, 5, 6, 1.02993, 0.0333122, 0.0353901, 0.0343695

   Note a ranged descriptor is expanded 1000x by default, and the descriptor string I used is from the test suite. https://github.com/bitcoin/bitcoin/blob/c7cfd20a77ce57d200b3b9e5e0dfb0d63818abdc/src/test/descriptor_tests.cpp#L210

2. Empact renamed this:
   Move Merge() to FlatSigningProvider::Merge()
   Avoid unnecessary signing provider copies on descriptor expansion
   on May 29, 2019
3. 
   DrahtBot commented at 6:19 am on May 29, 2019: contributor

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #23578 (Add external signer taproot support by Sjors)
   • #22558 (psbt: Taproot fields for PSBT by achow101)
   • #18815 (bench: Add logging benchmark by MarcoFalke)

   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.

4. Empact marked this as ready for review on May 29, 2019
5. DrahtBot added the label Tests on May 29, 2019
6. 
   practicalswift commented at 10:18 am on May 29, 2019: contributor

   @Empact Can you quantify the improvement via a benchmark?
7. Empact force-pushed on May 30, 2019
8. 
   Empact commented at 1:17 am on May 30, 2019: member

   Added bench info to the description.
9. laanwj added the label Wallet on Jun 13, 2019
10. 
    laanwj commented at 11:15 am on June 13, 2019: member

    Apart from the micro-benchmark testing this specific code: does this make signing (or any other RPC calls) noticeably faster?
11. 
    Empact commented at 10:04 pm on June 16, 2019: member

    Any suggestion as to how to produce a representative and broad test of this?
12. DrahtBot added the label Needs rebase on Jul 4, 2019
13. Empact force-pushed on Oct 9, 2019
14. DrahtBot removed the label Needs rebase on Oct 9, 2019
15. 
    Sjors commented at 1:06 pm on February 27, 2020: member

    @achow101 this might tie into #18204
16. DrahtBot added the label Needs rebase on Mar 13, 2020
17. Empact force-pushed on Mar 14, 2020
18. DrahtBot removed the label Needs rebase on Mar 14, 2020
19. 
    achow101 commented at 5:30 pm on June 11, 2020: member

    Concept ACK c059106b8f4c4c79229234fc658cbe6627f006d3

    Edit: There are a few places where Merge is being used in src/wallet/scriptpubkeyman.cpp and that’s causing a silent merge conflict with master.

20. MarcoFalke added the label Waiting for author on Jun 11, 2020
21. MarcoFalke removed the label Tests on Jun 11, 2020
22. Empact force-pushed on Jun 22, 2020
23. DrahtBot added the label Needs rebase on Jun 24, 2020
24. Empact force-pushed on Jun 24, 2020
25. DrahtBot removed the label Needs rebase on Jun 24, 2020
26. 
    meshcollider commented at 11:42 am on July 11, 2020: contributor

    Concept ACK
27. meshcollider removed the label Waiting for author on Jul 11, 2020
28. DrahtBot added the label Needs rebase on Apr 20, 2021
29. Move Merge() to FlatSigningProvider::Merge()

    Currently descriptor expansion unnecessarily copies the signing provider
    data once per expansion. Avoid this work by making it a member of the
    class and doing the merge in-place.
    
    And add a benchmark for descriptor expansion to characterize the change.

    346c9eefa3
30. Empact force-pushed on Sep 10, 2021
31. DrahtBot removed the label Needs rebase on Sep 10, 2021
32. 
    achow101 commented at 10:04 pm on December 20, 2021: member

    Silent merge conflict with master:

    0wallet/rpc/spend.cpp: In function ‘void FundTransaction(CWallet&, CMutableTransaction&, CAmount&, int&, const UniValue&, CCoinControl&, bool)’:
    1wallet/rpc/spend.cpp:545:51: error: ‘Merge’ was not declared in this scope
    2  545 |                 coinControl.m_external_provider = Merge(coinControl.m_external_provider, desc_out);
    

    ---

    This is a fairly simple change, and I’d like to get it in.

33. 
    MarcoFalke commented at 7:18 am on December 23, 2021: member

    Should this be marked “up for grabs”?
34. MarcoFalke closed this on Jan 5, 2022

    ---

35. MarcoFalke added the label Up for grabs on Jan 5, 2022
36. MarcoFalke removed the label Up for grabs on Aug 5, 2022
37. MarcoFalke referenced this in commit bf3f05f41d on Aug 12, 2022
38. sidhujag referenced this in commit f35b6d28c6 on Aug 12, 2022
39. bitcoin locked this on Aug 5, 2023

Contributors
Empact DrahtBot practicalswift laanwj Sjors achow101 meshcollider MarcoFalke

Labels
Wallet