fa93ef5a8aeae36304c792697a78af2d07fd9f41 refactor: Take Span in SetSeed (MarcoFalke)
Pull request description:
This makes calling code less verbose and less fragile. Also, by adding
the CKey::data() member function, it is now possible to call HexStr()
with a CKey object.
ACKs for top commit:
sipa:
utACK fa93ef5a8aeae36304c792697a78af2d07fd9f41
laanwj:
Code review ACK fa93ef5a8aeae36304c792697a78af2d07fd9f41
theStack:
Code-review ACK fa93ef5a8aeae36304c792697a78af2d07fd9f41
Tree-SHA512: 73fb999320719ad4b9ab5544018a7a083d140545c2807ee3582ecf7f441040a30b5157e85790b6b840af82f002a7faf30bd8162ebba5caaf2067391c43dc7e25
11115169a14d0d0be5b7b1c3f6fdc9673a9098d9 ci: Build fuzz with libsqlite3-dev (MarcoFalke)
fa7c6efca66627e4c76adecc824f96da220af69c fuzz: Add wallet fuzz test (MarcoFalke)
fa59d2ce5b8d6fe8c610f170a13675c756aef58f refactor: Use local args instead of global gArgs in CWallet::Create (MarcoFalke)
fadb44606f26a80daf4320eee046c9572e85fe3e build: Inline FUZZ_SUITE_LDFLAGS_COMMON (MarcoFalke)
Pull request description:
Initial sketch to fuzz descriptor wallets. Can be improved in the future.
ACKs for top commit:
mjdietzx:
Code review ACK 1111516
Tree-SHA512: b1d2f24504d1ed5f3c6a031210f04c27c13d4e15576c4acbf50ded37ac45f7b7a5c7553e91d81d4a06e9ea73b3d745a552218d3ef3b2932fa5325a8331b0d3fd
This makes calling code less verbose and less fragile. Also, by adding
the CKey::data() member function, it is now possible to call HexStr()
with a CKey object.
54011e7aa274bdc1b921440cc8b4623aa1e0d89e refactor: use CWallet const shared pointers when possible (Karl-Johan Alm)
96461989a2de737151bc4fb216221bf49cb53ce6 refactor: const shared_ptrs (Karl-Johan Alm)
Pull request description:
```C++
const std::shared_ptr<CWallet> wallet = x;
```
means we can not do `wallet = y`, but we can totally do `wallet->DestructiveOperation()`, contrary to what that line looks like.
This PR
* introduces a new convention: always use const shared pointers to `CWallet`s (even when we mutate the pointed-to thing)
* uses `const shared_ptr<const CWallet>` everywhere where wallets are not modified
In the future, this should preferably apply to all shared pointers, not limited to just `CWallet`s.
Both of these serve the same purpose: to dispell the misconception that `const shared_ptr<X>` immutates `X`. It doesn't, and it's dangerous to leave this misconception as is, for obvious reasons.
ACKs for top commit:
theStack:
re-ACK 54011e7aa274bdc1b921440cc8b4623aa1e0d89e
Tree-SHA512: 3bf4062fc821751be30770c6b4ead10a016847970f155a0a5156f304347d221b9830840030c2fbfba8cd1e282f4eda45f5b4107fe6df8138afdcb6c2e95a2836
Introduce convention to use const shared pointers everywhere, unless the shared pointer is modified at some point, which it very rarely is.
We want this convention, as it helps alleviate the misconception that a const shared pointer somehow results in a pointer to an immutable object, which is false.
Follow-up to:
* commit 700c42b85d20e624bef4228eef062c93084efab5, which replaced pIndex
with block_hash in AddToWalletIfInvolvingMe.
* commit 9700fcb47feca9d78e005b8d18b41148c8f6b25f, which replaced
posInBlock with confirm.nIndex.
In the method `CWallet::LoadActiveScriptPubKeyMan`, the map
`external_spk_managers` (or `internal_spk_managers`, if parameter
`internal` is false) is accessed via std::map::operator[], which means
that a default-ctored entry is created with a null-pointer as value, if
the key doesn't exist. As soon as this value is dereferenced, a
segmentation fault occurs, e.g. in `CWallet::KeypoolCountExternalKeys`.
The bevaviour can be reproduced by the following steps (starting with empty regtest datadir):
$ ./src/bitcoind -regtest -daemon
$ ./src/bitcoin-cli -regtest -named createwallet_name=wallet descriptors=true blank=true
$ cat regtest-descriptors.txt
[
{
"desc": "tr([e4445899/49'/1'/0']tprv8ZgxMBicQKsPd8jCeBWsYLEoWxbVgzJDatJ7XkwQ6G3uF4FsHuaziHQ5JZAW4K515nj6kVVwPaNWZSMEcR7aFCwL4tQqTcaoprMKTTtm6Zg/1/*)#mr3llm7f",
"timestamp": 1634652324,
"active": true,
"internal": true,
"range": [
0,
999
],
"next": 0
}
]
$ ./src/bitcoin-cli -regtest importdescriptors "$(cat regtest-descriptors.txt)"
[
{
"success": true
}
]
$ ./src/bitcoin-cli -regtest getwalletinfo
error: timeout on transient error: Could not connect to the server 127.0.0.1:18443 (error code 1 - "EOF reached")
Bug reported by Josef Vondrlik (josef-v).
a0efe529e4fd053b890450413b9ca5e1bcd8f2c2 Fix outdated comments referring to ::ChainActive() (Samuel Dobson)
Pull request description:
After #21866 there are a few outdated comments referring to `::ChainActive()`, which should instead refer to `ChainstateManager::ActiveChain()`.
ACKs for top commit:
jamesob:
ACK a0efe529e4
Tree-SHA512: 80da19c105ed29ac247e6df4c8e916c3bf3f37230b63f07302114eef9c115add673e9649f0bbe237295be0c6da7b1030b5b93e14daf6768f17ce5de7cf2c9ff2
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding calls
to methods which will be unsafe after the transaction to std::filesystem
to due lack of a boost::filesystem::path::imbue equivalent and inability
to set a predictable locale.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Kiminuo <kiminuo@protonmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
9d0379cea6c164610d05287ae6dd4e66f35b92b3 consensus: use <cstdint> over <stdint.h> in amount.h (fanquake)
863e52fe63a67fa020fb1ef527b9095a35ab77a5 consensus: make COIN & MAX_MONEY constexpr (fanquake)
d09071da5bc997f2de1f55ca7a9babc3d7619329 [MOVEONLY] consensus: move amount.h into consensus (fanquake)
Pull request description:
A first step (of a few) towards some source code reorganization, as well as making libbitcoinconsensus slightly more self contained.
Related to #15732.
ACKs for top commit:
MarcoFalke:
concept ACK 9d0379cea6c164610d05287ae6dd4e66f35b92b 🏝
Tree-SHA512: 97fc79262dcb8c00996852a288fee69ddf8398ae2c95700bba5b326f1f38ffcfaf8fa66e29d0cb446d9b3f4e608a96525fae0c2ad9cd531ad98ad2a4a687cd6a
928af61cdb2c4de1c3d10e6fda13bbba5ca0bba9 allow send rpc take external inputs and solving data (Andrew Chow)
e39b5a5e7aa4d015257565ca79dc7b1f7a65e074 Tests for funding with external inputs (Andrew Chow)
38f5642cccf2b6708e58f5e2af5ecdcf752e61ec allow fundtx rpcs to work with external inputs (Andrew Chow)
d5cfb864ae16da62399bc97ab1ed54d32cf0cce9 Allow Coin Selection be able to take external inputs (Andrew Chow)
a00eb388e8046fe105666445dff6c91e8f8664cb Allow CInputCoin to also be constructed with COutPoint and CTxOut (Andrew Chow)
Pull request description:
Currently `fundrawtransaction` and `walletcreatefundedpsbt` both do not allow external inputs as the wallet does not have the information necessary to estimate their fees.
This PR adds an additional argument to both those RPCs which allows the user to specify solving data. This way, the wallet can use that solving data to estimate the size of those inputs. The solving data can be public keys, scripts, or descriptors.
ACKs for top commit:
prayank23:
reACK 928af61cdb
meshcollider:
Re-utACK 928af61cdb2c4de1c3d10e6fda13bbba5ca0bba9
instagibbs:
crACK 928af61cdb2c4de1c3d10e6fda13bbba5ca0bba9
yanmaani:
utACK 928af61.
Tree-SHA512: bc7a6ef8961a7f4971ea5985d75e2d6dc50c2a90b44c664a1c4b0f1be5c1c97823516358fdaab35771a4701dbefc0862127b1d0d4bfd02b4f20d2befa4434700
240ea294d5e899a906f213f039b21e94c24d6018 doc: update doxygen documention of ComputeTimeSmart() and AddToWalletIfInvolvingMe() regarding rescanning_old_block parameter (BitcoinTsunami)
d6eb39af21810bf1c3bdce0ef2212c1ad6597bcd test: add functional test to check transaction time determination during block rescanning (BitcoinTsunami)
07b44f16e71b9df10dfac7f32f92997938f7e7aa wallet: fix ComputeTimeSmart algorithm to use blocktime during old block rescanning (BitcoinTsunami)
Pull request description:
The function ComputeTimeSmart in wallet.cpp assume that transaction are discovered in the right order.
Moreover the 'smarttime' determination algorithm is coded with realtime scenario in mind and not rescanning of old block.
The functional test demonstrate that if the user import a wallet, then rescan only recent history, and then rescan the entire history, the older transaction discovered would have an incorrect time determination.
In the context of rescanning old block, the only time value that as a meaning is the blocktime.
That's why I've fixed the problem with a simple separation between rescanning of old block and realtime time determination. The fix is written to have no impact on every realtime scenario and only impact the behaviour during a rescanning process.
This PR Fixes#20181.
To be fair, I don't think that this bug could be triggered with the wallet GUI, because it always proceed with a proper rescan.
But RPC API provide the possibility to trigger it. I've discovered it, because Specter desktop v0.10.0 was impacted. (https://github.com/cryptoadvance/specter-desktop/issues/680).
ACKs for top commit:
jonatack:
ACK 240ea294d5e899a906f213f039b21e94c24d6018 per `git diff b92d552 240ea29`, re-verified rebase to latest master + debug build clean + new test passes on the branch and fails on master, only change since my review a few hours ago is incorporation of latest review suggestions
meshcollider:
re-utACK 240ea294d5e899a906f213f039b21e94c24d6018
Tree-SHA512: 514b02e41d011ddfa325f5e8080b93800e1ea4ed5853fa420670a6ac700e8b463000dbea65f8ced8565cfb950c7f51b69154034dcb111e67aca3b964a0061494
Improve readability of code, simplify future scripted diff cleanup PRs, and be
more consistent with naming for GetBoolArg.
This will also be useful for replacing runtime settings type checking
with compile time checking.
-BEGIN VERIFY SCRIPT-
git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Followup to commit "MOVEONLY: CWallet transaction code out of
wallet.cpp/.h" that detaches and renames some CWalletTx methods, making
into them into standalone functions or CWallet methods instead.
There are no changes in behavior and no code changes that aren't purely
mechanical. It just gives spend and receive functions more consistent
names and removes the circular dependencies added by the earlier
MOVEONLY commit.
There are also no comment or documentation changes. Removed comments
from transaction.h are just migrated to spend.h, receive.h, and
wallet.h.
86beee05795216738f51fa744539336503c26fd9 Use waste metric for deciding which selection to use (Andrew Chow)
b3df0caf7c291a316298e54e73426c765e61c129 tests: Test GetSelectionWaste (Andrew Chow)
4f5ad43b1e05cd7b403f87aae4c4d42e5aea810b Add waste metric calculation function (Andrew Chow)
935b3ddf72aa390087684e03166c707f5b173434 scripted-diff: tests: Use KnapsackSolver directly (Andrew Chow)
6a023a6f904efe38dacd662d919aba74f066b1dc tests: Add KnapsackGroupOutputs helper function (Andrew Chow)
d5069fc1aa7d335f3043227f843cbb9d8ba1507b tests: Use SelectCoinsBnB directly instead of AttemptSelection (Andrew Chow)
54de7b47463d98f860167d4e0b7e4ebb3926b59c Allow the long term feerate to be configured, default of 10 sat/vb (Andrew Chow)
Pull request description:
Branch and Bound introduced a metric that we call waste. This metric is used as part of bounding the search tree, but it can be generalized to all coin selection solutions, including those with change. As such, this PR introduces the waste metric at a higher level so that we can run both of our coin selection algorithms (BnB and KnapsackSolver) and choose the one which has the least waste. In the event that both find a solution with the same change, we choose the one that spends more inputs.
Also this PR sets the long term feerate to 10 sat/vb rather than using the 1008 block estimate. This allows the long term feerate to be the feerate that we switch between consolidating and optimizing for fees. This also removes a bug where the long term feerate would incorrectly be set to the fallback fee. While this doesn't matter prior to this PR, it does have an effect following this. The long term feerate can be configured by the user through a new `-consolidatefeerate` option.
ACKs for top commit:
Xekyo:
reACK 86beee0 via git range-diff fe47558...86beee0
meshcollider:
re-utACK 86beee05795216738f51fa744539336503c26fd9
Tree-SHA512: 54b154b346538eca68ae2a3b83a033b495c1605c14f842bfc43ded2256b110983ce674c647fe753cf0305b1b178403d8d60d6d4203c7a712bec784be52e90d42
The long term feerate is really the highest feerate that the user is
comfortable with making consolidatory transactions. This is should thus
be something that can be configured by the user via a new startup option
-consolidatefeerate. The default value is 10 sat/vbyte, chosen
arbitrarily (it seems like a reasonable number).
c3c213215b25f3e6f36d46b1d49dfcc3040cee1c Use `context.args` in `src/wallet/load.cpp`. (Kiminuo)
25de4e77feddf9b2f4d134bab5faa26c3e5a764d Use `context.args` in `CWallet::Create` instead of `gArgs`. (Kiminuo)
aa5e7c9471c50771bc77b0ec4e0e0929e4a32eae Fix typo in bitcoin-cli.cpp (Kiminuo)
Pull request description:
The PR attempts to move us an inch towards the [goal](https://github.com/bitcoin/bitcoin/pull/21244#discussion_r615307465) by using `WalletContext` in `wallet.{h|cpp}` code instead of relying on the global state (i.e. `gArgs`).
Edit: The PR builds on #19101.
ACKs for top commit:
ryanofsky:
Code review ACK c3c213215b25f3e6f36d46b1d49dfcc3040cee1c. Changes since last review: just rebasing and adding wallet load commit
Tree-SHA512: 2b436f5a219e32c2d529f55a89edca086d949396cebf9e089a21aa7b1c180e3c0fb17468f415dfc834f8e1614f8b3914c7e9a0bd33b95e7e0199c0dfe5ca9490
But in case of no keys or a blank hd wallet the iterator would be skipped
and not set to false but true, since the loop would be not entered.
That had resulted in a wrong return and subsequent false HD and watch-only
icon display in gui when reloading a wallet after closing.
Update src/wallet/wallet.cpp
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
92993aa5cf37995e65e68dfd6f129ecaf418e01c Change SignTransaction's input_errors to use bilingual_str (Andrew Chow)
171366e89b828a557f8262d9dc14ff7a03f813f7 Use bilingual_str for address fetching functions (Andrew Chow)
9571c69b51115454c6a699be9492024f7b46c2b4 Add bilingual_str::clear() (Andrew Chow)
Pull request description:
In a couple of places in the wallet, errors are `std::string`. In order for these errors to be translated, change them to use `bilingual_str`.
ACKs for top commit:
hebasto:
re-ACK 92993aa5cf37995e65e68dfd6f129ecaf418e01c, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22337#pullrequestreview-694542729) review, verified with
klementtan:
Code review ACK 92993aa5cf37995e65e68dfd6f129ecaf418e01c
meshcollider:
Code review ACK 92993aa5cf37995e65e68dfd6f129ecaf418e01c
Tree-SHA512: 5400e419dd87db8c49b67ed0964de2d44b58010a566ca246f2f0760ed9ef6a9b6f6df7a6adcb211b315b74c727bfe8c7d07eb5690b5922fa5828ceef4c83461f
fa6fd3dd6a4e7f30eff5963836aed43fe01af078 wallet: Properly set fInMempool in mempool notifications (MarcoFalke)
Pull request description:
A wallet method (like bumping the fee) might have set `fInMempool` to false because the transaction was removed from the mempool (See commit fa4e088cbac035b8029a10b492849540150d0622).
Avoid setting it back to true (incorrectly) in the validation interface background thread.
Fixes#22357
ACKs for top commit:
ryanofsky:
Code review ACK fa6fd3dd6a4e7f30eff5963836aed43fe01af078. Only change since last review is extending workaround to `transactionRemovedFromMempool`. Since we know this workaround is imperfect and the goal of this PR is mainly to fix CI errors, I would probably be inclined to limit the workaround to as few places as possible where we have seen actual failures, instead of adding the workaround to as many places as possible, where there is some chance it might trigger new failures. But since this workaround is so straightforward and almost looks like a real fix, probably it doesn't matter.
meshcollider:
utACK fa6fd3dd6a4e7f30eff5963836aed43fe01af078
Tree-SHA512: d690136a577f1f532aa1fee80d3f6600ff7fc61286fbf564a53d7938d5ae52d33f0dbb0fef8b8c041a4970fb424f0b9f1ee7ce791e0ff8354e0000ecc9e22b84
DEBUG_LOCKORDER expects cs_wallet, cs_main, and cs_KeyStore to be
acquired in that order. However dumpwallet would take these in the order
cs_wallet, cs_KeyStore, cs_main. So when configured with
`--enable-debug`, it is possible to hit the lock order assertion when
using dumpwallet.
To fix this, cs_wallet and cs_KeyStore are no longer locked at the same
time. Instead cs_wallet will be locked first. Then the functions which
lock cs_main will be run. Lastly cs_KeyStore will be locked afterwards.
This avoids the lock order issue.
Furthermore, since GetKeyBirthTimes (only used by dumpwallet) also uses
a function that locks cs_main, and itself also locks cs_KeyStore, the
same reordering is done here.
6084d2caed9b2c70c0f19898c33ecb141fe603c8 wallet: do not spam about non-existent spk managers (S3RK)
Pull request description:
Avoid spam in logs during `loadwallet`, `listdescriptors` and probably other commands as well.
**`loadwallet` Before:**
```
2021-06-24T06:31:45Z init message: Loading wallet…
2021-06-24T06:31:45Z [desc] Wallet File Version = 169900
2021-06-24T06:31:45Z [desc] Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total. Unknown wallet records: 0
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Wallet completed loading in 197ms
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] setKeyPool.size() = 0
2021-06-24T06:31:45Z [desc] mapWallet.size() = 0
2021-06-24T06:31:45Z [desc] m_address_book.size() = 0
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] External scriptPubKey Manager for output type 2 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 0 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 1 does not exist
2021-06-24T06:31:45Z [desc] Internal scriptPubKey Manager for output type 2 does not exist
{
"name": "desc",
"warning": ""
}
```
**After:**
```
2021-06-24T06:26:58Z init message: Loading wallet…
2021-06-24T06:26:58Z [desc] Wallet File Version = 169900
2021-06-24T06:26:58Z [desc] Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total. Unknown wallet records: 0
2021-06-24T06:26:58Z [desc] Wallet completed loading in 158ms
2021-06-24T06:26:58Z [desc] setKeyPool.size() = 0
2021-06-24T06:26:58Z [desc] mapWallet.size() = 0
2021-06-24T06:26:58Z [desc] m_address_book.size() = 0
{
"name": "desc",
"warning": ""
}
```
ACKs for top commit:
achow101:
ACK 6084d2caed9b2c70c0f19898c33ecb141fe603c8
Tree-SHA512: c7d7345c3182a575db088fd731b7f6e428c42e4f3f2e10d5adb50bf74a2defe88768e65ebb91a08590be48cf766a5697e36fafa73f68ffe45e76a60600f072e2
In many places in ScriptPubKeyMan managing code, we assume that the
ScriptPubKeyMan being retrieved actually exists and is not a nullptr.
Thus removing a ScriptPubKeyMan requires erasing the object from the
map rather than setting it to a nullptr.
181181019c5baa3e2d5b675d1843a45aa028781c refactor: remove m_internal from DescriptorSPKman (S3RK)
Pull request description:
Rationale: improve consistency between `CWallet` and `DescriptorScriptPubKeyMan`; simplify `ScriptPubKeyMan` interface.
Descriptor in itself is neither internal or external. It's responsibility of a wallet to assign and manage descriptors for a specific purpose. Duplicating information about internalness of a descriptor could lead to inconsistencies and unexpected behaviour (for example misreporting keypool size).
ACKs for top commit:
instagibbs:
reACK 181181019c
achow101:
reACK 181181019c5baa3e2d5b675d1843a45aa028781c
Tree-SHA512: d5613b7f6795b290bfa0fd8cb0536de1714d0cf72cba402266bd06d550758ebad690b54fc0a336a1c7414b5814aa4a37c90a6ae89926474a97d30956d7e034ff
3efaf83c75cd8dc2fa084537b8ed6715fb58c04d wallet: deactivate descriptor (S3RK)
6737d9655bcf527afbd85d610d805a2d0fd28c4f test: wallet importdescriptors update existing (S3RK)
586f1d53d60880ea2873d860f95e3390016620d1 wallet: maintain SPK consistency on internal flag change (S3RK)
f1b7db14748d9ee04735b4968366d33bc89aea23 wallet: don't mute exceptions in importdescriptors (S3RK)
bf68ebc1cd555f791103f81adc9111e0e55c8003 wallet: allow to import same descriptor twice (S3RK)
Pull request description:
Rationale: allow updating existing descriptors with `importdescriptors` command.
Currently if you run same `importdescriptors` command twice with a descriptor containing private key you will get very confusing error — `Missing required fields`. What happens is that Wallet tries to write imported private key to the disk, but it exists already so we get `DB_KEYEXIST (-30995)` from BerkelyDB. Please note, that we set `DB_NOOVERWRITE` (I guess not to lose some keys accidentally). The exception is caught in `catch (...)` in rpcdump.cpp with a generic error.
With this PR if a descriptor is already present than we will update its activeness, internalness, label, range and next_index.
For the range only expansion is allowed (range start can only decrease, range end increase).
ACKs for top commit:
achow101:
re-ACK 3efaf83c75cd8dc2fa084537b8ed6715fb58c04d
meshcollider:
Code review ACK 3efaf83c75cd8dc2fa084537b8ed6715fb58c04d
jonatack:
Light ACK 3efaf83c75cd8dc2fa084537b8ed6715fb58c04d per `git range-diff a000cb0 5d96704 3efaf83` and as a sanity check, re-debug-built on debian with gcc 10.2.1 and clang 11, ran wallet_importdescriptors.py
Tree-SHA512: 122c4b621d64ec8a3b625f3aed9f01a2b5cbaf2029ad0325b5ff38d67fff5cd35324335fabe2dd5169548b01b267c81be6ae0f5c834342f3d5f6eeed515c4843
e6cf0ed92de31a5ac35a271b0da8f0a8364d1175 wallet, rpc: listdescriptors does not need unlocked (Andrew Chow)
3280704886b60644d103a5eb310691c003a39328 Pass in DescriptorCache to ToNormalizedString (Andrew Chow)
7a26ff10c2f2e139fbc63e2f37fb33ea4efae088 Change DescriptorImpl::ToStringHelper to use an enum (Andrew Chow)
75530c93a83f3e94bcb78b6aa463c5570c1e737e Remove priv option for ToNormalizedString (Andrew Chow)
74fede3b8ba69e2cc82c617cdf406ab79df58825 wallet: Upgrade existing descriptor caches (Andrew Chow)
432ba9e5434da90d2cf680f23e8c7b7164c9f945 wallet: Store last hardened xpub cache (Andrew Chow)
d87b544b834077f102724415e0fada6ee8b2def2 descriptors: Cache last hardened xpub (Andrew Chow)
cacc3910989c4f3d7afa530dbab042461426abce Move DescriptorCache writing to WalletBatch (Andrew Chow)
0b4c8ef75cd03c8f0a8cfadb47e0fbcabe3c5e59 Refactor Cache merging and writing (Andrew Chow)
976b53b085d681645fd3a008fe382de85647e29f Revert "Cache parent xpub inside of BIP32PubkeyProvider" (Andrew Chow)
Pull request description:
Currently fetching a normalized descriptor requires the wallet to be unlocked as it needs the private keys to derive the last hardened xpub. This is not very user friendly as normalized descriptors shouldn't require and don't involve the private keys except for derivation. We solve this problem by caching the last hardened xpub (which has to be derived at some point when generating the address pool).
However the last hardened xpub was not already being cached. We only cached the immediate parent xpub and derived child keys. For example, with a descriptor derivation path of `/84'/0'/0'/0/*`, the parent xpub that is cached is `m/84'/0'/0'/0`, and the child keys of `m/84'/0'/0'/0/i` (note that child keys would not be cached in this case). This parent xpub is not suitable for the normalized descriptor form as we want the key at `m/84'/0'/0'`. So this PR adds another field to `DescriptorCache` to cache the last hardened xpub so that we can use them for normalized descriptors.
Since `DescriptorCache` is changing, existing descriptor wallets need to be upgraded to use this new cache. The upgrade will occur in the background either at loading time (if the wallet is not encrypted) or at unlocking time in the same manner that `UpgradeKeyMetadata` operates. It will use a new wallet flag `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` to indicate whether the descriptor wallet has the last hardened xpub cache.
Lastly `listdescriptors` will not require the wallet to be locked and `getaddressinfo`'s `parent_desc` will always be output (assuming the upgrade has occurred).
ACKs for top commit:
fjahr:
tACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175
S3RK:
reACK e6cf0ed
jonatack:
Semi ACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175 reviewed, debug-built and ran unit tests and some of the descriptor functional tests at each commit. I'm not very familiar with this code and it could be clearer to the uninitiated IMHO, so I'm not confident enough to give a full ACK. Various minor suggestions follow, most of them for readability, feel free to pick and choose.
meshcollider:
Code review + functional test run ACK e6cf0ed92de31a5ac35a271b0da8f0a8364d1175
Tree-SHA512: ac27aade8644525cd65bfcaf27ff32afb974085b1451faf4ff68c6671a690bd6a41d4f39a33cbf461ae0fbe85995c0a4c08dbd36171da1c1d2a1d00053ad298d
Descriptor in itself is neither internal or external.
It's responsibility of a wallet to assign and manage descriptors
for a specific purpose. Duplicating such information could lead to
inconsistencies and unexpected behaviour.
