Skip to content

Conversation

furszy
Copy link
Member

@furszy furszy commented Apr 13, 2022

Fixes #23144.

We currently return a general "Insufficient funds" from Coin
Selection when we actually skipped unconfirmed UTXOs that
surpassed the mempool ancestors limit.

This PR make the error clearer by returning:
"Unconfirmed UTXOs are available, but spending them creates
a chain of transactions that will be rejected by the mempool"

Also, added an early return from Coin Selection if the sum of
the discarded coins decreases the available balance below the
target amount.

@furszy furszy force-pushed the 2022_wallet_operationresult branch 2 times, most recently from 143af80 to ba6dc81 Compare April 13, 2022 16:42
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 14, 2022

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 Xekyo, achow101, S3RK

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:

  • #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

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.

Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last 2 commits should be dropped from this PR and a new PR for each one opened. They are orthogonal to the main changes here.

@furszy
Copy link
Member Author

furszy commented Apr 15, 2022

Hey, thanks for the review @achow101.

The last 2 commits should be dropped from this PR and a new PR for each one opened. They are orthogonal to the main changes here.

Sounds good, will move them to their own PRs. I added them here basically because I wasn't totally happy with the extra wasted computation neither, so.. thought on other ways to speed up the process.


Will tackle the code styling changes as well 👍.

Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like 0bd751f "spend: refactor the entire CreateTransaction flow to return a CallResult." could be squashed into 3318ee2 "send: move wallet CreateTransaction* flow to use OperationResult/CallResult."

@furszy furszy force-pushed the 2022_wallet_operationresult branch 3 times, most recently from ac054d6 to d9b8ea6 Compare April 19, 2022 21:05
@furszy
Copy link
Member Author

furszy commented Apr 19, 2022

kk, done both. You caught me doing some further changes.

Have re-coded the mempool ancestors/descendants cache differently, much cleaner and less intrusive. It's now integrated into the available coin information (the changes are quite small now, it shouldn't require another PR for it but let me know what you think achow101).

Plus cleaned the CreateTransaction changes a bit more.

@furszy
Copy link
Member Author

furszy commented Apr 22, 2022

Aside from achow101 review changes, the first part:

Have re-coded the mempool ancestors/descendants cache differently, much cleaner and less intrusive. It's now integrated into the available coin information

Was the move from fbc130a to d9b8ea6 because now the ancestors and descendants count is available inside COutput. Which simplified the changes greatly.

And the second part was an unification for 76aa7d0 of the previous two emplace_back calls into a single one inside AvailableCoins and moving the ancestors/descendants getter call after the basic checks.

@bitcoin bitcoin deleted a comment from Szatan181 Apr 22, 2022
@achow101
Copy link
Member

The first 3 commits are fine (besides the comments I left above), but I'm still not sure that this is the correct approach to adding the error message. It seems like it's a ton of added complexity to have the second vector just to be able to say whether the result would have a too long mempool chain.

@furszy furszy force-pushed the 2022_wallet_operationresult branch from d9b8ea6 to 8052976 Compare April 22, 2022 21:06
@furszy
Copy link
Member Author

furszy commented Apr 22, 2022

yeah.. have been thinking about it too.

  1. The initial proposed solution was simple but came with an extra AttemptSelection call.

  2. The second one introduced the coin control unconfirmed UTXO filtering functionality, which got us two good performance improvement:
    -> We no longer loop, several times, over coins that will be reject by the mempool restrictions during the whole selection process.
    -> And we no longer lock the mempool mutex two times per available UTXO for each of the many 'AttemptSelection/GroupOutputs' calls to get the ancestors and descendants count. Now is done only once per unconfirmed tx in the mempool.

    But.. introduced that ugly extra uncof_coins vector to AvailableCoins.


Now.. next step forward:

  1. Have removed the secondary vector, and all that extra complexity, and replaced it with a simple bool has_long_chain_of_unconf which can be used to notify the user about the existence of a long chain of unconfirmed transactions.
    Then he/she can know exactly how much unconfirmed balance has calling getbalances after receiving the error.

