Avoid unnecessary signing provider copies on descriptor expansion #16116

pull Empact wants to merge 1 commits into bitcoin:master from Empact:merge-flat-signing-provider changing 7 files +51 −23
  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

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

    • #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. DrahtBot cross-referenced this on Feb 25, 2020 from issue descriptors: improve descriptor cache and cache xpubs by achow101
  16. DrahtBot cross-referenced this on Feb 27, 2020 from issue Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101
  17. Sjors commented at 1:06 PM on February 27, 2020: member

    @achow101 this might tie into #18204

  18. DrahtBot cross-referenced this on Mar 3, 2020 from issue build: Enable -Wsuggest-override if available by hebasto
  19. DrahtBot added the label Needs rebase on Mar 13, 2020
  20. Empact force-pushed on Mar 14, 2020
  21. DrahtBot removed the label Needs rebase on Mar 14, 2020
  22. DrahtBot cross-referenced this on Apr 1, 2020 from issue UI external signer support (e.g. hardware wallet) by Sjors
  23. DrahtBot cross-referenced this on Apr 1, 2020 from issue External signer support - Wallet Box edition by Sjors
  24. DrahtBot cross-referenced this on Apr 29, 2020 from issue bench: Add logging benchmark by MarcoFalke
  25. 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.

  26. MarcoFalke added the label Waiting for author on Jun 11, 2020
  27. MarcoFalke removed the label Tests on Jun 11, 2020
  28. Empact force-pushed on Jun 22, 2020
  29. DrahtBot added the label Needs rebase on Jun 24, 2020
  30. Empact force-pushed on Jun 24, 2020
  31. DrahtBot removed the label Needs rebase on Jun 24, 2020
  32. meshcollider commented at 11:42 AM on July 11, 2020: contributor

    Concept ACK

  33. meshcollider removed the label Waiting for author on Jul 11, 2020
  34. DrahtBot cross-referenced this on Feb 20, 2021 from issue A few descriptor improvements to prepare for Taproot support by sipa
  35. DrahtBot cross-referenced this on Mar 4, 2021 from issue Basic Taproot signing support for descriptor wallets by sipa
  36. DrahtBot added the label Needs rebase on Apr 20, 2021
  37. 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
  38. Empact force-pushed on Sep 10, 2021
  39. DrahtBot removed the label Needs rebase on Sep 10, 2021
  40. DrahtBot cross-referenced this on Sep 21, 2021 from issue psbt: Taproot fields for PSBT by achow101
  41. DrahtBot cross-referenced this on Nov 14, 2021 from issue wallet: Avoid underpaying transaction fees when signing taproot spends by achow101
  42. DrahtBot cross-referenced this on Nov 23, 2021 from issue Add external signer taproot support by Sjors
  43. achow101 commented at 10:04 PM on December 20, 2021: member

    Silent merge conflict with master:

    wallet/rpc/spend.cpp: In function ‘void FundTransaction(CWallet&, CMutableTransaction&, CAmount&, int&, const UniValue&, CCoinControl&, bool)’:
wallet/rpc/spend.cpp:545:51: error: ‘Merge’ was not declared in this scope
  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.

  44. MarcoFalke commented at 7:18 AM on December 23, 2021: member

    Should this be marked "up for grabs"?

  45. MarcoFalke closed this on Jan 5, 2022
  46. MarcoFalke added the label Up for grabs on Jan 5, 2022
  47. achow101 cross-referenced this on Aug 4, 2022 from issue refactor: Avoid copies in FlatSigningProvider Merge by MarcoFalke
  48. MarcoFalke removed the label Up for grabs on Aug 5, 2022
  49. MarcoFalke cross-referenced this on Aug 5, 2022 from issue bench: Add a benchmark for descriptor expansion by MarcoFalke
  50. MarcoFalke referenced this in commit bf3f05f41d on Aug 12, 2022
  51. sidhujag referenced this in commit f35b6d28c6 on Aug 12, 2022
  52. bitcoin locked this on Aug 5, 2023
Contributors
EmpactDrahtBotpracticalswiftlaanwjSjorsachow101meshcolliderMarcoFalke
Labels
Wallet
Linked (view graph)
#25748 refactor: Avoid copies in FlatSigningProvider Merge#25785 bench: Add a benchmark for descriptor expansion
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:54 UTC