It returns an incorrect result when called with a Decimal,
for which the "//" operator works differently.
Also drop unnecessary call to satoshi_round.
Github-Pull: #24239
Rebased-From: d1fab9d5d2
(cherry picked from commit 269553fe73b17f8acda3071a48836c66092d31d0)
NB: This code is commented out in elements and the cherry-pick is done
commenting out the code too.
Because of floating point precision issues, not all of the rounding done
is always correct. To fix this, the fee calculation for
assert_fee_amount is changed to better reflect how CFeeRate::GetFee does
it.
First the feerate is converted to an int representing sat/kvb. Then this
is multiplied by the transaction size, divivided by 1000, and rounded up
to the nearest sat. The result is then converted back to BTC (divided by
1e8) and then rounded down to the nearest sat to avoid precision errors.
Github-Pull: #22949
Rebased-From: 80dc829be7
(cherry picked from commit c768bfa08af034c744402d4294cc323d653b97b8)
When calculating a txs absolute fee, if the fee is rounded down to the
nearest satoshi, it is possible for the coin selection algorithms to
undercalculate the fee needed. This can lead to an assertion error in
some situations. One such scenario is added to
rpc_fundrawtransaction.py.
Github-Pull: #22949
Rebased-From: ce2cc44afd
(cherry picked from commit f66bc42957ad2e86982c8c487f821683d3009b43)
When calculating the fee for a given tx size from a fee rate, we should
always round up to the next satoshi. Otherwise, if we round down (via
truncation), the calculated fee may result in a fee with a feerate
slightly less than targeted.
This is particularly important for coin selection as a slightly lower
feerate than expected can result in a variety of issues.
Github-Pull: #22949
Rebased-From: 0fbaef9676
(cherry picked from commit bd7e08e36bf2e1238ddf8cc01433f8db82f848c9)
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.
(cherry picked from commit e5e3ec2700)
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.
(cherry picked from commit fac694be4c)
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.
Modifies the getnewblockhex json rpc call to accept an array of
commitments instead of a single commitment.
Backwards compatibility is maintained by first attempting to parse as a
string for a singular commitment.
A single 0x00 byte indicates a zero length field; we must skip parsing
that field otherwise the length will be expected to be read again for
the vector that is passed in to revieve the value.
This allows PSBT_ELEMENTS_GLOBAL_SCALAR to be parsed when it is
serialized according to the spec, i.e. both of the following cases
will correctly parse to the same representation:
$cli decodepsbt 'cHNldP8B+wQCAAAAAQIEAgAAAAEEAQABBQEAJ/wEcHNldAABAgMEBQYHCAkKCwwNDg8QERITFBUWFxgZGhscHR4fIAEAAA=='
and
$cli decodepsbt 'cHNldP8B+wQCAAAAAQIEAgAAAAEEAQABBQEAJ/wEcHNldAABAgMEBQYHCAkKCwwNDg8QERITFBUWFxgZGhscHR4fIAAA'
PSBT_ELEMENTS_GLOBAL_SCALAR is the only PSBT/PSET field that contains
key data but no value data and so is the only field that currently hits
this special case.
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.
This facilitates the following workflow:
1) Obtain an updated psbt with in_witness_utxo and in_utxo_rangeproof
2) Get the blinding key from the input utxo address obtained from input
script pubkey without revealing master blinding key
3) Rewind the proof to obtain blinding factors and implement stateless
blinding
ApproximateBestSubset had an edge case (due to not using
GetSelectionAmount) where it was possible for it to return success but
fail to select enough to cover transaction fees. A test is added that
could trigger this failure prior to the fix being implemented.
Github-Pull: bitcoin/bitcoin#22686
Rebased-From: 92885c4f69
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.
