diff options
| author | fanquake <fanquake@gmail.com> | 2021-08-16 11:00:33 +0800 |
|---|---|---|
| committer | fanquake <fanquake@gmail.com> | 2021-08-16 11:23:53 +0800 |
| commit | 820129aee9fdd0b3f238e9feef60330342d61966 (patch) | |
| tree | 78db04821e506a1977b39c700340172224644a29 /src/wallet | |
| parent | 11c7d001a9c3b600ee47d5851054e8d5c01a3de1 (diff) | |
| parent | 92885c4f69a5e6fc4989677d6e5be8a666fbff0d (diff) | |
| download | bitcoin-820129aee9fdd0b3f238e9feef60330342d61966.tar.xz | |
Merge bitcoin/bitcoin#22686: wallet: Use GetSelectionAmount in ApproximateBestSubset
92885c4f69a5e6fc4989677d6e5be8a666fbff0d test: Test for ApproximateBestSubset edge case with too little fees (Andrew Chow)
d9262324e80da32725e21d4e0fbfee56f25490b1 wallet: Assert that enough was selected to cover the fees (Andrew Chow)
2de222c40198d3d528668d78cc52e2ce3fa96765 wallet: Use GetSelectionAmount for target value calculations (Andrew Chow)
Pull request description:
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
ACKs for top commit:
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/22686/commits/92885c4f69a5e6fc4989677d6e5be8a666fbff0d
Tree-SHA512: bd61fa61ffb60873e097737eebea3afe8a42296ba429de9038b3a4706763b34de9409de6cdbab21ff7f51f4787b503f840873182d9c4a1d6e12a54b017953547
Diffstat (limited to 'src/wallet')
| -rw-r--r-- | src/wallet/coinselection.cpp | 4 | ||||
| -rw-r--r-- | src/wallet/spend.cpp | 4 |
2 files changed, 6 insertions, 2 deletions
diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 6d502e1df1..25b1ee07e4 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -195,7 +195,7 @@ static void ApproximateBestSubset(const std::vector<OutputGroup>& groups, const //the selection random. if (nPass == 0 ? insecure_rand.randbool() : !vfIncluded[i]) { - nTotal += groups[i].m_value; + nTotal += groups[i].GetSelectionAmount(); vfIncluded[i] = true; if (nTotal >= nTargetValue) { @@ -205,7 +205,7 @@ static void ApproximateBestSubset(const std::vector<OutputGroup>& groups, const nBest = nTotal; vfBest = vfIncluded; } - nTotal -= groups[i].m_value; + nTotal -= groups[i].GetSelectionAmount(); vfIncluded[i] = false; } } diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index cd51ead539..3bb7134b24 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -778,6 +778,10 @@ bool CWallet::CreateTransactionInternal( fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); } + // The only time that fee_needed should be less than the amount available for fees (in change_and_fee - change_amount) is when + // we are subtracting the fee from the outputs. If this occurs at any other time, it is a bug. + assert(coin_selection_params.m_subtract_fee_outputs || fee_needed <= change_and_fee - change_amount); + // Update nFeeRet in case fee_needed changed due to dropping the change output if (fee_needed <= change_and_fee - change_amount) { nFeeRet = change_and_fee - change_amount; |