wallet: Use GetSelectionAmount in ApproximateBestSubset #22686

achow101 · 2021-08-12T02:16:05Z

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

achow101 · 2021-08-12T02:16:48Z

For 22.0 backport (this bug may cause other issues that are not as immediately obvious).

fanquake · 2021-08-12T03:11:21Z

Failure in the previous releases CI. Similar to #18737 / #18737:

 test  2021-08-12T02:39:40.038000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
                                       self.run_test()
                                     File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_fee_estimation.py", line 271, in run_test
                                       check_estimates(self.nodes[1], self.fees_per_kb)
                                     File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_fee_estimation.py", line 155, in check_estimates
                                       check_smart_estimates(node, fees_seen)
                                     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
                                       % (feerate, last_feerate))
                                   AssertionError: Estimated fee (0.000238) larger than last fee (0.000201) for lower number of confirms
 test  2021-08-12T02:39:40.040000Z TestFramework (DEBUG): Closing down network thread 
 test  2021-08-12T02:39:40.041000Z TestFramework (INFO): Stopping nodes 
 test  2021-08-12T02:39:40.041000Z TestFramework.node0 (DEBUG): Stopping node

DrahtBot · 2021-08-12T19:18:15Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22049 (rpc: allow specifying min chain depth for inputs in fund calls by champo)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs 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.

fanquake · 2021-08-13T00:30:55Z

cc @xekyo

instagibbs

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.

test/functional/rpc_fundrawtransaction.py

src/wallet/spend.cpp

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.

achow101 · 2021-08-13T04:35:16Z

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.

I've added a specific explanation in the first commit

meshcollider

utACK 6e6a7e1

Verified the test fails on master but passes with this PR

instagibbs · 2021-08-15T07:12:46Z

utACK 92885c4

murchandamus · 2021-08-18T19:18:53Z

Post-merge utACK 92885c4

fanquake · 2021-08-19T00:00:08Z

@xekyo thanks for following up.

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. Github-Pull: bitcoin#22686 Rebased-From: 2de222c

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

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. Github-Pull: bitcoin#22686 Rebased-From: 92885c4

hebasto · 2021-08-19T07:51:47Z

Backported in #22629.

S3RK

See #22742

S3RK · 2021-08-19T08:14:06Z

test/functional/rpc_fundrawtransaction.py

+        tx = tester.createrawtransaction([], [{funds.getnewaddress(): 2}])
+        no_change_tx = tester.fundrawtransaction(tx, {"subtractFeeFromOutputs": [0]})
+
+        overhead_fees = feerate * len(tx) / 2 / 1000


Why do we divide by 2 here?

To get the byte length of the hex string.

S3RK · 2021-08-19T08:14:33Z

test/functional/rpc_fundrawtransaction.py

+        lower_bound = Decimal("2.00000001") - fees
+        # The target value must be at most 2 - cost_of_change - not_input_fees - min_change (these are all
+        # included in the target before ApproximateBestSubset).
+        upper_bound = Decimal("2.0") - cost_of_change - overhead_fees - Decimal("0.01")


The test also passes with much higher upper_bound.

The lower and upper bounds are the minimum and maximum values that cause this test to fail prior to this PR. So a higher upper bound should still pass after this PR, and all values in between should pass after this PR.

8dcbbbe test: fix bug in 22686 (S3RK) Pull request description: It seems there is a bug in the test in bitcoin#22686, the code behaviour itself looks correct. Instead of verifying the scenario from bitcoin#22670 with both `upper_bound` and `lower_bound` for the transaction amount, the tests verified `lower_bound` two times. This fix is to properly use function parameter instead of a variable from the scope. The test still passes with both values, so no code changes are required. ACKs for top commit: achow101: ACK 8dcbbbe Tree-SHA512: 82837b2e00bd075a7b6b7a31031f4d3ba1ab4bd5967e90488f527fcc618aeb3945d6ae3ed66b9eb11616dda3ef376c5001559d2a7a79a759fc6800358bf52537