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.
Adds a --descriptors option globally to the test framework. This will
make the test create and use descriptor wallets. However some tests may
not work with this.
Some tests are modified to work with --descriptors and run with that
option in test_runer:
* wallet_basic.py
* wallet_encryption.py
* wallet_keypool.py
* wallet_keypool_topup.py
* wallet_labels.py
* wallet_avoidreuse.py
In advance of deprecating the generate RPC method, make some small
changes to a small number of inidividual test cases:
- make memory checking less prescriptive in wallet_basic.py
- replace calls to generate with generatetoaddress in wallet_keypool.py
- replace calls to generate with generatetoaddress and fixup label
issues in wallet_labels.py
- replace calls to generate with generatetoaddress in wallet_multiwallet.py
c1dde3a949 No longer shutdown after encrypting the wallet (Andrew Chow)
d7637c5a3f After encrypting the wallet, reload the database environment (Andrew Chow)
5d296ac810 Add function to close all Db's and reload the databae environment (Andrew Chow)
a769461d5e Move BerkeleyEnvironment deletion from internal method to callsite (Andrew Chow)
Pull request description:
This is the replacement for #11678 which implements @ryanofsky's [suggestion](https://github.com/bitcoin/bitcoin/pull/11678#pullrequestreview-76464511).
Shutting down the software was to prevent the BDB environment from writing unencrypted private keys to disk in the database log files, as was noted [here](https://bitcointalk.org/index.php?topic=51474.msg616068#msg616068). This PR replaces the shutdown behavior with a CDBEnv flush, close, and reopen which achieves the same effect: everything is cleanly flushed and closed, the log files are removed, and then the environment reopened to continue normal operation.
To ensure that no unencrypted private keys are in the log files after encrypting the wallet, I wrote [this script](https://gist.github.com/achow101/7f7143e6c3d3fdc034d3470e72823e9d) to pull private keys from the original wallet file and searches for these keys in the log files (note that you will have to change your file paths to make it work on your own machine).
As for concerns about private keys being written to slack space or being kept in memory, these behaviors no longer exist after the original wallet encryption PR and the shutting down solution from 2011.
cc @ryanofsky
Tree-SHA512: 34b894283b0677a873d06dee46dff8424dec85a2973009ac9b84bcf3d22d05f227c494168c395219d9aee3178e420cf70d4b3eeacc9785aa86b6015d25758e75
Change all instances of validateaddress to getaddressinfo since it seems that
no test actually uses validateaddress for actually validating addresses.
-BEGIN VERIFY SCRIPT-
find ./test/functional -path '*py' -not -path ./test/functional/wallet_disable.py -not -path ./test/functional/rpc_deprecated.py -not -path ./test/functional/wallet_address_types.py -exec sed -i'' -e 's/validateaddress/getaddressinfo/g' {} \;
-END VERIFY SCRIPT-
