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

achow101 · 2021-02-05T00:34:08Z

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 · 2021-02-05T07:59:26Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207 (MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky)
Add Single Random Draw as an additional coin selection algorithm #17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
Use effective values throughout coin selection #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 · 2021-02-06T18:08:18Z

Concept ACK, was thinking about this as well

meshcollider · 2021-02-16T22:02:35Z

Concept / light-look-through ACK

ryanofsky

Code review ACK 7b4b48f. 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!

ryanofsky · 2021-02-17T22:21:36Z

src/wallet/wallet.cpp

-                    return false;
-                }
-
+                nFeeNeeded = coin_selection_params.effective_fee.GetFee(nBytes);


In commit "wallet: Use existing feerate instead of getting a new one" (81b6a6b)

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

src/wallet/wallet.cpp

src/wallet/wallet.h

luke-jr · 2021-03-01T02:43:06Z

src/wallet/wallet.h

    size_t tx_noinputs_size = 0;
    //! Indicate that we are subtracting the fee from outputs
    bool m_subtract_fee_outputs = false;
    bool m_avoid_partial_spends = false;

-    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) :
+    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) :


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

See #21083 (comment)

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

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:

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

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

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 · 2021-03-14T02:03:10Z

Code review ACK 7b4b48f

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 · 2021-03-15T16:17:38Z

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.

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

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.

achow101 · 2021-03-16T17:43:43Z

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

murchandamus

Code Review ACK (cb93fbd)

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

src/wallet/wallet.cpp

glozow

utACK cb93fbd

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.

src/wallet/wallet.cpp

src/wallet/wallet.h

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.

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.

src/wallet/test/coinselector_tests.cpp

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

murchandamus

tACK f9cd2bf

murchandamus · 2021-03-16T21:28:48Z

src/wallet/wallet.h

        use_bnb(use_bnb),
        change_output_size(change_output_size),
        change_spend_size(change_spend_size),
-        effective_fee(effective_fee),
+        m_effective_feerate(effective_feerate),


Should perhaps all of these now start with m_?

👍 Good for followup?

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

glozow · 2021-03-16T21:35:12Z

reACK f9cd2bf

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 · 2021-03-16T22:04:40Z

Code review re-ACK f9cd2bf

Checked range-diff since last review.

meshcollider

Code review + test run ACK f9cd2bf

…s during coin selection

fjahr · 2021-03-18T00:01:56Z

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 · 2021-03-18T19:58:08Z

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 · 2021-03-24T04:13:38Z

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?

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". Github-Pull: bitcoin#21083 Rebased-From: 1a6a0b0

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. Github-Pull: bitcoin#21083 Rebased-From: e2f429e

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. Github-Pull: bitcoin#21083 Rebased-From: 448d04b

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. Github-Pull: bitcoin#21083 Rebased-From: bdd0c29

It's a feerate, not a fee. Also follow the style guide for member names. Github-Pull: bitcoin#21083 Rebased-From: f9cd2bf

achow101 · 2021-03-24T04:50:48Z

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

Done in #21520

It's a feerate, not a fee. Also follow the style guide for member names. Github-Pull: bitcoin#21083 Rebased-From: f9cd2bf

…s during coin selection d61fb07 Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow) 5fc381e wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow) bcd7166 wallet: Move long term feerate setting to CreateTransaction (Andrew Chow) 34c89f9 wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow) 48fc675 wallet: Use existing feerate instead of getting a new one (Andrew Chow) Pull request description: Backport of #21083 ACKs for top commit: MarcoFalke: cherry-pick-only re-ACK d61fb07 🔙 instagibbs: utACK d61fb07 Tree-SHA512: 23b212301bb467153dd9723903918ae01dd520525c81d541c411e7a4381e46594fe032e2a7c06ddcff7dc56dcb546991d50187c33fcff08ec45bd835cc01bd19

DrahtBot added the Wallet label Feb 5, 2021

This was referenced Feb 5, 2021

wallet, refactor: return out-params of CreateTransaction() as optional struct #20640

Merged

Add Single Random Draw as an additional coin selection algorithm #17526

Merged

Use effective values throughout coin selection #17331

Merged

maflcko mentioned this pull request Feb 10, 2021

Transaction fee and change calculation failed... using v0.21.0 binary #19229

Closed

This was referenced Feb 17, 2021

MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207

Merged

refactor: Make CWalletTx sync state type-safe #21206

Merged

ryanofsky approved these changes Feb 17, 2021

View reviewed changes

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

maflcko added this to the 0.21.1 milestone Feb 19, 2021

laanwj added this to Blockers in High-priority for review Feb 25, 2021

luke-jr reviewed Mar 1, 2021

View reviewed changes

DrahtBot mentioned this pull request Mar 4, 2021

