Add xkey matching with PSBT::Input::bip32_derivation - #2
Conversation
afilini
left a comment
afilini
left a comment
There was a problem hiding this comment.
Just looking at the technical side of it, I've added a few comments for things I think you could improve.
More broadly though, I'm not convinced by these changes: in general I think we should always add to the PSBT as much information as possible. If, let's say, the creator of the PSBT didn't know about one xpub I don't think that's a good reason for not adding it, if we know it. It could be an argument for also adding the corresponding bip32 path, but that's out of scope for this exercise.
| }; | ||
|
|
||
| psbt.xpub.insert(xpub.xkey, origin); | ||
| // Check if this key matches with the PBBT-input's bip32 keymap. |
| // We need this to ensure we are not adding unrelated keys that might | ||
| // be present in the descriptor, into the PSBT global keymap. | ||
| psbt.inputs.iter().for_each(|input| { | ||
| input.bip32_derivation.iter().for_each(|(_, keysource)| { |
There was a problem hiding this comment.
To support taproot descriptors you should also look at tap_key_origins.
There was a problem hiding this comment.
Nice idea.. will add..
| // Check if this key matches with the PBBT-input's bip32 keymap. | ||
| // We need this to ensure we are not adding unrelated keys that might | ||
| // be present in the descriptor, into the PSBT global keymap. | ||
| psbt.inputs.iter().for_each(|input| { |
There was a problem hiding this comment.
I would rewrite this as two nested for loops, so that you can break out of both (using a label) once you insert the key. Otherwise you are just potentially wasting iterations doing nothing because you've already found and added the key.
This should also let you remove the clone() on the origin when inserting
There was a problem hiding this comment.
Yup that makes more sense than nested iterators..
|
Thanks @afilini for the look.. I have few more questions..
My thought behind this was, if we have a minscript with multiple execution branch, and we only used one branch to satisfy the witness script, this logic will then push all the keys associated with the full descriptor into the global map.. And for taproot cases, no. of execution branches can be very large too.. So makes sense to have some kinda gate over there, and not just push all the keys we can find from a descriptor?? Future proposal: |
|
Yes, what you described also makes sense, but it really depends on what one is trying to do specifically. Ideally we would have options to tweak this, maybe we are the only "updater" and so we have to add everything ourselves, maybe we are just finishing up the work of somebody else. My guess is that here we should just try to keep it simple, we are not building a full wallet after all |
|
Yes that makes sense.. There would be many other logic like that to handle and its out of scope for the exercise.. But thought this one could be mentioned as a generic thing to do, especially in Taproot kinda env.. Maybe we can just add a note in this exercise like "Something you might wanna keep in mind while using Any more ideas you have on how we can expand the PSBT exercise?? Seems very useful for wallet developers.. If you have any mental lists let me know.. I would try to sketch out some exercises from them.. |
2 participants
Proposing a small update in the
psbt-xpubexercise.It occurred to me that while iterating via
Descriptor::for_each_key(), we should also ensure that we are inserting the right keys into the global key map. If we happen to have a descriptor of many miniscript branches, and we need only one/or some of them to be included for that specific transaction, we should only add the related xpubs in the global map.Iterating over all the keys will insert all of them, even if they aren't used in the final witness stack.
I made the check by doing a
match()of the xpub in descriptor and thge PSBT input'sbip32_derivationkeymap. But this require secp context. So has extra overburden.. Is there any better way to do this??