wallet: Avoid requesting fee rates multiple times during coin selection

achow101 commented at 0:34 am on February 5, 2021: member

During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in CreateTransactionInternal. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If pick_new_inputs == false, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn’t and this causes coin selection to fail.

Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of CreateTransactionInternal, store them in CoinSelectionParams, and use them where needed.

While some of these fee rates probably don’t need this caching, I’ve done it for consistency and the guarantee that they remain the same.

Fixes #19229

DrahtBot added the label Wallet on Feb 5, 2021

DrahtBot commented at 7:59 am on February 5, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21207 (MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky)
#17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
#17331 (Use effective values throughout coin selection by achow101)

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.

jonatack commented at 6:08 pm on February 6, 2021: member

Concept ACK, was thinking about this as well

meshcollider commented at 10:02 pm on February 16, 2021: contributor

Concept / light-look-through ACK

in src/wallet/wallet.cpp:2958 in 81b6a6b12b outdated

2961-                    // eventually allow a fallback fee
2962-                    error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.");
2963-                    return false;
2964-                }
2965-
2966+                nFeeNeeded = coin_selection_params.effective_fee.GetFee(nBytes);

ryanofsky commented at 10:21 pm on February 17, 2021:

In commit “wallet: Use existing feerate instead of getting a new one” (81b6a6b12b46ee351a293ae4928e815c37f193d3)

Review note: this assignment is equivalent to previous GetMinimumFee assignment because GetMinimumFeeRate is always assigned to nFeeRateNeeded line 2810, and nFeeRateNeeded is always assigned to coin_selection_params.effective_fee on line 2896 (thanks to pick_new_inputs spaghetti) and because GetMinimumFee() is just a wrapper for GetMinimumFeeRate().GetFee()

in src/wallet/wallet.cpp:2814 in 81b6a6b12b outdated

2813@@ -2814,6 +2814,11 @@ bool CWallet::CreateTransactionInternal(
2814                 error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), nFeeRateNeeded.ToString(FeeEstimateMode::SAT_VB));
2815                 return false;
2816             }
2817+            if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) {
2818+                // eventually allow a fallback fee
2819+                error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.");

ryanofsky commented at 10:25 pm on February 17, 2021:

In commit “wallet: Use existing feerate instead of getting a new one” (81b6a6b12b46ee351a293ae4928e815c37f193d3)

It appears there is a change behavior here. “Fee estimation failed” error will now take priority over “insufficient funds”, “Signing transaction failed”, and other errors. It would be good to mention this in the commit message or at least say that this isn’t a pure refactoring like it might seem to be (could mention the pick new inputs race condition behavior change as well).

achow101 commented at 8:29 pm on February 19, 2021:

Will update the commit message if I have to retouch.

achow101 commented at 5:01 pm on March 16, 2021:

Updated the commit message

in src/wallet/wallet.cpp:2805 in befa7ef609 outdated

2806@@ -2807,11 +2807,11 @@ bool CWallet::CreateTransactionInternal(
2807             CFeeRate discard_rate = GetDiscardRate(*this);
2808 
2809             // Get the fee rate to use effective values in coin selection
2810-            CFeeRate nFeeRateNeeded = GetMinimumFeeRate(*this, coin_control, &feeCalc);
2811+            coin_selection_params.effective_fee = GetMinimumFeeRate(*this, coin_control, &feeCalc);

ryanofsky commented at 10:39 pm on February 17, 2021:

In commit “wallet: Replace nFeeRateNeeded with effective_fee” (befa7ef609c977e6baa609900cedf3d41da12e53)

Review notes: It is safe to set coin_selection_params.effective_fee here, because there is no other code except SelectCoins called on line 2897 and the nFeeRet >= nFeeNeeded branch line 2959 below which ever read effective_fee, and both of these pieces of code come after the place where effective_fee was previously set on line 2896.

Also, it is nice to see the unnecessary nFeeRateNeeded variable being removed.

Would be good to mention this is a pure refactoring that doesn’t change behavior in the commit description (same for the following 2 commits).

achow101 commented at 8:29 pm on February 19, 2021:

Will update the commit message if I have to retouch.

achow101 commented at 5:01 pm on March 16, 2021:

Updated the commit message

Xekyo commented at 7:07 pm on March 16, 2021:

It seems to me that effective_fee is a feerate, not a fee.

achow101 commented at 8:39 pm on March 16, 2021:

Renamed to m_effective_feerate.

in src/wallet/wallet.h:616 in d3191194a8 outdated

612     //! Indicate that we are subtracting the fee from outputs
613     bool m_subtract_fee_outputs = false;
614     bool m_avoid_partial_spends = false;
615 
616-    CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_fee, size_t tx_noinputs_size, bool avoid_partial) :
617+    CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_fee, CFeeRate long_term_feerate, size_t tx_noinputs_size, bool avoid_partial) :

