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

    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.

  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
EmpactDrahtBotpracticalswiftlaanwjSjorsachow101meshcolliderMarcoFalke
Labels
Wallet
Linked (view graph)
#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-04-17 09:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me