Then.. the last commit is just to make the change look nicer. AvailableCoins now will return a CoinsResult struct instead of receiving the vCoins vector reference (which was being cleared at the beginning of the method anyway).

achow101 added a commit to bitcoin-core/gui that referenced this pull request Mar 6, 2023
…e it from Coin Selection

6a302d4 wallet: single output groups filtering and grouping process (furszy)
bd91ed1 wallet: unify outputs grouping process (furszy)
5596200 test: coinselector_tests refactor, use CoinsResult instead of plain std::vector (furszy)
34f54a0 wallet: decouple outputs grouping process from each ChooseSelectionResult (furszy)
461f082 refactor: make OutputGroup::m_outputs field a vector of shared_ptr (furszy)
d8e749b test: wallet, add coverage for outputs grouping process (furszy)
06ec8f9 wallet: make OutputGroup "positive_only" filter explicit (furszy)

Pull request description:

  The idea originates from bitcoin/bitcoin#24845 (comment).

  Note:
  For clarity, it's recommended to start reviewing from the end result to understand the structure of the flow.

  #### GroupOutputs function rationale:
  If "Avoid Partial Spends" is enabled, the function gathers outputs with the same script together inside a container. So Coin Selection can treats them as if them were just one possible input and either select them all or not select them.

  #### How the Inputs Fetch + Selection process roughly works:

  ```
  1. Fetch user’s manually selected inputs.
  2. Fetch wallet available coins (walks through the entire wallet txes map) and insert them into a set of vectors (each vector store outputs from a single type).
  3. Coin Selection Process:
     Call `AttemptSelection` 8 times. Each of them expands the coin eligibility filter (accepting a larger subset of coins in the calculation) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds.

     Each `AttemptSelection` call performs the following actions:
       - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and a combination of all of them):
         Call ‘ChooseSelectionResult’ providing the respective, filtered by type, coins vector. Which:
             I. Groups the outputs vector twice (one for positive only and a second one who includes the negative ones as well).
                - GroupOutputs walks-through the entire inputted coins vector one time at least, + more if we are avoiding partial spends, to generate a vector of OutputGroups.
             II. Then performs every coin selection algorithm using the recently created vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD.
             III. Then returns the best solution out of them.
  ```

  We perform the general operation of gathering outputs, with the same script, into a single container inside:
  Each coins selection attempt (8 times —> each coin eligibility filter), for each of the outputs vector who were filtered by type (plus another one joining all the outputs as well if needed), twice (one for the positive only outputs effective value and a second one for all of them).

  So, in the worst case scenario where no solution is found after the 8 Coin Selection attempts, the `GroupOutputs` function is called 80 times (8 * 5 * 2).

  #### Improvements:

  This proposal streamlines the process so that the output groups, filtered by coin eligibility and type, are created in a single loop outside of the Coin Selection Process.

  The new process is as follows:

  ```
  1. Fetch user’s manually selected inputs.
  2. Fetch wallet available coins.
  3. Group outputs by each coin eligibility filter and each different output type found.
  4. Coin Selection Process:
     Call AttemptSelection 8 times. Each of them expands the coin eligibility filter (accepting different output groups) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds.

     Each ‘AttemptSelection’ call performs the following actions:
        - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and all of them):
            A. Call ‘ChooseSelectionResult’ providing the respective, filtered by type, output group. Which:
               I. Performs every coin selection algorithm using the provided vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD.
               II. Then returns the best solution out of them.
  ```

  Extra Note:
  The next steps after this PR will be to:
  1) Merge `AvailableCoins` and `GroupOutputs` processes.
  2) Skip entire coin selection rounds if no new coins are added into the subsequent round.
  3) Remove global feerates from the OutputGroup class.
  4) Remove secondary "grouped" tx creation from `CreateTransactionInternal` by running Coin Selection results over the aps grouped outputs vs non-aps ones.

