bumpfee: avoid making bumped transactions with too low fee when replacing outputs #27308

achow101 commented at 11:05 PM on March 22, 2023: member

When replacing the outputs of a transaction during bumpfee, it is possible to accidentally create a transaction that will not be accepted into the mempool as it does not meet the incremental relay fee requirements. This occurs because the size estimation used for checking the provided feerate does not account for the replaced outputs; it instead uses the original outputs. When the replaced outputs is significantly different from the original, there can be a large difference in estimated transaction sizes that can make a transaction miss the absolute fee requirements for the incremental relay fee. Unfortunately we do not currently inform the user when the bumped transaction fails to relay, so they could use bumpfee and think the transaction has been bumped when it actually has not.

This issue is resolved by replacing the outputs before doing the size estimation, and also updating the feerate checker to use the actual fee values when calculating the required minimum fee.

Also added a test for this scenario.

DrahtBot commented at 11:05 PM on March 22, 2023: contributor

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	ishaanam, Xekyo
Concept ACK	glozow
Stale ACK	furszy

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:

#27195 (bumpfee: allow send coins back to yourself by furszy)
#26467 (bumpfee: Allow the user to choose which output is change by achow101)
#26152 (Bump unconfirmed ancestor transactions to target feerate by Xekyo)
#25979 ([WIP] wallet: standardize change output detection process by furszy)

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.

glozow commented at 10:15 AM on March 23, 2023: member

concept ACK

glozow added the label Wallet on Mar 23, 2023

in src/wallet/feebumper.cpp:88 in a1e2cae80f outdated

  83 | @@ -84,15 +84,12 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CWalletTx& wt
  84 |  
  85 |      CFeeRate incrementalRelayFee = std::max(wallet.chain().relayIncrementalFee(), CFeeRate(WALLET_INCREMENTAL_RELAY_FEE));
  86 |  
  87 | -    // Given old total fee and transaction size, calculate the old feeRate
  88 | -    const int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));

furszy commented at 1:04 PM on March 26, 2023:

Missed to remove the wtx function's arg. It's not needed anymore.

achow101 commented at 2:13 PM on April 10, 2023:

Done

in src/wallet/feebumper.cpp:255 in a1e2cae80f outdated

 251 | @@ -254,6 +252,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
 252 |              txin.scriptSig.clear();
 253 |              txin.scriptWitness.SetNull();
 254 |          }
 255 | +        mtx.vout = txouts;

furszy commented at 1:06 PM on March 26, 2023:

while you are here, could rename mtx so it doesn't shadow the function's arg (or rename the argument).

achow101 commented at 2:13 PM on April 10, 2023:

Changed to temp_mtx

furszy approved

furszy commented at 1:33 PM on March 26, 2023: member

ACK 26b4c664

Was checking the same few days ago.

ishaanam commented at 11:27 PM on March 31, 2023: contributor

ACK 26b4c66418f97b4d13989a5c6cf2a7827af4fc1e

Edit: test failed on master as expected

in test/functional/wallet_bumpfee.py:673 in 26b4c66418 outdated

 669 | @@ -668,5 +670,30 @@ def test_no_more_inputs_fails(self, rbf_node, dest_address):
 670 |      self.clear_mempool()
 671 |  
 672 |  
 673 | +def test_feerate_checks_replaced_outputs(self, rbf_node):

murchandamus commented at 5:33 PM on April 7, 2023:

If I understand this test right, it checks whether the absolute fee of a smaller replacement transaction sufficiently surpasses that of a larger original transaction.

I looked around in wallet_bumpfee.py, and had the impression that we generally seem to assume that the replacement is the same size as the original. If we don’t check it yet, we should perhaps also add a test case where a larger replacement transaction supersedes a smaller original’s feerate sufficiently (although I might have overlooked that test case).

achow101 commented at 2:17 PM on April 10, 2023:

If I understand this test right, it checks whether the absolute fee of a smaller replacement transaction sufficiently surpasses that of a larger original transaction.

No, it doesn't check any feerates since we can't see the candidate bump transaction. The calculations here are just for figuring out a feerate that would fail the modified checks and checking that bumpfee fails as expected.

I looked around in wallet_bumpfee.py, and had the impression that we generally seem to assume that the replacement is the same size as the original. If we don’t check it yet, we should perhaps also add a test case where a larger replacement transaction supersedes a smaller original’s feerate sufficiently (although I might have overlooked that test case).

There are no size checks since we are not making specific bump transactions to test replacement behavior. Rather we checking whether the bumpfee command succeeds. This isn't a test of mempool replacement policy.

murchandamus commented at 5:37 PM on April 7, 2023: contributor

ACK 26b4c66418f97b4d13989a5c6cf2a7827af4fc1e

bumpfee: Check the correct feerate when replacing outputs

When doing the feerate check for bumped transactions that replace the
outputs, we need to consider that the size of the new outputs may be
different from the old outputs and calculate the minimum feerate accordingly.

be177c15a4

tests: Make sure that bumpfee feerate checks work when replacing outputs

When replacing the outputs of a transaction, we can end up with
fees that are drastically different from the original. This tests that
the feerate checks we perform will properly detect when the bumping tx
will have an insufficient feerate.

d52fa1b0a5

achow101 force-pushed on Apr 10, 2023

ishaanam commented at 10:24 PM on April 14, 2023: contributor

reACK d52fa1b0a5a8eecbe1e296a44b72965717e9235b

DrahtBot requested review from furszy on Apr 14, 2023

DrahtBot requested review from murchandamus on Apr 14, 2023

murchandamus commented at 6:01 AM on April 15, 2023: contributor

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

DrahtBot removed review request from murchandamus on Apr 15, 2023

fanquake merged this on Apr 15, 2023

fanquake closed this on Apr 15, 2023

sidhujag referenced this in commit 1ecf186c84 on Apr 17, 2023

bitcoin locked this on Apr 14, 2024