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.
This one took a little bit of work. Basically everywhere that Andy's new code
didn't compile, I looked for a similarly shaped line in the pre-PR diff between
Bitcoin and Elements and adjusted it in the same way. Was pretty typical; all
CAmounts become CAmountMaps, etc.
I then spent about 7 hours chasing down a coin selection failure in the blocksign
functional tests, which ultimately turned out to be a Core issue. I added a
stopgap fix (commented, search for "stopgap" in wallet.cpp), and opened the issue
https://github.com/bitcoin/bitcoin/issues/20347
There was also a bug where we were not estimating change output sizes correctly
in case we had massive blinded outputs, which I had to fix to get the functional
tests to pass.
This is the start of some nontrivial wallet refactoring by achow. See
https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes
for a high-level design.
This PR moves some stuff out of CWallet into a dummy "box" LegacyScriptPubKeyMan
which is (currently) very tightly coulped to CWallet. Because of the coupling there
are currently null-checks that cannot fail, things which assume non-nullness which
will eventually be wrong, and a plethora of currently-equivalent ways to get from
a CWallet to a provider or back.
Our approach is basically to ignore the refactoring; leave the blinding key stuff
in CWallet and have it call into the wallet's provider when we are obtaining the
underlying keys.
Later, probably in a post-rebase PR, we should rethink how we manage blinding keys
to more closely match Core's "all keys go into providers" model.