ACKs for top commit:
  S3RK:
    ReACK 6a302d4
  achow101:
    ACK 6a302d4
  theStack:
    re-ACK 6a302d4 🥥

Tree-SHA512: dff849063be328e7d9c358ec80239a6db2cd6131963b511b83699b95b337d3106263507eaba0119eaac63e6ac21c6c42d187ae23d79d9220b90c323d44b01d24
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 7, 2023
…m Coin Selection

6a302d4 wallet: single output groups filtering and grouping process (furszy)
bd91ed1 wallet: unify outputs grouping process (furszy)
5596200 test: coinselector_tests refactor, use CoinsResult instead of plain std::vector (furszy)
34f54a0 wallet: decouple outputs grouping process from each ChooseSelectionResult (furszy)
461f082 refactor: make OutputGroup::m_outputs field a vector of shared_ptr (furszy)
d8e749b test: wallet, add coverage for outputs grouping process (furszy)
06ec8f9 wallet: make OutputGroup "positive_only" filter explicit (furszy)

Pull request description:

  The idea originates from bitcoin#24845 (comment).

  Note:
  For clarity, it's recommended to start reviewing from the end result to understand the structure of the flow.

  #### GroupOutputs function rationale:
  If "Avoid Partial Spends" is enabled, the function gathers outputs with the same script together inside a container. So Coin Selection can treats them as if them were just one possible input and either select them all or not select them.

  #### How the Inputs Fetch + Selection process roughly works:

  ```
  1. Fetch user’s manually selected inputs.
  2. Fetch wallet available coins (walks through the entire wallet txes map) and insert them into a set of vectors (each vector store outputs from a single type).
  3. Coin Selection Process:
     Call `AttemptSelection` 8 times. Each of them expands the coin eligibility filter (accepting a larger subset of coins in the calculation) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds.

     Each `AttemptSelection` call performs the following actions:
       - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and a combination of all of them):
         Call ‘ChooseSelectionResult’ providing the respective, filtered by type, coins vector. Which:
             I. Groups the outputs vector twice (one for positive only and a second one who includes the negative ones as well).
                - GroupOutputs walks-through the entire inputted coins vector one time at least, + more if we are avoiding partial spends, to generate a vector of OutputGroups.
             II. Then performs every coin selection algorithm using the recently created vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD.
             III. Then returns the best solution out of them.
  ```

  We perform the general operation of gathering outputs, with the same script, into a single container inside:
  Each coins selection attempt (8 times —> each coin eligibility filter), for each of the outputs vector who were filtered by type (plus another one joining all the outputs as well if needed), twice (one for the positive only outputs effective value and a second one for all of them).

  So, in the worst case scenario where no solution is found after the 8 Coin Selection attempts, the `GroupOutputs` function is called 80 times (8 * 5 * 2).

  #### Improvements:

  This proposal streamlines the process so that the output groups, filtered by coin eligibility and type, are created in a single loop outside of the Coin Selection Process.

  The new process is as follows:

  ```
  1. Fetch user’s manually selected inputs.
  2. Fetch wallet available coins.
  3. Group outputs by each coin eligibility filter and each different output type found.
  4. Coin Selection Process:
     Call AttemptSelection 8 times. Each of them expands the coin eligibility filter (accepting different output groups) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds.

     Each ‘AttemptSelection’ call performs the following actions:
        - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and all of them):
            A. Call ‘ChooseSelectionResult’ providing the respective, filtered by type, output group. Which:
               I. Performs every coin selection algorithm using the provided vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD.
               II. Then returns the best solution out of them.
  ```

  Extra Note:
  The next steps after this PR will be to:
  1) Merge `AvailableCoins` and `GroupOutputs` processes.
  2) Skip entire coin selection rounds if no new coins are added into the subsequent round.
  3) Remove global feerates from the OutputGroup class.
  4) Remove secondary "grouped" tx creation from `CreateTransactionInternal` by running Coin Selection results over the aps grouped outputs vs non-aps ones.

