wallet: bugfix, invalid CoinsResult cached total amount #26560

furszy · 2022-11-23T15:13:40Z

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

The CoinsResult::Erase method removes 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 on master is not a problem, because since #25685
we are no longer using the method. But, it's a bug on v24
(check #26559).

This method it's being fixed and not removed because I'm later using it to solve
another bug inside this PR.
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

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.
Adds test coverage for the CoinsResult::Erase method.

DrahtBot · 2022-11-23T15:13:43Z

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	achow101, josibake
Stale ACK	Xekyo, S3RK, glozow

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:

#26573 (Wallet: don't underestimate the fees when spending a Taproot output by darosior)
#26567 (Wallet: estimate the size of signed inputs using descriptors by darosior)
#25729 (wallet: Check max transaction weight in CoinSelection by aureleoules)

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.

achow101

Good catch! Unfortunate that this was found right as we're doing the 24.0 release.

src/wallet/spend.cpp

S3RK

oh wow, that's a nasty bug.

Almost Code Review ACK modulo use of available_coins.total_amount with SFFO in the last commit.

I was quite surprised something like this wasn't caught by some assert. So I dig up and maybe we can add something like e783af3 while you're at it

test/functional/rpc_fundrawtransaction.py

src/wallet/test/spend_tests.cpp

S3RK · 2022-11-26T16:36:00Z

src/wallet/test/spend_tests.cpp

@@ -112,5 +112,38 @@ BOOST_FIXTURE_TEST_CASE(FillInputToWeightTest, BasicTestingSetup)
    // Note: We don't test the next boundary because of memory allocation constraints.
 }

+BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)


nit: I'm not sure having second test provides enough extra value. I'd rather keep one, but ultimately up to you.

yeah. I almost remove it but.. ended up leaving because (1) the severity of the bug and (2) because it's easier to debug and assert that the code is failing where is supposed to be failing (in the functional test case, as we use the send RPC command, we do have other stuff involved).

I'm fine either way, if this still doesn't convince you and more people don't see it useful, can remove it for sure.

I also agree that the second unit test (which isn't really a unit test) seems redundant and also a little confusing to me. I think the unit test for CoinsResult::Erase in an earlier commit is correct for a unit test and the functional test is better suited for testing the wallet behavior

src/wallet/spend.cpp

src/wallet/spend.h

Sjors · 2022-11-28T13:54:40Z

Asking here instead of in the backport: is this only in v24 or also earlier versions?

fanquake · 2022-11-28T14:03:16Z

Asking here instead of in the backport: is this only in v24 or also earlier versions?

My understanding is only v24.x. Seems like the buggy behaviour was introduced in #24584 + #25734?

furszy · 2022-11-28T14:04:31Z

Asking here instead of in the backport: is this only in v24 or also earlier versions?

Yeah. @Sjors only for v24. The dangerous bug was introduced in #25734 which is only on v24, and after branching off, we merged #25685 which fixed it without noticing it.

furszy · 2022-11-28T19:48:05Z

Feedback tackled, thanks @S3RK and @achow101.

Changes:

In the tests, removed the unneeded coins lock.
Now SelectCoins returns early if the wallet's available coins total_effective_amount is not available (it does not fallback to use the raw total amount anymore).

luke-jr · 2022-11-29T04:10:40Z

I think probably CoinsResult::coins should be a std::map<OutputType, std::unordered_set<COutput>> instead of [map of] vectors. It's too strange that we could end up with the same output in there twice. Maybe even an assert failure if we try to add the same ones twice?

luke-jr

The bugfix should also be done in its own commit, absent any refactoring.

I think this means just dropping the break and replacing the find_if with remove_if?

josibake

ACK 1b14bd7

Glad you caught this! Surprised and pretty bummed this slipped through in #25734 😞

Left a few suggestions, but non-blocking.

src/wallet/spend.cpp

src/wallet/spend.h

josibake · 2022-11-29T14:05:43Z

src/wallet/test/spend_tests.cpp

@@ -112,5 +112,38 @@ BOOST_FIXTURE_TEST_CASE(FillInputToWeightTest, BasicTestingSetup)
    // Note: We don't test the next boundary because of memory allocation constraints.
 }

+BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)


I also agree that the second unit test (which isn't really a unit test) seems redundant and also a little confusing to me. I think the unit test for CoinsResult::Erase in an earlier commit is correct for a unit test and the functional test is better suited for testing the wallet behavior

test/functional/rpc_fundrawtransaction.py

src/wallet/spend.cpp

furszy · 2022-11-29T15:10:42Z

I think probably CoinsResult::coins should be a std::map<OutputType, std::unordered_set<COutput>> instead of [map of] vectors. It's too strange that we could end up with the same output in there twice. Maybe even an assert failure if we try to add the same ones twice?

@luke-jr: we can't end up with the same output twice there. That struct is loaded inside AvailableCoins which traverses the wallet's txes map once.
In v24, where we don't have #25685 merge, the preset inputs were fetched twice: first inside AvailableCoins which was appending them to the CoinsResult vectors and secondly inside SelectCoins where we were manually fetching them from the wallet txes map here (by calling GetWalletTx(outpoint)).
Then, SelectCoins was removing the preset inputs from the CoinsResult vectors before start the automatic coin selection process so the process does not select them (bug was here, the Erase function was not removing them). Then, at the end of the selection process, we merge both results (the preset inputs selection result and the automatic coin selection result).

…e set The loop break shouldn't have being there.

josibake · 2022-11-29T17:03:35Z

fwiw, I ran the failing CI test locally and it passed, so I think all you need to do is restart the test

furszy · 2022-11-29T22:57:47Z

Updated per feedback, thanks everyone. Small diff.

Changes:

Per @S3RK feedback, cherry-picked the simple fallback assertion (eb76557).
I still think that we might want to merge different SelectionResult objects that share some outputs in the future but.. now it's definitely not the right time to do it.
Per @josibake feedback, added #26559 mention in the functional test cases and few more details in the comments.

achow101 · 2022-11-29T23:06:59Z

~~ACK eb76557~~

josibake · 2022-11-30T09:56:52Z

ACK eb76557

reviewed the diff, looks good

murchandamus

ACK eb76557

murchandamus · 2022-11-30T19:54:30Z

src/wallet/test/spend_tests.cpp

+        coin_control.Select(outpoint);
+    }
+
+    // First case, use 'subtract_fee_from_outputs' to make the wallet create a tx that pays an insane fee on v24.0.


Just a nit, but the term "insane fee" had me thinking that this test has us paying a huge amount of fees, something that would be conflicting with our sanity checks.

yeah ok, I used that term because the nFeeRet calculation prior to the SFFO process, can go insanely high based on the duplicated preset inputs, then we deduct it from the recipient's target.
will improve the documentation.

src/wallet/test/spend_tests.cpp

murchandamus · 2022-11-30T19:56:59Z

test/functional/rpc_fundrawtransaction.py

+        # 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 preset inputs, minus one, amounts in
+        # fee :(.


I see, that is the context that I was missing above! :)

murchandamus · 2022-11-30T20:24:26Z

src/wallet/spend.cpp

+
+            // update cached amounts
+            total_amount -= coin.txout.nValue;
+            if (coin.HasEffectiveValue()) total_effective_amount = *total_effective_amount - coin.GetEffectiveValue();


Are we sure that total_effective_value is always set here?

