Add Inventory::network_hash() method #515
Conversation
|
Well, tbh before the |
src/network/message_blockdata.rs
Outdated
| /// Return the item value represented as a SHA256-d hash. | ||
| pub fn hash(&self) -> sha256d::Hash { | ||
| match *self { | ||
| Inventory::Error => sha256d::Hash::default(), |
There was a problem hiding this comment.
I feel a bit uneasy about returning an all-zero "hash". I don't know how exactly you'd misuse this in a dangerous manner that isn't already misguided without that fact, but there might exist one. Would it be too bad to return a Result<Hash, Error>? Or is this just stupid in this context (I'm not entirely sure what you want to use the fn for)?
There was a problem hiding this comment.
I think it can be done through Option<sha256d::Hash> return type
There was a problem hiding this comment.
Yeah, right, the error type would only have a single instance anyway.
There was a problem hiding this comment.
Hmm, fair enough, though in the protocol, the Error variant is represented by 32 0-bytes as well. So maybe "hash" isn't the right name in that case, maybe we'd do an as_bytes() -> [u8; 32] even though using sha256d::Hash is just so much more convenient. Not to speak about hex representation and it's reversal problems etc.
src/network/message_blockdata.rs
Outdated
| /// Return the item value represented as a SHA256-d hash. | ||
| pub fn hash(&self) -> sha256d::Hash { | ||
| match *self { | ||
| Inventory::Error => sha256d::Hash::default(), |
There was a problem hiding this comment.
I think it can be done through Option<sha256d::Hash> return type
src/network/message_blockdata.rs
Outdated
| @@ -43,6 +43,20 @@ pub enum Inventory { | |||
| WitnessBlock(BlockHash), | |||
| } | |||
|
|
|||
| impl Inventory { | |||
| /// Return the item value represented as a SHA256-d hash. | |||
| pub fn hash(&self) -> sha256d::Hash { | |||
There was a problem hiding this comment.
8000Not sure this is a good name for the function taking into account collision with non-bitcoin Hash trait (non-critical, but may be still important). It could be that From<Inventory> for sha256d::Hash will work better; or if we'd like to support optional, a try_as_hash() as a function name
|
What if we compromised and just called the method |
|
Addressed comments and rebased. |
|
If we're returning an Alternately, since this is an object that users expect might be represented as all-zeroes, would it make sense to return a I'd be fine either way. |
I think not because there is no guarantee that we won't have inventory items in the future that use different hashes. Like single sha256 instead of double.
Hmm, not sure what users expect here. Tbh if you want all zeroes in the |
| @@ -42,6 +42,22 @@ pub enum Inventory { | |||
| }, | |||
| } | |||
|
|
|||
| impl Inventory { | |||
| /// Return the item value represented as a SHA256-d hash. | |||
There was a problem hiding this comment.
None should be documented.
|
Yeah I don't think a newtype makes much sense. Protocol-level it could mean anything. I would be something like |
I'm not positive we won't ever had inv items that are not
sha256d::Hash, though. I would expect them to stay like that, but we never know.Would accept that as a reasonable objection against this helper.