rpc: include_unsafe option for fundrawtransaction #21359

Merged

achow101 added 2 commits March 16, 2021 12:30

achow101 force-pushed the createtx-same-feerate branch 2 times, most recently from cca8b00 to cb93fbd Compare March 16, 2021 17:27

murchandamus reviewed Mar 16, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

glozow reviewed Mar 16, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.h Outdated Show resolved Hide resolved

achow101 added 2 commits March 16, 2021 16:32

achow101 force-pushed the createtx-same-feerate branch from cb93fbd to 3e77fbc Compare March 16, 2021 20:39

glozow reviewed Mar 16, 2021

View reviewed changes

src/wallet/test/coinselector_tests.cpp Outdated Show resolved Hide resolved

Rename CoinSelectionParams::effective_fee to m_effective_feerate

f9cd2bf

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

achow101 force-pushed the createtx-same-feerate branch from 3e77fbc to f9cd2bf Compare March 16, 2021 21:17

murchandamus reviewed Mar 16, 2021

View reviewed changes

meshcollider reviewed Mar 17, 2021

View reviewed changes

meshcollider merged commit d25e28c into bitcoin:master Mar 17, 2021

meshcollider removed this from Blockers in High-priority for review Mar 17, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 17, 2021

Merge bitcoin#21083: wallet: Avoid requesting fee rates multiple time…

f13672e

…s during coin selection

achow101 added a commit to achow101/bitcoin that referenced this pull request Mar 24, 2021

Rename CoinSelectionParams::effective_fee to m_effective_feerate

4b7d19d

It's a feerate, not a fee. Also follow the style guide for member names. Github-Pull: bitcoin#21083 Rebased-From: f9cd2bf

achow101 mentioned this pull request Mar 24, 2021

[0.21] wallet: Avoid requesting fee rates multiple times during coin selection #21520

Merged

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

achow101 added a commit to achow101/bitcoin that referenced this pull request Apr 1, 2021

Rename CoinSelectionParams::effective_fee to m_effective_feerate

d61fb07

It's a feerate, not a fee. Also follow the style guide for member names. Github-Pull: bitcoin#21083 Rebased-From: f9cd2bf

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

achow101 commented Feb 5, 2021 •

edited

DrahtBot commented Feb 5, 2021 •

edited

jonatack commented Feb 6, 2021

meshcollider commented Feb 16, 2021

ryanofsky left a comment •

edited

ryanofsky Feb 17, 2021

luke-jr Mar 1, 2021 •

edited

achow101 Mar 4, 2021

jnewbery Mar 16, 2021

achow101 Mar 16, 2021

fjahr commented Mar 14, 2021

chappjc commented Mar 15, 2021

achow101 commented Mar 16, 2021

murchandamus left a comment •

edited

glozow left a comment

murchandamus left a comment

murchandamus Mar 16, 2021

glozow Mar 16, 2021

achow101 Mar 16, 2021

glozow commented Mar 16, 2021

fjahr commented Mar 16, 2021

meshcollider left a comment

fjahr commented Mar 18, 2021

chappjc commented Mar 18, 2021

fanquake commented Mar 24, 2021

achow101 commented Mar 24, 2021

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

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

Conversation

achow101 commented Feb 5, 2021 • edited

DrahtBot commented Feb 5, 2021 • edited

Conflicts

jonatack commented Feb 6, 2021

meshcollider commented Feb 16, 2021

ryanofsky left a comment • edited

Choose a reason for hiding this comment

ryanofsky Feb 17, 2021

Choose a reason for hiding this comment

luke-jr Mar 1, 2021 • edited

Choose a reason for hiding this comment

achow101 Mar 4, 2021

Choose a reason for hiding this comment

jnewbery Mar 16, 2021

Choose a reason for hiding this comment

achow101 Mar 16, 2021

Choose a reason for hiding this comment

fjahr commented Mar 14, 2021

chappjc commented Mar 15, 2021

achow101 commented Mar 16, 2021

murchandamus left a comment • edited

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

murchandamus left a comment

Choose a reason for hiding this comment

murchandamus Mar 16, 2021

Choose a reason for hiding this comment

glozow Mar 16, 2021

Choose a reason for hiding this comment

achow101 Mar 16, 2021

Choose a reason for hiding this comment

glozow commented Mar 16, 2021

fjahr commented Mar 16, 2021

meshcollider left a comment

Choose a reason for hiding this comment

fjahr commented Mar 18, 2021

chappjc commented Mar 18, 2021

fanquake commented Mar 24, 2021

achow101 commented Mar 24, 2021

achow101 commented Feb 5, 2021 •

edited

DrahtBot commented Feb 5, 2021 •

edited

ryanofsky left a comment •

edited

luke-jr Mar 1, 2021 •

edited

murchandamus left a comment •

edited