Avoid unnecessary signing provider copies on descriptor expansion by Empact · Pull Request #16116 · bitcoin/bitcoin

Empact · 2019-05-29T05:39:26Z

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 Empact@604606d
ExpandDescriptor, 5, 6, 1.22676, 0.0392286, 0.0428134, 0.0403623
This PR 7748ecf
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.

bitcoin/src/test/descriptor_tests.cpp

Line 210 in c7cfd20

    
           Check("sh(wsh(multi(16,KzoAz5CanayRKex3fSLQ2BwJpN7U52gZvxMyk78nDMHuqrUxuSJy,KwGNz6YCCQtYvFzMtrC6D3tKTKdBBboMrLTsjr2NYVBwapCkn7Mr,KxogYhiNfwxuswvXV66eFyKcCpm7dZ7TqHVqujHAVUjJxyivxQ9X,L2BUNduTSyZwZjwNHynQTF14mv2uz2NRq5n5sYWTb4FkkmqgEE9f,L1okJGHGn1kFjdXHKxXjwVVtmCMR2JA5QsbKCSpSb7ReQjezKeoD,KxDCNSST75HFPaW5QKpzHtAyaCQC7p9Vo3FYfi2u4dXD1vgMiboK,L5edQjFtnkcf5UWURn6UuuoFrabgDQUHdheKCziwN42aLwS3KizU,KzF8UWFcEC7BYTq8Go1xVimMkDmyNYVmXV5PV7RuDicvAocoPB8i,L3nHUboKG2w4VSJ5jYZ5CBM97oeK6YuKvfZxrefdShECcjEYKMWZ,KyjHo36dWkYhimKmVVmQTq3gERv3pnqA4xFCpvUgbGDJad7eS8WE,KwsfyHKRUTZPQtysN7M3tZ4GXTnuov5XRgjdF2XCG8faAPmFruRF,KzCUbGhN9LJhdeFfL9zQgTJMjqxdBKEekRGZX24hXdgCNCijkkap,KzgpMBwwsDLwkaC5UrmBgCYaBD2WgZ7PBoGYXR8KT7gCA9UTN5a3,KyBXTPy4T7YG4q9tcAM3LkvfRpD1ybHMvcJ2ehaWXaSqeGUxEdkP,KzJDe9iwJRPtKP2F2AoN6zBgzS7uiuAwhWCfGdNeYJ3PC1HNJ8M8,L1xbHrxynrqLKkoYc4qtoQPx6uy5qYXR5ZDYVYBSRmCV5piU3JG9)))","sh(wsh(multi(16,03669b8afcec803a0d323e9a17f3ea8e68e8abe5a278020a929adbec52421adbd0,0260b2003c386519fc9eadf2b5cf124dd8eea4c4e68d5e154050a9346ea98ce600,0362a74e399c39ed5593852a30147f2959b56bb827dfa3e60e464b02ccf87dc5e8,0261345b53de74a4d721ef877c255429961b7e43714171ac06168d7e08c542a8b8,02da72e8b46901a65d4374fe6315538d8f368557dda3a1dcf9ea903f3afe7314c8,0318c82dd0b53fd3a932d16e0ba9e278fcc937c582d5781be626ff16e201f72286,0297ccef1ef99f9d73dec9ad37476ddb232f1238aff877af19e72ba04493361009,02e502cfd5c3f972fe9a3e2a18827820638f96b6f347e54d63deb839011fd5765d,03e687710f0e3ebe81c1037074da939d409c0025f17eb86adb9427d28f0f7ae0e9,02c04d3a5274952acdbc76987f3184b346a483d43be40874624b29e3692c1df5af,02ed06e0f418b5b43a7ec01d1d7d27290fa15f75771cb69b642a51471c29c84acd,036d46073cbb9ffee90473f3da429abc8de7f8751199da44485682a989a4bebb24,02f5d1ff7c9029a80a4e36b9a5497027ef7f3e73384a4a94fbfe7c4e9164eec8bc,02e41deffd1b7cce11cde209a781adcffdabd1b91c0ba0375857a2bfd9302419f3,02d76625f7956a7fc505ab02556c23ee72d832f1bac391bcd2d3abce5710a13d06,0399eb0a5487515802dc14544cf10b3666623762fbed2ec38a3975716e2c29c232)))", SIGNABLE, {{"a9147fc63e13dc25e8a95a3cee3d9a714ac3afd96f1e87"}});

DrahtBot · 2019-05-29T06:19:48Z

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.

practicalswift · 2019-05-29T10:18:05Z

@Empact Can you quantify the improvement via a benchmark?

Empact · 2019-05-30T01:17:34Z

Added bench info to the description.

laanwj · 2019-06-13T11:15:46Z

Apart from the micro-benchmark testing this specific code: does this make signing (or any other RPC calls) noticeably faster?

Empact · 2019-06-16T22:04:05Z

Any suggestion as to how to produce a representative and broad test of this?

Sjors · 2020-02-27T13:06:24Z

@achow101 this might tie into #18204

achow101 · 2020-06-11T17:30:19Z

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.

meshcollider · 2020-07-11T11:42:45Z

Concept ACK

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.

achow101 · 2021-12-20T22:04:27Z

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.

maflcko · 2021-12-23T07:18:15Z

Should this be marked "up for grabs"?

…pansion 4786959 bench: Add a benchmark for descriptor expansion (Ben Woosley) Pull request description: Taken from bitcoin/bitcoin#16116 , as requested here: bitcoin/bitcoin#25748 (comment) ACKs for top commit: achow101: ACK 4786959 Tree-SHA512: f2efdf8f84e1783c7c298abe65123191d25cab0a9da2d0ff5957a60acc2d10e356151d7ecec0d98d28c456f42ddef50efd70c7edc0c9012df2a977e080515b9d

