[24.x] wallet: bugfix, double-counted preset inputs during Coin Selection #26559
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e set The loop break shouldn't have being there.
Here I confuse the wallet to make it select the preset inputs twice during the coin selection process (on v24.0). Making it think that the selection process result covers the entire tx target when it does not. It's actually creating a tx that sends more coins than what inputs are covering for. Which, combined with the subtract fee from outputs option, makes the wallet generate and broadcast a tx that pays up to the sum of all the inputs, minus one, amounts in fee! ---------- Note: To make review simpler, have coded the same test cases twice. First as a unit test and secondly, inside a functional test too.
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
maflcko
changed the title
furszy
changed the title
furszy
changed the title
Contributor
|
Code Review ACK 8ef560a |
Member
|
|
Member
furszy
changed the title
achow101
added a commit
to bitcoin-core/gui
that referenced
this pull request
Dec 5, 2022
…ed total amount 7362f8e refactor: make CoinsResult total amounts members private (furszy) 3282fad wallet: add assert to SelectionResult::Merge for safety (S3RK) c4e3b7d wallet: SelectCoins, return early if wallet's UTXOs cannot cover the target (furszy) cac2725 test: bugfix, coinselector_test, use 'CoinsResult::Erase/Add' instead of direct member access (furszy) cf79384 test: Coin Selection, duplicated preset inputs selection (furszy) 341ba7f test: wallet, coverage for CoinsResult::Erase function (furszy) f930aef wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy) Pull request description: This comes with #26559. Solving few bugs inside the wallet's transaction creation process and adding test coverage for them. Plus, making use of the `CoinsResult::total_amount` cached value inside the Coin Selection process to return early if we don't have enough funds to cover the target amount. ### Bugs 1) The `CoinsResult::Erase` method removes only one output from the available coins vector (there is a [loop break](https://github.com/bitcoin/bitcoin/blob/c1061be14a515b0ed4f4d646fcd0378c62e6ded3/src/wallet/spend.cpp#L112) that should have never been there) and not all the preset inputs. Which on master is not a problem, because since [#25685](bitcoin/bitcoin#25685) we are no longer using the method. But, it's a bug on v24 (check [#26559](bitcoin/bitcoin#26559)). This method it's being fixed and not removed because I'm later using it to solve another bug inside this PR. 2) As we update the total cached amount of the `CoinsResult` object inside `AvailableCoins` and we don't use such function inside the coin selection tests (we manually load up the `CoinsResult` object), there is a discrepancy between the outputs that we add/erase and the total amount cached value. ### Improvements * This makes use of the `CoinsResult` total amount field to early return with an "Insufficient funds" error inside Coin Selection if the tx target amount is greater than the sum of all the wallet available coins plus the preset inputs amounts (we don't need to perform the entire coin selection process if we already know that there aren't enough funds inside our wallet). ### Test Coverage 1) Adds test coverage for the duplicated preset input selection bug that we have in v24. Where the wallet invalidly selects the preset inputs twice during the Coin Selection process. Which ends up with a "good" Coin Selection result that does not cover the total tx target amount. Which, alone, crashes the wallet due an insane fee. But.. to make it worst, adding the subtract fee from output functionality to this mix ends up with the wallet by-passing the "insane" fee assertion, decreasing the output amount to fulfill the insane fee, and.. sadly, broadcasting the tx to the network. 2) Adds test coverage for the `CoinsResult::Erase` method. ------------------------------------ TO DO: * [ ] Update [#26559 ](bitcoin/bitcoin#26559) description. ACKs for top commit: achow101: ACK 7362f8e glozow: ACK 7362f8e, I assume there will be a followup PR to add coin selection sanity checks and we can discuss the best way to do that there. josibake: ACK [7362f8e](bitcoin/bitcoin@7362f8e) Tree-SHA512: 37a6828ea10d8d36c8d5873ceede7c8bef72ae4c34bef21721fa9dad83ad6dba93711c3170a26ab6e05bdbc267bb17433da08ccb83b82956d05fb16090328cba
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Dec 5, 2022
… amount 7362f8e refactor: make CoinsResult total amounts members private (furszy) 3282fad wallet: add assert to SelectionResult::Merge for safety (S3RK) c4e3b7d wallet: SelectCoins, return early if wallet's UTXOs cannot cover the target (furszy) cac2725 test: bugfix, coinselector_test, use 'CoinsResult::Erase/Add' instead of direct member access (furszy) cf79384 test: Coin Selection, duplicated preset inputs selection (furszy) 341ba7f test: wallet, coverage for CoinsResult::Erase function (furszy) f930aef wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy) Pull request description: This comes with bitcoin#26559. Solving few bugs inside the wallet's transaction creation process and adding test coverage for them. Plus, making use of the `CoinsResult::total_amount` cached value inside the Coin Selection process to return early if we don't have enough funds to cover the target amount. ### Bugs 1) The `CoinsResult::Erase` method removes only one output from the available coins vector (there is a [loop break](https://github.com/bitcoin/bitcoin/blob/c1061be14a515b0ed4f4d646fcd0378c62e6ded3/src/wallet/spend.cpp#L112) that should have never been there) and not all the preset inputs. Which on master is not a problem, because since [bitcoin#25685](bitcoin#25685) we are no longer using the method. But, it's a bug on v24 (check [bitcoin#26559](bitcoin#26559)). This method it's being fixed and not removed because I'm later using it to solve another bug inside this PR. 2) As we update the total cached amount of the `CoinsResult` object inside `AvailableCoins` and we don't use such function inside the coin selection tests (we manually load up the `CoinsResult` object), there is a discrepancy between the outputs that we add/erase and the total amount cached value. ### Improvements * This makes use of the `CoinsResult` total amount field to early return with an "Insufficient funds" error inside Coin Selection if the tx target amount is greater than the sum of all the wallet available coins plus the preset inputs amounts (we don't need to perform the entire coin selection process if we already know that there aren't enough funds inside our wallet). ### Test Coverage 1) Adds test coverage for the duplicated preset input selection bug that we have in v24. Where the wallet invalidly selects the preset inputs twice during the Coin Selection process. Which ends up with a "good" Coin Selection result that does not cover the total tx target amount. Which, alone, crashes the wallet due an insane fee. But.. to make it worst, adding the subtract fee from output functionality to this mix ends up with the wallet by-passing the "insane" fee assertion, decreasing the output amount to fulfill the insane fee, and.. sadly, broadcasting the tx to the network. 2) Adds test coverage for the `CoinsResult::Erase` method. ------------------------------------ TO DO: * [ ] Update [bitcoin#26559 ](bitcoin#26559) description. ACKs for top commit: achow101: ACK 7362f8e glozow: ACK 7362f8e, I assume there will be a followup PR to add coin selection sanity checks and we can discuss the best way to do that there. josibake: ACK [7362f8e](bitcoin@7362f8e) Tree-SHA512: 37a6828ea10d8d36c8d5873ceede7c8bef72ae4c34bef21721fa9dad83ad6dba93711c3170a26ab6e05bdbc267bb17433da08ccb83b82956d05fb16090328cba
Member
|
I've now included this in #26616. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here I make the wallet select the preset inputs
twice during the coin selection process (on v24.0).
Making it think that the selection process result covers
the entire tx target when it does not. It's actually creating
a tx that sends more coins than what inputs are covering for.
Which, combined with the SFFO option, makes the wallet
incorrectly reduce the tx recipient's amount by the difference
between the original target and the double-counted inputs.
Which means, a created and relayed tx sending less coins to
the destination than what the user inputted.
Technical Explanation
The reason why this happens is that the
CoinsResult::Erasemethod is removing only one output from the available coins
vector (there is a loop break that should have never been
there) and not all the preset inputs.
Which makes the Coin Selection process receive the preset
inputs inside the selectable coins and inside the preset
inputs vector.
Which is not good, because we merge the preset inputs inside
the automatic Coin Selection result manually at the end of the
process, so this result can already contain the preset inputs.
So.. this ends up with the Coin Selection result having less
inputs selected and not covering the entire target amount.
Which, alone, makes the wallet crash inside the "insane fee" assertion.
But.. to make it worst, adding the subtract fee from output functionality
to this mix ends up with the wallet by-passing the "insane" fee assertion,
decreasing the output amount to fulfill the insane fee, and.. sadly,
broadcasting the tx, with a reduced recipient's amount, to the network.
Important Note
This is only a problem for v24.0 and not in master.
Since #25685, we no longer use the
CoinsResult::Erasemethod insidethe Coin Selection Process.