Add PsbtInputExt::update_with_descriptor #339

LLFourn · 2022-04-07T05:04:26Z

Fixes #335

This populates all relevant fields in a PSBT input according to a descriptor. Includes previously not yet implemented logic for populating bip32_derivation and tap_key_origins. Also optionally checks the witness_utxo and non_witness_utxo.

I renamed PsbtExt::update_desc to PsbtExt::update_inp_with_descriptor which calls update_with_descriptor
internally but retrieves the vout for the input for the non_witness_utxo check automatically. I removed the derivation range functionality because it didn't feel useful (and if it is then it probably belongs in a separate function). Let me know if you want me to add it back where it was or to provide an additional utility function to find which index a script pubkey was derived from.

sanket1729 · 2022-04-10T19:31:31Z

I removed the derivation range functionality because it didn't feel useful (and if it is then it probably belongs in a separate function)

Sorry for the delayed review. I was busy with the conference in Miami. I was trying to emulate bitcoin core's utxoupdatepsbt RPC. I think having the range in the descriptor is certainly useful to scan the output because users don't have to derive the output then. It might be useful in the case where the creator(the entity creating the psbt) is separate from the updater (the entity updating the psbt with utxo information).

But I do think that you are right that this functionality belongs in a separate method. Perhaps it should be a method to Descriptor<DescriptorPublicKey> instead of Psbt.

sanket1729

Did an initial review. Do you need this for the 7.0.0 release of rust-miniscript? I think this might take some time to get iterate.

I would prefer to get a release out for 7.0.0 out as soon as rust-bitcoin 0.28.0 is out. If rust-bitcoin is delayed even further maybe we can get this in 7.0.0 too.

src/descriptor/mod.rs

src/psbt/mod.rs

sanket1729 · 2022-04-10T20:32:33Z

src/psbt/mod.rs

+            }
+        }
+        // only set the input once all the checks have passed
+        *self = inp;


Can you please make the above into a separate function that only operates on inp.

fn update_with_descriptor( &mut self, ... ) -> ... { let mut inp = self.clone(); update_with_descriptor_helper(&mut inp, ...); *self = inp; }

This way we can be sure that we don't accidentally mutate self in the function body in the future.

We should also avoid the allocation here, but I can do that might be complicated code-wise. I will perhaps address that in future as a low-priority issue.

I removed the allocation with a helper function that both the checked and unchecked version use in: 8c8f82b

The idea is that the helper optionally checks the SPK is correct before it mutates anything.

src/psbt/mod.rs

LLFourn · 2022-04-11T01:27:41Z

I removed the derivation range functionality because it didn't feel useful (and if it is then it probably belongs in a separate function)

Sorry for the delayed review. I was busy with the conference in Miami. I was trying to emulate bitcoin core's utxoupdatepsbt RPC. I think having the range in the descriptor is certainly useful to scan the output because users don't have to derive the output then. It might be useful in the case where the creator(the entity creating the psbt) is separate from the updater (the entity updating the psbt with utxo information).

On the contrary, that was very fast considering. Thanks.

But I do think that you are right that this functionality belongs in a separate method. Perhaps it should be a method to Descriptor<DescriptorPublicKey> instead of Psbt.

Will do. In general couldn't these update PSBT functions go on Descriptor? I was guessing they were on an extension trait because you wanted to strictly keep anything to do with PSBTs in one module. Perhaps we could just add additional impls on Descriptor<DescriptorPublicKey> in psbt/mod.rs (or maybe psbt/descriptor_ext.rs)?

Did an initial review. Do you need this for the 7.0.0 release of rust-miniscript? I think this might take some time to get iterate.

I would prefer to get a release out for 7.0.0 out as soon as rust-bitcoin 0.28.0 is out. If rust-bitcoin is delayed even further maybe we can get this in 7.0.0 too.

I think we will only be blocked on this by sometime in mid-may so I could wait for a follow up release around then. I'll try keep this PR as ready as possible as I can though in case we can sneak it in to 7.0.0 :)

Working on it!

sanket1729

Addressed some comments

src/descriptor/mod.rs

src/psbt/mod.rs

LLFourn · 2022-04-11T11:43:52Z

Made it a draft while it's not ready for merge. I've changed the API so there's a checked and unchecked version of the update input method. The checked version is on the PSBT (where the full data is there so we can do the non_witness_utxo txid check) and the unchecked version is on the input. It think it's better :)

What remains:

Fix pkh keys not being included in tapscripts (test that fails has been done)
Create new separate test for the checked version of the function.
separate the checked version into helper as mentioned here: Add PsbtInputExt::update_with_descriptor #339 (comment)

As a heads up I don't think I'm going to be able to work on this again until Friday.

sanket1729 · 2022-04-11T23:05:24Z

I like the latest commit. Will do another round of review tomorrow.

LLFourn · 2022-04-15T01:38:20Z

This is almost ready it just needs some tests on the checked version of the function. I've just moved logic to a helper function and removed the clone of the PSBT. I also stop requiring that non_witness_utxo is present for segwitv0 descriptors since this is easy to enforce outside. Here's what's changed so far for CHANGELOG:

Rename update_desc to update_input_with_descriptor which additionally populates bip32_derivation and tap_key_origins and more thoroughly checks the validity of witness_utxo and non_witness_utxo.
Add PsbtInputExt::update_with_descriptor_unchecked which operates on an psbt::Input the above but without the checks.
Add is_segwit to DescriptorType

afilini · 2022-04-15T10:24:35Z

I was quickly skimming through the code and noticed:

The descriptor must not have any wildcards in it otherwise an error will be returned however it can (and should) have extended keys in it.

I wonder if I should prioritize my work upstreaming the equivalent of the DerivedDescriptorKey type that we have in BDK, which would maybe help the users make less mistakes by enforcing rules with the type system.

Sidenote: I love all these new traits and various bits of utilities being exposed in miniscript. We currently have our own in BDK, but I'll be very happy to delete some code and start using yours!

afilini · 2022-04-15T10:26:12Z

src/descriptor/mod.rs

@@ -259,6 +260,19 @@ pub enum DescriptorType {
    Tr,
 }

