-
Notifications
You must be signed in to change notification settings - Fork 167
Implement GetKey for KeyMap
#765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,324 @@ | ||||||
| // SPDX-License-Identifier: CC0-1.0 | ||||||
|
|
||||||
| //! A map of public key to secret key. | ||||||
|
|
||||||
| use core::iter; | ||||||
|
|
||||||
| use bitcoin::psbt::{GetKey, GetKeyError, KeyRequest}; | ||||||
| use bitcoin::secp256k1::{Secp256k1, Signing}; | ||||||
|
|
||||||
| #[cfg(doc)] | ||||||
| use super::Descriptor; | ||||||
| use super::{DescriptorKeyParseError, DescriptorPublicKey, DescriptorSecretKey, SinglePubKey}; | ||||||
| use crate::prelude::{btree_map, BTreeMap}; | ||||||
|
|
||||||
| /// Alias type for a map of public key to secret key. | ||||||
| /// | ||||||
| /// This map is returned whenever a descriptor that contains secrets is parsed using | ||||||
| /// [`Descriptor::parse_descriptor`], since the descriptor will always only contain | ||||||
| /// public keys. This map allows looking up the corresponding secret key given a | ||||||
| /// public key from the descriptor. | ||||||
| #[derive(Debug, Clone, Eq, PartialEq)] | ||||||
| pub struct KeyMap { | ||||||
| map: BTreeMap<DescriptorPublicKey, DescriptorSecretKey>, | ||||||
| } | ||||||
|
|
||||||
| impl KeyMap { | ||||||
| /// Creates a new empty `KeyMap`. | ||||||
| #[inline] | ||||||
| pub fn new() -> Self { Self { map: BTreeMap::new() } } | ||||||
|
|
||||||
| /// Inserts secret key into key map returning the associated public key. | ||||||
| #[inline] | ||||||
| pub fn insert<C: Signing>( | ||||||
| &mut self, | ||||||
| secp: &Secp256k1<C>, | ||||||
| sk: DescriptorSecretKey, | ||||||
| ) -> Result<DescriptorPublicKey, DescriptorKeyParseError> { | ||||||
| let pk = sk.to_public(secp)?; | ||||||
| if !self.map.contains_key(&pk) { | ||||||
| self.map.insert(pk.clone(), sk); | ||||||
| } | ||||||
| Ok(pk) | ||||||
| } | ||||||
|
|
||||||
| /// Gets the secret key associated with `pk` if `pk` is in the map. | ||||||
| #[inline] | ||||||
| pub fn get(&self, pk: &DescriptorPublicKey) -> Option<&DescriptorSecretKey> { self.map.get(pk) } | ||||||
|
|
||||||
| /// Returns the number of items in this map. | ||||||
| #[inline] | ||||||
| pub fn len(&self) -> usize { self.map.len() } | ||||||
|
|
||||||
| /// Returns true if the map is empty. | ||||||
| #[inline] | ||||||
| pub fn is_empty(&self) -> bool { self.map.is_empty() } | ||||||
| } | ||||||
|
|
||||||
| impl Default for KeyMap { | ||||||
| fn default() -> Self { Self::new() } | ||||||
| } | ||||||
|
|
||||||
| impl IntoIterator for KeyMap { | ||||||
| type Item = (DescriptorPublicKey, DescriptorSecretKey); | ||||||
| type IntoIter = btree_map::IntoIter<DescriptorPublicKey, DescriptorSecretKey>; | ||||||
|
|
||||||
| #[inline] | ||||||
| fn into_iter(self) -> Self::IntoIter { self.map.into_iter() } | ||||||
| } | ||||||
|
|
||||||
| impl iter::Extend<(DescriptorPublicKey, DescriptorSecretKey)> for KeyMap { | ||||||
| #[inline] | ||||||
| fn extend<T>(&mut self, iter: T) | ||||||
| where | ||||||
| T: IntoIterator<Item = (DescriptorPublicKey, DescriptorSecretKey)>, | ||||||
| { | ||||||
| self.map.extend(iter) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| impl GetKey for KeyMap { | ||||||
| type Error = GetKeyError; | ||||||
|
|
||||||
| fn get_key<C: Signing>( | ||||||
| &self, | ||||||
| key_request: KeyRequest, | ||||||
| secp: &Secp256k1<C>, | ||||||
| ) -> Result<Option<bitcoin::PrivateKey>, Self::Error> { | ||||||
| Ok(self.map.iter().find_map(|(k, v)| { | ||||||
| match k { | ||||||
| DescriptorPublicKey::Single(ref pk) => match key_request { | ||||||
| KeyRequest::Pubkey(ref request) => match pk.key { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In d2c954a: I think if you replace with you can reduce the amount of indentation and should be able to reduce the number of branches. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, at least it looks better. I tried this, see: oleonardolima@4f304d1 |
||||||
| SinglePubKey::FullKey(ref pk) => { | ||||||
| if pk == request { | ||||||
| match v { | ||||||
| DescriptorSecretKey::Single(ref sk) => Some(sk.key), | ||||||
| _ => unreachable!("Single maps to Single"), | ||||||
|
||||||
| _ => unreachable!("Single maps to Single"), | |
| _ => return Err(GetKeyError::InvalidKey), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, no
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI is so awesome I think we are all going to be out of a job soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not if your job is to have fun :)
Copilot
AI
Dec 16, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of the unreachable! macro can be problematic if assumptions change. Consider handling this case more gracefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing get_key implementation for Xpriv does not cover the scenario when the KeyRequest::Bip32(..) uses the key origin information, and it'd need to be covered here (it's a scenario that's being tested by BDK).
I handled it this way in my implementation, see: oleonardolima@4f304d1#diff-9b4da42f4bda05e0c63a0bae838b0eae32dfc2d5511a0c8142db8acfa9337aa2R142-R160 it's tested by oleonardolima@4f304d1#diff-9b4da42f4bda05e0c63a0bae838b0eae32dfc2d5511a0c8142db8acfa9337aa2R391-R422
Copilot
AI
Dec 16, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of the unreachable! macro can be problematic if assumptions change. Consider handling this case more gracefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In d2c954a:
Both the
contains_keycheck and the clone are unnecessary here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a dig and the
BTreeMap::insertis a little different to this one because the value in the map can differ but because our map is mapping pk to sk it cannot, right? If that assumption is correct then I believe the code is correct if we want to unconditionally return the public key.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah, the
clonemay be necessary.But the check isn't doing anything except a pointless lookup.