ACKs for top commit:
  S3RK:
    ReACK 6a302d4
  achow101:
    ACK 6a302d4
  theStack:
    re-ACK 6a302d4 🥥

Tree-SHA512: dff849063be328e7d9c358ec80239a6db2cd6131963b511b83699b95b337d3106263507eaba0119eaac63e6ac21c6c42d187ae23d79d9220b90c323d44b01d24
@furszy furszy force-pushed the 2022_wallet_operationresult branch 3 times, most recently from c0c8784 to 3c95271 Compare March 7, 2023 21:50
@furszy furszy changed the title WIP, wallet: createTransaction, return proper error description for "too-long-mempool-chain" wallet: return error msg for "too-long-mempool-chain" Mar 7, 2023
@furszy furszy force-pushed the 2022_wallet_operationresult branch from 3c95271 to 2074260 Compare March 7, 2023 22:02
@furszy furszy force-pushed the 2022_wallet_operationresult branch from 2074260 to 793d633 Compare March 7, 2023 23:34
@furszy furszy marked this pull request as ready for review March 7, 2023 23:36
furszy added 2 commits March 10, 2023 11:29
We currently return "Insufficient funds" which doesn't really
describe what went wrong; the tx creation failed because of
a long-mempool-chain, not because of a lack of funds.

Also, return early from Coin Selection if the sum of the
discarded coins decreases the available balance below the
target amount.
@furszy furszy force-pushed the 2022_wallet_operationresult branch from 793d633 to f3221d3 Compare March 10, 2023 14:30
@furszy
Copy link
Member Author

furszy commented Mar 10, 2023

Thanks @S3RK!, feedback tackled.

We now check, and return early, if there is no enough available balance after filtering groups and before running the selection algos.

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f3221d3

@achow101
Copy link
Member

ACK f3221d3

@DrahtBot DrahtBot requested review from murchandamus and removed request for murchandamus March 17, 2023 22:10

// Check if we still have enough balance after applying filters (some coins might be discarded)
CAmount total_discarded = 0;
CAmount total_unconf_long_chain = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: do I understand correctly with current filters total_discarded is always equal to total_unconf_long_chain?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nop. Could also have zero conf change (or send-to-self) if the wallet's m_spend_zero_conf_change flag is disabled. In other words, unconfirmed coins accepted by the mempool limits (ancestors/descendants < 25).

@S3RK
Copy link
Contributor

S3RK commented Mar 23, 2023

Code review ACK f3221d3

@glozow glozow merged commit 381593c into bitcoin:master Mar 23, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 24, 2023
…hain"

f3221d3 test: add wallet too-long-mempool-chain error coverage (furszy)
acf0119 wallet: return error msg for too-long-mempool-chain failure (furszy)

Pull request description:

  Fixes bitcoin#23144.

  We currently return a general "Insufficient funds" from Coin
  Selection when we actually skipped unconfirmed UTXOs that
  surpassed the mempool ancestors limit.

  This PR make the error clearer by returning:
  "Unconfirmed UTXOs are available, but spending them creates
  a chain of transactions that will be rejected by the mempool"

  Also, added an early return from Coin Selection if the sum of
  the discarded coins decreases the available balance below the
  target amount.

ACKs for top commit:
  achow101:
    ACK f3221d3
  S3RK:
    Code review ACK f3221d3
  Xekyo:
    ACK f3221d3

Tree-SHA512: 13e5824b75ac302280ff894560a4ebf32a74f32fe49ef8281f2bc99c0104b92cef33d3b143c6e131f3a07eafe64533af7fc60abff585142c134b9d6e531a6a66
@furszy furszy deleted the 2022_wallet_operationresult branch May 27, 2023 01:51
@bitcoin bitcoin locked and limited conversation to collaborators May 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plumb "too-long-mempool-chain" to RPC error for send/sendtoaddress
7 participants