[Wallet] Do not perform ECDSA signing in the fee calculation inner loop.

gmaxwell commented at 8:57 am on January 4, 2017: contributor

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

fanquake added the label Wallet on Jan 4, 2017

gmaxwell renamed this:
~~[Wallet] Do not perform ECDSA in the fee calculation inner loop.~~
[Wallet] Do not perform ECDSA signing in the fee calculation inner loop.
on Jan 4, 2017

laanwj commented at 9:09 am on January 4, 2017: member

Concept ACK - I thought this was already the case!

jonasschnelli commented at 9:25 am on January 4, 2017: contributor

Concept ACK.

morcos commented at 1:56 pm on January 4, 2017: member

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.

sipa commented at 2:40 pm on January 4, 2017: member

Concept ACK. I knew it wasn’t the case, but it seems to obvious…

morcos commented at 7:37 pm on January 4, 2017: member

utACK aaa3cfc

dcousens approved

in src/wallet/wallet.cpp: in aaa3cfc485 outdated

2541@@ -2558,6 +2542,37 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2542                 continue;
2543             }
2544         }
2545+
2546+        if (sign)

laanwj commented at 10:00 am on January 5, 2017:

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.

sipa commented at 3:25 pm on January 5, 2017:

Agree!

laanwj commented at 10:00 am on January 5, 2017: member

utACK aaa3cfc

laanwj approved

morcos commented at 2:37 pm on January 5, 2017: member

@gmaxwell oh hmm… You are now signing outside of cs_main, cs_wallet.
I suppose that is ok?

sipa commented at 2:56 pm on January 5, 2017: member

I think keystore is thread safe (and that that’s all that is needed for signing).

in src/wallet/wallet.cpp: in aaa3cfc485 outdated

2511-                // Limit size
2512-                if (GetTransactionWeight(wtxNew) >= MAX_STANDARD_TX_WEIGHT)
2513-                {
2514-                    strFailReason = _("Transaction too large");
2515-                    return false;
2516+                // Remove scriptSigs to eliminate dummy signatures for fee calculation

sipa commented at 3:20 pm on January 5, 2017:

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.

gmaxwell commented at 6:16 pm on January 5, 2017:

I reordered the sentence to make it more clear that I mean the “for fee estimation dummies”. Should be fixed.

in src/wallet/wallet.cpp: in aaa3cfc485 outdated

2541@@ -2558,6 +2542,37 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2542                 continue;
2543             }
2544         }
2545+
2546+        if (sign)
2547+        {
2548+            CTransaction txNewConst(txNew);
2549+            int nIn = 0;
2550+            BOOST_FOREACH(const PAIRTYPE(const CWalletTx*,unsigned int)& coin, setCoins)

sipa commented at 3:22 pm on January 5, 2017:

Feel like switching this to for (const auto& coin : setCoins) ?

gmaxwell commented at 6:16 pm on January 5, 2017:

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.

sipa commented at 3:27 pm on January 5, 2017: member

utACK aaa3cfc485bc570a3e537349130a3471a8a4d3a8

Wallet: Do not perform ECDSA in the fee calculation inner loop.

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?

b3d7b1cbe7

gmaxwell commented at 6:17 pm on January 5, 2017: contributor

@morcos

oh hmm… You are now signing outside of cs_main, cs_wallet. I suppose that is ok?

It’s not outside of the lock unless I’m missing something!

morcos commented at 6:54 pm on January 5, 2017: member

@gmaxwell oops, yeah i was misreading braces..

well maybe you should move it out? :)

sdaftuar approved

sdaftuar commented at 9:26 pm on January 5, 2017: member

utACK b3d7b1cbe7afdc6a63bbcbe938e8639deedb04a1

sipa commented at 9:28 pm on January 5, 2017: member

utACK b3d7b1cbe7afdc6a63bbcbe938e8639deedb04a1

sipa merged this on Jan 5, 2017

sipa closed this on Jan 5, 2017

sipa referenced this in commit a7d55c9338 on Jan 5, 2017

codablock referenced this in commit 2dd62abe32 on Jan 18, 2018

andvgal referenced this in commit b88650b35a on Jan 6, 2019

CryptoCentric referenced this in commit 6a3279c4c7 on Feb 26, 2019

CryptoCentric referenced this in commit dea87cbb15 on Feb 26, 2019

Fuzzbawls referenced this in commit 75d52340ee on Dec 19, 2020

MarcoFalke locked this on Sep 8, 2021

[Wallet] Do not perform ECDSA signing in the fee calculation inner loop. #9465