test: make wallet_upgradetohd.py works with descriptors #6763
Conversation
knst
force-pushed
the
test-wallet-upgradetohd-2
branch
from
49211b1 to
0d85b33
Compare
WalkthroughThe changes add support for descriptor wallets in functional wallet tests by appending a descriptor wallet test invocation in 📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between 0d85b3320510c71fba415f1f4283d63da717d067 and 04c8c97. 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)test/functional/**/*.pyInstructions used from: Sources:
🧠 Learnings (3)📓 Common learningstest/functional/test_framework/util.py (1)test/functional/wallet_upgradetohd.py (2)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (8)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
test/functional/wallet_mnemonicbits.py (1)
20-36: Consider movingget_mnemonicto a shared test utility module.This method is duplicated in
wallet_upgradetohd.py(as noted by the TODO comment there). To follow the DRY principle, consider extracting this intotest_framework/util.pyor a dedicated wallet test utility module.
🧹 Nitpick comments (1)
test/functional/wallet_upgradetohd.py (1)
250-271: Consider deriving test addresses programmatically instead of hardcoding them.While the test logic is sound, hardcoding addresses like
yLpq97zZUsFQ2rdMqhcPKkYT36MoPK4HobandyYBPeZQcqgQHu9dxA5pKBWtYbK2hwfFHxfmakes the test fragile. If the address derivation logic changes, these hardcoded values would need manual updates.Consider deriving these addresses from the mnemonic and passphrase programmatically to make the test more maintainable.
Would you like me to help implement a solution that derives these addresses from the mnemonic?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d5fc8bf and 0d85b3320510c71fba415f1f4283d63da717d067.
📒 Files selected for processing (3)
test/functional/test_runner.py(1 hunks)test/functional/wallet_mnemonicbits.py(1 hunks)test/functional/wallet_upgradetohd.py(12 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
test/functional/test_runner.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
test/functional/wallet_mnemonicbits.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
test/functional/wallet_upgradetohd.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
🔇 Additional comments (3)
test/functional/test_runner.py (1)
324-324: LGTM!The addition of the descriptor wallet test variant is consistent with the established pattern in the test suite and properly integrates the new descriptor wallet support added to
wallet_upgradetohd.py.test/functional/wallet_mnemonicbits.py (1)
91-98: Good refactoring to unify wallet creation and mnemonic verification.The changes simplify the test by using a consistent approach for both descriptor and legacy wallets:
- Create a blank wallet with the appropriate descriptor setting
- Upgrade to HD
- Use the unified
get_mnemonichelper for verificationThis removes conditional logic and makes the test more maintainable.
test/functional/wallet_upgradetohd.py (1)
32-33: Clean approach to wallet initialization.Creating a blank wallet and explicitly importing the deterministic key is more explicit and works consistently across both wallet types.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify or remove the ambiguous TODO comments.
The TODO comments "drop it" and "drop it too!" are unclear. If these lines need to be removed, please either remove them now or provide more context about why they should be dropped in the future.
🤖 Prompt for AI Agents
In test/functional/wallet_upgradetohd.py around lines 229 to 232, the TODO
comments "drop it" and "drop it too!" are ambiguous and unclear. Either remove
these TODO comments entirely if the related lines are no longer needed, or
replace the comments with a clear explanation of what exactly should be dropped
and why, to provide proper context for future maintenance.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@PastaPastaPasta it's [semi-related] bug. addressed here: #6772
There was a problem hiding this comment.
I wish at least the todo here is more clear
There was a problem hiding this comment.
my bad, it should reads together but seems as wording bad... It will disappear soon as 6772 is merged:
# TODO - implement auto-unlock descriptor wallet
<couple lines>
# TODO - drop it too!
There was a problem hiding this comment.
I made #6772 ready for review, merge that one first then @PastaPastaPasta
knst
force-pushed
the
test-wallet-upgradetohd-2
branch
from
0d85b33 to
1e70ee8
Compare
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
please address vague TODO
…ts same as legacy wallets ae49943 refactor: drop unused includes from wallet.cpp (Konstantin Akimov) 10d9a67 fix: let upgradetohd work for encrypted Descriptor wallets same as legacy wallets (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented `upgradetohd` has 3rd argument 'passphrase' specially for encrypted wallet but it is ignored for Descriptor Wallet and wallet should be manually unlocked before call of `upgradetohd`. ## What was done? RPC `upgradetohd` actually works now with enctypred Descriptor Wallets without requirement to unlock them first. ## How Has This Been Tested? Tests to be merged in #6763 ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] 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 ACKs for top commit: PastaPastaPasta: utACK ae49943 UdjinM6: utACK ae49943 Tree-SHA512: 8c342cb4f5c76689c8e6f9fc36fbc59e8cd9ada75065b2a9a43896171dab1f4c41a7dd3f8a8edcb1f9918bcff675dbecf45cd5df32306c704f45e362853ce168
… fix error message 4dca765 test: follow-up #6762 - removed related TODO from functional test (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Once both #6762 and #6763 are merged, no more need to have a workaround with manual lock / unlock of descriptor wallet before `upgradetohd`. I fixes CI failure on develop also due to conflict. ## What was done? Removed this workaround from `wallet_upgradetohd.py` ## How Has This Been Tested? See CI ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] 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 ACKs for top commit: DashCoreAutoGuix: utACK 4dca765 (used wrong account, disregard) PastaPastaPasta: utACK 4dca765 kwvg: utACK 4dca765 Tree-SHA512: 435359f53f42c29f3e6885ab6fbeabd1a31135fe67a15b894e6f4a92a0aced3e96e0f1bb38598eb74eb251df1f6ae603e033ee41e3bf500bcc4d6c5c1031b800
…r wallets same as legacy wallets ae49943 refactor: drop unused includes from wallet.cpp (Konstantin Akimov) 10d9a67 fix: let upgradetohd work for encrypted Descriptor wallets same as legacy wallets (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented `upgradetohd` has 3rd argument 'passphrase' specially for encrypted wallet but it is ignored for Descriptor Wallet and wallet should be manually unlocked before call of `upgradetohd`. ## What was done? RPC `upgradetohd` actually works now with enctypred Descriptor Wallets without requirement to unlock them first. ## How Has This Been Tested? Tests to be merged in dashpay#6763 ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] 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 ACKs for top commit: PastaPastaPasta: utACK ae49943 UdjinM6: utACK ae49943 Tree-SHA512: 8c342cb4f5c76689c8e6f9fc36fbc59e8cd9ada75065b2a9a43896171dab1f4c41a7dd3f8a8edcb1f9918bcff675dbecf45cd5df32306c704f45e362853ce168
…iptors 04c8c97 test: move helper get_mnemonic to test_framework/util (Konstantin Akimov) 1e70ee8 test: enable --descriptors for wallet_upgradetohd.py (Konstantin Akimov) 0322b7c test: ensure determinism of addresses for specific mnemonic (Konstantin Akimov) af4b618 test: make wallet_upgradetohd works for descriptor wallets too (Konstantin Akimov) 0267fb7 fix: copyright for wallet_upgradetohd.py (Konstantin Akimov) 62c4ec1 test: remove duplicated code from wallet_mnemonicbits.py by using helper get_mnemonic (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented `upgradetohd` has comprehensive tests for case of blank legacy wallet -> HD legacy wallet. This PR replaces dashpay#6762 ## What was done? - add descriptor wallets to scope of wallet_updatetohd.py - add tests for deterministic of generated addresses from mnemonic (with & without mnemonic_passphrase) ## How Has This Been Tested? See `wallet_upgadetohd.py` ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] 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 ACKs for top commit: UdjinM6: utACK 04c8c97 Tree-SHA512: 7d007b48642f938efb501ef9b36a1e3506e9f658e332a5a845b91784298935669d747c7a616c558b4727644540d9e481d94690ee05e921d6bed6aea4253a7b5a
…remove workaround and fix error message 4dca765 test: follow-up dashpay#6762 - removed related TODO from functional test (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Once both dashpay#6762 and dashpay#6763 are merged, no more need to have a workaround with manual lock / unlock of descriptor wallet before `upgradetohd`. I fixes CI failure on develop also due to conflict. ## What was done? Removed this workaround from `wallet_upgradetohd.py` ## How Has This Been Tested? See CI ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] 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 ACKs for top commit: DashCoreAutoGuix: utACK 4dca765 (used wrong account, disregard) PastaPastaPasta: utACK 4dca765 kwvg: utACK 4dca765 Tree-SHA512: 435359f53f42c29f3e6885ab6fbeabd1a31135fe67a15b894e6f4a92a0aced3e96e0f1bb38598eb74eb251df1f6ae603e033ee41e3bf500bcc4d6c5c1031b800
3 participants
Issue being fixed or feature implemented
upgradetohdhas comprehensive tests for case of blank legacy wallet -> HD legacy wallet.This PR replaces #6762
What was done?
How Has This Been Tested?
See
wallet_upgadetohd.pyBreaking Changes
N/A
Checklist: