This should be reviewed carefully. The new availablecoins_test was
showing an intermittent error [1]. The test variants were split out into
their own test cases to work around the issue, but requires further
investigation.
[1]: https://github.com/ElementsProject/elements/issues/1370
Please reviewthis commit *extremely thoroughly*.
This commit changed how certain transaction size calculations are done.
This altered fee estiimation for certain elements transactions, and caused a lot
of things to break in the `wallet_bumpfee` functional test.
After *extensive* work (~10 hours), I managed to figure all this out and properly
reconcile the functional test to work with the new fee calculations.
However, the original bitcoin commit didn't have any changes to this functional test.
So, it's also simply possible that I may have made an actual bug and these fee issues
I was seeing in the functional test were pointing at this bug.
I'm not sure if there is a bug. I don't think so... I think the transaction estimations just got
bigger in certain cases.
But... please verify this. Look into the diff of this commit, and compare how use_max_sig used
to be calculated in a bunch of different ways for different use cases, but is now being calculated
only once (inside of DummySignInput), in the same way for all use cases. Did I accidentally introduce
a bug there?
In other words... I'm not certain about the wallet changes here. Please double and triple check them.
Thanks!
I commented out a new assertion on transaction size in test/functional/rpc_fundrawtransaction.py.
That calculation needs to be updated to work properly for elements.
This was a tricky merge (wallet refactor). Please double check my
changes to CreateTransactionInternal(...) in src/wallet/spend.cpp, and to
SendGenerationTransaction(...) in src/wallet/rpc/elements.cpp.
Lastly, I got all the tests passing, but I'm not certain that the changes
with fixed_change_pos in CreateTransactionInternal were correct.
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)
This merge was a bit tricky, since in Elements the witnesses are not
part of the transaction inputs themselves. This merge should be reviewed
carefully.
Prior to coin selection we need to indicate that the issuances will take
extra space, otherwise we may fail to select enough coins to cover our
fees, triggering the new "fee needed exceeds fees available" assertion.
The Elements 22 blinding logic has an edge case where when we drop change,
leaving only a single blinded output, we recompute a bunch of blinding
data to handle the potential for us to have 0 inputs and 1 output to blind.
(BlindTransaction will fail in this case because it cannot make the
transaction balance with only one output to mess with.)
In this recomputation, we dropped more data than we meant to, causing us
to incorrectly blind an output.
First, this reverts commit ca2d72ae8b to reinstate
an assertion that was added in Bitcoin #22686. It did not compile because our
`change_and_fee` variable is a map rather than number; I changed it to use
`map_change_and_fee.at(policyAsset)` to match the equivalent change 2 lines down
from a5d97b363b (merge of Bitcoin #22008).
Then fix the following bugs:
1. Change the new test in rpc_fundrawtransaction.py to bump the -maxtxfee value,
which we'd otherwise exceed, failing the test and masking actual failures.
(This was just caused by the extreme fee settings of the test combined with
Elements' large transactions.)
2. Change the fee-output size estimation for `tx_noinputs_size` to be 46 rather
than 44 bytes; we forgot that even null surjection/rangeproofs need a 0 byte
when output witnesses are present. This mistake triggered the new assertion.
3. Correct the logic in which change outputs are sometimes dropped even when
they are the only blinded output in a transaction with blinded inputs. This
would cause the new test to fail with `bad-txn-inputs-ne-outputs`; I'm very
surprised that no existing tests hit this.
(I have an existing comment block in this code where I "promise" that I had
a good reason for doing something mysterious related to blinding. I was not
able to reverse-engineer my intention here, though I think it is related to
this, but since I couldn't understand it I just left this block intact and
worked around it.)
4. This then triggered the assertion again since the coin selection code
assumes that sufficiently-small change will always be dropped. If we prevent
this drop we will have under-funded the transaction.
To fix this we add Yet Another Flag `may_need_blinded_dummy` in which we add
extra weight to `tx_noinputs_size` in the case that we're doing a blinded tx
but have no blind destinations. We turn this off after coin selection if it
turns out that we don't have any blinded inputs, though ofc at that point
much of the damage/inefficiency has already been done..
5. Fix some constants in other functional tests which assumed precise fee
calculations; these precise values changed because of fixes (2) and (4).
There is one new FIXME, which is that the "dummy change" value will now be a
zero-valued OP_RETURN but we still put a full-size rangeproof and surjection
proof on it. There is some plausible privacy benefit to this but not much,
and wasting 5000+ bytes rather than the ~65 needed for an exact-value proof
is not worth it. We will fix this in the future when we overhaul the wallet
blinding logic.
When the fee is not subtracted from the outputs, the amount that has
been reserved for the fee (change_and_fee - change_amount) must be
enough to cover the fee that is needed. It would be a bug to not do so,
so use an assert to make this obvious if such a situation were to occur.
Github-Pull: bitcoin/bitcoin#22686
Rebased-From: d9262324e8
Run coin selection on each OutputType separately, choosing the best
solution according to the waste metric.
This is to avoid mixing UTXOs that are of different OutputTypes,
which can hurt privacy.
If no single OutputType can fund the transaction, then coin selection
considers the entire wallet, potentially mixing (current behavior).
This is done inside AttemptSelection so that all OutputTypes are
considered at each back-off in coin selection.
Pass the whole CoinsResult struct to SelectCoins instead of only a
vector. This means we now have to remove preselected coins from each
OutputType vector and shuffle each vector individually.
Pass the whole CoinsResult struct to AttemptSelection. This involves
moving the logic in AttemptSelection to a newly named function,
ChooseSelectionResult. This will allow us to run ChooseSelectionResult
over each OutputType in a later commit. This ensures the backoffs work
properly.
Update unit and bench tests to use CoinResult.