+impl DescriptorType {


I would also add a is_taproot() method, even though it will be unused here in miniscript. I'm sure somebody will find that useful!

Maybe segwit_version(&self) -> Option<u8> would be the most general?

LLFourn · 2022-04-15T10:52:26Z

hey @afilini. Thanks for taking a look.

I was quickly skimming through the code and noticed:

The descriptor must not have any wildcards in it otherwise an error will be returned however it can (and should) have extended keys in it.

I wonder if I should prioritize my work upstreaming the equivalent of the DerivedDescriptorKey type that we have in BDK, which would maybe help the users make less mistakes by enforcing rules with the type system.

Indeed it would be good to have some type safety here for keys that are wildcard free (but might require further derivation).

Sidenote: I love all these new traits and various bits of utilities being exposed in miniscript. We currently have our own in BDK, but I'll be very happy to delete some code and start using yours!

FWIW the context of this PR is that I was porting your work from BDK to my sketch of bdk_core and saw that this lib was offering some of the same functionality already but just needed to set the key origin stuff. I think it would great if it could all be maintained here.

sanket1729

concept ACK and code review ACK. Can you please squash this into cleaner commits?

sanket1729 · 2022-04-16T01:28:42Z

src/psbt/mod.rs

+
+        if let Some(check_script) = check_script {
+            if check_script != derived.script_pubkey() {
+                return Ok((derived, false));


nit: I can see why you did this. Can we get away with returning Option<Descriptor> ?

If it's too much effort, you can add a comment explaining the return type of the function. (i.e the bool denotes the spk check)

I opted to not do Option<...> because it would mean an unwrap for one of the unchecked caller which seemed like the worse option. I added a comment explaining the awkwardness.

LLFourn · 2022-04-17T22:40:30Z

I've just added a test for the checked version of the function and cleaned up some comments/naming. Commits have been squashed. There's one on the end which adds segwit_version instead of is_segwit which hasn't been reviewed yet.

Updated CHANGELOG

- Rename `update_desc` to `update_input_with_descriptor` which additionally populates `bip32_derivation` and `tap_key_origins` and more thoroughly checks the validity of `witness_utxo` and `non_witness_utxo`.
- Add `PsbtInputExt::update_with_descriptor_unchecked` which operates on an `psbt::Input` and does the above but without the checks.
- Add `find_derivation_index_for_spk` which allows you to scan a derivation range to find the one that produced a script pubkey (this feature was extracted and removed from `update_desc`).
- Add `segwit_version` to `DescriptorType`

Ready to go from my perspective :)

LLFourn · 2022-04-21T08:57:28Z

Rebased on master.

LLFourn · 2022-04-21T23:13:46Z

CI failed but I don't think it's because of the PR.

sanket1729 · 2022-04-22T00:09:43Z

Another rebase should resolve the CI issue. We just merged #363

To replace the functionality lost by changing `update_desc`

LLFourn · 2022-04-25T02:16:09Z

Rebased again :)

[EDIT] Also made some doc fixups.

This populates all relevant fields in a PSBT input according to a descriptor. Includes previously not yet implemented logic for populating bip32_derivation and tap_key_origins. I renamed "PsbtExt::update_desc" to "PsbtExt::update_input_with_descriptor" which uses the same logic as update_with_descriptor internally but additionally does some sanity checks. I removed scanning logic for update_desc because you can do it with find_derivation_index_for_spk.

sanket1729

ACK e85b99c. Left a small comment, that need not be addressed here.

Will wait sometime for a review from @apoelstra

src/descriptor/mod.rs

tcharding

My review adds very little of value, please do not implement suggestions unless you are rebasing anyways. I have next to zero knowledge of rust-miniscript.

src/descriptor/mod.rs

src/psbt/mod.rs

tcharding · 2022-04-27T22:49:28Z

src/psbt/mod.rs

+    /// Note that his method doesn't check that the `witness_utxo` or `non_witness_utxo` is
+    /// consistent with the descriptor. To do that see [`update_input_with_descriptor`].
+    ///
+    /// ## Return value


I didn't mention it at other places so as not to derail your PR with docs issues but since you add a section header here might as well mention it, by convention I believe this should be

Suggested change

/// ## Return value

/// # Returns

An example can be seen here: https://doc.rust-lang.org/rustdoc/documentation-tests.html

(Although while digging for a reference for you I found many non-uniform examples :(

I can't find this convention on that page?

Soz for being confusing. The link shows the # Examples header not the # Returns header, I couldn't find a link with all the typical headers (Returns, Panics, Examplse ect.).

No need to change the PR, I'm not at all fussed, I haven't even started shaking my docs clean up wand at rust-miniscript yet :)

src/psbt/mod.rs

LLFourn · 2022-04-28T04:45:45Z

@sanket1729 @tcharding Thanks. I added two fixup commits to address the remarks. Seemed better to avoid doing more rebasing until @apoelstra has had a look. Unfortunately it looks like github still doesn't have a autosquash and merge feature so that'll have to be done manually once it's ready.

tcharding · 2022-04-28T05:52:30Z

src/descriptor/mod.rs

+    /// If it finds a match then it returns the index it was derived at and the concrete
+    /// descriptor at that index. If the descriptor is non-derivable then it will simply check the
+    /// script pubkey against the descriptor and return it if it matches (in this case the index
+    /// returned will be meaningless).


Cool, TIL we derive at an index not from an index. Cheers.

tcharding · 2022-04-28T05:53:58Z

FTR my lack of ACK should not be seen to imply anything. I just don't know this code well enough to do so.

sanket1729

utACk f40dc83

apoelstra

ACK e85b99c

I didn't check that the PSBT logic is exactly what it should be, but ACK this API change (I think it's much cleaner than what we had before) as well as the implementation.

sanket1729 · 2022-04-30T20:56:41Z

You ACKed a different tip, but I think we are good to merge this.

9cf065b Define a new type for derived `DescriptorPublicKey`s (Alekos Filini) Pull request description: After a brief chat on IRC and in PR #339, here's a quick draft for a new type specific to derived `DescriptorPublicKey`s. This is the same design we use in BDK, where we wrap a normal `DescriptorPublicKey` and a secp context in a new struct. This has the drawback that the struct carries a lifetime and generic for the ctx, but I couldn't think of any other way to get this to work unless we drop the implementation of `ToPublicKey` for `DerivedDescriptorKey` which in my opinion can be very useful. ACKs for top commit: apoelstra: ACK 9cf065b sanket1729: ACK 9cf065b Tree-SHA512: 3dd486d52c589d104da9611c65229988825cc79115a1ff45d1a39aa63e49d2d2a76b40b21a278a58d0b8566b2b37682ffc5efbbb78779ef4e5dadeaf14bbcc6c

LLFourn force-pushed the psbt_update_input branch 6 times, most recently from 3cb78d8 to d9c5c99 Compare April 7, 2022 06:18

sanket1729 reviewed Apr 10, 2022

View reviewed changes

sanket1729 reviewed Apr 11, 2022

View reviewed changes

src/descriptor/mod.rs Outdated Show resolved Hide resolved

src/psbt/mod.rs Outdated Show resolved Hide resolved

src/psbt/mod.rs Outdated Show resolved Hide resolved

LLFourn force-pushed the psbt_update_input branch 2 times, most recently from 9009f1f to 88e49fe Compare April 11, 2022 05:19

LLFourn marked this pull request as draft April 11, 2022 11:34

LLFourn force-pushed the psbt_update_input branch from 8c8f82b to acf6ddb Compare April 15, 2022 01:32

LLFourn marked this pull request as ready for review April 15, 2022 01:44

afilini reviewed Apr 15, 2022

View reviewed changes

afilini mentioned this pull request Apr 15, 2022

Define a new type for derived DescriptorPublicKeys #345

Merged

sanket1729 reviewed Apr 16, 2022

View reviewed changes

LLFourn force-pushed the psbt_update_input branch from acf6ddb to 4ddc565 Compare April 17, 2022 22:24

LLFourn force-pushed the psbt_update_input branch from 4ddc565 to 51031b5 Compare April 21, 2022 08:55

Add find_derivation_index_for_spk

f3198dc

To replace the functionality lost by changing `update_desc`

LLFourn force-pushed the psbt_update_input branch from 51031b5 to 1bd31e6 Compare April 25, 2022 02:11

LLFourn added 2 commits April 25, 2022 12:24

Add segwit_version to DescriptorType

e85b99c

LLFourn force-pushed the psbt_update_input branch from 1bd31e6 to e85b99c Compare April 25, 2022 02:24

sanket1729 previously approved these changes Apr 27, 2022

View reviewed changes

src/descriptor/mod.rs Outdated Show resolved Hide resolved

sanket1729 mentioned this pull request Apr 27, 2022

Enable edition 2018 #365

Merged

tcharding reviewed Apr 27, 2022

View reviewed changes

LLFourn added 2 commits April 28, 2022 14:28

fixup! Add segwit_version to DescriptorType

3374e11

fixup! Add find_derivation_index_for_spk

f40dc83

LLFourn dismissed sanket1729’s stale review via f40dc83 April 28, 2022 04:38

tcharding reviewed Apr 28, 2022

View reviewed changes

sanket1729 approved these changes Apr 28, 2022

View reviewed changes

apoelstra approved these changes Apr 30, 2022

View reviewed changes

sanket1729 merged commit 965a136 into rust-bitcoin:master Apr 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PsbtInputExt::update_with_descriptor #339

Add PsbtInputExt::update_with_descriptor #339

LLFourn commented Apr 7, 2022 •

edited

Loading

sanket1729 commented Apr 10, 2022

sanket1729 left a comment

sanket1729 Apr 10, 2022

LLFourn Apr 15, 2022

LLFourn commented Apr 11, 2022 •

edited

Loading

sanket1729 left a comment

LLFourn commented Apr 11, 2022

sanket1729 commented Apr 11, 2022

LLFourn commented Apr 15, 2022

afilini commented Apr 15, 2022

afilini Apr 15, 2022

LLFourn Apr 15, 2022

LLFourn commented Apr 15, 2022

sanket1729 left a comment

sanket1729 Apr 16, 2022

LLFourn Apr 17, 2022

LLFourn commented Apr 17, 2022

LLFourn commented Apr 21, 2022

LLFourn commented Apr 21, 2022

sanket1729 commented Apr 22, 2022

LLFourn commented Apr 25, 2022 •

edited

Loading

sanket1729 left a comment

tcharding left a comment

tcharding Apr 27, 2022

LLFourn Apr 28, 2022

tcharding Apr 28, 2022 •

edited

Loading

tcharding Apr 28, 2022 •

edited

Loading

LLFourn commented Apr 28, 2022

tcharding Apr 28, 2022

tcharding commented Apr 28, 2022

sanket1729 left a comment

apoelstra left a comment

sanket1729 commented Apr 30, 2022

Add PsbtInputExt::update_with_descriptor #339

Add PsbtInputExt::update_with_descriptor #339

Conversation

LLFourn commented Apr 7, 2022 • edited Loading

sanket1729 commented Apr 10, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LLFourn commented Apr 11, 2022 • edited Loading

sanket1729 left a comment

Choose a reason for hiding this comment

LLFourn commented Apr 11, 2022

sanket1729 commented Apr 11, 2022

LLFourn commented Apr 15, 2022

afilini commented Apr 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LLFourn commented Apr 15, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LLFourn commented Apr 17, 2022

LLFourn commented Apr 21, 2022

LLFourn commented Apr 21, 2022

sanket1729 commented Apr 22, 2022

LLFourn commented Apr 25, 2022 • edited Loading

sanket1729 left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

tcharding Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

LLFourn commented Apr 28, 2022

Choose a reason for hiding this comment

tcharding commented Apr 28, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 commented Apr 30, 2022

LLFourn commented Apr 7, 2022 •

edited

Loading

LLFourn commented Apr 11, 2022 •

edited

Loading

LLFourn commented Apr 25, 2022 •

edited

Loading

tcharding Apr 28, 2022 •

edited

Loading

tcharding Apr 28, 2022 •

edited

Loading