ryanofsky commented at 11:01 pm on February 17, 2021:

In commit “wallet: Move long term feerate setting to CreateTransaction” (d3191194a8ced53ae511551dce1321c0416d9e6d)

In this commit and next commit, usage of SelectCoins is now more error prone because caller is responsible for setting m_long_term_feerate and m_discard_feerate variables in some cases (in other cases they are apparently never used), and there’s not documentation about when to set them. Also these changes add to a proliferation of CoinSelectionParams constructor arguments can get easily confused together.

A safer approach might be to make these std::optional<CFeeRate> members instead of CFeeRate members so the CoinSelectionParams constructor doesn’t have to be involved with them at all. Then they can be directly set by name, and explicit errors will be triggered if they are ever accessed without being set.

achow101 commented at 8:28 pm on February 19, 2021:

The constructor parameters are more to make the tests easier to implement.

ryanofsky approved

ryanofsky commented at 11:14 pm on February 17, 2021: member

Code review ACK 7b4b48f6aa186d878984ae14727e5fa203f0d9b6. What a fucking horror show this code is! But overall the change does make it better. I left some suggestions below, but they’re not important so feel free to ignore!

MarcoFalke added the label Needs backport (0.21) on Feb 19, 2021

MarcoFalke added this to the milestone 0.21.1 on Feb 19, 2021

laanwj added this to the "Blockers" column in a project

in src/wallet/wallet.h:617 in 7b4b48f6aa outdated

613     //! Indicate that we are subtracting the fee from outputs
614     bool m_subtract_fee_outputs = false;
615     bool m_avoid_partial_spends = false;
616 
617-    CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_fee, size_t tx_noinputs_size, bool avoid_partial) :
618+    CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_fee, CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) :

luke-jr commented at 2:43 am on March 1, 2021:

I suggest not expanding the monster constructor here. Overall code readability seems to improve just assigning the new members after construction.

(Also much cleaner to backport…)

achow101 commented at 6:21 pm on March 4, 2021:

See #21083 (review)

jnewbery commented at 1:47 pm on March 16, 2021:

re https://github.com/bitcoin/bitcoin/pull/21083/files#r584415074 and #21083 (review):

I agree with @ryanofsky and @luke-jr that expanding this very large ctor signature makes the code less readable. Russ’s suggestion of using std::optionals is good. Alternatively, I think these small changes would make things more readable:

add line breaks to the declaration and call sites, so there aren’t 200+ column lines
add doxygen comments to the members so it’s obvious what they’re doing
use inline comments in the call sites:

0-CoinSelectionParams coin_selection_params(false, 0, 0, CFeeRate(0), CFeeRate(0), CFeeRate(0), 0, false);
1+CoinSelectionParams coin_selection_params(/* use_bnb= */ false, /* change_output_size= */ 0,
2+                                          /* change_spend_size= */ 0, /* effective_fee= */ CFeeRate(0),
3+                                          /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
4+                                          /* tx_noinputs_size= */ 0, /* avoid_partial= */ false);

c++20 will bring designated initializers, which make initializing large structs like this much more readable.

achow101 commented at 5:01 pm on March 16, 2021:

I’ve tried to make it more readable. However I also think that setting all of these things should be set in the constructor and that the current thing where we set all the parameters ad-hoc is not a good design.

fjahr commented at 2:03 am on March 14, 2021: member

Code review ACK 7b4b48f6aa186d878984ae14727e5fa203f0d9b6

The code is definitely an improvement and can be merged as-is.

Looking at #19229 I was skeptical initially that this would fix it fully because, even though nobody said It explicitly, I assume some people have tried running the command multiple times. It sounded like it was a particular tx that caused these issues, rather than it just happening inconsistently on different txs which would seem more reasonable if a volatile fee market is the cause for this. But I have stared at the code for a pretty long time now (particularly the subtractfeefromamount scenario because that was mentioned several times in #19229) and I couldn’t find another edge case that might cause this error. So I think the best path forward is to try and see if this code fixes the issue for those that have observed it. At least one user has already indicated in #19229 that they didn’t see the issue again after running this PR so that’s a good sign.

chappjc commented at 4:17 pm on March 15, 2021: none

even though nobody said It explicitly, I assume some people have tried running the command multiple times

That’s correct, @fjahr, we have observed that a retry can still be met with the error. Bad luck maybe?

If a 0.21 backport branch were available, I might be able to try this out.

wallet: Use existing feerate instead of getting a new one

During each loop of CreateTransaction, instead of constantly getting a
new feerate, use the feerate that we have already fetched for all
fee calculations. Thix fixes a race condition where the feerate required
changes during each iteration of the loop.

This commit changes behavior as the "Fee estimation failed" error will
now take priority over "Signing transaction failed".

1a6a0b0dfb

wallet: Replace nFeeRateNeeded with effective_fee

Make sure that all fee calculations use the same feerate.
coin_selection_params.effective_fee is the variable we use for all fee
calculations, so get rid of remaining nFeeRateNeeded usages and just
directly set coin_selection_params.effective_fee.

Does not change behavior.

e2f429e6bb

achow101 force-pushed on Mar 16, 2021

achow101 commented at 5:43 pm on March 16, 2021: member

@chappjc Here is a branch of this PR backported to 0.21: https://github.com/achow101/bitcoin/tree/0.21-createtx-same-feerate

in src/wallet/wallet.cpp:2820 in cb93fbda15 outdated

2819                 return false;
2820             }
2821 
2822+            // Get long term estimate
2823+            CCoinControl cc_temp;
2824+            cc_temp.m_confirm_target = 1008;

glozow commented at 7:21 pm on March 16, 2021:

Since you’re touching this, seems appropriate to have magic numbers like this be static const

achow101 commented at 8:39 pm on March 16, 2021:

Changed to use the function we have to fetch the maximum confirmation target.

Xekyo commented at 7:26 pm on March 16, 2021: member

