ElementsProject / elements-miniscript

Creative Commons Zero v1.0 Universal
11 stars 14 forks source link

Reference implementation of CT descriptors #31

Closed apoelstra closed 1 year ago

apoelstra commented 1 year ago

I'm aware that there are already a basic form of CT descriptors in this library, and this does not remove them, it just adds a new module which should act as a reference implementation for https://github.com/ElementsProject/ELIPs/pull/1

WIP because it needs a new rust-elements release to get tagged hashes in bitcoin_hashes.

apoelstra commented 1 year ago

Oops, missed some files. Added and force-pushed. I squashed everything because the individual commits weren't split out right; let me know if I should split them up again.

apoelstra commented 1 year ago

@sanket1729 regarding "should we remove the Blinded descriptors now", yes, and re "should this be called Descriptor, I think yes, the goal is to use it as confidential::Descirptor. We can also re-export it under a new name.

sanket1729 commented 1 year ago

@apoelstra, agreed on both things

apoelstra commented 1 year ago

So, where I'm at with this is that I need

sanket1729 commented 1 year ago

Thanks for the enumerated list. Left some feedback on the rust-elements repo.

sanket1729 commented 1 year ago

@apoelstra, what is the status of this project/PR?

apoelstra commented 1 year ago

@sanket1729 I intend to finish this and merge it. I think I've got everything I need now? Will try to do it over the weekend or early next week.

apoelstra commented 1 year ago

Rebase only.

apoelstra commented 1 year ago

cc @sanket1729 I have finally finished this PR, with test vectors and everything.

No rush, but ready for review.

LeoComandini commented 1 year ago

It would be nice to have elements_miniscript::confidential::Descriptor::parse_descriptor implemented so we can use descriptors strings with (extended) private keys. Now with from_str we can only parse descriptors with public keys. It would allow to create test vectors with private keys.

apoelstra commented 1 year ago

On @LeoComandini out-of-band advice we may modify this scheme so that the "bare pubkey" variant p2c-commits to the scriptpubkey. Conveniently, even when the scriptpubkey is exposed alongside the resulting key, nobody can "undo" the p2c and learn the original public key. So we get very good on-chain privacy here, even when one participant in a multiparty setup is reusing keys.

Will update after some more thought. Writing here so I don't forget.

sanket1729 commented 1 year ago

Can now be rebased

apoelstra commented 1 year ago

Nice. I need to add some xpub-based test vectors but this is otherwise ready for review.

apoelstra commented 1 year ago

cc @sanket1729 ready for review

apoelstra commented 1 year ago

Checked against the spec. Any reason why don't have wildcards in BlindedKey?

There isn't a type called BlindedKey, but I guess you mean, "why not have wildcards in the blinding key". The reason is a bit dumb -- it's because we have a generic keytype for the main descriptor, but for the blinding key we have either (a) a generic type, or (b) a DescriptorSecretKey.

Generic key types don't, in general, have wildcards. Normally, wildcards are handled by people taking a generic descriptor, using DescriptorPublicKey with it, and then using some convenience methods around Descriptor::translate_pk to map every pubkey to a DefiniteDescriptorKey at a particular index. The descriptor itself doesn't understand anything about indices.

So, when the descriptor has an extra "blinding key" which needs to be updated in tandem with the the rest of the keys, our generic-key model breaks down. I think the right way to handle this would be to link DescriptorPublicKey and DescriptorSecretKey somehow, so that we could say that CT view descriptors had a blinding key which was Pk::SecretKey or something rather than a concrete key type. Then for the specific case of DescriptorPublicKey, we could support deriving public and secret keys at the same time.

Does this make sense? Basically the issue is that we're mixing a generic keytype and a concrete one, and our API does not handle this gracefully.

sanket1729 commented 1 year ago

@apoelstra, I get the issues with DescriptorSecretKey. One of the main benefits of decsriptors is backing up and recovery.

I guess this is more a question of a spec. Perhaps, I am misunderstanding something here, if ct(xpub/*,wsh(ms)), would the backups not grow linearly instead of just a single copy?

apoelstra commented 1 year ago

@sanket1729 I don't understand what you're saying. The spec allows wildcards. The reference implementation just doesn't support it because I didn't want to do the rearchitecture work required for it.

apoelstra commented 1 year ago

Going to merge this; can address comment nits in a followup PR.