fix: error instead of panic for hardened blinding keys - #103
Merged
Conversation
Key::to_public_key passed the DefiniteDescriptorKey to bare::tweak_key, which reaches it through ToPublicKey::to_public_key. That impl unwraps the ConversionError from derive_public_key, so a bare blinding key with a hardened derivation step panicked rather than returning an error. Derive the key explicitly and propagate the failure through the existing Error::Conversion variant, then pass the resulting bitcoin::PublicKey to tweak_key, whose ToPublicKey impl is infallible. tweak_key's public signature is unchanged. Add tests for both routes to the panic: an explicit hardened step in the blinding key path, and a hardened wildcard resolved by at_derivation_index. Closes ElementsProject#102
Member
Author
|
Key::Bare(k) => {
let k = k.at_derivation_index(index)?.into_descriptor_public_key();
if let DescriptorPublicKey::XPub(ref xpub) = k {
if xpub.derivation_path.into_iter().any(|c| c.is_hardened()) {
return Err(ConversionError::HardenedChild);
}
}
Key::Bare(k)
} |
Member
|
Possibly related to rust-bitcoin/rust-miniscript#830 which required a followup rust-bitcoin/rust-miniscript#913 But I can't really tell because this code is so old. For now I'll just accept this patch. Upstream we are hoping to pull all the descriptor key crap out into its own crate that we can share between rust-miniscript and elements-miniscript. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Return an error instead of panic when a blinding key uses hardened derivation
The code that turned the descriptor's blinding key into an actual public
key assumed the step could never fail, so asking such a descriptor for
its address crashed instead of returning an error.
The new tests cover both ways of running into this: writing a hardened
step directly into the blinding key, and using a hardened wildcard that
gets filled in with a specific index later.
Closes #102