Add public-key-only validation for wallet_name_from_descriptor#482
Add public-key-only validation for wallet_name_from_descriptor#482AmosOO7 wants to merge 3 commits into
wallet_name_from_descriptor#482Conversation
…_name_from_descriptor - Add check that parsed KeyMap is empty after descriptor parsing - Reject descriptors containing private keys with clear error message - Apply validation to both main and change descriptors - Add test_wallet_name_from_descriptor_public_key_check regression test Resolves TODO comment in wallet_name_from_descriptor function. Ensures wallet names are derived from public information only.
…_name_from_descriptor - Add check that parsed KeyMap is empty after descriptor parsing - Reject descriptors containing private keys with clear error message - Apply validation to both main and change descriptors - Add test_wallet_name_from_descriptor_public_key_check regression test Resolves TODO comment in wallet_name_from_descriptor function. Ensures wallet names are derived from public information only.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #482 +/- ##
==========================================
+ Coverage 80.30% 80.91% +0.60%
==========================================
Files 24 24
Lines 5417 5469 +52
Branches 245 244 -1
==========================================
+ Hits 4350 4425 +75
+ Misses 989 967 -22
+ Partials 78 77 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| .to_string(); | ||
| let (descriptor, keymap) = descriptor.into_wallet_descriptor(secp, network_kind)?; | ||
| if !keymap.is_empty() { | ||
| return Err(DescriptorError::Miniscript(miniscript::Error::Unexpected( |
There was a problem hiding this comment.
Since there are already dedicated variants for validation rules like this (HardenedDerivationXpub and ExternalAndInternalAreTheSame), would it make sense to add one here so callers can match on this condition without inspecting the message string?
There was a problem hiding this comment.
Thanks, after the comments from @ValuedMammal in #481, this will be removed entirely.
| assert!(result.is_ok()); | ||
|
|
||
| // Test with empty descriptor | ||
| let public_empty = ""; |
There was a problem hiding this comment.
This test returns early at parse_descriptor before keymap.is_empty() is ever checked. It's testing parse error propagation, not the new check. Maybe clarify the comment to reflect that?
There was a problem hiding this comment.
Thanks, This will be removed entirely.
| Err(DescriptorError::Miniscript(Unexpected(..))) | ||
| )); | ||
| // Test with change descriptor containing public keys (should be ok) | ||
| let public_change_without_private = "wpkh([9a6a2580/84'/1'/0']tpubDDnGNapGEY6AZAdQbfRJgMg9fvz8pUBrLwvyvUqEgcUfgzM6zc2eVK4vY9x9L5FJWdX8WumXuLEDV5zDZnTfbn87vLe9XceCFwTu9so9Kks/1/*)"; |
There was a problem hiding this comment.
This is a duplicate of public_change above, as they have same descriptor string. One can be removed.
| @@ -2797,16 +2797,23 @@ where | |||
| T: IntoWalletDescriptor, | |||
| { | |||
| // TODO: check descriptors contains only public keys | |||
There was a problem hiding this comment.
This can be removed now that the check is implemented
…or checksums - Update `wallet_name_from_descriptor` to compute descriptor checksum explicitly - Remove reliance on `to_string()` implicitly containing `#checksum` - Clarify that wallet name is defined by public descriptor checksums - Add regression test for equivalent xpub/xprv wallet names
3 participants
Summary
Testing
cargo test test_wallet_name_from_descriptor_public_key_check
Checklists
All Submissions:
just pbefore pushingCloses #481