Maybe assume(total_effective_amount.has_value() here?

I had this question myself. I figured if we remove the coin with effective value, then the coin was added before, hence we have total_effective_value defined. Please correct me if my reasoning is flawed.

yeah. As we only use CoinsResult::Add to append new coins: if we have the coin inside CoinsResult, and the coin has an effective value, then we must have total_effective_amount.

murchandamus · 2022-11-30T20:35:33Z

src/wallet/spend.cpp

+    CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.total_amount :
+            (available_coins.total_effective_amount.has_value() ? *available_coins.total_effective_amount : 0);


We will never use SelectCoins() without a feerate. If you end up not having total_effective_amount here, something went terribly wrong. Perhaps just:

Suggested change

CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.total_amount :

(available_coins.total_effective_amount.has_value() ? *available_coins.total_effective_amount : 0);

assume(available_coins.total_effective_amount.has_value());

CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.total_amount : *available_coins.total_effective_amount);

murchandamus · 2022-11-30T20:38:46Z

src/wallet/coinselection.cpp

@@ -452,12 +452,16 @@ void SelectionResult::AddInputs(const std::set<COutput>& inputs, bool subtract_f

 void SelectionResult::Merge(const SelectionResult& other)
 {
+    // Obtain the expected selected inputs count after the merge (for now, duplicates are not allowed)


Suggested change

// Obtain the expected selected inputs count after the merge (for now, duplicates are not allowed)

// Store sum of combined input sets to check that no UTXO was used twice

this relies on #26560 (comment) (where we are talking about whether should include the assertion commit here or in a follow-up PR).
Please, feel free to comment there.

glozow

strong Concept ACK, I understand the bug has a significant impact and agree with the fix, but the documentation should be changed as I think it is currently incorrect and could be misleading to someone in the future trying to debug / reproduce what was wrong with v24.0.

TLDR I don't think the "insane fee" bug is actually possible? I've made some suggestions for what I think the documentation should be inline (same suggestion wrt the commit messages).

glozow · 2022-12-01T17:36:37Z

src/wallet/coinselection.cpp

    m_target += other.m_target;
    m_use_effective |= other.m_use_effective;
    if (m_algo == SelectionAlgorithm::MANUAL) {
        m_algo = other.m_algo;
    }
    util::insert(m_selected_inputs, other.m_selected_inputs);
+    assert(m_selected_inputs.size() == expected_count);


I suggest removing this last commit for this PR, and following up with an "add sanity checks to coin selection code" PR.

Of course we don't want to create a bad transaction, but an assert would cause the user's node to crash in case we have implemented it wrong. I think something better could be to add if(!Assume(...)) {return "sorry, something went wrong and transaction creation failed. please report this bug...";} which should stop the wallet from doing something horrible but also not crash. Apologies if this is bikesheddy - my concrete suggestion is to remove this for now, and do a followup PR for these types of changes. Other appropriate sanity checks could include AddInput() never adds an input that's already there, SelectCoins result should cover selection_target, etc.

test/functional/rpc_fundrawtransaction.py

glozow · 2022-12-01T17:56:16Z

test/functional/rpc_fundrawtransaction.py

+
+        # First case, use 'subtract_fee_from_outputs' to make the wallet create a tx that pays an insane fee on v24.0.
+        # (further details in #26559).
+        assert_raises_rpc_error(-4, "Insufficient funds", wallet.send, outputs=[{wallet.getnewaddress(address_type="bech32"): 14}], options=options)


Cherry-picking this test on top of 24.x, I can see that the wallet mistakenly uses a preset input twice and deducts the fee+5BTC from the payment, but the fee is actually exactly what is expected (since at the end the transaction only has 10BTC inputs and not 15): https://github.com/glozow/bitcoin/blob/test-n26560/test/functional/rpc_fundrawtransaction.py#L1250-L1266

@achow101 suggested that maybe nFeeRet from FundTransaction() or CreatedTransactionResult::fee could be incorrect but I wasn't able to get a test to show that. @achow101 then pointed me to this code showing the sffo fee should be correctly set before CreateTransaction returns:

bitcoin/src/wallet/spend.cpp

Lines 971 to 1004 in f668a3a

// Reduce output values for subtractFeeFromAmount

if (coin_selection_params.m_subtract_fee_outputs) {

CAmount to_reduce = fee_needed - nFeeRet;

int i = 0;

bool fFirst = true;

for (const auto& recipient : vecSend)

{

if (i == nChangePosInOut) {

++i;

}

CTxOut& txout = txNew.vout[i];

if (recipient.fSubtractFeeFromAmount)

{

txout.nValue -= to_reduce / outputs_to_subtract_fee_from; // Subtract fee equally from each selected recipient

if (fFirst) // first receiver pays the remainder not divisible by output count

{

fFirst = false;

txout.nValue -= to_reduce % outputs_to_subtract_fee_from;

}

// Error if this output is reduced to be below dust

if (IsDust(txout, wallet.chain().relayDustFee())) {

if (txout.nValue < 0) {

return util::Error{_("The transaction amount is too small to pay the fee")};

} else {

return util::Error{_("The transaction amount is too small to send after the fee has been deducted")};

}

}

}

++i;

}

nFeeRet = fee_needed;

I could be mistaken, but in that case could you maybe write a test for v24.0 that shows what the expected vs actual fee are? Otherwise I'd suggest removing this from the comments/commit message/PR descriptions.

Having 2 test cases for sffo and non-sffo codepaths is still meaningful, but I think the current comment is misleading and can cause us to have trouble understanding this test in the future.

Done, updated per feedback. Thanks.

Some context:
The "insane fee" label came from the nFeeRet calculation. Which, in case of this bug, sets a high value then is later deducted from the recipient's amount. Which.. as we spoke via dm, it's true that this is internal to the wallet only.. so the description should be something like "double-counted preset inputs during Coin Selection incorrectly deduces the user's selected recipient's amount" (or something similar)

furszy · 2022-12-01T18:51:46Z

Ok great, thanks everyone. Now I see where the confusion arises.
Will be tackling all the feedback and updating the PR soon.

This exercises the bug inside CoinsResult::Erase that ends up on (1) a wallet crash or (2) a created and broadcasted tx that contains a reduced recipient's amount. This is covered by making the wallet selects the preset inputs twice during the coin selection process. Making the wallet 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 recipient's amount by the difference between the original target and the wrongly counted inputs. Which means, a created and relayed tx sending less coins to the destination than what the user inputted.

… of direct member access Aside from the cleanup, this solves a bug in the following-up commit. Because, in these tests, we are manually adding/erasing outputs from the CoinsResult object but never updating the internal total amount field.

…target The CoinsResult class will now count the raw total amount and the effective total amount internally (inside the 'CoinsResult::Add' and 'CoinsResult::Erase' methods). So there is no discrepancy between what we add/erase and the total values. (which is what was happening on the coinselector_test because the 'CoinsResult' object is manually created there, and we were not keeping the total amount in sync with the outputs being added/removed).

glozow

ACK cf79384 (I assume these 3 are the backport commits) Verified that the tests in cf79384 pass with the fix, fail on 24.x without the fix. I now think the documentation is accurate and would be helpful in a situation in which the test has broken again - thanks for taking the feedback!

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.

src/wallet/test/spend_tests.cpp

glozow · 2022-12-05T10:46:59Z

src/wallet/coinselection.cpp

    m_target += other.m_target;
    m_use_effective |= other.m_use_effective;
    if (m_algo == SelectionAlgorithm::MANUAL) {
        m_algo = other.m_algo;
    }
    util::insert(m_selected_inputs, other.m_selected_inputs);
+    assert(m_selected_inputs.size() == expected_count);


Right, so the question of how to implement sanity checks warrants additional discussion and/or refactoring but that discussion shouldn't hold up this PR. Hence the recommendation to remove it.

I disagree that it's harmless to have this assert after the fix; the bug can still exist or be reintroduced. But assuming these last few commits won't be backported and we'll have a followup PR to add/change sanity checks, fine with leaving it in this PR.

achow101 · 2022-12-05T16:50:37Z

ACK 7362f8e

josibake

ACK 7362f8e

Give the discussion, I think it's best if we drop 3282fad and address it in a follow-up, but not worth holding the PR up if the author feels differently.

A big thanks to furszy for incorporating all of the feedback regarding the tests and documentation: everything is very clear now as to what's being tested and why

josibake · 2022-12-05T16:45:25Z

src/wallet/coinselection.cpp

    m_target += other.m_target;
    m_use_effective |= other.m_use_effective;
    if (m_algo == SelectionAlgorithm::MANUAL) {
        m_algo = other.m_algo;
    }
    util::insert(m_selected_inputs, other.m_selected_inputs);
+    assert(m_selected_inputs.size() == expected_count);


It seems we all (mostly) agree that the assert is not ideal and how to deal with this should be addressed in a follow-up with more discussion, so I'd recommend dropping the commit so we don't merge code that we know will be removed/refactored shortly after and to avoid any confusion when backporting commits from this PR for v24

… 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

…e set The loop break shouldn't have being there. Github-Pull: bitcoin#26560 Rebased-From: f930aef

Github-Pull: bitcoin#26560 Rebased-From: 341ba7f

This exercises the bug inside CoinsResult::Erase that ends up on (1) a wallet crash or (2) a created and broadcasted tx that contains a reduced recipient's amount. This is covered by making the wallet selects the preset inputs twice during the coin selection process. Making the wallet 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 recipient's amount by the difference between the original target and the wrongly counted inputs. Which means, a created and relayed tx sending less coins to the destination than what the user inputted. Github-Pull: bitcoin#26560 Rebased-From: cf79384

fanquake · 2022-12-05T18:17:24Z

Backported the 3 commits in #26616.

8b726bf test: Coin Selection, duplicated preset inputs selection (furszy) 9d73176 test: wallet, coverage for CoinsResult::Erase function (furszy) 195f0df wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy) e5d097b [test] Add p2p_tx_privacy.py (dergoegge) c842670 [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge) e15b306 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge) 95fded1 wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow) d464b2a tests: Test for migrating encrypted wallets (Andrew Chow) 7a97a56 wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow) Pull request description: Backports remaining changes on the 24.0.1 milestone. Currently backports: * #26594 * #26569 * #26560 ACKs for top commit: josibake: ACK 8b726bf Tree-SHA512: db77ec1a63a7b6a4412750a0f4c0645681fc346a5df0a7cd38d5d27384e1d0fa95f3953af90042afe131ddbd4b6a6e009527095f13e9f58c0190cd378738a9e5

