Skip to content

CT descriptor: remove generic from blinding key #59

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 19, 2023
Merged

CT descriptor: remove generic from blinding key #59

merged 4 commits into from
Oct 19, 2023

Conversation

@LeoComandini
Copy link
Contributor

@LeoComandini LeoComandini commented Oct 12, 2023

The generic was only used by the Bare variant, which was inconsistent with its corresponding "secret" variant, View.

The generic could be used to handle the conversion from ConfidentialDescriptor<DescriptorPublicKey> to
Descriptor<DefiniteDescriptorKey>, specifically making sure that the descriptor blinding key does not have wildcards.

For the Bare variant this could be done, but this cannot happen for the View variant, since we don't have the definite version of DescriptorSecretKey.

The proposed solution consists in:

  • Remove generic from descriptor blinding key (this commit)
  • Add ConfidentialDescriptor::at_derivation_index that "removes" wildcards from both the "bitcoin" descriptor and the descriptor blinding key. This makes deriving an address from a CT descriptor with wildcards less cumbersome.
  • Change Key::to_public_key(&self, secp, spk) to return a Result and error if Key has some wildcards. This will fix the case ct(xprv/*,desc) which is currently broken.
  • Add test cases for blinding keys with wildcards
  • Add test vectors with blinding keys with wildcards to ELIP-150

@apoelstra
Copy link
Member

apoelstra commented Oct 12, 2023

Yeah, this solution seems good to me. It's not great but nothing I could come up with was great either.

I'd like to fix CI though before moving forward on this.

@LeoComandini LeoComandini force-pushed the 2023-10-12-confidential-descriptor-key branch from f9d6655 to 51e7f22 Compare October 13, 2023 10:54
@LeoComandini
Copy link
Contributor Author

LeoComandini commented Oct 13, 2023

fix CI

Fixed by @RCasatta in #60 , I'll rebase on top of that once merged

@LeoComandini
Copy link
Contributor Author

LeoComandini commented Oct 13, 2023 •
edited
Loading

Proposed additional test vectors for ELIP-150
(will post there once this gets a round of review)

View Descriptor with wildcard: ct(xprv9s21ZrQH143K28NgQ7bHCF61hy9VzwquBZvpzTwXLsbmQLRJ6iV9k2hUBRt5qzmBaSpeMj5LdcsHaXJvM7iFEivPryRcL8irN7Na9p65UUb/*,elwpkh(xpub661MyMwAqRbcEcT9W98HZP2kFzyzQQZkYnrRnrM8uD8kH8kSeFoQHq1x2iihLgC6PXGy5LrjCL66uSNhJ8pwjfx2rMUTLWuRMns2EG9xnjs/*))#

  • Index: 1,
  • Descriptor blinding private key: xprv9s21ZrQH143K28NgQ7bHCF61hy9VzwquBZvpzTwXLsbmQLRJ6iV9k2hUBRt5qzmBaSpeMj5LdcsHaXJvM7iFEivPryRcL8irN7Na9p65UUb/*
  • Confidential address: el1qqf6690fpw2y00hv5a84zsydjgztg2089d5xnll4k4cstzn63uvgudd907qpvlvvwd5ym9gx7j0v46elf23kfxunucm6ejjyk0
  • Unconfidential address: ert1qkjhlqqk0kx8x6zdj5r0f8k2avl54gmyn7qjk2k

Non-View Descriptor with wildcard: ct(xpub661MyMwAqRbcEcT9W98HZP2kFzyzQQZkYnrRnrM8uD8kH8kSeFoQHq1x2iihLgC6PXGy5LrjCL66uSNhJ8pwjfx2rMUTLWuRMns2EG9xnjs/*,elwpkh(xpub661MyMwAqRbcEcT9W98HZP2kFzyzQQZkYnrRnrM8uD8kH8kSeFoQHq1x2iihLgC6PXGy5LrjCL66uSNhJ8pwjfx2rMUTLWuRMns2EG9xnjs/*))#

  • Index: 1,
  • Descriptor blinding public key: xpub661MyMwAqRbcEcT9W98HZP2kFzyzQQZkYnrRnrM8uD8kH8kSeFoQHq1x2iihLgC6PXGy5LrjCL66uSNhJ8pwjfx2rMUTLWuRMns2EG9xnjs/*
  • Confidential address: el1qqf6690fpw2y00hv5a84zsydjgztg2089d5xnll4k4cstzn63uvgudd907qpvlvvwd5ym9gx7j0v46elf23kfxunucm6ejjyk0
  • Unconfidential address: ert1qkjhlqqk0kx8x6zdj5r0f8k2avl54gmyn7qjk2k

RCasatta
RCasatta reviewed Oct 13, 2023
View reviewed changes
Copy link
Collaborator

@RCasatta RCasatta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/confidential/mod.rs
Comment on lines +107 to +132
.ok()
.ok_or(ConversionError::HardenedChild)?,
Copy link
Collaborator

@RCasatta RCasatta Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map_err ?

Copy link
Contributor Author

@LeoComandini LeoComandini Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the same done in DescriptorPublicKey::at_derivation_index...
https://github.com/ElementsProject/elements-miniscript/blob/master/src/descriptor/key.rs#L632

@LeoComandini
CT descriptor: remove generic from blinding key
da8d16c 
The generic was only used by the `Bare` variant, which was
inconsistent with its corresponding "secret" variant, `View`.

The generic could be used to handle the conversion from
`ConfidentialDescriptor<DescriptorPublicKey>` to
`Descriptor<DefiniteDescriptorKey>`, specifically making sure
that the descriptor blinding key does not have wildcards.

For the `Bare` variant this could be done, but this cannot
happen for the `View` variant, since we don't have the definite
version of `DescriptorSecretKey`.
@LeoComandini
CT descriptor: implement at_derivation_index
8ffda8c
@LeoComandini
CT descriptor: handle bare/view keys with wildcards
eafc28d
@LeoComandini
CT descriptor: test cases with wildcards
f4be6af
@LeoComandini LeoComandini force-pushed the 2023-10-12-confidential-descriptor-key branch from 51e7f22 to f4be6af Compare October 16, 2023 08:38
@LeoComandini
Copy link
Contributor Author

LeoComandini commented Oct 17, 2023

fix CI

Fixed by @RCasatta in #60 , I'll rebase on top of that once merged

@apoelstra rebased and CI is now green

apoelstra
apoelstra approved these changes Oct 19, 2023
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f4be6af

@apoelstra
Copy link
Member

apoelstra commented Oct 19, 2023

Looks good to me. Obviously there are still some open API questions about doing derivations but the result of this is much better than the existing code.

@apoelstra apoelstra merged commit af410f6 into ElementsProject:master Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RCasatta RCasatta RCasatta left review comments

@apoelstra apoelstra apoelstra approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LeoComandini @apoelstra @RCasatta