backport: bitcoin#18836, #19046, #19490, #20403, #21127, #21238, #21329 - descriptor wallets part V & sethdseed #6017
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
knst
force-pushed
the
bp-descriptors-5
branch
2 times, most recently
from
dc92bb1
to
2a3166b
Compare
UdjinM6
reviewed
May 8, 2024
| @@ -4272,6 +4270,85 @@ static RPCHelpMan send() | |||
| }; | |||
| } | |||
|
|
|||
| static RPCHelpMan sethdseed() | |||
| { | |||
| return RPCHelpMan{"sethdseed", | |||
| @@ -665,7 +665,8 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, | |||
| } else if (strType != DBKeys::BESTBLOCK && strType != DBKeys::BESTBLOCK_NOMERKLE && | |||
| strType != DBKeys::MINVERSION && strType != DBKeys::ACENTRY && | |||
| strType != DBKeys::VERSION && strType != DBKeys::SETTINGS && | |||
| strType != DBKeys::PRIVATESEND_SALT && strType != DBKeys::COINJOIN_SALT) { | |||
| strType != DBKeys::PRIVATESEND_SALT && strType != DBKeys::COINJOIN_SALT && | |||
| strType != DBKeys::FLAGS) { | |||
There was a problem hiding this comment.
21127 should drop this: e3968fa#diff-49a07da77d79017222ba989b99048c0a629d3230988ce7b6b74df0d14f727626R557-R563 no?
…ted changes The key difference between bitcoin's and dash's implementation that sethdseed does not update existing seed for wallet. Seed can be set only once. It behave similarly to `upgradetohd` rpc, but since v20.1 all wallets are HD and the name `upgradetohd` is not relevant more.
This commit helps to unify our implementation with bitcoin which has the similar refactoring in bitcoin#17681 but we DNM it due to difference in sethdseed behavior: we do not replace seed ever
… with Add/Load variants 3a9aba2 Split SetWalletFlags into Add/LoadWalletFlags (Andrew Chow) d9cd095 Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan (Andrew Chow) 0122fba Split SetHDChain into AddHDChain and LoadHDChain (Andrew Chow) Pull request description: `SetHDChaiin`, `SetActiveScriptPubKeyMan`, and `SetWalletFlags` have a `memonly` argument which is kind of confusing, as noted in bitcoin#17681 (comment). This PR replaces those functions with `Add*` and `Load*` variants so that they follow the pattern used elsewhere in the wallet. `AddHDChain`, `AddActiveScriptPubKeyMan`, and `AddWalletFlags` both set their respective variables in `CWallet` and writes them to disk. These functions are used by the actions which modify the wallet such as `sethdseed`, `importdescriptors`, and creating a new wallet. `LoadHDChain`, `LoadActiveScriptPubKeyMan`, and `LoadWalletFlags` just set the `CWallet` variables. These functions are used by `LoadWallet` when loading the wallet from disk. ACKs for top commit: jnewbery: Code review ACK 3a9aba2 ryanofsky: Code review ACK 3a9aba2. Only changes since last review tweaks making m_wallet_flags updates more safe meshcollider: utACK 3a9aba2 Tree-SHA512: 365aeaafc5ba42879c0eb797ec3beb29ab70e27f917dc880763f743420b3be6ddf797240996beed8a9ad70fb212c2590253c6b44c9dc244529c3939d9538983f
facd7dd wallet: Fix typo in comments; Simplify assert (MarcoFalke) Pull request description: Follow up to bitcoin#19046 (comment) and bitcoin#19046 (comment) ACKs for top commit: practicalswift: ACK facd7dd jonatack: ACK facd7dd hebasto: ACK facd7dd, spelling verified with `test/lint/lint-spelling.sh`: all remaining warnings are false positive. Tree-SHA512: 2b185d138058840db56726bb6bcc42e5288a954e2a410c49e04806a047fbbdaf0bb2decc70ecf7613c69caa766655705ca44151613e7ea5015b386d1e726d870
5f9c0b6 wallet: Remove -upgradewallet from dummywallet (MarcoFalke) a314271 test: Remove unused wallet.dat (MarcoFalke) bf76359 tests: Test specific upgradewallet scenarios and that upgrades work (Andrew Chow) 4b418a9 test: Add test_framework/bdb.py module for inspecting bdb files (Andrew Chow) 092fc43 tests: Add a sha256sum_file function to util (Andrew Chow) 0bd995a wallet: upgrade the CHDChain version number when upgrading to split hd (Andrew Chow) 8e32e1c wallet: remove nWalletMaxVersion (Andrew Chow) bd7398c wallet: have ScriptPubKeyMan::Upgrade check against the new version (Andrew Chow) 5f72054 wallet: Add GetClosestWalletFeature function (Andrew Chow) 842ae38 wallet: Add utility method for CanSupportFeature (Andrew Chow) Pull request description: This PR cleans up the wallet upgrade mechanism a bit, fixes some probably bugs, and adds more test cases. The `nWalletMaxVersion` member variable has been removed as it made `CanSupportFeature` unintuitive and was causing a couple of bugs. The reason this was introduced originally was to allow a wallet upgrade to only occur when the new feature is first used. While this makes sense for the old `-upgradewallet` option, for an RPC, this does not quite make sense. It's more intuitive for an upgrade to occur if possible if the `upgradewallet` RPC is used as that's an explicit request to upgrade a particular wallet to a newer version. `nWalletMaxVersion` was only relevant for upgrades to `FEATURE_WALLETCRYPT` and `FEATURE_COMPRPUBKEY` both of which are incredibly old features. So for such wallets, the behavior of `upgradewallet` will be that the feature is enabled immediately without the wallet needing to be encrypted at that time (note that `FEATURE_WALLETCRYPT` indicates support for encryption, not that the wallet is encrypted) or for a new key to be generated. `CanSupportFeature` would previously indicate whether we could upgrade to `nWalletMaxVersion` not just whether the current wallet version supported a feature. While this property was being used to determine whether we should upgrade to HD and HD chain split, it was also causing a few bugs. Determining whether we should upgrade to HD or HD chain split is resolved by passing into `ScriptPubKeyMan::Upgrade` the version we are upgrading to and checking against that. By removing `nWalletMaxVersion` we also fix a bug where you could upgrade to HD chain split without the pre-split keypool. `nWalletMaxVersion` was also the version that was being reported by `getwalletinfo` which meant that the version reported was not always consistent across restarts as it depended on whether `upgradewallet` was used. Additionally to make the wallet versions consistent with actually supported versions, instead of just setting the wallet version to whatever is given to `upgradewallet`, we normalize the version number to the closest supported version number. For example, if given 150000, we would store and report 139900. Another bug where CHDChain was not being upgraded to the version supporting HD chain split is also fixed by this PR. Lastly several more tests have been added. Some refactoring to the test was made to make these tests easier. These tests check specific upgrading scenarios, such as from non-HD (version 60000) to HD to pre-split keypool. Although not specifically related to `upgradewallet`, `UpgradeKeyMetadata` is now being tested too. Part of the new tests is checking that the wallet files are identical before and after failed upgrades. To facilitate this, a utility function `sha256sum_file` has been added. Another part of the tests is to examine the wallet file itself to ensure that the records in the wallet.dat file have been correctly modified. So a new `bdb.py` module has been added to deserialize the BDB db of the wallet.dat file. This format isn't explicitly documented anywhere, but the code and comments in BDB's source code in file `dbinc/db_page.h` describe it. This module just dumps all of the fields into a dict. ACKs for top commit: MarcoFalke: approach ACK 5f9c0b6 laanwj: Code review ACK 5f9c0b6 jonatack: ACK 5f9c0b6, approach seems fine, code review, only skimmed the test changes but they look well done, rebased on current master, debug built and verified the `wallet_upgradewallet.py` test runs green both before and after running `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2` Tree-SHA512: 7c4ebf420850d596a586cb6dd7f2ef39c6477847d12d105fcd362abb07f2a8aa4f7afc5bfd36cbc8b8c72fcdd1de8d2d3f16ad8e8ba736b6f4f31f133fe5feba
…coverage 3eb6f8b wallet (not for backport): improve upgradewallet error messages (Jon Atack) ca8cd89 wallet: fix and improve upgradewallet error responses (Jon Atack) 99d56e3 wallet: fix and improve upgradewallet result responses (Jon Atack) 2498b04 Don't upgrade to HD split if it is already supported (Andrew Chow) c46c18b wallet: refactor GetClosestWalletFeature() (Jon Atack) Pull request description: This follows up on bitcoin#18836 and bitcoin#20282 to fix and improve the as-yet unreleased `upgradewallet` feature and also implement review follow-up in bitcoin#18836 (comment). This PR fixes 4 upgradewallet issues: - this bug: bitcoin#20403 (comment) - it returns nothing in the absence of an RPC error, which isn't reassuring for users - it returns the same thing both in the case of a successful upgrade and when no upgrade took place - the error message object is currently dead code This PR fixes the above and provides: ...user feedback to not silently return without upgrading ``` { "wallet_name": "disable private keys", "previous_version": 169900, "current_version": 169900, "result": "Already at latest version. Wallet version unchanged." } ``` ...better feedback after successfully upgrading ``` { "wallet_name": "watch-only", "previous_version": 159900, "current_version": 169900, "result": "Wallet upgraded successfully from version 159900 to version 169900." } ``` ...helpful error responses ``` { "wallet_name": "blank", "previous_version": 169900, "current_version": 169900, "error": "Cannot downgrade wallet from version 169900 to version 159900. Wallet version unchanged." } { "wallet_name": "blank", "previous_version": 130000, "current_version": 130000, "error": "Cannot upgrade a non HD split wallet from version 130000 to version 169899 without upgrading to support pre-split keypool. Please use version 169900 or no version specified." } ``` updated help: ``` upgradewallet ( version ) Upgrade the wallet. Upgrades to the latest version if no version number is specified. New keys may be generated and a new wallet backup will need to be made. Arguments: 1. version (numeric, optional, default=169900) The version number to upgrade to. Default is the latest wallet version. Result: { (json object) "wallet_name" : "str", (string) Name of wallet this operation was performed on "previous_version" : n, (numeric) Version of wallet before this operation "current_version" : n, (numeric) Version of wallet after this operation "result" : "str", (string, optional) Description of result, if no error "error" : "str" (string, optional) Error message (if there is one) } ``` ACKs for top commit: achow101: ACK 3eb6f8b MarcoFalke: review ACK 3eb6f8b 🛡 Tree-SHA512: b767314069e26b5933b123acfea6aa40708507f504bdb22884da020a4ca1332af38a7072b061e36281533af9f4e236d94d3c129daf6fe5b55241127537038eed
9305862 wallet: load flags before everything else (Sjors Provoost) Pull request description: Load and set wallet flags before processing other records. That way we can take them into account while processing those other records. Suggested here: bitcoin#16546 (comment) ACKs for top commit: laanwj: Code review ACK 9305862 gruve-p: ACK bitcoin@9305862 achow101: ACK 9305862 Tree-SHA512: 7104523e369ce3c670571fe5e8b52c67b9ca92b8e36a2da5eb6f9f8bf8ed0544897007257204b68f6f371d682b3ef0d0635d36e6e8416ac74af1999d9fbc869c
…root support 0b188b7 Clean up context dependent checks in descriptor parsing (Pieter Wuille) 33275a9 refactor: move uncompressed-permitted logic into ParsePubkey* (Pieter Wuille) 17e006f refactor: split off subscript logic from ToStringHelper (Pieter Wuille) 6ba5dda Account for key cache indices in subexpressions (Pieter Wuille) 4441c6f Make DescriptorImpl support multiple subscripts (Pieter Wuille) a917478 refactor: move population of out.scripts from ExpandHelper to MakeScripts (Pieter Wuille) 84f3939 Remove support for subdescriptors expanding to multiple scripts (Pieter Wuille) Pull request description: These are a few refactors and non-invasive improvements to the descriptors code to prepare for adding Taproot descriptors. None of the commits change behavior in any way, except the last one which improves error reporting a bit. ACKs for top commit: S3RK: reACK 0b188b7 Sjors: re-ACK 0b188b7 achow101: re-ACK 0b188b7 Tree-SHA512: cb4e999134aa2bace0e13d4883454c65bcf1369e1c8585d93cc6444ddc245f3def5a628d58af7dab577e9d5a4a75d3bb46f766421fcc8cc5c85c01a11f148b3f
…use in normalized descriptors e6cf0ed wallet, rpc: listdescriptors does not need unlocked (Andrew Chow) 3280704 Pass in DescriptorCache to ToNormalizedString (Andrew Chow) 7a26ff1 Change DescriptorImpl::ToStringHelper to use an enum (Andrew Chow) 75530c9 Remove priv option for ToNormalizedString (Andrew Chow) 74fede3 wallet: Upgrade existing descriptor caches (Andrew Chow) 432ba9e wallet: Store last hardened xpub cache (Andrew Chow) d87b544 descriptors: Cache last hardened xpub (Andrew Chow) cacc391 Move DescriptorCache writing to WalletBatch (Andrew Chow) 0b4c8ef Refactor Cache merging and writing (Andrew Chow) 976b53b 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 e6cf0ed S3RK: reACK e6cf0ed jonatack: Semi ACK e6cf0ed 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 e6cf0ed Tree-SHA512: ac27aade8644525cd65bfcaf27ff32afb974085b1451faf4ff68c6671a690bd6a41d4f39a33cbf461ae0fbe85995c0a4c08dbd36171da1c1d2a1d00053ad298d
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Next batch of backports related to descriptor wallets.
See related issue to track a progress: https://github.com/dashpay/dash-issues/issues/59
Changes in this PR also used in "hardware signing" feature (coming later)
What was done?
sethdseedthat is based on [wallet] Upgrade path for non-HD wallets to HD bitcoin/bitcoin#12560 and newer related changes.hdChaintom_hd_chain(see wallet: Keep inactive seeds after sethdseed and derive keys from them as needed bitcoin/bitcoin#17681 which is DNM).How Has This Been Tested?
Run unit/functional tests.
The backports bitcoin#18836 and bitcoin#20403 are heavily modified to adopt
wallet_upgradewallet.pyto our codebase.Breaking Changes
N/A
Though,
sethdseedimplementation is not a final version as it is now, can be removed (superseeded byupgradetohdor got mnemonic-feature and super-seedupgradetohd).Checklist: