Make `hash_newtype` evocative of the output #1659

Kixunil · 2023-02-22T13:13:25Z

The API guidelines say macro input should be evocative of the output.
hash_newtype didn't have this property.

This change makes it look exactly like the resulting struct, $len
parameter was removed since it's not needed, reversing is controlled
using an attribute. The macro is also better documented and ready to be
extended in the future.

The tagged SHA256 newtype is not yet modified because it has a more
complicated input parameters.

Closes #1648

We want to get rid of this argument since its value is implied by the inner hash type. First we stop using it.

Now that the `$len` argument is no longer used, remove it completely.

The API guidelines say macro input should be evocative of the output. `hash_newtype` didn't have this property. This change makes it look exactly like the resulting struct, `$len` parameter was removed since it's not needed, reversing is controlled using an attribute. The macro is also better documented and ready to be extended in the future. The tagged SHA256 newtype is not yet modified because it has a more complicated input parameters. Closes rust-bitcoin#1648

apoelstra · 2023-02-22T16:15:28Z

Looks good to me!

I'm ambivalent about whether we try to get this in before release. But ACK for now.

apoelstra

ACK 06f1f02

tcharding

This is cool, complicated as f***, but very cool. I had a good long play with it including trying to write broken things to see if the compiler caught them. Cheers.

I"m good for this to merge into the release.

EDIT: The suggested comments are hard to see where they go because of which line I clicked + on, sorry about that.

tcharding · 2023-02-22T21:10:22Z

bitcoin/src/crypto/sighash.rs

-#[rustfmt::skip]
 sha256t_hash_newtype!(TapSighash, TapSighashTag, MIDSTATE_TAPSIGHASH, 64,
    doc="Taproot-tagged hash with tag \"TapSighash\".


I think we want to keep this skip, without it, IIRC, the args get put on separate lines (from memory only, I did not check).

Oh, crap, will fix in a followup PR to not trigger re-review.

Could also be pulled into the rustfmt PR (sorry @tcharding for putting it on you :P)

tcharding · 2023-02-22T21:11:08Z

bitcoin/src/hash_types.rs

+        pub struct FilterHash(sha256d::Hash);
+        /// Filter header, as defined in BIP-157
+        pub struct FilterHeader(sha256d::Hash);
+    }


This new usage is sick bro, nice effort!

tcharding · 2023-02-22T21:17:43Z

hashes/src/util.rs

+            #[allow(unused)] // the user of macro may not need this
            pub fn from_hash(inner: $hash) -> $newtype {


Why does the linter warn for non-use of a public method? Is it when hash_newtype! is used to create a private type?

Yes, when pub is ommitted and the methods are not used the compiler complains.

tcharding · 2023-02-22T22:15:04Z

hashes/src/util.rs

+#[doc(hidden)]
+#[macro_export]
+macro_rules! hash_newtype_our_attrs {
+    (hash_newtype($($attr:tt)*)) => { $($attr)* };
+    ($($ignore:tt)*) => {};
+}


This is unused but hidden, should be removed, right?

Oh, yes, I rewrote it so many times to get right I didn't notice it.

tcharding · 2023-02-22T22:17:28Z

hashes/src/util.rs

+        )+
+    };
+}
+


The hash_newtype_struct macro took me a while to work out. Perhaps throw a comment on it.

/// Declares the `$newtype` struct, the recursive macro calls are to strip the `#[hash_newtype(direction)]` attribute from the attributes list.

Yeah, I guess I should document my design somewhere so that if people attempt "simpler" design they don't get into insane hair pulling like I did.

Yeah my thought process while reading was "why the f*** did he do that ... oh for that reason, ouch that would have hurt to work out" :)

tcharding · 2023-02-22T22:19:30Z

hashes/src/util.rs

+    ($hash:ty, #[hash_newtype(backward)] $($others:tt)*) => { { $crate::hash_newtype_forbid_direction!(backward, $($others)*); true } };
+    ($hash:ty, #[$($ignore:tt)*]  $($others:tt)*) => { $crate::hash_newtype_get_direction!($hash, $($others)*) };
+}
+


Perhaps add a comment

/// Gets the boolean to use for DISPLAY_BACKWARD from all attributes i.e., turns `#[hash_newtype(backward)]` into `true`.

On hash_newtype_get_direction

tcharding · 2023-02-22T22:19:46Z

hashes/src/util.rs

+        $crate::hash_newtype_forbid_direction!($direction, $(#[$others])*)
+    };
+}
+


Perhaps

/// Asserts at compile time that multiple `#[newtype(direction)]` attributes were not present on the new type.

tcharding · 2023-02-22T22:20:55Z

hashes/src/util.rs

@@ -110,28 +110,77 @@ macro_rules! engine_input_impl(


 /// Creates a new newtype around a [`Hash`] type.


perhaps

/// Creates a new `$newtype`, which is a wrapper around a [`Hash`] type (`$hash`).

On hash_newtype_get_direction, also a possible alternate name for this macro is hash_newtype_forbid_duplicate_direction_attributes

I think after refactoring this should be sufficiently obvious?

tcharding · 2023-02-22T22:21:23Z

hashes/src/util.rs

+/// You can use any valid visibility specifier in place of `pub` or you can leave it out if you
+/// don't want the type or its field to be private.


Suggested change

/// You can use any valid visibility specifier in place of `pub` or you can leave it out if you

/// don't want the type or its field to be private.

/// You can use any valid visibility specifier in place of `pub` or you can omit either, or both, if

/// you want the type or its field to be private.

tcharding · 2023-02-22T22:21:42Z

hashes/src/util.rs

+/// Whether the hash is reversed or not depends on the inner type. However you can override it like
+/// this:


Suggested change

/// Whether the hash is reversed or not depends on the inner type. However you can override it like

/// this:

/// Whether the hash is reversed or not when displaying depends on the inner type. However you can

/// override it like this:

Bizarre that these suggestions are below the other ones on this page since they make suggested changes to code earlier in the file?

I believe GH changed recently to put the comments here in the order you wrote them.

Cool, I can use "as above" without it getting put in the wrong place - lets see how this goes.

apoelstra · 2023-02-23T18:21:08Z

@tcharding looks like your nits are mostly about documentation -- ok if I merge this?

tcharding · 2023-02-23T21:10:21Z

@tcharding looks like your nits are mostly about documentation -- ok if I merge this?

Go for it! Thanks for checking.

8ccfb41 Improve documentation of `hash_newtype!` (Martin Habovstiak) 58876e2 Remove unused macro (Martin Habovstiak) Pull request description: Removed unused macro and improved documentation to address review of #1659 - see commits. I also added a note about recursion. ACKs for top commit: apoelstra: ACK 8ccfb41 tcharding: ACK 8ccfb41 Tree-SHA512: 3b4b0c4ffc8a5166619110d9dcb51affd5cafbb2af84a55dd540a815e4702514d99c71dc1c54aca27fb91970e7e7189d1dffb4f7da7951b0f71336ef6f32d30b

apoelstra · 2024-02-19T18:15:14Z

Oops, looks like we reversed the direction of SegwitV0Sighash here, and possibly others. When reviewing I misread the old macro as being "default to serializing forward" but I think it was actually "default to serializing same as the wrapped type".

Kixunil · 2024-02-19T18:19:56Z

Damn! I need to go now so don't have time to investigate, hopefully later.

apoelstra · 2024-02-19T18:22:47Z

Damn! I need to go now so don't have time to investigate, hopefully later.

No worries. We did this almost a year ago and this is the first complaint I've gotten about it (the Liquid team is working through several versions of rust-bitcoin now and reporting problems they bump into). So we can wait a few more days. But we should fix this for 0.32 (or at least, make a deliberate decision for each hashtype).

Unfortunately this'll be a little annoying/tedious to deal with. We probably want to create encoding unit tests for all of the hash types in rust-bitcoin 0.28 and then check that the same test passes today.

Kixunil · 2024-02-19T18:24:04Z

I think we should also backport the fix.

This can be checked against the 0.29.x branch, and against the commit prior to rust-bitcoin#1659 (40c2467^) and you will see that it is consistent EXCEPT: * In rust-bitcoin 0.29.x we did not have multiple sighash types, only `Sighash`; we now have `LegacySighash`, `SegwitV0Sighash`, and `TapSighash`. * In rust-bitcoin#1565 we deliberately changed the display direction of the sighashes, to match BIP 143. Fixes rust-bitcoin#2495.

… in the library b816c0b hash_types: add unit tests for display of all hash types in the library (Andrew Poelstra) Pull request description: This can be checked against the 0.29.x branch, and against the commit prior to #1659 (40c2467^) and you will see that it is consistent EXCEPT: * In rust-bitcoin 0.29.x we did not have multiple sighash types, only `Sighash`; we now have `LegacySighash`, `SegwitV0Sighash`, and `TapSighash`. * In #1565 we deliberately changed the display direction of the sighashes, to match BIP 143. Fixes #2495. ACKs for top commit: tcharding: That's a win. ACK b816c0b Tree-SHA512: 5b44f40165699910ea9ba945657cd1f960cf00a0b4dfa44c513feb3b74cda33ed80d3551042c15b85d6e57c30a54242db202eefd9ec8c8b6e1498b5578e52800

Kixunil added 3 commits February 22, 2023 01:12

Stop using $len in hash_newtype

752817e

We want to get rid of this argument since its value is implied by the inner hash type. First we stop using it.

Remove the $len argument from hash_newtype

b018f3e

Now that the `$len` argument is no longer used, remove it completely.

Kixunil force-pushed the hash-newtype-remove-len branch from 76850c0 to 06f1f02 Compare February 22, 2023 13:36

apoelstra approved these changes Feb 22, 2023

View reviewed changes

tcharding approved these changes Feb 22, 2023

View reviewed changes

apoelstra merged commit 9c1872f into rust-bitcoin:master Feb 24, 2023

Kixunil deleted the hash-newtype-remove-len branch February 24, 2023 19:51

Kixunil mentioned this pull request Feb 24, 2023

hash_newtype cleanup #1670

Merged

tcharding mentioned this pull request Feb 22, 2024

We may have hash printing backwards wrong for some of the hashes #2495

Closed

apoelstra mentioned this pull request Feb 29, 2024

hash_types: add unit tests for display of all hash types in the library #2528

Merged

		#[allow(unused)] // the user of macro may not need this
		pub fn from_hash(inner: $hash) -> $newtype {

		@@ -110,28 +110,77 @@ macro_rules! engine_input_impl(


		/// Creates a new newtype around a [`Hash`] type.

		/// You can use any valid visibility specifier in place of `pub` or you can leave it out if you
		/// don't want the type or its field to be private.

		/// Whether the hash is reversed or not depends on the inner type. However you can override it like
		/// this:

Make hash_newtype evocative of the output #1659

Make hash_newtype evocative of the output #1659

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Make `hash_newtype` evocative of the output #1659

Make `hash_newtype` evocative of the output #1659