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
fa4339e4c1 Extract CTxIn::MAX_SEQUENCE_NONFINAL constant (MarcoFalke)
Pull request description:
Extracting the constant makes it possible to attach documentation to it.
Also, rework the docs for the other "sequence constants".
ACKs for top commit:
w0xlt:
reACK fa4339e for specifying the transaction version.
darosior:
re-ACK fa4339e4c1
luke-jr:
crACK fa4339e4c1
Tree-SHA512: 8d8f3dd5afb33eb5b72aa558e1e03de874c5ed02aa1084888e92ed86f3aaa5c725db45ded02e14cdfa67a92ac6774e97185b697f20a8ab63abbfcaa2fcd1fc6a
fac8165443 Remove unused checkFinalTx (MarcoFalke)
fa272eab44 wallet: Avoid dropping confirmed coins (MarcoFalke)
888841ea8d interfaces: Remove unused is_final (MarcoFalke)
dddd05e7a3 qt: Treat unconfirmed txs as unconfirmed (MarcoFalke)
Pull request description:
The wallet has several issues:
## Unconfirmed txs in the GUI
The GUI clumsily attempts to guess if unconfirmed txs are locked until a future time. This is currently based on the locktime only, not nSequence, thus wrong. Fix this by removing the clumsy code and treat all unconfirmed txs as unconfirmed. The GUI already prints whether a tx is in the mempool, in which case the user knows that the tx wasn't locked until a future time. If the tx is not in the mempool, it might be better to report the exact reject reason from the mempool instead of using incorrect heuristics.
## Confirmed txs in the wallet
The wallet drops coins that it incorrectly assumes to be locked until a future time, even if they are already confirmed in the chain. This is because the wallet is using the wrong time (adjusted network time) instead of MTP, due to the `-1` default argument of `CheckFinalTx`.
The issues are fixed in separate commits and there is even a test.
ACKs for top commit:
achow101:
ACK fac8165443
prayank23:
reACK https://github.com/bitcoin/bitcoin/pull/24067/commits/fac816544317cee6553d60cb0f5f24f6f9ec98de
glozow:
code review ACK fac8165443, I understand now how this fixes both issues.
Tree-SHA512: 210afb855f4c6d903fee49eba6b1a9735d699cf0168b669eabb38178e53b3a522258b7cc669f52489c6cd3e38bf358afde12eef3ba2e2f2ffaeb06b8f652ccd0
e5b6aef612 Move CBlockFileInfo::ToString method where class is declared (Russell Yanofsky)
f7086fd8ff Add src/wallet/* code to wallet:: namespace (Russell Yanofsky)
90fc8b089d Add src/node/* code to node:: namespace (Russell Yanofsky)
Pull request description:
There are no code changes, this is just adding `namespace` and `using` declarations and `node::` or `wallet::` qualifiers in some places.
Motivations for this change are:
- To make it easier to see when node and wallet code is being accessed places where it shouldn't be. For example if GUI code is accessing node and wallet internals or if wallet and node code are referencing each other.
- To make source code organization clearer ([#15732](https://github.com/bitcoin/bitcoin/issues/15732)), being able to know that `wallet::` code is in `src/wallet/`, `node::` code is in `src/node/`, `init::` code is in `src/init/`, `util::` code is in `src/util/`, etc.
Reviewing with `git log -p -n1 -U0 --word-diff-regex=.` can be helpful to verify this is only updating declarations, not changing code.
ACKs for top commit:
achow101:
ACK e5b6aef612
MarcoFalke:
Concept ACK e5b6aef612🍨
Tree-SHA512: 3797745c90246794e2d55a2ee6e8b0ad5c811e4e03a242d3fdfeb68032f8787f0d48ed4097f6b7730f540220c0af99ef423cd9dbe7f76b2ec12e769a757a2c8d
In SelectCoins, for our preset inputs, we combine all of the preset
inputs into a single OutputGroup. This allows us to combine the preset
inputs with additional selection algo results.
9d0379cea6 consensus: use <cstdint> over <stdint.h> in amount.h (fanquake)
863e52fe63 consensus: make COIN & MAX_MONEY constexpr (fanquake)
d09071da5b [MOVEONLY] consensus: move amount.h into consensus (fanquake)
Pull request description:
A first step (of a few) towards some source code reorganization, as well as making libbitcoinconsensus slightly more self contained.
Related to #15732.
ACKs for top commit:
MarcoFalke:
concept ACK 9d0379cea6 🏝
Tree-SHA512: 97fc79262dcb8c00996852a288fee69ddf8398ae2c95700bba5b326f1f38ffcfaf8fa66e29d0cb446d9b3f4e608a96525fae0c2ad9cd531ad98ad2a4a687cd6a
Conflicts were mostly obvious, except for those in CreateTransactionInternal.
(This function was moved from wallet.cpp to spend.cpp, and substantially
rewritten, between 0.21 and 22.0.) For those I manually applied the changes
from the diff, which wound up taking a slightly different form.
Also had to update the new test because the `addresses` field of the RPC
output was removed.
