[Wallet] Do not perform ECDSA signing in the fee calculation inner loop. #9465
Conversation
gmaxwell
changed the title
|
Concept ACK - I thought this was already the case! |
|
Concept ACK. |
|
Concept ACK, will review. I was wondering about this myself. This will also make #9404 even a smaller change as we won't need to track variance. |
|
Concept ACK. I knew it wasn't the case, but it seems to obvious... |
|
utACK aaa3cfc |
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
A nice by-product of this change is that the signing is factored out. Which means that at some point this huge function could be split into multiple clearly defined functions.
|
utACK aaa3cfc |
|
@gmaxwell oh hmm... You are now signing outside of cs_main, cs_wallet. |
|
I think keystore is thread safe (and that that's all that is needed for
signing).
|
sipa
left a comment
sipa
left a comment
There was a problem hiding this comment.
utACK aaa3cfc485bc570a3e537349130a3471a8a4d3a8
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
This is a bit confusing to me. We're not elimining the dummy signatures in order to do fee calculation, but removing the dummy signatures which were added in order to do fee calculation.
There was a problem hiding this comment.
I reordered the sentence to make it more clear that I mean the "for fee estimation dummies". Should be fixed.
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
Feel like switching this to for (const auto& coin : setCoins) ?
There was a problem hiding this comment.
There were only a couple extra changes to do all of them and avoid making the function inconsistent in its usage, so I did. Fixed.
src/wallet/wallet.cpp
Outdated
Performing signing in the inner loop has terrible performance when many passes through are needed to complete the selection. Signing before the algorithm is complete also gets in the way of correctly setting the fee (e.g. preventing over-payment when the fee required goes down on the final selection.) Use of the dummy might overpay on the signatures by a couple bytes in uncommon cases where the signatures' DER encoding is smaller than the dummy: Who cares?
It's not outside of the lock unless I'm missing something! |
|
@gmaxwell oops, yeah i was misreading braces.. well maybe you should move it out? |
|
utACK b3d7b1c |
…tion inner loop. b3d7b1c Wallet: Do not perform ECDSA in the fee calculation inner loop. (Gregory Maxwell)
…calculation inner loop. b3d7b1c Wallet: Do not perform ECDSA in the fee calculation inner loop. (Gregory Maxwell)
…calculation inner loop. b3d7b1c Wallet: Do not perform ECDSA in the fee calculation inner loop. (Gregory Maxwell)
…calculation inner loop. b3d7b1c Wallet: Do not perform ECDSA in the fee calculation inner loop. (Gregory Maxwell)
…calculation inner loop. b3d7b1c Wallet: Do not perform ECDSA in the fee calculation inner loop. (Gregory Maxwell)
…fee calculation loop e5aab53 Refactor: initialize dummy Spend/Output descriptions only once (random-zebra) 6099737 Sapling: Skip proofs and signatures during fee calculation loop (random-zebra) 5d8ba3d Sapling: add option to skip ProveAndSing in TransactionBuilder::Build (random-zebra) efd7139 Sapling: Decouple ProveAndSign from Build (random-zebra) 44620de [Wallet] Do not perform ECDSA signing in the fee calculation inner loop (random-zebra) Pull request description: First commit backports bitcoin#9465. Following commits adapt the same idea to sapling transactions, including also the spend/output descriptors. This gives a **huge** speed-up in the transaction-creation process (especially with many inuputs and/or outputs). Down-side: less time for the awesome shield animation in the GUI :) ~~Built on top of #2064 to remove the conflicts.~~ ACKs for top commit: furszy: Nice one!, ACK e5aab53 Fuzzbawls: ACK e5aab53 Tree-SHA512: 22fb95e8f5d635e7031e911edcfed66b6f15168ca8027ad3080ce01f8e93b1032a86988d9463425eb4f2755f286b299328910538d23fe15bbee8cfbc3763b8fe
8 participants
Performing signing in the inner loop has terrible performance
when many passes through are needed to complete the selection.
Signing before the algorithm is complete also gets in the way
of correctly setting the fee (e.g. preventing over-payment when
the fee required goes down on the final selection.)
Use of the dummy might overpay on the signatures by a couple bytes
in uncommon cases where the signatures' DER encoding is smaller
than the dummy: Who cares? (Probability ~= 1/256^(bytes/2.))