…error messages 76dc547 gui: create tx, launch error dialog if backend throws runtime_error (furszy) f4d7947 wallet: coin selection, add duplicated inputs checks (furszy) 0aa065b wallet: return accurate error messages from Coin Selection (furszy) 7e8340a wallet: make SelectCoins flow return util::Result (furszy) e5e147f wallet: refactor eight consecutive 'AttemptSelection' calls into a loop (furszy) Pull request description: Work decoupled from #25806, which cleanup and improves the Coin Selection flow further. Adding the capability to propagate specific error messages from the Coin Selection process to the user. Instead of always returning the general "Insufficient funds" message which is not always accurate to what happened internally. Letting us instruct the user how to proceed under certain circumstances. The following error messages were added: 1) If the selection result exceeds the maximum transaction weight, we now will return: -> "The inputs size exceeds the maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs". 2) If the user pre-selected inputs and disallowed the automatic coin selection process (no other inputs are allowed), we now will return: -> "The preselected coins total amount does not cover the transaction target. Please allow other inputs to be automatically selected or include more coins manually". 3) The double-counted preset inputs during Coin Selection error will now throw an "internal bug detected" message instead of crashing the node. The essence of this work comes from several comments: 1. bitcoin/bitcoin#26560 (comment) 2. bitcoin/bitcoin#25729 (comment) 3. bitcoin/bitcoin#25269 (review) 4. bitcoin/bitcoin#23144 (which is connected to #24845) ACKs for top commit: ishaanam: crACK 76dc547 achow101: ACK 76dc547 aureleoules: ACK 76dc547 theStack: ACK 76dc547 🌇 Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5

…ssages 76dc547 gui: create tx, launch error dialog if backend throws runtime_error (furszy) f4d7947 wallet: coin selection, add duplicated inputs checks (furszy) 0aa065b wallet: return accurate error messages from Coin Selection (furszy) 7e8340a wallet: make SelectCoins flow return util::Result (furszy) e5e147f wallet: refactor eight consecutive 'AttemptSelection' calls into a loop (furszy) Pull request description: Work decoupled from bitcoin#25806, which cleanup and improves the Coin Selection flow further. Adding the capability to propagate specific error messages from the Coin Selection process to the user. Instead of always returning the general "Insufficient funds" message which is not always accurate to what happened internally. Letting us instruct the user how to proceed under certain circumstances. The following error messages were added: 1) If the selection result exceeds the maximum transaction weight, we now will return: -> "The inputs size exceeds the maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs". 2) If the user pre-selected inputs and disallowed the automatic coin selection process (no other inputs are allowed), we now will return: -> "The preselected coins total amount does not cover the transaction target. Please allow other inputs to be automatically selected or include more coins manually". 3) The double-counted preset inputs during Coin Selection error will now throw an "internal bug detected" message instead of crashing the node. The essence of this work comes from several comments: 1. bitcoin#26560 (comment) 2. bitcoin#25729 (comment) 3. bitcoin#25269 (review) 4. bitcoin#23144 (which is connected to bitcoin#24845) ACKs for top commit: ishaanam: crACK 76dc547 achow101: ACK 76dc547 aureleoules: ACK 76dc547 theStack: ACK 76dc547 🌇 Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5

