Re-enables and fixes the script_assets_test by changing feature_taproot.py to include the genesis block hash when dumping the json tests.
Requires the json file from https://github.com/ElementsProject/qa-assets/pull/1
Co-authored-by: Andrew Poelstra <apoelstra@wpsoftware.net>
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.
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
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.
a67a2df330 pset: remove one more intermediate-zero check from the blinding logic (Andrew Poelstra)
9eb285c19d test: add test for confidential non-wallet-owned change (Andrew Poelstra)
185d473fe8 walletcreatefundedpsbt: add functional test for blinding edge cases (Andrew Poelstra)
533da12c2c wallet: make sure extra OP_RETURN output is blinded when called from fundraw (Andrew Poelstra)
b09b63bd1b pset: allow input blinding factors to sum to zero, or value to be 0 (Andrew Poelstra)
9afcb83baf wallet: correctly handle blinding of manually-set change addresses (Andrew Poelstra)
9813c3e74a wallet: fix "cannot unblind IsMine output" check in SignPSBT (Andrew Poelstra)
7103471fd5 walletcreatefundedpsbt: signal blinding data correctly to `FundTransaction` (Andrew Poelstra)
Pull request description:
Fixes#1049
Needs backport to 0.21 (and a new rc).
Although there are several bugs here, none affect the functionaries. There is a more thorough summary in the second-to-last commit message.
ACKs for top commit:
achow101:
ACK a67a2df330
Tree-SHA512: 25066c29f080e43cd00c5b33c60a986a8cb5bbf4ca01ceb3b4182c5b8f61979a1d6d946b8f28fa871ec90f44bdb6fc22014b5c11280cf081181db102cd588cdf
This test actually hits almost all of the edge cases covered by the
previous commits.
1. The "`walletcreatefundedpsbt` does not propagate blinding factors"
issue is actually covered by several existing tests, which did not
fail, but which revealed other bugs as soon as I fixed the issue.
2. Similarly, the "properly blind (or don't blind) explicit change
addresses" issue was covered by existing tests, but was revealed
upon fixing the issue with verifying unblindability in SignPSBT,
since it resulted in using the wrong blinding key to produce
rangeproofs for change.
2a. (While addressing this, I was able to test the SignPSBT checks
extensively, because the code was producing bad rangeproofs. But
in the final result this is *not* covered, since it is hard, or
hopefully impossible, to cajole the wallet into producing outputs
that can't be unblinded now.)
3. Once that was fixed, the existing tests create an output whose
total blinding factor (`v_b + v*a_b`) is zero, which it does by
having no blinded inputs, explicit change/fee, and a single
blinded output. This output is spent in the new test. This tests
one of the "blinding may fail due to zero intermediate values"
cases.
4. The new test then intentionally tests that `walletcreatefundedpsbt`
creates OP_RETURN outputs only when it is legitimately given a
"blinded inputs, no blinded outputs" transaction, which tests the
original bug (that blinding factors were propagated correctly from
`walletfundpsbt`).
5. In doing so, it hit the "OP_RETURN outputs are not blinded when
called from fundrawtransaction" bug.
6. And once this was fixed, `BlindPSBT` has to blind a zero-valued
output, which triggers the other "blinding may fail due to zero
intermediate values" case.
For purposes of assigning blame, I'd say that half of this omnibug came
from the new PSET logic and half was latent in Elements for years. The
most concerning thing, I'd say, is the "explicit change addresses cause
outputs to be blinded to the wrong key" bug, which I believe has been
around for the entire history of Elements.
Asset issuance rangeproofs are not actually covered by SIGHASH_RANGEPROOF
so this test serves more as a sanity check that our various signing
mechanisms still work with issuances. Sure enough, it uncovered a PSET
bug (fixed) and a blindrawtransaction bug (left a FIXME).
