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.
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).
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
This fixes a bug that was eliminated by #22008 -- although a conceptually
similar one was reintroduced (basically, we do a "test blinding" for fee
estimation, then potentially delete a change output, then we actually blind
the tranasction .... but if removing the change output pushes us into an
edge-case scenario for blinding, Bad Things happen).
Patched in a simple hack. We should fix this properly in a post-22 PR.
Unrelatedly, corrected a comment in the functional test.
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
b2e1cc38de Regression test for pegin validation issues during sync (Glenn Willen)
d5042b41c8 Finish removing 'recheckpeginblockinterval'; move MainchainRPCCheck (Glenn Willen)
313f73d5b2 When validation is waiting for parent chain daemon, "stall". (Andrew Poelstra)
Pull request description:
Forward-port of #1022
ACKs for top commit:
gwillen:
utACK b2e1cc38de, verified that it contains only the requested changes from da11d7b6fd.
Tree-SHA512: 971b6a137efdc54f84995b17595346bf3d3ebd5a04eef1676225c664923e0b52393550bc51ea366c4d3010aa35a9de16da0643bb5bfb40c7eb788a180c99b1a0
Instead of not failing and silently allowing bad PSETs to be created,
provide error messages that explain why the operation the user did was
wrong. Specifically for combining PSETs such that they would become
fully blinded but have imbalanced values and blinders, and for blinding
a PSET when the wallet has a blind input but no outputs to blind (would
result in an imbalance).
Add a regression test for https://github.com/ElementsProject/elements/issues/891 .
This checks that we can sync successfully when making a bunch of new blocks
just as we have transient loss of parent daemon connectivity. This reliably
fails without the fix, and reliably succeeds with it. (It stands in for the
situation, more common in production, where we sync faster than the parent
daemon can keep up after a long outage.)
Currently, if -validatepegin is given, and block validation can't proceed
because the parent chain is not synced, we mark the block invalid and put
it in a queue to be "revalidated" later. Unfortunately, marking a block
invalid has downstream consequences, in particular causing descendant blocks
to be marked invalid, which are not currently fixed by the queue.
Instead, we'll use a different strategy: if the mainchain daemon isn't
sufficiently synced to validate a block, we will "stall" connecting that
block to the chain, and have ActivateBestChain simply keep the tip at the
previous block until we're ready.
We can still download and validate (partly) blocks past this point while
we're waiting. They will be connected once the parent chain daemon catches
up.
