Do not allow uncompressed pubkey in wpkh(KEY) output descriptor
#88
Conversation
kiminuo
commented
May 13, 2020
kiminuo
commented
|
Fuzz crash is thanks to #89. Hopefully. |
There was a problem hiding this comment.
Hi, thanks for tackling this. I think this can be extended to include wsh cases too. We can use MiniscriptKey::from_str() instead of str::from_str and that would guide rest of implementation.
You would have to pass the compressed flag from the Terminal::from_tree().
For sh, pkh, pk: the flag would be false
For everything else, the compressed_only flag should be true
src/descriptor/mod.rs
Outdated
| @@ -405,7 +405,7 @@ where | |||
| Ok(Descriptor::ShWsh(sub)) | |||
| } | |||
| ("wpkh", 1) => expression::terminal(&newtop.args[0], |pk| { | |||
| Pk::from_str(pk).map(Descriptor::ShWpkh) | |||
| str::FromStr::from_str(pk).map(Descriptor::ShWpkh) | |||
There was a problem hiding this comment.
I think Segwit rules would apply here and has to be strict compressed only.
src/miniscript/astelem.rs
Outdated
| expression::terminal(&top.args[0], |x| Pk::from_str(x).map(Terminal::PkK)) | ||
| } | ||
| ("pk_k", 1) => expression::terminal(&top.args[0], |x| { | ||
| str::FromStr::from_str(x).map(Terminal::PkK) |
There was a problem hiding this comment.
Can we use MiniscriptKey::from_str here? That should also help for extending the PR to wsh/sh.
See the following comment on a suggestion for the implementation
| @@ -381,7 +381,9 @@ where | |||
| } | |||
| } | |||
| match (frag_name, top.args.len() as u32) { | |||
| ("pk", 1) => expression::terminal(&top.args[0], |pk| Pk::from_str(pk).map(Policy::Key)), | |||
| ("pk", 1) => expression::terminal(&top.args[0], |pk| { | |||
There was a problem hiding this comment.
I think we can relax the compiler here to allow 65 bytes keys.
There was a problem hiding this comment.
I'm not sure I follow. Could you elaborate on that?
kiminuo
force-pushed
the
feature/issue-73-uncompressed-pubkey-in-witness-descriptors
branch
from
38206f3 to
360afcd
Compare
> wpkh(KEY) (not inside wsh): P2WPKH output for the given compressed pubkey. https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md
kiminuo
force-pushed
the
feature/issue-73-uncompressed-pubkey-in-witness-descriptors
branch
from
360afcd to
4f50f44
Compare
kiminuo
force-pushed
the
feature/issue-73-uncompressed-pubkey-in-witness-descriptors
branch
from
9865694 to
f337f24
Compare
I'll just rephrase what you said to make sure I understand correctly. So we have fixed and now we would like to continue with where we would like to pass |
|
In the mean time, I have added a new commit that should address some of your suggestions. The goal is to squash the commits when everything is done. |
|
Hi @kiminuo ,
Sorry for being unclear. I will leave detailed feedback in the following review. |
|
On second thought, I think maybe altering the trait for FromTree might not be a good idea as this will result in passing the flag in all recursive calls of Maybe there is a better way than doing this, but I can't think as of now. I am wondering if selectively supporting 65 bytes keys for non-segwit is a good idea given the code changes. @apoelstra, what are your opinions on this? |
|
I wouldn't be opposed to alterning the trait. It is annoying and ugly but not overly so. What I'd really like though is to somehow use the type system to eliminate these checks entirely. Like, to make it impossible to construct a segwit Miniscript with 65-byte keys. But this might not be (cleanly) possible in Rust. |
|
Consider also that in segwit v1 we will lose the |
|
Hey @kiminuo , sorry about this. We decided to overrule this approach with a generic script context parameter that should allow us for an easier transition to tapscript. |
|
No problem :) |
|
Closing in favor of #97 |
kiminuo
deleted the
feature/issue-73-uncompressed-pubkey-in-witness-descriptors
branch
