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
0628311b53 chainparams: change magic bytes for liquidv1test (Andrew Poelstra)
Pull request description:
This is basically a Liquid-prod-ish version of regtest. Its default magic should not collide with that for Liquid prod.
ACKs for top commit:
psgreco:
UTACK 0628311b53
Tree-SHA512: 50c387ffeb62d42ffd0b42d0f43335966feb64a50140159c615474b9c864222e2a7510bb3b07aa95f62a977d0e7472d905bc97a06a3ace80aae0a519031c446e
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.
There are a number of special cases (and inconsistencies with wallet.cpp)
in the blinding logic that can prevent blinding from succeeding when various
intermediate values are zero. This fixes two of them.
A proper fix would involve adding new APIs to secp-zkp and then overhauling
all of the wallet blinding code. But for now let's get the tests working.
When the user specifies a change address manually, use the change address
to obtain blinding parameters (either extract the blinding key from the
address or don't blind the change).
The previous behavior would assume that the change address was owned by
the wallet and always generate a blinding key internally. If the user
were to pass a non-wallet-owned change address, the result would be an
output that could not be unblinded by its owner.
d1a1d19113 test: add decodepsbt and analyzepsbt tests for blinding proofs (Andrew Poelstra)
03b835c887 PSET: output warnings in decodepsbt and analyzepsbt about blinding status (Andrew Poelstra)
694ec795e4 PSET: fix asset proof generation and verification (Andrew Poelstra)
c6f801d4ce PSET: encapsulate blind proof checks into one method (Andrew Poelstra)
35cfda73b9 PSET: do not assume in GetUnsignedTx that explicit amounts/assets are available (Andrew Poelstra)
c88eb96e74 pset: check that we can get the blinding factors from any IsMine outputs before signing (Andrew Poelstra)
9c55d0a175 pset: only check asset/amount proofs in case both explicit+blinded values are provided (Andrew Poelstra)
Pull request description:
After conversation with @stepansnigirev in #1026 I realize we need to adjust our sanity checks on asset proofs. In particular,
* Rather than enforcing that all confidential values have corresponding explicit values with proofs, we should only check that **if** both confidential and explicit values are present, they are connected by a proof.
* Further, the wallet should check before signing that any `IsMine` scripts have confidential values that we are actually able to spend (i.e. rewinding works).
This PR implements these two things, and also extends the `analyzepsbt` and `decodepsbt` RPC calls to provide more information if the amount/asset proofs are missing or invalid. It adds tests for these and also demonstrates that `combinepsbt` does not really work without explicit data present.
`walletprocesspsbt`, when blinding, will still produce these proofs and there is currently no way to disable this behavior.
ACKs for top commit:
achow101:
ACK d1a1d19113
Tree-SHA512: 472ee0fe285308e803cbc3f5885e513ec91d1b3f18a2973772cd5a88d29fa4dba9441d9b3bd58847564e43d9a7478bbc75af62d9326c727709b5ff2dc646714a
A couple issues with this -- one is that it was verifying the asset
commitment against itself, rather than against the explicit asset,
and the other is that the verification logic had an extra `== 0` at
the end which inverted the verification check.
Both pretty embarassing to have not caught in review..
71c9e90fa0 test: add another functional test for rangeproofs in issuances (Andrew Poelstra)
cf4136b3b0 SIGHASH_RANGEPROOF and asset issuance rangeproofs (Andrew Poelstra)
d4a0b6285a test: check asset issuances with SIGHASH_RANGEPROOF (Andrew Poelstra)
1b23ad7093 pset: fix encoding of blinded asset issuances (Andrew Poelstra)
0907fa0f6f test: support asset issuance in test framework sighashes (Andrew Poelstra)
34eafaf761 test: sign using the PSET interface and SIGHASH_RANGEPROOF (Andrew Poelstra)
9c3c61aa0a psbt: make sure rangeproofs are present in the confidential version of the unsigned tx (Andrew Poelstra)
7d39e430c7 test: check that `signrawtransaction` does not erase RANGEPROOF signatures (Andrew Poelstra)
7a3cbda848 sign.cpp: turn on SIGHASH_SCRIPT_RANGEPROOF when checking already-existing signatures (Andrew Poelstra)
ac54a2f765 test: check that signing with the RANGEPROOF flag works (Andrew Poelstra)
ab5b376b72 SignatureHash: fix rangeproof hash for SIGHASH_RANGEPROOF (Andrew Poelstra)
Pull request description:
Fixes#1026
Supercedes #932
ACKs for top commit:
achow101:
ACK 71c9e90fa0
Tree-SHA512: ddc8074b6783344aa8b04de9e0a3d9320f70aa5f2bccd05d7e329110d861e46b2730b230a1b89ddb414dec8e9d31d04ab66e589d3f9648692fd990d03c369e81
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).
Currently when a user uses `signrawtransaction` on a transaction that is
already partially signed with SIGHASH_RANGEPROOF signatures, the existing
signatures will be deleted because they are not recognized as valid. This
makes it impossible to collabratively sign transactions using this RPC.
Fix this.
The logic for computing the uncached version of the sighash changed during
the 0.21 rebase, such that it no longer matched the cached version. As the
changed hash is used during signing (not verification!), this was not a
forking change (and our existing functional test would have caught such a
forking change since it uses Python to independently compute the hash).
But it still broke signing.
Test in next commit.
3cb9612faa test: add test for Taproot activation (Andrew Poelstra)
5291c0d9d9 chainparams: add undocumented regtest/testnet only -con_taproot_signal_start option (Andrew Poelstra)
cc6b933478 add missing taproot activation params for Liquid v1 (Andrew Poelstra)
bec6bcf31b versionbits: allow specific deployments to override the signalling/threshold values (Andrew Poelstra)
Pull request description:
Sets Taproot to start signalling around noon (California time) on Nov 1, 2021, assuming 95% of blocks are produced between now and then.
Will activate after one week of 100% signalling. If we can pull this off on the first or second try, we will beat Bitcoin which currently looks like it will activate on Nov 16.
**Edit:** actually, even without Speedy Trial, there is one full period (week) where Taproot will be "locked in" but not "active". So it will activate on Nov 15 at the earliest.
ACKs for top commit:
achow101:
ACK 3cb9612faa
Tree-SHA512: c3a80d39ba86a0d762a3057cb9c45e379c70c2daee8ff2e54978a4c32118c935c8909d872ac9f873e283441c52c1974c06268f10dc7bf9e9e000c21063ac84d7
1204b91c2d re-enable fallbackfee by default (Andrew Poelstra)
Pull request description:
Upstream disabled the default `fallbackfee` option in 0.17, which caused us some user error reports in 0.18. Re-enable the option in 0.21.
ACKs for top commit:
stevenroose:
utACK 1204b91c2d
Tree-SHA512: 62d053405c00f5ca31ae3fb24ab8def2c9e3bd52909dfac57e65224947295a56f0b8a322ffeb0dfcdcb3103fcaf310a9554f3ed3a8490859ca82aaeae26c7fb2
e57b4c51ad regtest: change network magic away from liquid v1 (Andrew Poelstra)
Pull request description:
We don't want connectivity issues between Liquid and custom test networks.
ACKs for top commit:
stevenroose:
ACK e57b4c51ad
Tree-SHA512: 3ddd626496d7877551c41b089932784bcb752962089fd3e6baa5e9ea6acac44b1b4c5c915b061892b712849d12455fae00347885cecb0e0a3b59a134d91d4014
fc8bfc1f2c claimpegin: add regression test for behavior when fallbackfee is disabled (Andrew Poelstra)
fd520079a1 claimpegin: sanity-check fee in case fallbackfee is disabled, throw explicit exception (Andrew Poelstra)
Pull request description:
Produce a much less confusing error message in `claimpegin` in the case that estimatesmartfee fails and no fallbackfee is set.
ACKs for top commit:
stevenroose:
ACK fc8bfc1f2c
Tree-SHA512: 2ae658ab3dc90b7319d047c192401a0dcc97e015e45b5b0db71db1d89d63605ba107312d8b4cf1c4aaef72437aa20aa33b442744f64fc08535b15610e6f4a526
bcd7de55a4 fuzz: update psbt fuzztest for PSBT2 changes (Andrew Poelstra)
Pull request description:
The `psbt.tx` Optional is no longer safe to dereference just because decoding succeeded.
ACKs for top commit:
achow101:
ACK bcd7de55a4
Tree-SHA512: 701ace683a2615a944903ec69a20d1808da03009bec6e7af76954476d4e66b122fe01657285252b90fad92cc8aadd46095c80a1ccf454ae11f0e86a550ce1a0b
048ca4a550 pset: fix logic bug in `analyzepsbt` so that the "blinder" role may be returned (Andrew Poelstra)
ec27e74f21 pset_tutorial: write 2-party PSET coinjoin demo (Andrew Poelstra)
a7643260a4 assets_tutorial: move Daemon into its own file (Andrew Poelstra)
Pull request description:
Adds a second executable tutorial which uses the PSET API to produce a swap transaction.
ACKs for top commit:
achow101:
ACK 048ca4a550
Tree-SHA512: 61720d905401a79dc9b6d6ae20a69b6216592e6b5b245c7e11c0b8323ff125a27a9d4eb1ba5b76b7024c91dc3dd0765c19ec7ae06315f5e719758d3569b0cb6f