Code Review ACK (https://github.com/bitcoin/bitcoin/pull/21083/commits/cb93fbda152fe7427502b41ad897f40be13fc33b)

Great improvement! Please do rename the effective_fee to effective_feerate, though, if you have to update again.

in src/wallet/wallet.h:611 in cb93fbda15 outdated

606@@ -607,16 +607,21 @@ struct CoinSelectionParams
607     size_t change_output_size = 0;
608     size_t change_spend_size = 0;
609     CFeeRate effective_fee = CFeeRate(0);
610+    CFeeRate m_long_term_feerate = CFeeRate(0);
611+    CFeeRate m_discard_feerate = CFeeRate(0);

glozow commented at 7:31 pm on March 16, 2021:

CFeeRate default constructor gives it a 0, so this is not needed in the declaration

0    CFeeRate m_long_term_feerate;
1    CFeeRate m_discard_feerate;

achow101 commented at 8:39 pm on March 16, 2021:

done

glozow commented at 7:45 pm on March 16, 2021: member

utACK https://github.com/bitcoin/bitcoin/commit/cb93fbda152fe7427502b41ad897f40be13fc33b

I think this is a good change but actually could go a lot further, heh. In my opinion everything in CoinSelectionParams should be const and grabbed before coin selection begins. And a second attempt (e.g. not using bnb anymore) can be a copy/move of the first set of params.

wallet: Move long term feerate setting to CreateTransaction

Instead of setting the long term feerate for each SelectCoinsMinConf
iteration, set it once during CreateTransaction and let it be shared
with each SelectCoinsMinConf through
coin_selection_params.m_long_term_feerate.

Does not change behavior.

448d04b931

wallet: Move discard feerate fetching to CreateTransaction

Instead of fetching the discard feerate for each SelectCoinsMinConf
iteration, fetch and cache it once during CreateTransaction so that it
is shared for each SelectCoinsMinConf through
coin_selection_params.m_discard_feerate.

Does not change behavior.

bdd0c2934b

achow101 force-pushed on Mar 16, 2021

Rename CoinSelectionParams::effective_fee to m_effective_feerate

It's a feerate, not a fee. Also follow the style guide for member names.

f9cd2bfbcc

in src/wallet/test/coinselector_tests.cpp:39 in 3e77fbc3ee outdated

34@@ -35,7 +35,10 @@ static CAmount balance = 0;
35 CoinEligibilityFilter filter_standard(1, 6, 0);
36 CoinEligibilityFilter filter_confirmed(1, 1, 0);
37 CoinEligibilityFilter filter_standard_extra(6, 6, 0);
38-CoinSelectionParams coin_selection_params(false, 0, 0, CFeeRate(0), 0, false);
39+CoinSelectionParams coin_selection_params(/* use_bnb= */ false, /* change_output_size= */ 0,
40+                                          /* change_spend_size= */ 0, /* effective_fee= */ CFeeRate(0),

glozow commented at 9:14 pm on March 16, 2021:

missed one 👀

0                                          /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(0),

achow101 commented at 9:17 pm on March 16, 2021:

Oops. Fixed.

achow101 force-pushed on Mar 16, 2021

in src/wallet/wallet.h:622 in f9cd2bfbcc

620+                        CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) :
621         use_bnb(use_bnb),
622         change_output_size(change_output_size),
623         change_spend_size(change_spend_size),
624-        effective_fee(effective_fee),
625+        m_effective_feerate(effective_feerate),

Xekyo commented at 9:28 pm on March 16, 2021:

Should perhaps all of these now start with m_?

glozow commented at 9:34 pm on March 16, 2021:

👍 Good for followup?

achow101 commented at 9:35 pm on March 16, 2021:

They should, but that’s an unrelated change. The newly added members use m_.

Xekyo commented at 9:30 pm on March 16, 2021: member

tACK https://github.com/bitcoin/bitcoin/pull/21083/commits/f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3

glozow commented at 9:35 pm on March 16, 2021: member

reACK https://github.com/bitcoin/bitcoin/commit/f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3

Changes since last review:

Rename effective_fee to m_effective_feerate
Remove unnecessary CFeeRate(0)s in declaration
Grab long term target from longStats instead of hard-coding 1008

fjahr commented at 10:04 pm on March 16, 2021: member

Code review re-ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3

Checked range-diff since last review.

meshcollider commented at 0:14 am on March 17, 2021: contributor

Code review + test run ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3

meshcollider merged this on Mar 17, 2021

meshcollider closed this on Mar 17, 2021

meshcollider removed this from the "Blockers" column in a project

sidhujag referenced this in commit f13672e13b on Mar 17, 2021

fjahr commented at 0:01 am on March 18, 2021: member

even though nobody said It explicitly, I assume some people have tried running the command multiple times

That’s correct, @fjahr, we have observed that a retry can still be met with the error. Bad luck maybe?

If a 0.21 backport branch were available, I might be able to try this out. @chappjc please let us still know how it’s going. Thanks!

chappjc commented at 7:58 pm on March 18, 2021: none

I haven’t been able to test in our use case, but incidentally the error has been rare lately even with release. I will provide feed back when possible.

fanquake commented at 4:13 am on March 24, 2021: member

Here is a branch of this PR backported to 0.21: https://github.com/achow101/bitcoin/tree/0.21-createtx-same-feerate @achow101 did you want to open a PR to 0.21 with this branch?

achow101 referenced this in commit 48fc675163 on Mar 24, 2021

achow101 referenced this in commit 34c89f92f3 on Mar 24, 2021

achow101 referenced this in commit bcd716670b on Mar 24, 2021

achow101 referenced this in commit 5fc381e443 on Mar 24, 2021

achow101 referenced this in commit 4b7d19d456 on Mar 24, 2021

achow101 commented at 4:50 am on March 24, 2021: member

@achow101 did you want to open a PR to 0.21 with this branch?

Done in #21520

fanquake removed the label Needs backport (0.21) on Mar 31, 2021

achow101 referenced this in commit d61fb07da7 on Apr 1, 2021

fanquake referenced this in commit 0fe5b6130c on Apr 16, 2021

DrahtBot locked this on Aug 16, 2022

wallet: Avoid requesting fee rates multiple times during coin selection #21083

Conflicts