Remove fuzztaget and cleanup psbt finalizer #290
Conversation
sanket1729
force-pushed
the
pr289_feedback
branch
from
18d5479 to
7f4adf5
Compare
src/psbt/finalizer.rs
Outdated
| })?; | ||
| if target != flag { | ||
| if target != ecdsa_sig.hash_ty { | ||
| // hash_ty is guaranteed to follow standardness rules |
There was a problem hiding this comment.
In 7f4adf5:
I think this comment can be dropped -- standardness isn't actually relevant to this check, which is just that the signature hashtype matches the one required by the PSBT.
Also, I don't think that standardness is actually guaranteed here. See this code: rust-bitcoin/rust-bitcoin#669 (comment)
There was a problem hiding this comment.
I think that is probably a bug upstream because we will risk generating wrong signatures. Anything that is consensus valid sighashtype byte (0x01, 0x02, x03, 0x81, 0x82, and 0x83) is mapped to sighash all. Consider a case with 0x05 sighash byte, while signing, we sighash_u32 as 0x05u32, but while serializing it, we check it as 0x01 because the information that sighash_all from 0x05 is lost.
There was a problem hiding this comment.
oo good catch!
I think the bug is actually that we're losing information, rather than that we allow non-standard sighash types in PSBT. I can see how this happened, since this recent PSBT thing is the first time that we've ever deliberately supported non-standard sighash types..
There was a problem hiding this comment.
I opened rust-bitcoin/rust-bitcoin#777 about this.
For now I still think we should remove this comment, because standardness is irrelevant, although you could add a comment linking to that rust-bitcoin issue.
sanket1729
force-pushed
the
pr289_feedback
branch
from
7f4adf5 to
bb9f2a0
Compare
sanket1729
force-pushed
the
pr289_feedback
branch
from
bb9f2a0 to
a142ab4
Compare
|
The sighash issue was taken upstream to rust-bitcoin and resolved cleanly there. The current code only parses |
|
ack |
No description provided.