fix(descriptor): make descriptor_id fallible for unsupported derivations - #1102
Arowolokehinde wants to merge 1 commit into
Conversation
`Descriptor::descriptor_id` panicked for parseable descriptors that cannot be derived at a concrete index: multipath descriptors, and descriptors retaining hardened public key derivations or hardened wildcards. The check must run before calling `at_derivation_index` rather than mapping its error. For the hardened cases miniscript panics inside `DescriptorPublicKey::at_derivation_index` instead of returning `ConversionError`, so there is no `Err` to map. The guard is still required once a fixed Miniscript release is pinned, since `bdk_chain::DescriptorExt::descriptor_id` unwraps `at_derivation_index(0)` and these descriptors remain panicable there regardless. The signature becomes `Result<Arc<DescriptorId>, DescriptorError>`, a breaking change for Kotlin and Swift callers.
6a8ed2c to
10b730c
Compare
Arowolokehinde
changed the title
| // Miniscript 12.3.7 panics before returning `ConversionError` for these paths, and | ||
| // `bdk_chain`'s `descriptor_id` unwraps the result — so this check is still needed | ||
| // even once a fixed Miniscript release is pinned. | ||
| let contains_hardened_public_key_derivation = self.extended_descriptor.for_any_key(|key| { |
There was a problem hiding this comment.
Since #1079 adds this same key traversal to derive_address, could we extract the hardened-public-derivation check into a private helper and reuse it in both methods when the branches are reconciled? Both methods guard the same at_derivation_index precondition, and duplicating it risks the two panic protections drifting as key handling changes.
There was a problem hiding this comment.
Yeah, i thought so too.
i can create a followup for this when the two PRs has been merged
|
@reez, could you approve the first-time contributor CI run and apply |
For sure, thanks for the heads up! |
4 participants
Description
Descriptor::descriptor_idpanics for parseable descriptors that cannot be derivedat a concrete index: multipath descriptors, and descriptors retaining hardened
public key derivations or hardened wildcards. It now returns
DescriptorError::MultiPathorDescriptorError::HardenedDerivationXpubinstead.The signature becomes
Result<Arc<DescriptorId>, DescriptorError>, which is abreaking change for Kotlin and Swift callers.
Fixes #1094.
Notes to the reviewers
Scoped to
descriptor_id.derive_addresshas the same hardened-derivationproblem, but that's already covered by DRAFT fix(descriptor): reject hardened public derivation paths #1079, so I've left it untouched.
The check has to run before
at_derivation_index, not viamap_erron itsresult. For the hardened cases miniscript panics inside
DescriptorPublicKey::at_derivation_indexrather than returningConversionError, so there's noErrto map. Only multipath returns cleanly.The guard is still required once a fixed Miniscript release is pinned, since
bdk_chain::DescriptorExt::descriptor_idunwrapsat_derivation_index(0).Multipath already shows this: miniscript returns
Errcleanly and the unwrappanics regardless.
Documentation
bdk_wallet—bdk_chain::DescriptorExt::descriptor_id, re-exported asbdk_wallet::chain::DescriptorExtrust-miniscript—DescriptorPublicKey::at_derivation_indexChecklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes: