fix for intermittent assertion error in getselectionwaste
introduced in f46275d011 as part of merge
for bitcoin/bitcoin#25647
the assertion would trigger in a fundrawtransaction call in
feature_confidential_transactions.py where the transaction inputs
included a non-policy asset.
during the loop in getselectionwaste, if the non-policy input waste was
calculated first the assertion would be triggered because
selected_effective_value has not yet met the selection target, because
non-policy coins have 0 effective value
the fix changed the assertion to an if statement, only adding to the
waste metric when selected_effective_value meets the target.
This was a difficult merge as always with diffs that touch spend.cpp and
createtransactioninternal. Added a FIXME NB for fundrawtransaction with
external outputs, which MUST be fixed before a new release.
Squash of 2 commits from https://github.com/ElementsProject/elements/pull/1258
- correct application of non_policy_effective_value to policy output in KnapsackSolver
- replace bad fee amount assert with error log and graceful failure in CWallet::CreateTransactionInternal
(cherry picked from commit 0f92a38254)
Update src/wallet/coinselection.cpp
Co-authored-by: Byron Hambly <byron@hambly.dev>
(cherry picked from commit cf0f56107b)
The 'GroupOutputs()' function performs the same
calculations for only-positive and mixed groups,
the only difference is that when we look for
only-positive groups, we discard negative utxos.
So, instead of wasting resources calling GroupOutputs()
for positive-only first, then call it again to include
the negative ones in the result, we can execute
GroupOutputs() only once, including in the response
both group types (positive-only and mixed).
And not hide it inside the `OutputGroup::Insert` method.
This method does not return anything if insertion fails.
We can know before calling `Insert` whether the coin
will be accepted or not.
76dc547ee7 gui: create tx, launch error dialog if backend throws runtime_error (furszy)
f4d79477ff wallet: coin selection, add duplicated inputs checks (furszy)
0aa065b14e wallet: return accurate error messages from Coin Selection (furszy)
7e8340ab1a wallet: make SelectCoins flow return util::Result (furszy)
e5e147fe97 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. https://github.com/bitcoin/bitcoin/pull/26560#discussion_r1037395665
2. https://github.com/bitcoin/bitcoin/pull/25729#discussion_r940619491
3. https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-1135240825
4. https://github.com/bitcoin/bitcoin/issues/23144 (which is connected to #24845)
ACKs for top commit:
ishaanam:
crACK 76dc547ee7
achow101:
ACK 76dc547ee7
aureleoules:
ACK 76dc547ee7
theStack:
ACK 76dc547ee7🌇
Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5
81d4a2b14f refactor: Move feerate comparison invariant outside of the loop (yancy)
365aca4045 refactor: Simplify feerate comparison statement (yancy)
Pull request description:
This is a small nit, however I think it's more understandable to write:
`utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee`
vs
`(utxo_pool.at(0).fee - utxo_pool.at(0).long_term_fee) > 0`
ACKs for top commit:
Xekyo:
ACK 81d4a2b14f
achow101:
ACK 81d4a2b14f
aureleoules:
ACK 81d4a2b14f
Tree-SHA512: 3e89377989c36716b53114fe40178261671dde5688075fab1c21ec173ac310f8c84ed6af90354d7c329176cb7262dfcaa7191fd19847d3b7147a9a10c3e31176
As no process should be able to trigger this error
using the regular transaction creation process, throw
a runtime_error if happens to tell users/devs to
report the bug if happens.
First step towards decoupling the pre-selected-inputs fetching functionality
from `SelectCoins`. Which, will let us not waste resources calculating the
available coins if one of the pre-set inputs has an error.
(right now, if one of the pre-set inputs is invalid, we first walk through
the entire wallet txes map just to end up failing right after it finish)