484812

Gj

DrahtBot added the Wallet label Nov 23, 2022

achow101 reviewed Nov 23, 2022

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 23, 2022

Bump unconfirmed ancestor transactions to target feerate #26152

Merged

furszy force-pushed the 2022_wallet_bugfix_coinsresult branch from 0fd9be1 to f20f88f Compare November 24, 2022 00:32

fanquake requested review from murchandamus and S3RK November 24, 2022 12:30

This was referenced Nov 25, 2022

Wallet: estimate the size of signed inputs using descriptors #26567

Merged

Wallet: don't underestimate the fees when spending a Taproot output #26573

Closed

S3RK reviewed Nov 26, 2022

View reviewed changes

fanquake mentioned this pull request Nov 28, 2022

[24.x] wallet: bugfix, double-counted preset inputs during Coin Selection #26559

Closed

fanquake requested a review from josibake November 28, 2022 14:04

furszy force-pushed the 2022_wallet_bugfix_coinsresult branch from f20f88f to 1b14bd7 Compare November 28, 2022 19:32

luke-jr suggested changes Nov 29, 2022

View reviewed changes

josibake approved these changes Nov 29, 2022

View reviewed changes

wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of th…

f930aef

…e set The loop break shouldn't have being there.

furszy force-pushed the 2022_wallet_bugfix_coinsresult branch from 1b14bd7 to 8f9bd17 Compare November 29, 2022 22:45

furszy force-pushed the 2022_wallet_bugfix_coinsresult branch from 8f9bd17 to eb76557 Compare November 29, 2022 23:00

DrahtBot mentioned this pull request Nov 30, 2022

wallet: Check max transaction weight in CoinSelection #25729

Merged

murchandamus reviewed Nov 30, 2022

View reviewed changes

glozow reviewed Dec 1, 2022

View reviewed changes

furszy and others added 6 commits December 2, 2022 12:39

test: wallet, coverage for CoinsResult::Erase function

341ba7f

wallet: add assert to SelectionResult::Merge for safety

3282fad

refactor: make CoinsResult total amounts members private

7362f8e

furszy force-pushed the 2022_wallet_bugfix_coinsresult branch from eb76557 to 7362f8e Compare December 2, 2022 16:23

glozow reviewed Dec 5, 2022

View reviewed changes

glozow requested review from josibake and achow101 December 5, 2022 11:12

josibake reviewed Dec 5, 2022

View reviewed changes

achow101 merged commit f0c4807 into bitcoin:master Dec 5, 2022

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Dec 5, 2022

wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of th…

195f0df

…e set The loop break shouldn't have being there. Github-Pull: bitcoin#26560 Rebased-From: f930aef

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Dec 5, 2022

test: wallet, coverage for CoinsResult::Erase function

9d73176

Github-Pull: bitcoin#26560 Rebased-From: 341ba7f

fanquake mentioned this pull request Dec 5, 2022

[24.x] Backports for 24.0.1 #26616

Merged

This was referenced Dec 6, 2022

wallet: group outputs only once, decouple it from Coin Selection #25806

Merged

wallet: Coin Selection, return accurate error messages #26661

Merged

ghost mentioned this pull request Jan 6, 2023

Socratic Seminar 14 TriangleBitDevs/TriangleBitDevs.github.io#4

Closed

484812 reviewed Mar 27, 2023

View reviewed changes

furszy deleted the 2022_wallet_bugfix_coinsresult branch May 27, 2023 01:47

bitcoin locked and limited conversation to collaborators Aug 31, 2024

		CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.total_amount :
		(available_coins.total_effective_amount.has_value() ? *available_coins.total_effective_amount : 0);

	// Obtain the expected selected inputs count after the merge (for now, duplicates are not allowed)
	// Store sum of combined input sets to check that no UTXO was used twice

	// Reduce output values for subtractFeeFromAmount
	if (coin_selection_params.m_subtract_fee_outputs) {
	CAmount to_reduce = fee_needed - nFeeRet;
	int i = 0;
	bool fFirst = true;
	for (const auto& recipient : vecSend)
	{
	if (i == nChangePosInOut) {
	++i;
	}
	CTxOut& txout = txNew.vout[i];

	if (recipient.fSubtractFeeFromAmount)
	{
	txout.nValue -= to_reduce / outputs_to_subtract_fee_from; // Subtract fee equally from each selected recipient

	if (fFirst) // first receiver pays the remainder not divisible by output count
	{
	fFirst = false;
	txout.nValue -= to_reduce % outputs_to_subtract_fee_from;
	}

	// Error if this output is reduced to be below dust
	if (IsDust(txout, wallet.chain().relayDustFee())) {
	if (txout.nValue < 0) {
	return util::Error{_("The transaction amount is too small to pay the fee")};
	} else {
	return util::Error{_("The transaction amount is too small to send after the fee has been deducted")};
	}
	}
	}
	++i;
	}
	nFeeRet = fee_needed;

wallet: bugfix, invalid CoinsResult cached total amount #26560

wallet: bugfix, invalid CoinsResult cached total amount #26560

Uh oh!

Conversation

furszy commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugs

Improvements

Test Coverage

Uh oh!

DrahtBot commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

S3RK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sjors commented Nov 28, 2022

Uh oh!

fanquake commented Nov 28, 2022

Uh oh!

furszy commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Nov 28, 2022

Uh oh!

luke-jr commented Nov 29, 2022

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

josibake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

furszy commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josibake commented Nov 29, 2022

Uh oh!

furszy commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josibake commented Nov 30, 2022

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy commented Nov 23, 2022 •

edited

Loading

DrahtBot commented Nov 23, 2022 •

edited

Loading

furszy commented Nov 28, 2022 •

edited

Loading

furszy commented Nov 29, 2022 •

edited

Loading

furszy commented Nov 29, 2022 •

edited

Loading

achow101 commented Nov 29, 2022 •

edited

Loading

furszy Dec 2, 2022 •

edited

Loading