Improve sighash_u32 handling
#401
Conversation
|
This means we will be supporting using sighash flags that we don't know about. Unknown sighash flags will almost certainly have new semantics that this method is not taking into consideration. Instead of this, I'd prefer to just take a |
|
@stevenroose I'm not really sure what you mean (what would you prefer the code to look like? take |
|
Well, we don't support Bitcoin Cash. What I suggest is not allowing to use an arbitrary |
|
Soft fork definetely takes more coordination effort than changing a set of enum fields in an open-source library like this after the actual softfork happened. So I am after having enum type as an input w/o |
|
I like this |
| @@ -618,6 +623,11 @@ impl SigHashType { | |||
| pub fn as_u32(&self) -> u32 { *self as u32 } | |||
| } | |||
|
|
|||
| impl From<SigHashType> for u32 { | |||
There was a problem hiding this comment.
Hmm, how come you're allowed to do that here? I thought you could only make trait implementation "in the same crate as the definition of either the trait or the type implemented on"? I thought for this we needed impl Into<u32> for SigHashType instead. Interesting...
There was a problem hiding this comment.
Orphan rules are more complex than that. My understanding is that since SigHashType is defined in the current crate, there can be no other crates that would be able to impl From<SigHashType> for u32 as all types would be foreign to it. I don't remember all the rules but my mental model is "if anything in that impl block is yours, you're free to do it, unless you can't".
The best thing I could google about it RN is rust-lang/rfcs#1856
There was a problem hiding this comment.
Hmm, this then somehow makes the assumption that "stdlib can never impl traits using types outside of stdlib", which is a special assumption that only holds for stdlib and not for other crates.
It does make sense though. I often implement Into in that case instead of From, but a From impl results in an Into impl but not the other way around IIRC. Even though Into is usually the one used in methods.
src/blockdata/transaction.rs
Outdated
| @@ -325,7 +329,8 @@ impl Transaction { | |||
| /// # Panics | |||
| /// Panics if `input_index` is greater than or equal to `self.input.len()` | |||
| /// | |||
| pub fn signature_hash(&self, input_index: usize, script_pubkey: &Script, sighash_u32: u32) -> SigHash { | |||
| pub fn signature_hash<U: Into<u32>>(&self, input_index: usize, script_pubkey: &Script, sighash_u32: U) -> SigHash { | |||
There was a problem hiding this comment.
Perhaps also rename the argument to sighash_type then?
|
Rebased. |
The fact that
sighash_u32isu32is a bit confusing and inconvenient.First commit adds some explanation.
Second helps with the convenience.