Skip to content

Avoid unnecessary signing provider copies on descriptor expansion #16116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Empact wants to merge 1 commit into from
Closed

Avoid unnecessary signing provider copies on descriptor expansion #16116

Empact wants to merge 1 commit into from

Conversation

@Empact
Copy link
Contributor

@Empact Empact commented May 29, 2019 •
edited
Loading

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"}});

@Empact Empact changed the title Move Merge() to FlatSigningProvider::Merge() Avoid unnecessary signing provider copies on descriptor expansion May 29, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented May 29, 2019 •
edited
Loading

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.

@Empact Empact marked this pull request as ready for review May 29, 2019 06:32
@DrahtBot DrahtBot added the Tests label May 29, 2019
@practicalswift
Copy link
Contributor

practicalswift commented May 29, 2019

@Empact Can you quantify the improvement via a benchmark?

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

Empact commented May 30, 2019

Added bench info to the description.

@laanwj laanwj added the Wallet label Jun 13, 2019
@laanwj
Copy link
Member

laanwj commented Jun 13, 2019 •
edited
Loading

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

@Empact
Copy link
Contributor Author

Empact commented Jun 16, 2019

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

@Empact Empact force-pushed the merge-flat-signing-provider branch from 7748ecf to 70d23eb Compare October 9, 2019 12:40
This was referenced Feb 25, 2020
Merged
Merged
@Sjors
Copy link
Member

Sjors commented Feb 27, 2020

@achow101 this might tie into #18204

@DrahtBot DrahtBot mentioned this pull request Mar 3, 2020
Merged
@Empact Empact force-pushed the merge-flat-signing-provider branch from 70d23eb to c059106 Compare March 14, 2020 19:26
This was referenced Apr 1, 2020
Closed
Merged
@DrahtBot DrahtBot mentioned this pull request Apr 29, 2020
Merged
@achow101
Copy link
Member

achow101 commented Jun 11, 2020 •
edited
Loading

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.

@Empact Empact force-pushed the merge-flat-signing-provider branch from c059106 to 6c00640 Compare June 22, 2020 20:03
@Empact Empact force-pushed the merge-flat-signing-provider branch from 6c00640 to 5024d30 Compare June 24, 2020 20:23
@meshcollider
Copy link
Contributor

meshcollider commented Jul 11, 2020

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Feb 20, 2021
Merged
@DrahtBot DrahtBot mentioned this pull request Mar 4, 2021
Merged
@Empact
Move Merge() to FlatSigningProvider::Merge()
346c9ee 
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.
@Empact Empact force-pushed the merge-flat-signing-provider branch from 5024d30 to 346c9ee Compare September 10, 2021 04:00
@DrahtBot DrahtBot mentioned this pull request Sep 21, 2021
Merged
@DrahtBot DrahtBot mentioned this pull request Nov 14, 2021
Closed
@DrahtBot DrahtBot mentioned this pull request Nov 23, 2021
Merged
@achow101
Copy link
Member

achow101 commented Dec 20, 2021 •
edited
Loading

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
Copy link
Member

maflcko commented Dec 23, 2021

Should this be marked "up for grabs"?

@maflcko maflcko closed this Jan 5, 2022
@achow101 achow101 mentioned this pull request Aug 4, 2022
Merged
@maflcko maflcko mentioned this pull request Aug 5, 2022
Merged
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Aug 12, 2022
Merge bitcoin/bitcoin#25785: bench: Add a benchmark for descriptor ex…
bf3f05f 
…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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 12, 2022
Merge bitcoin#25785: bench: Add a benchmark for descriptor expansion
f35b6d2 
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
@bitcoin bitcoin locked and limited conversation to collaborators Aug 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@Empact @DrahtBot @practicalswift @laanwj @Sjors @achow101 @meshcollider @maflcko