Note that this is a breaking change in Core's RPC -- if you add pegins
to createfundedpsbt then it won't do coin selection unless you provide
the additional new add_inputs option.
This is the 43-commit descriptor wallet PR. It was remarkably easy to merge, given
its magnitude. With this commit Elements supports importing Bitcoin descriptors
and deriving (Bitcoin) addresses, though of course it does not support blinding
yet. That is a post-rebase project.
The material changes were:
1. Changing constants in the tests (super annoying but nothing surprising)
2. Adding a missing "skip if this coin is not ours" check in src/script/sign.cpp
which was causing us to erroneously remove existing witnesses from transactions.
This wasn't a problem before this commit since we would only ask specific
scriptpubkeymans to sign, and we'd never ask any to sign inputs we didn't
own. Andy simplified the logic here to always try every scriptpubkeyman,
which means they have to play a bit more nicely with each other.
Other than that, this was a big diff with many conflicts but literally all of
them were "we both added code" and the resolution was to take both sides.
This modifies the CreateTransaction loop in a way not remotely worth the complexity,
and includes an absurdly fragile test where I had to add a bunch of trace statements
and tweak pretty-much every single hardcoded number. Not to name names, but it was
Sjors. (In fairness, the PR is a pure simplification of the CreateTransaction logic,
and it wasn't hard to merge even. It was just the test that caused my grief.)
Adapting the "use a dummy CTxDestination in the case that we cannot retrieve one from
the wallet" logic to our `mapScriptChange` map was not trivial. On my first attempt I
incorrectly assigned a positive vout index to the dummy script, which caused us to
call `ReturnDestination` later on the (unused) dummy destination. This is harmless now,
but when descriptor wallets are introduced in #16528, they introduce an edge case where
returning a null destination can incorrectly mark the 0th key of a BIP32 range as
unused. This triggered a test failure much later, in #19504, which uses descriptor
wallets in fundrawtransaction. The bug was that we'd import a descriptor, mark the
first key as being used, lock the wallet, call `fundrawtransaction` on a transaction
that did not require change (incorrectly marking the first key as unused but leaving
it in the descriptor ScriptPubKeyMan's cache), then call `fundrawtransaction` again
on a transaction that *did* require change. The wallet would then incorrectly retrieve
the "unused" key from cache and use it for change, rather than correctly failing and
advising the user that it could not produce change with a locked wallet and empty
keypool. This was not a fun bug to track down.
Another interesting observation is that branch-and-bound uses the CT size-overestimate
for change when trying to create changeless outputs, while our normal dust detection
uses Core's unchanged "an output is 133 bytes" logic. So when BnB is used we're willing
to delete a far bigger change output than we are when we don't use BnB.
Lest you think this works in Core, they're also inconsistent because BnB uses a
normal fee estimate for gauging change cost, while non-BnB uses the discardfee rate.
My advice is to hold your nose, pull stuff in from Core as it comes in, and thanks
to Andy's efforts things are getting better. Don't bother reviewing this too closely.
Fixes the change-size estimation bug that I fixed in #17290, though incompletely
(Steven made up a size for the surjection proof which was different from the
size that I made up....this time I actually checked with libsecp to get an upper
bound.) He also found another place we were doing the wrong estimation, which
I had missed on my pass. So between the two of us I think we've done some good.
Also moves some which sets code coin_selection_params.tx_noinputs_size to after
some Elements sanity checks, which I think will have zero observable effect
(or non-observable effect) but it's part of the PR so I'm keeping it. Though
updated since we can now use BnB even with subtract-fee-from-output.
I really like this PR, but it wound up being pretty nontrivial to merge.
The crux is that it pulls PSBT signing logic into scriptpubkey manager,
which is where it belongs, but for us this means reasoning about pegins
inside script/sign.cpp.
However, sign.cpp is part of libbitcoin_common, which does not include
anything for reasoning about PoW or RPC (lol) or anything heavy about
that. This means that some pegin validation had to remain split between
the wallet/rpc layer and sign.cpp. I added a new file script/pegins.cpp
which has the (one) method we actually need in sign.cpp.
Aside from that, this diff is very large but is mostly just moving our
code changes to wallet/psbtwallet.* into wallet/wallet.* where those
functions now live. As far as review, it's probably not worthwhile to
spend too much too much time on this since it's going to be change
again in #16528 and others. The test coverage is pretty extensive.
Moved GetOnlinePakKey to the scriptpubkey manager; the other blinding
stuff remains in the CWallet. At some point after the rebase we should
consider whether there's a better separation to be had between the PAK
related stuff now that we have wallet boxes.
This PR associates OutputType::BECH32 to PAK online keys, where before we
were able to directly access keys from the keypool. In a future refactoring
we should give PAK keys their own output type (and own scriptpubkey manager)
so that the wallet won't accept payments "to the PAK key".
Also changes `ReserveDestination::SetBlindingPubKey` to use a visitor pattern
to apply a blinding pubkey to a destination directly, rather than using the
old hacky method of regenerating the destination by pulling its key (which
is no longer contained in the class) out and giving it to a new constructor.
This was a long-overdue refactoring and the minimal-diff way to get the code
compiling (and it's not bad, maybe 10LOC to add a new visitor class) but
nonetheless I apologize for sticking this into a merge commit.
This Elements PR includes components of Core PR #17211, which since the
refactors to use effective value landed, no longer provides the right
error message when a user provides an unowned input from a wallet tx.
See https://github.com/bitcoin/bitcoin/pull/17211#pullrequestreview-528389011
This breaks a functional test which was included in this PR, but which
conveniently has been changed in the current version of the Core PR. I
fixed the behavior (commented, in SelectCoins) rather than updating the
test to the most recent version.