4786959 bench: Add a benchmark for descriptor expansion (Ben Woosley) Pull request description: Taken from bitcoin#16116 , as requested here: bitcoin#25748 (comment) ACKs for top commit: achow101: ACK 4786959 Tree-SHA512: f2efdf8f84e1783c7c298abe65123191d25cab0a9da2d0ff5957a60acc2d10e356151d7ecec0d98d28c456f42ddef50efd70c7edc0c9012df2a977e080515b9d

Empact changed the title ~~Move Merge() to FlatSigningProvider::Merge()~~ Avoid unnecessary signing provider copies on descriptor expansion May 29, 2019

Empact marked this pull request as ready for review May 29, 2019 06:32

DrahtBot added the Tests label May 29, 2019

Empact force-pushed the merge-flat-signing-provider branch from d7d31d2 to 7748ecf Compare May 30, 2019 01:07

laanwj added the Wallet label Jun 13, 2019

DrahtBot added the Needs rebase label Jul 4, 2019

Empact force-pushed the merge-flat-signing-provider branch from 7748ecf to 70d23eb Compare October 9, 2019 12:40

DrahtBot removed the Needs rebase label Oct 9, 2019

This was referenced Feb 25, 2020

descriptors: improve descriptor cache and cache xpubs #18204

Merged

Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528

Merged

DrahtBot mentioned this pull request Mar 3, 2020

build: Enable -Wsuggest-override if available #16710

Merged

DrahtBot added the Needs rebase label Mar 13, 2020

Empact force-pushed the merge-flat-signing-provider branch from 70d23eb to c059106 Compare March 14, 2020 19:26

DrahtBot removed the Needs rebase label Mar 14, 2020

This was referenced Apr 1, 2020

UI external signer support (e.g. hardware wallet) #16549

Closed

External signer support - Wallet Box edition #16546

Merged

DrahtBot mentioned this pull request Apr 29, 2020

bench: Add logging benchmark #18815

Merged

maflcko added Waiting for author and removed Tests labels Jun 11, 2020

Empact force-pushed the merge-flat-signing-provider branch from c059106 to 6c00640 Compare June 22, 2020 20:03

DrahtBot added the Needs rebase label Jun 24, 2020

Empact force-pushed the merge-flat-signing-provider branch from 6c00640 to 5024d30 Compare June 24, 2020 20:23

DrahtBot removed the Needs rebase label Jun 24, 2020

meshcollider removed the Waiting for author label Jul 11, 2020

DrahtBot mentioned this pull request Feb 20, 2021

A few descriptor improvements to prepare for Taproot support #21238

Merged

DrahtBot mentioned this pull request Mar 4, 2021

Basic Taproot signing support for descriptor wallets #21365

Merged

DrahtBot added the Needs rebase label Apr 20, 2021

Empact force-pushed the merge-flat-signing-provider branch from 5024d30 to 346c9ee Compare September 10, 2021 04:00

DrahtBot removed the Needs rebase label Sep 10, 2021

DrahtBot mentioned this pull request Sep 21, 2021

psbt: Taproot fields for PSBT #22558

Merged

DrahtBot mentioned this pull request Nov 14, 2021

wallet: Avoid underpaying transaction fees when signing taproot spends #23502

Closed

DrahtBot mentioned this pull request Nov 23, 2021

Add external signer taproot support #23578

Merged

maflcko closed this Jan 5, 2022

maflcko added the Up for grabs label Jan 5, 2022

achow101 mentioned this pull request Aug 4, 2022

refactor: Avoid copies in FlatSigningProvider Merge #25748

Merged

maflcko removed the Up for grabs label Aug 5, 2022

maflcko mentioned this pull request Aug 5, 2022

bench: Add a benchmark for descriptor expansion #25785

Merged

bitcoin locked and limited conversation to collaborators Aug 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid unnecessary signing provider copies on descriptor expansion#16116

Avoid unnecessary signing provider copies on descriptor expansion#16116
Empact wants to merge 1 commit intobitcoin:masterfrom
Empact:merge-flat-signing-provider

Empact commented May 29, 2019 •

edited

Loading

Uh oh!

DrahtBot commented May 29, 2019 •

edited

Loading

Uh oh!

practicalswift commented May 29, 2019

Uh oh!

Empact commented May 30, 2019

Uh oh!

laanwj commented Jun 13, 2019 •

edited

Loading

Uh oh!

Empact commented Jun 16, 2019

Uh oh!

Sjors commented Feb 27, 2020

Uh oh!

achow101 commented Jun 11, 2020 •

edited

Loading

Uh oh!

meshcollider commented Jul 11, 2020

Uh oh!

achow101 commented Dec 20, 2021 •

edited

Loading

Uh oh!

maflcko commented Dec 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

Empact commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented May 29, 2019

Uh oh!

Empact commented May 30, 2019

Uh oh!

laanwj commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Jun 16, 2019

Uh oh!

Sjors commented Feb 27, 2020

Uh oh!

achow101 commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Jul 11, 2020

Uh oh!

achow101 commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Empact commented May 29, 2019 •

edited

Loading

DrahtBot commented May 29, 2019 •

edited

Loading

laanwj commented Jun 13, 2019 •

edited

Loading

achow101 commented Jun 11, 2020 •

edited

Loading

achow101 commented Dec 20, 2021 •

edited

Loading