wallet: Move fee underpayment check to after all fee has been set #26643

achow101 · 2022-12-05T20:41:39Z

Currently the fee underpayment check occurs right after we calculate what the transaction's fee should be. However the fee paid by the transaction at that time does not always match. Notably, when doing SFFO, the fee paid at that time will almost always be less than the fee required, which then required having a bypass of the underpayment check that results in SFFO payments going through when they should not.

This PR moves the underpayment check to after fees have been finalized so that we always check whether the fee is being underpaid. This removes the exception for SFFO and unifies this behavior for both SFFO and non-SFFO txs.

DrahtBot · 2022-12-05T20:41:42Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, S3RK, glozow
Stale ACK	john-moffett

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26661 (wallet: Coin Selection, return accurate error messages by furszy)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction 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.

S3RK · 2022-12-06T08:23:01Z

src/wallet/spend.cpp

@@ -952,19 +958,6 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
    CAmount fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
    CAmount current_fee = result->GetSelectedValue() - recipients_sum - change_amount;


Should we define it the same way as down below?

-CAmount current_fee = result->GetSelectedValue() - recipients_sum - change_amount; +CAmount current_fee = result->GetSelectedValue() - CalculateOutputValue(txNew);

S3RK · 2022-12-06T08:23:27Z

src/wallet/spend.cpp

@@ -756,6 +757,11 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng
    }
 }

+static CAmount CalculateOutputValue(const CMutableTransaction& tx)


Have you considered making it a member of CMutableTransaction?

nFeeRet represents the fee that the transaction currently pays. Update it's name to reflect that.

john-moffett

ACK 212e149

src/wallet/spend.cpp

furszy · 2022-12-08T21:04:00Z

src/wallet/spend.cpp

    // Reduce output values for subtractFeeFromAmount
    if (coin_selection_params.m_subtract_fee_outputs) {
-        CAmount to_reduce = fee_needed - nFeeRet;
+        CAmount to_reduce = fee_needed - current_fee;


This is probably too much but.. could fee_needed be lower than current_fee?
Because.. if that happens, to_reduce will be negative so instead of reducing value from the outputs, we would be adding it.

Yes, with a sufficiently large change output script and feerate, this is possible, and (currently) expected behavior. We test for it at spend_tests.cpp:61.

Interesting. That is the opposite to what someone would expect from a functionality called "subtract fee from outputs".
but.. all good anyway, something for later.

furszy

There is something that is making some noise inside me.

Based on the convo that we had above #26643 (comment).

Inside 8c8e173, you moved the change output increment due a fees overpay below the SFFO functionality. So, now if there is a fee surplus, instead of adding it to the change output first, we will add it to the recipients amount if SFFO is enabled.
(if there is an extra fee, "fee needed" will be lower than "current fee" so the SFFO to_reduce amount will be negative, so instead of subtract the amount we will add it to the recipients amount. Sending more coins than what the user specified to send to each SFFO enabled output instead of sending them back to us in the change output).

Should create a test for this (the current SubtractFee tests aren't contemplating change). It smells bad.

It doesn't make sense to be checking whether the fee paid is underpaying before we've finished setting the fees. So do that after we have done the reduction for SFFO and change adjustment for fee overpayment.

We need to check that the fee is not negative even before it is finalized. The setting of fees for SFFO may adjust the fee to be "correct" and no longer negative, but erroneously reduce the amounts too far. So we need to check this condition before we do those adjustments.

achow101 · 2022-12-09T20:15:50Z

Inside 8c8e173, you moved the change output increment due a fees overpay below the SFFO functionality. So, now if there is a fee surplus, instead of adding it to the change output first, we will add it to the recipients amount if SFFO is enabled.
(if there is an extra fee, "fee needed" will be lower than "current fee" so the SFFO to_reduce amount will be negative, so instead of subtract the amount we will add it to the recipients amount. Sending more coins than what the user specified to send to each SFFO enabled output instead of sending them back to us in the change output).

I've flipped it back around so the order stays the same.

However, I don't think the scenario you describe is even at all possible. The only times that fee_needed < current_fee is possible are if the change was small and thrown away as fees, or due to rounding errors in the fee estimates for coin selection. The the former case, the condition you mention cannot be reached because there is no change output. Because SFFO does not account for fees at all (it doesn't include any fees in the selection target and it only uses the real values during the selection algorithms), the latter case cannot be reached. Thus the scenario you describe is not possible.

furszy · 2022-12-10T00:24:34Z

ok great, thanks. The noice is no longer there.

Because SFFO does not account for fees at all (it doesn't include any fees in the selection target and it only uses the real values during the selection algorithms), the latter case cannot be reached. Thus the scenario you describe is not possible.

In that case, what about Assume(current_fee == 0) for SFFO?.

furszy

Code review ACK 798430d

S3RK · 2022-12-12T07:45:47Z

Code review ACK 798430d

glozow

utACK 798430d, code looks correct to me

… fee has been set 798430d wallet: Sanity check fee paid cannot be negative (Andrew Chow) c1a84f1 wallet: Move fee underpayment check to after fee setting (Andrew Chow) e5daf97 wallet: Rename nFeeRet in CreateTransactionInternal to current_fee (Andrew Chow) Pull request description: Currently the fee underpayment check occurs right after we calculate what the transaction's fee should be. However the fee paid by the transaction at that time does not always match. Notably, when doing SFFO, the fee paid at that time will almost always be less than the fee required, which then required having a bypass of the underpayment check that results in SFFO payments going through when they should not. This PR moves the underpayment check to after fees have been finalized so that we always check whether the fee is being underpaid. This removes the exception for SFFO and unifies this behavior for both SFFO and non-SFFO txs. ACKs for top commit: S3RK: Code review ACK 798430d furszy: Code review ACK 798430d glozow: utACK 798430d, code looks correct to me Tree-SHA512: 720e8a3dbdc9937b12ee7881eb2ad58332c9584520da87ef3080e6f9d6220ce8d3bd8b9317b4877e56a229113437340852976db8f64df0d5cc50723fa04b02f0

DrahtBot added the Wallet label Dec 5, 2022

DrahtBot mentioned this pull request Dec 6, 2022

wallet: Change coin selection fee assert to error #26611

Merged

S3RK reviewed Dec 6, 2022

View reviewed changes

This was referenced Dec 6, 2022

wallet: group outputs only once, decouple it from Coin Selection #25806

Merged

wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction #25273

Merged

DrahtBot added the Needs rebase label Dec 6, 2022

wallet: Rename nFeeRet in CreateTransactionInternal to current_fee

e5daf97

nFeeRet represents the fee that the transaction currently pays. Update it's name to reflect that.

achow101 force-pushed the move-fee-underpay-check branch from 6e73768 to 212e149 Compare December 6, 2022 21:58

achow101 marked this pull request as ready for review December 6, 2022 21:59

DrahtBot removed the Needs rebase label Dec 6, 2022

john-moffett approved these changes Dec 7, 2022

View reviewed changes

furszy reviewed Dec 8, 2022

View reviewed changes

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

furszy reviewed Dec 8, 2022

View reviewed changes

achow101 force-pushed the move-fee-underpay-check branch from 212e149 to 1f6b0fa Compare December 8, 2022 21:26

DrahtBot mentioned this pull request Dec 8, 2022

wallet: Coin Selection, return accurate error messages #26661

Merged

furszy reviewed Dec 9, 2022

View reviewed changes

achow101 added 2 commits December 9, 2022 14:52

wallet: Move fee underpayment check to after fee setting

c1a84f1

It doesn't make sense to be checking whether the fee paid is underpaying before we've finished setting the fees. So do that after we have done the reduction for SFFO and change adjustment for fee overpayment.

achow101 force-pushed the move-fee-underpay-check branch from 1f6b0fa to 798430d Compare December 9, 2022 20:10

furszy approved these changes Dec 10, 2022

View reviewed changes

fanquake requested review from john-moffett and S3RK and removed request for john-moffett December 10, 2022 10:27

fanquake requested a review from glozow December 12, 2022 11:42

glozow reviewed Dec 12, 2022

View reviewed changes

achow101 merged commit 8f30211 into bitcoin:master Dec 13, 2022

bitcoin locked and limited conversation to collaborators Dec 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: Move fee underpayment check to after all fee has been set #26643

wallet: Move fee underpayment check to after all fee has been set #26643

Uh oh!

achow101 commented Dec 5, 2022 •

edited

Loading

Uh oh!

DrahtBot commented Dec 5, 2022 •

edited

Loading

Uh oh!

S3RK Dec 6, 2022

Uh oh!

S3RK Dec 6, 2022

Uh oh!

john-moffett left a comment

Uh oh!

Uh oh!

furszy Dec 8, 2022

Uh oh!

achow101 Dec 8, 2022

Uh oh!

furszy Dec 9, 2022

Uh oh!

furszy left a comment •

edited

Loading

Uh oh!

achow101 commented Dec 9, 2022

Uh oh!

furszy commented Dec 10, 2022

Uh oh!

furszy left a comment

Uh oh!

S3RK commented Dec 12, 2022

Uh oh!

glozow left a comment

Uh oh!

Uh oh!

		@@ -952,19 +958,6 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
		CAmount fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
		CAmount current_fee = result->GetSelectedValue() - recipients_sum - change_amount;

wallet: Move fee underpayment check to after all fee has been set #26643

wallet: Move fee underpayment check to after all fee has been set #26643

Uh oh!

Conversation

achow101 commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

S3RK Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

S3RK Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

achow101 Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

furszy Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Dec 9, 2022

Uh oh!

furszy commented Dec 10, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

S3RK commented Dec 12, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Dec 5, 2022 •

edited

Loading

DrahtBot commented Dec 5, 2022 •

edited

Loading

furszy left a comment •

edited

Loading