-
Notifications
You must be signed in to change notification settings - Fork 37.7k
wallet: return error msg for "too-long-mempool-chain" #24845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
143af80
to
ba6dc81
Compare
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. ConflictsReviewers, 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. |
There was a problem hiding this 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.
Hey, thanks for the review @achow101.
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 👍. |
3d48753
to
fbc130a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ac054d6
to
d9b8ea6
Compare
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 |
Aside from achow101 review changes, the first part:
Was the move from fbc130a to d9b8ea6 because now the ancestors and descendants count is available inside And the second part was an unification for 76aa7d0 of the previous two |
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. |
d9b8ea6
to
8052976
Compare
yeah.. have been thinking about it too.
Now.. next step forward:
Then.. the last commit is just to make the change look nicer. |
…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
…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
c0c8784
to
3c95271
Compare
3c95271
to
2074260
Compare
2074260
to
793d633
Compare
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.
793d633
to
f3221d3
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f3221d3
ACK f3221d3 |
|
||
// 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; |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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).
Code review ACK f3221d3 |
…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
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.