backport: bitcoin#19137, #19253, #20365, #20687, #23349 - descriptor wallets part III #5965
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
added
RPC
knst
changed the title
knst
force-pushed
the
bp-descriptors-3
branch
2 times, most recently
from
72db5c0
to
8bbff5f
Compare
knst
changed the title
src/wallet/wallettool.cpp
Outdated
| // NOTE: drop this condition after removing option to create non-HD wallets | ||
| if (spk_man->IsHDEnabled()) { | ||
| spk_man->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/""); | ||
| } | ||
| spk_man->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/""); |
There was a problem hiding this comment.
we still allow non-hd correct?
There was a problem hiding this comment.
indeed, probably it would fail for non-hd.
GenerateNewHDChain has never been called before, now it is called always. I will prepare a fix
…side WalletTool It should be get-or-create instead just-get
…wallet 173cc9b test: walettool create descriptors (Ivan Metlushko) 345e88e wallettool: add param to create descriptors wallet (Ivan Metlushko) 6d3af3a wallettool: pass in DatabaseOptions into MakeWallet (Ivan Metlushko) Pull request description: Rationale: expose and promote descriptor wallets in more places; make cli tool more consistent with `createwallet` rpc. Add `-descriptors` parameter which is off by default. When specified it will create a new descriptors wallet with sqlite backend, which is consistent with `createwallet` rpc. This PR is based on a suggestion from **ryanofsky** bitcoin#19137 (comment) Example: ``` $ ./src/bitcoin-wallet -wallet=fewty -descriptors create Topping up keypool... Wallet info =========== Name: fewty Format: sqlite Descriptors: yes Encrypted: no HD (hd seed available): yes Keypool Size: 6000 Transactions: 0 Address Book: 0 ``` ``` $ ./src/bitcoin-wallet -wallet=fewty create Topping up keypool... Wallet info =========== Name: fewty Format: bdb Descriptors: no Encrypted: no HD (hd seed available): yes Keypool Size: 2000 Transactions: 0 Address Book: 0 ``` ACKs for top commit: achow101: ACK 173cc9b ryanofsky: Code review ACK 173cc9b. This seems pretty nicely implemented now, with opportunities to clean up more and dedup later MarcoFalke: Concept ACK 173cc9b 🌠 Tree-SHA512: cc32ba336ff709de2707ee15f495b4617908e8700ede8401a58e894f44cda485c544d644023c9a6604d88a62db9d92152383ee2e8abf691688c25cf6e222c622
…h bitcoin-wallet 5b6b5ef util: Use FEATURE_LATEST for wallets created with bitcoin-wallet (Hennadii Stepanov) Pull request description: Since the 49d2374 commit was athored by **jonasschnelli** in 2016, the wallet version was bumped twice: in 2017 (bitcoin#11250) and in 2018 (bitcoin#12560). This PR bumps the version of wallets created with `bitcoin-wallet` offline tool. On master (04437ee) -- `"walletversion": 139900`: ``` $ src/bitcoin-wallet -signet -wallet=211025-test-master create Topping up keypool... Wallet info =========== Name: 211025-test-master Format: sqlite Descriptors: yes Encrypted: no HD (hd seed available): yes Keypool Size: 6000 Transactions: 0 Address Book: 0 $ src/bitcoin-cli -signet -rpcwallet=211025-test-master getwalletinfo { "walletname": "211025-test-master", "walletversion": 139900, "format": "sqlite", "balance": 0.00000000, "unconfirmed_balance": 0.00000000, "immature_balance": 0.00000000, "txcount": 0, "keypoolsize": 3000, "keypoolsize_hd_internal": 3000, "paytxfee": 0.00000000, "private_keys_enabled": true, "avoid_reuse": false, "scanning": false, "descriptors": true } ``` With this PR -- `"walletversion": 169900`: ``` $ src/bitcoin-wallet -signet -wallet=211025-test-pr create Topping up keypool... Wallet info =========== Name: 211025-test-pr Format: sqlite Descriptors: yes Encrypted: no HD (hd seed available): yes Keypool Size: 6000 Transactions: 0 Address Book: 0 $ src/bitcoin-cli -signet -rpcwallet=211025-test-pr getwalletinfo { "walletname": "211025-test-pr", "walletversion": 169900, "format": "sqlite", "balance": 0.00000000, "unconfirmed_balance": 0.00000000, "immature_balance": 0.00000000, "txcount": 0, "keypoolsize": 3000, "keypoolsize_hd_internal": 3000, "paytxfee": 0.00000000, "private_keys_enabled": true, "avoid_reuse": false, "scanning": false, "descriptors": true } ``` ACKs for top commit: lsilva01: Code Review ACK 5b6b5ef stratospher: ACK 5b6b5ef. rajarshimaitra: ACK bitcoin@5b6b5ef meshcollider: Code review ACK 5b6b5ef Tree-SHA512: 0221e76fa8f29037920d0a483c742bf270ecaead45f30230943b78775aaea63ac052e43fe712d15c2326e515dea2d2ac82de0924882598421c1874f2e6f442a6
825fcae [tests] Replace bytes literals with hex literals (John Newbery) 64eca45 [tests] Fix pep8 style violations in address.py (John Newbery) b230f8b [tests] Correct docstring for address.py (John Newbery) ea70e6a [tests] Tidy up imports in address.py (John Newbery) 7f639df [tests] Remove unused optional verify_checksum parameter (John Newbery) 011e784 [tests] Rename segwit encode and decode functions (John Newbery) e455713 [tests] Move bech32 unit tests to test framework (John Newbery) Pull request description: Lots of small fixes: - moving unit tests to test_framework implementation files - renaming functions to be clearer - removing multiple imports - removing unreadable byte literals from the code - fixing pep8 violations - correcting out-of-date docstring ACKs for top commit: jonatack: re-ACK 825fcae per `git range-diff a0a422c 7edcdcd 825fcae` and verified `wallet_address_types.py` and `wallet_basic.py --descriptors` (the failure on one travis job) are green locally. MarcoFalke: ACK 825fcae fanquake: ACK 825fcae - looks ok to me. Tree-SHA512: aea509c27c1bcb94bef11205b6a79836c39c62249672815efc9822f411bc2e2336ceb3d72b3b861c3f4054a08e16edb28c6edd3aa5eff72eec1d60ea6ca82dc4
23cac24 tests: Test bitcoin-wallet dump and createfromdump (Andrew Chow) a88c320 wallettool: Add createfromdump command (Andrew Chow) e1e7a90 wallettool: Add dump command (Andrew Chow) Pull request description: Adds two commands to the `bitcoin-wallet` tool: `dump` and `createfromdump`. These commands will be useful for a wallet storage migration in the future. It is also generally useful to have a storage agnostic dump like this. These commands are similar to BDB's `db_dump` and `db_load` tools. This can also be useful for manual construction of a wallet file for tests. `dump` outputs every key-value pair from the wallet as comma separated hex. Each key-value pair is on its own line with the key and value in hex separated by a comma. This is output to the file specified by the new `-dumpfile` option. `createfromdump` takes a file produced by `dump` and creates a new wallet file with exactly the records specified in that file. A new option `-dumpfile` is added to the wallet tool. When used with `dump`, the records will be written to the specified file. When used with `createfromdump`, the file is read and the key-value pairs constructed from it. `createfromdump` requires `-dumpfile`. A simple round-trip test is added to the `tool_wallet.py`. This PR is based on bitcoin#19334, ACKs for top commit: Sjors: re-utACK 23cac24 MarcoFalke: re review ACK 23cac24 only change is rebase and removing useless shared_ptr wrapper 🎼 ryanofsky: Code review ACK 23cac24. Only changes since last review rebase and changing a pointer to a reference Tree-SHA512: 2d63cf62baca3d16495aa698dc02f7d889c81b41015e9c92c23c275bb4a690fc176d351c3fd7f310bd6b17f5a936cc9be694cbecd702af741b96c0f530e72fa2
…t tool option fae32f2 wallet: Add missing check for -descriptors wallet tool option (MarcoFalke) faf8f61 test: Add missing check for is_sqlite_compiled (MarcoFalke) fa7dde1 wallet: Pass ArgsManager into ExecuteWalletToolFunc instead of using global (MarcoFalke) Pull request description: Also, fix a test failure when compiled without sqlite ACKs for top commit: ryanofsky: Code review ACK fae32f2. Thanks for implementing the -descriptors check and dealing with the test failure! jonatack: Code review utACK fae32f2 Tree-SHA512: 3d7710694085822739a8316e4abc6db270799ca6ff6b0f9e5563ae240da65ae6a9cab7ba2647feae6ba540dac40b55b38ed41c8f6ed0bf02a3d1536284448927
Debug logs should not be printed to stdout, stderr exists for it. Private keys should not be printed to console by activation very general macros name "ENABLE_DASH_DEBUG". This macros is used only for hdchain private keys, the location util/system.h is too general for it. Functional test "tool_wallet.py" is failed due to unexpected output for tsan. It seems as easier to remove this logs due to too many issues with it rather than address all of them.
To rebase on top of current develop to fix ff-merge.
Fixed here, also tests for it: 31ffb78 |
Merged
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Apr 16, 2024
…itcoin#21302, partial bitcoin#20267 - descriptor wallets part IV ceefab5 fix: feature_backwards compatible works now with as expected if no bdb compiled (Konstantin Akimov) b20f812 fix: follow-up fixes for functional tests used protx (Konstantin Akimov) 655146d Merge bitcoin#21302: wallet: createwallet examples for descriptor wallets (W. J. van der Laan) 99a8b60 Merge bitcoin#21063: wallet, rpc: update listdescriptors response format (fanquake) 6ee2c7c Merge bitcoin#21277: wallet: listdescriptors uses normalized descriptor form (Wladimir J. van der Laan) 8bacdbf Merge bitcoin#19136: wallet: add parent_desc to getaddressinfo (Samuel Dobson) f567de0 chore: release notes for 5965 with wallet tool improvements (Konstantin Akimov) 0daf360 chore: add TODO to implement mnemonic for descriptor wallets (Konstantin Akimov) 5016294 chore: move functional test wallet_multiwallet from category "slow 5 minutes" to "fast test" (Konstantin Akimov) ef7ce87 fix: remove workarounds introduced due to missing bitcoin#20267 (bdb is not compiled) (Konstantin Akimov) 06b2d85 partial Merge bitcoin#20267: Disable and fix tests for when BDB is not compiled (Wladimir J. van der Laan) Pull request description: ## Issue being fixed or feature implemented dashpay/dash-issues#59 ## Extra notes This commit `chore: move functional test wallet_multiwallet from category "slow 5 minutes" to "fast test"` is not directly connected to descriptor wallets, but added to this PR due to conflicts with 20267 ## What was done? It steadily improves support of descriptor wallets in Dash core. Done backports and related fixes: - partial bitcoin#20267 - bitcoin#19136 - bitcoin#21277 - bitcoin#21063 - bitcoin#21302 Beside backports and related fixes, this PR includes release notes for previous batch of backports for descriptor wallets support #5965 ## How Has This Been Tested? Run unit functional tests ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: f4b2033f8c4fa1d0f72cfc31378909703b3ae8f44748989ff00c3e71311ac80ac37837137133c7e4a166823a941ed7df10efa09c89f5b213f3c8ede7d3d6e8f4
5 tasks
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
Related issue: https://github.com/dashpay/dash-issues/issues/59
What was done?
Backports:
Beside new backports there are fixes for old backports bitcoin#17261.
How Has This Been Tested?
Run unit and functional tests
Breaking Changes
Behavior of WalletTool is changed: wallets have a newer version, they have HD chain inside.
Checklist: