Add Inventory::network_hash() method #515

stevenroose · 2020-11-09T19:57:36Z

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.

apoelstra

seems reasonable to me. I might argue for returning a [u8; 32] which is super easy to convert to any 32-byte hash type, and also we avoid returning the 0 "hash"

stevenroose · 2020-11-14T12:45:45Z

Well, tbh before the Inventory refactor, the type was simply a tuple of (InvType, sha256d::Hash) which I think it's also kinda how it's represented in Core. (Even though Core has uint256..)

sgeisler · 2020-12-15T15:04:04Z

src/network/message_blockdata.rs

+    /// Return the item value represented as a SHA256-d hash.
+    pub fn hash(&self) -> sha256d::Hash {
+        match *self {
+            Inventory::Error => sha256d::Hash::default(),


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)?

I think it can be done through Option<sha256d::Hash> return type

Yeah, right, the error type would only have a single instance anyway.

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.

dr-orlovsky · 2020-12-15T16:07:24Z

src/network/message_blockdata.rs

+    /// Return the item value represented as a SHA256-d hash.
+    pub fn hash(&self) -> sha256d::Hash {
+        match *self {
+            Inventory::Error => sha256d::Hash::default(),


I think it can be done through Option<sha256d::Hash> return type

dr-orlovsky · 2020-12-15T16:08:54Z

src/network/message_blockdata.rs

@@ -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 {


Not 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

apoelstra · 2021-12-20T14:57:40Z

What if we compromised and just called the method network_hash(), which would no longer use the hash name which tends to collide in Rust, and maybe hints to the user that this is a weird sort of "hash" which might be all zeros? (Or at least, is there for use on the network and not necessarily as a general-purpose hash.)

stevenroose · 2023-04-09T22:15:41Z

Addressed comments and rebased.

tcharding

ACK 06b14f7

apoelstra · 2023-04-10T12:36:13Z

If we're returning an Option anyway would it make sense to return an actual hash type rather than a [u8; 32] array?

Alternately, since this is an object that users expect might be represented as all-zeroes, would it make sense to return a [u8;32] that might be all zeroes rather than an Option?

I'd be fine either way.

stevenroose · 2023-04-10T14:34:33Z

If we're returning an Option anyway would it make sense to return an actual hash type rather than a [u8; 32] array?

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.

Alternately, since this is an object that users expect might be represented as all-zeroes, would it make sense to return a [u8;32] that might be all zeroes rather than an Option?

I'd be fine either way.

Hmm, not sure what users expect here. Tbh if you want all zeroes in the Error variant, you can simply do unwrap_or_default. Maybe I could document that it only returns None for the Error variant, but I would expect that to be trivial..

Kixunil

Concept ACK, would a separate newtype make sense? Looks like it wouldn't?

Error being mapped to None rather than [0; 32] looks better to me.

bitcoin/src/network/message_blockdata.rs

Kixunil · 2023-04-13T09:45:52Z

bitcoin/src/network/message_blockdata.rs

@@ -42,6 +42,22 @@ pub enum Inventory {
    },
 }

+impl Inventory {
+    /// Return the item value represented as a SHA256-d hash.


None should be documented.

stevenroose · 2023-04-13T15:21:58Z

Yeah I don't think a newtype makes much sense. Protocol-level it could mean anything. I would be something like InventoryItemInner which is kinda silly.

apoelstra

ACK 3d524e0

tcharding

ACK 3d524e0

9850550 Move AsRef impl block next to Index (Tobin C. Harding) 4d42e8e Derive Copy and Clone (Tobin C. Harding) b38ae97 Implement stable comparison functionality (Tobin C. Harding) 630fc1f Remove len and is_empty from impl_array_newtype macros (Tobin C. Harding) 9788b6d Remove leading colons from impl_array_newtype methods (Tobin C. Harding) 2bb08c2 Remove as_[mut_]ptr from impl_array_newtype macros (Tobin C. Harding) 3e28070 Duplicate impl_array_newtype (Tobin C. Harding) 6358903 Add newline to end of file (Tobin C. Harding) Pull request description: Supersedes rust-bitcoin#515 The primary aim of this PR is to fix the fact that we currently implement various comparison traits (`Ord`, `PartialEq`) by comparing the inner byte arrays. These bytes are meant to be opaque and are not guaranteed across versions of `libsecp256k1`. This PR is a bit involved because I had to detangle all the various types (across both `secp256k1` and `secp256k1-sys`) that use the `impl_array_newtype` macro. - Patch 1: is trivial cleanup - Patch 2: Step one of the PR is duplicating the macro into both crates so we can tease them apart. - Patch 3-5: Are cleanup of the now duplicated `impl_array_newtype` macros - Patch 6: Is the meat and potatoes - Patch 7,8: Further minor clean ups to the macro I had a lot of fun with this PR, I hope you enjoy it too. Fix: rust-bitcoin#463 ACKs for top commit: apoelstra: ACK 9850550 Tree-SHA512: 160381e53972ff882ceb1d2d47bac56a7301a2d13bfe75d3f6ff658ab2c6fbe516ad856587c4d23f98524205918ca1a5f9b737e35c23c7a01b476c92d8d1792f

apoelstra previously approved these changes Nov 9, 2020

View reviewed changes

sgeisler reviewed Dec 15, 2020

View reviewed changes

dr-orlovsky reviewed Dec 15, 2020

View reviewed changes

stevenroose dismissed apoelstra’s stale review via 06b14f7 April 9, 2023 22:15

stevenroose force-pushed the inv-hash branch from 717fc9a to 06b14f7 Compare April 9, 2023 22:15

tcharding previously approved these changes Apr 10, 2023

View reviewed changes

stevenroose changed the title ~~Add Inventory::hash() method~~ Add Inventory::network_hash() method Apr 10, 2023

Kixunil reviewed Apr 13, 2023

View reviewed changes

Add Inventory::network_hash() method

3d524e0

stevenroose dismissed tcharding’s stale review via 3d524e0 April 13, 2023 15:23

stevenroose force-pushed the inv-hash branch from 06b14f7 to 3d524e0 Compare April 13, 2023 15:23

apoelstra approved these changes Apr 13, 2023

View reviewed changes

tcharding approved these changes Apr 13, 2023

View reviewed changes

apoelstra merged commit 3795f60 into rust-bitcoin:master Apr 13, 2023
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Inventory::network_hash() method #515

Add Inventory::network_hash() method #515

stevenroose commented Nov 9, 2020

apoelstra left a comment

stevenroose commented Nov 14, 2020

sgeisler Dec 15, 2020 •

edited

dr-orlovsky Dec 15, 2020

sgeisler Dec 15, 2020

stevenroose Nov 13, 2021

dr-orlovsky Dec 15, 2020

dr-orlovsky Dec 15, 2020

apoelstra commented Dec 20, 2021 •

edited

stevenroose commented Apr 9, 2023

tcharding left a comment

apoelstra commented Apr 10, 2023

stevenroose commented Apr 10, 2023

Kixunil left a comment

Kixunil Apr 13, 2023

stevenroose Apr 13, 2023

stevenroose commented Apr 13, 2023

apoelstra left a comment

tcharding left a comment

Add Inventory::network_hash() method #515

Add Inventory::network_hash() method #515

Conversation

stevenroose commented Nov 9, 2020

apoelstra left a comment

Choose a reason for hiding this comment

stevenroose commented Nov 14, 2020

sgeisler Dec 15, 2020 • edited

Choose a reason for hiding this comment

dr-orlovsky Dec 15, 2020

Choose a reason for hiding this comment

sgeisler Dec 15, 2020

Choose a reason for hiding this comment

stevenroose Nov 13, 2021

Choose a reason for hiding this comment

dr-orlovsky Dec 15, 2020

Choose a reason for hiding this comment

dr-orlovsky Dec 15, 2020

Choose a reason for hiding this comment

apoelstra commented Dec 20, 2021 • edited

stevenroose commented Apr 9, 2023

tcharding left a comment

Choose a reason for hiding this comment

apoelstra commented Apr 10, 2023

stevenroose commented Apr 10, 2023

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil Apr 13, 2023

Choose a reason for hiding this comment

stevenroose Apr 13, 2023

Choose a reason for hiding this comment

stevenroose commented Apr 13, 2023

apoelstra left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

sgeisler Dec 15, 2020 •

edited

apoelstra commented Dec 20, 2021 •

edited