Implement `GetKey` for `KeyMap` #765

tcharding · 2024-10-29T01:22:37Z

Create a KeyMap type to replace the current BTreeMap alias and implement bitcoin::psbt::GetKey for it.

Close: #709

tcharding · 2024-10-29T01:31:37Z

apoelstra · 2024-10-29T12:23:45Z

Nice! Yeah, this is probably the right approach.

In 00342a1 you have a bunch of unreachables because you assume that public keys map consistently to secret keys. I think this is a reasonable assumption, but you need to enforce it in the insert method. Or perhaps you could change insert to just take a secret key, and do the pubkey computation itself.

Alternately, you could make insert be non-pub so that the type can only be constructed by calling Descriptor::parse_descriptor. This might be the better approach. (Though in this case I'd still add an assertion to insert checking consistency of the public and secret keys.)i

LLFourn · 2024-10-30T00:00:08Z

Thanks @tcharding. It might be nice to be able to append your key maps onto each other so you keep one big jumble of secret keys across all your descriptors in the application. This is possible now but would be harder to do if you make the field private (which does seem like the right thing in general). So maybe an .extend with another keymap. If you make it private you probably want to make a way to iterate it too.

tcharding · 2024-10-30T00:28:58Z

Yes I threw the pub inner field so as not to have to implement the whole BTreeMap API but private inner is probably better as you say - I can do this now we a have loose concept ack.

ValuedMammal · 2024-11-07T21:36:46Z

Concept ACK

sanket1729 · 2024-11-11T07:28:27Z

It might be nice to be able to append your key maps onto each other so you keep one big jumble of secret keys across all your descriptors in the application.

Maybe in the longer term, a cleaner solution is to implement rust-bitcoin to implement GetKey for a tuple, vec, set of things that already implement GetKey.

impl_get_key_tuple!((T1, T2))
impl GetKey for Vec<T> where T: GetKey {
}
impl GetKey for BtreeSet<T>...

sanket1729 · 2024-11-11T07:29:05Z

Concept ACK.

tcharding · 2024-12-12T23:41:19Z

Implemented iter::IntoIterator and iter::Extend, would love someone else to give a few minutes to thinking if this is all of the iterator traits we need please.

Also made new take only the secret key. I did not remove any unreachable calls, nor did I add an assertion on the pk/sk since pk in insertion is now controlled.

Create a `KeyMay` type to replace the current `BTreeMap` alias and implement `bitcoin::psbt::GetKey` for it. Close: rust-bitcoin#709

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

src/descriptor/key_map.rs:143

The use of the unreachable! macro can be problematic if assumptions change. Consider handling this case more gracefully.

unreachable!("XPrv maps to XPrv")

src/descriptor/key_map.rs:22

[nitpick] The name KeyMap is quite generic. Consider renaming it to something more descriptive like DescriptorKeyMap.

pub struct KeyMap {

apoelstra · 2024-12-16T17:06:58Z

src/descriptor/key_map.rs

+                            if pk == request {
+                                match v {
+                                    DescriptorSecretKey::Single(ref sk) => Some(sk.key),
+                                    _ => unreachable!("Single maps to Single"),


The use of the unreachable! macro can be problematic if assumptions change. Consider handling this case more gracefully.

Suggested change

_ => unreachable!("Single maps to Single"),

_ => return Err(GetKeyError::InvalidKey),

AI is so awesome I think we are all going to be out of a job soon.

Not if your job is to have fun :)

2024-12-16T17:06:59Z

src/descriptor/key_map.rs

+                                            None
+                                        }
+                                    }
+                                    _ => unreachable!("XPrv maps to XPrv"),


The use of the unreachable! macro can be problematic if assumptions change. Consider handling this case more gracefully.

2024-12-16T17:06:59Z

src/descriptor/key_map.rs

+                                    DescriptorSecretKey::MultiXPrv(xpriv) => {
+                                        Some(xpriv.xkey.to_priv())
+                                    }
+                                    _ => unreachable!("MultiXPrv maps to MultiXPrv"),


The use of the unreachable! macro can be problematic if assumptions change. Consider handling this case more gracefully.

apoelstra · 2024-12-16T17:15:16Z

Too bad. copilot has been around long enough that you'd think it would work, but 100% of its suggestions are wrong and it's full of irritating woke language like "harmful" and "problematic".

apoelstra · 2024-12-16T17:23:37Z

src/descriptor/key_map.rs

+        Ok(self.map.iter().find_map(|(k, v)| {
+            match k {
+                DescriptorPublicKey::Single(ref pk) => match key_request {
+                    KeyRequest::Pubkey(ref request) => match pk.key {


In d2c954a:

I think if you replace

match k { Thing => match key_request { Thing2 => ...

with

match (k, key_request) { (Thing, Thing) => {

you can reduce the amount of indentation and should be able to reduce the number of branches.

sanket1729 · 2024-12-16T17:30:10Z

Too bad. copilot has been around long enough that you'd think it would work, but 100% of its suggestions are wrong and it's full of irritating woke language like "harmful" and "problematic".

:) . Still too far. Let's try it in another 6 months.

sanket1729 · 2024-12-16T17:32:39Z

@apoelstra @tcharding wdyt about #765 (comment)? I think that would solve future API requests like extend, merge etc

apoelstra · 2024-12-16T18:33:40Z

Maybe in the longer term, a cleaner solution is to implement rust-bitcoin to implement GetKey for a tuple, vec, set of things that already implement GetKey.

concept ACK, though we haven't really looked at GetKey upstream with an eye to 1.0. Unsure what the trait will look like when we do.

tcharding force-pushed the 10-29-key-map branch 2 times, most recently from ba5b2bf to 4a3bd7e Compare October 29, 2024 01:31

tcharding force-pushed the 10-29-key-map branch 2 times, most recently from 81b4923 to 00342a1 Compare October 29, 2024 02:25

ValuedMammal mentioned this pull request Dec 1, 2024

Remove signers from bdk_wallet bitcoindevkit/bdk#1619

Open

4 tasks

tcharding force-pushed the 10-29-key-map branch from 00342a1 to 4920626 Compare December 12, 2024 23:36

tcharding force-pushed the 10-29-key-map branch from 4920626 to a97c37a Compare December 12, 2024 23:45

Implement GetKey for KeyMap

d2c954a

Create a `KeyMay` type to replace the current `BTreeMap` alias and implement `bitcoin::psbt::GetKey` for it. Close: rust-bitcoin#709

tcharding force-pushed the 10-29-key-map branch from a97c37a to d2c954a Compare December 13, 2024 00:19

sanket1729 requested a review from Copilot December 16, 2024 17:06

Copilot AI reviewed Dec 16, 2024

View reviewed changes

apoelstra reviewed Dec 16, 2024

View reviewed changes

ValuedMammal mentioned this pull request Dec 18, 2024

chore: should we remove unused SignerError variants ? bitcoindevkit/bdk#1484

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `GetKey` for `KeyMap` #765

Implement `GetKey` for `KeyMap` #765

tcharding commented Oct 29, 2024 •

edited

Loading

tcharding commented Oct 29, 2024

apoelstra commented Oct 29, 2024

LLFourn commented Oct 30, 2024

tcharding commented Oct 30, 2024

ValuedMammal commented Nov 7, 2024

sanket1729 commented Nov 11, 2024

sanket1729 commented Nov 11, 2024

tcharding commented Dec 12, 2024

Copilot AI left a comment

Copilot AI Dec 16, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

apoelstra Dec 16, 2024

tcharding Dec 16, 2024

yancyribbens Dec 16, 2024

Copilot AI Dec 16, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI Dec 16, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

apoelstra commented Dec 16, 2024

apoelstra Dec 16, 2024

sanket1729 commented Dec 16, 2024

sanket1729 commented Dec 16, 2024

apoelstra commented Dec 16, 2024

	_ => unreachable!("Single maps to Single"),
	_ => return Err(GetKeyError::InvalidKey),

Implement GetKey for KeyMap #765

Are you sure you want to change the base?

Implement GetKey for KeyMap #765

Conversation

tcharding commented Oct 29, 2024 • edited Loading

tcharding commented Oct 29, 2024

apoelstra commented Oct 29, 2024

LLFourn commented Oct 30, 2024

tcharding commented Oct 30, 2024

ValuedMammal commented Nov 7, 2024

sanket1729 commented Nov 11, 2024

sanket1729 commented Nov 11, 2024

tcharding commented Dec 12, 2024

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Dec 16, 2024

Choose a reason for hiding this comment

apoelstra Dec 16, 2024

Choose a reason for hiding this comment

tcharding Dec 16, 2024

Choose a reason for hiding this comment

yancyribbens Dec 16, 2024

Choose a reason for hiding this comment

Copilot AI Dec 16, 2024

Choose a reason for hiding this comment

Copilot AI Dec 16, 2024

Choose a reason for hiding this comment

apoelstra commented Dec 16, 2024

apoelstra Dec 16, 2024

Choose a reason for hiding this comment

sanket1729 commented Dec 16, 2024

sanket1729 commented Dec 16, 2024

apoelstra commented Dec 16, 2024

Implement `GetKey` for `KeyMap` #765

Implement `GetKey` for `KeyMap` #765

tcharding commented Oct 29, 2024 •

edited

Loading