The m_value used for the target calculation in ApproximateBestSubset is incorrect, it should be GetSelectionAmount. This causes a bug that is only apparent when the minimum relay fee is set to be very high.
A test case is added for this, in addition to an assert in CreateTransactionInternal that would have also caught this issue if someone were able to hit the edge case.
Fixes #22670
Failure in the previous releases CI. Similar to #18737 / #18737:
0 test 2021-08-12T02:39:40.038000Z TestFramework (ERROR): Assertion failed
1 Traceback (most recent call last):
2 File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
3 self.run_test()
4 File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_fee_estimation.py", line 271, in run_test
5 check_estimates(self.nodes[1], self.fees_per_kb)
6 File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_fee_estimation.py", line 155, in check_estimates
7 check_smart_estimates(node, fees_seen)
8 File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_fee_estimation.py", line 145, in check_smart_estimates
9 % (feerate, last_feerate))
10 AssertionError: Estimated fee (0.000238) larger than last fee (0.000201) for lower number of confirms
11 test 2021-08-12T02:39:40.040000Z TestFramework (DEBUG): Closing down network thread
12 test 2021-08-12T02:39:40.041000Z TestFramework (INFO): Stopping nodes
13 test 2021-08-12T02:39:40.041000Z TestFramework.node0 (DEBUG): Stopping node
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
991+ # than any single input available, and require more than 1 input. So we make 3 outputs
992+ for i in range(0, 3):
993+ funds.sendtoaddress(tester.getnewaddress(address_type="bech32"), 1)
994+ self.nodes[0].generate(1)
995+
996+ # With 3 outputs of 1 BTC, the target value must between 1 and 2 BTC. We choose a value where with 2 inputs, the
can this be done programmatically?
blocks of comments means the chances of the description and/or the test going stale quite high
in other words, can we get size estimations, calculate the final total, round properly, using python rather than english?
978+ # that this bug is not detected. It is most easily observed when the minimum relay fee is set to
979+ # an extremely high value (0.1 BTC/kvB in this test).
980+ self.log.info("Test issue 22670 ApproximateBestSubset bug")
981+ # Make sure the default wallet will not be loaded when restarted with a high minrelaytxfee
982+ self.nodes[0].unloadwallet(self.default_wallet_name, False)
983+ self.restart_node(0, ["-minrelaytxfee=0.1", "-changetype=bech32"])
DEFAULT_MAX_RAW_TX_FEE_RATE constant and use that so you can refer to it later as well?
777@@ -778,6 +778,10 @@ bool CWallet::CreateTransactionInternal(
778 fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
779 }
780
781+ // The only time that fee_needed should be less than the amount available for fees (in change_and_fee - change_amount) is when
782+ // we are subtracting the fee from the outputs. If this occurs at any other time, it is a bug.
783+ assert(coin_selection_params.m_subtract_fee_outputs || fee_needed <= change_and_fee - change_amount);
am I correct in noting that change_and_fee - change_amount may be non-zero in coin_selection_params.m_subtract_fee_outputs instance in the case where change is dropped to fee?
just thinking if any tighter assertion can be placed.
m_subtract_fee_outputs, change_and_fee - change_amount can be positive, negative, or 0. It’s pretty much a useless value in that case.
To be clear, the bug is that it is using the output value, even if we’re not subtracting fee from outputs. We use GetSelectionAmount to switch between effective and output value as our target based on subtracting fee from outputs or not.
This explanation should go in the commit message, or at least the OP to get pulled in on merge.
For target value calculations, GetSelectionAmount should be used, not
m_effective_value or m_value.
Specifically, ApproximateBestSubset mistakenly uses m_value when
calculating whether the target value has been met. This has been changed
to use GetSelectionAmount.
When the fee is not subtracted from the outputs, the amount that has
been reserved for the fee (change_and_fee - change_amount) must be
enough to cover the fee that is needed. It would be a bug to not do so,
so use an assert to make this obvious if such a situation were to occur.
ApproximateBestSubset had an edge case (due to not using
GetSelectionAmount) where it was possible for it to return success but
fail to select enough to cover transaction fees. A test is added that
could trigger this failure prior to the fix being implemented.
To be clear, the bug is that it is using the output value, even if we’re not subtracting fee from outputs. We use
GetSelectionAmountto switch between effective and output value as our target based on subtracting fee from outputs or not.This explanation should go in the commit message, or at least the OP to get pulled in on merge.
I’ve added a specific explanation in the first commit
utACK 6e6a7e1fe874f0d93de7254d0884a1b9a95f6b19
Verified the test fails on master but passes with this PR
998+ # Create transactions in order to calculate fees for the target bounds that can trigger this bug
999+ change_tx = tester.fundrawtransaction(tester.createrawtransaction([], [{funds.getnewaddress(): 1.5}]))
1000+ tx = tester.createrawtransaction([], [{funds.getnewaddress(): 2}])
1001+ no_change_tx = tester.fundrawtransaction(tx, {"subtractFeeFromOutputs": [0]})
1002+
1003+ overhead_fees = feerate * len(tx) / 2 / 1000
1018+ # but not enough to need 3 inputs when not considering the fee.
1019+ # So the target value must be at least 2.00000001 - fee.
1020+ lower_bound = Decimal("2.00000001") - fees
1021+ # The target value must be at most 2 - cost_of_change - not_input_fees - min_change (these are all
1022+ # included in the target before ApproximateBestSubset).
1023+ upper_bound = Decimal("2.0") - cost_of_change - overhead_fees - Decimal("0.01")
782+ // we are subtracting the fee from the outputs. If this occurs at any other time, it is a bug.
783+ assert(coin_selection_params.m_subtract_fee_outputs || fee_needed <= change_and_fee - change_amount);
784+
785 // Update nFeeRet in case fee_needed changed due to dropping the change output
786 if (fee_needed <= change_and_fee - change_amount) {
787 nFeeRet = change_and_fee - change_amount;
nit: this logic looks a little bit funny now. I think it might make more sense to have modified this if block as
0 if (fee_needed <= change_and_fee - change_amount) {
1 nFeeRet = change_and_fee - change_amount;
2 } else {
3  // The only time that fee_needed should be less than the amount available for fees (in change_and_fee - change_amount) is when
4  // we are subtracting the fee from the outputs. If this occurs at any other time, it is a bug.
5 assert(coin_selection_params.m_subtract_fee_outputs);
6 }
which is logically equivalent but doesn’t do the same check twice
Labels
Wallet
Milestone
22.0