Add Hash256 associated type #434

sanket1729 · 2022-06-13T17:12:36Z

Implement a new hash type that does not display/FromStr with DISPLAY_BACKWARD. Should we make this type upstream in bitcoin-hashes?
Remove the PkTranslator trait as it was causing conflicting implementations downstream.
Replace the above with more macros :(

apoelstra · 2022-06-15T13:12:47Z

@sipa in your miniscript implementation, do you serialize hashes (e.g. in sha256(...) fragments) forward or backward?

sipa · 2022-06-15T14:50:14Z

@apoelstra They're treated as byte arrays, so the order of 2-character-hex units is the same as the order of the bytes as they appear in the script.

apoelstra · 2022-06-15T15:27:46Z

Perfect, thank you!

apoelstra · 2022-06-15T15:59:01Z

Done reviewing 4ffabb7. ACK except for the one nit.

Arguably this should be two PRs -- the hash256 stuff in the first two commits looks unobjectionable (and it's not even breaking, since amusingly we previously had a shitload of manual byte-reversing logic..). The second two are potentially more controversial, since we introduce a macro-based API to simulate a trait hierarchy. Given the limitations of Rust's type system, I think this is the right approach, but it's definitely not a great user experience to need to know about these macros.... I guess we will just have to be careful to document them well.

nit: there are a typos in the last two commit messages

apoelstra · 2022-06-15T15:59:20Z

sorry, tried to click "comment" while scratching my arm and the mouse moved.

sanket1729 · 2022-06-15T19:19:27Z

@apoelstra split into two PRs.

The second two are potentially more controversial, since we introduce a macro-based API to simulate a trait hierarchy. Given the limitations of Rust's type system, I think this is the right approach, but it's definitely not a great user experience to need to know about these macros. I guess we will just have to be careful to document them well.

I will document them more carefully in the upcoming PR and will add a macro exporting. My plan is to add an example using the API after we are merged all the associated fields (ripemd160/hash160/older/after).

The users don't have to know about them, they still implement all of the methods, but it's annoying.

sanket1729 · 2022-06-15T19:46:45Z

The nightly formatter had configuration errors that were fixed in the first commit.

sanket1729 · 2022-06-15T19:49:18Z

@tcharding, it occurs to me that if rustfmt is breaking(two times now) in nightly. Perhaps we should switch to stable? Are the benefits of nightly worth it over stable?

We can give it a try for a few more weeks to test the stability. If it breaks again, we should consider moving back to the stable version.

tcharding · 2022-06-16T00:36:21Z

@tcharding, it occurs to me that if rustfmt is breaking(two times now) in nightly. Perhaps we should switch to stable? Are the benefits of nightly worth it over stable?

The reason we moved to nightly was that many of the config options are not available on stable. With the current contention over using rustfmt in rust-bitcoin we need the nightly config options if we are to have any hope of enabling rustfmt there.

I do not think that 'breaking' is the correct term, rustfmt nighltly changes and our config file needed updating.

I have two suggestions

Use a fixed version of nightly on CI and then update it manually periodically
Every time rustfmt changes do a PR to fix it

I like (1) because I can do it as part of my routine work and then others are not inconvenienced by rustfmt. Devs could see the CI verison used and use the same one locally (if I can figure out an easy way to do so).

We can give it a try for a few more weeks to test the stability. If it breaks again, we should consider moving back to the stable version.

I think I answered this above, are you open to the suggestions? Sorry that rust-miniscript is taking a hit here because of rust-bitcoin.

sanket1729 · 2022-06-16T01:34:10Z

I like (1) because I can do it as part of my routine work and then others are not inconvenienced by rustfmt.

Thanks. I like (1) too. We are in agreement with the next steps.

tcharding

This can be rebased now that #424 has merged (the first 5 patches of this PR).

I'd prefer to see the rustfmt stuff as a separate PR so other open PRs could use it, I imagine everyone is going to have red in the CI pipeline because of this but its not that big a deal.

High level comment: seems like this DISPLAY_BACKWARDS const makes the types in bitcoin_hashes less usable. Perhaps it should be a field on the type so users can control the direction of the output? I had to hack around it once recently also.

tcharding · 2022-06-16T00:51:30Z

src/miniscript/hash256.rs

+        feature = "schemars",
+        schemars(schema_with = "crate::util::json_hex_string::len_32")
+    )]


We don't have schemars dependency, did you mean to add it? Will need the module in bitcoin_hashes/src/util.rs as well.

This is a fail in the tooling, compiler does not flag this and neither does clippy.

Wow, embarrassing copy-paste error here.

Lol, I should've also flagged this in review rather than shrugging "I guess we capitulated about supporting schemars at some point"

tcharding · 2022-06-16T01:07:09Z

src/lib.rs

@@ -159,13 +159,18 @@ pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Ha
    /// Used in the sha256 fragment
    type Sha256: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash;

+    /// The associated [`hash256::Hash`] type for this [`MiniscriptKey`] type.
+    /// Used in the sha256 fragment


Suggested change

/// Used in the sha256 fragment

/// The associated [`hash256::Hash`] for this `MiniscriptKey`, used in the hash256 fragment.

If this form is agreeably, then could use it for the comment on Sha256 also.

tcharding · 2022-06-16T01:26:41Z

src/test_utils.rs

@@ -43,6 +43,15 @@ impl Translator<String, bitcoin::PublicKey, ()> for StrKeyTranslator {
        .unwrap();
        Ok(hash)
    }
+
+    fn hash256(&mut self, _hash256: &String) -> Result<hash256::Hash, ()> {
+        // hard coded value


This comment adds no value, we can see its a hard coded value. Perhaps comment why this value is used or don't comment at all.

Removing the comment

tcharding · 2022-06-16T01:29:14Z

src/miniscript/astelem.rs

-                    x.reverse();
-                    write!(f, "hash256({})", sha256d::Hash::from_inner(x))
-                }
+                Terminal::Hash256(ref h) => write!(f, "hash256({})", h), // reverse would handled correctly by rust-bitcoin


This comment is incorrect, right? We are handling the display here in this lib now, right?

Yep, this comment is incorrect. Thanks for catching this!

tcharding · 2022-06-16T01:29:20Z

src/miniscript/astelem.rs

-                x.reverse();
-                write!(f, "hash256({})", sha256d::Hash::from_inner(x))
-            }
+            Terminal::Hash256(ref h) => write!(f, "hash256({})", h), // reverse correctly handled upstream


And this one?

tcharding · 2022-06-16T01:32:38Z

src/lib.rs

+    /// The associated [`hash256::Hash`] type for this [`MiniscriptKey`] type.
+    /// Used in the sha256 fragment
+    type Hash256: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash;
+


re: Hash, Sha256, and Hash256

From the docs on the trait its not really clear, at least to me, what is the relation between these three types. Two of them are named after fragments but one is not and they are all 'hashes' but there are other hash fragments as well. Can be add some documentation to try and help devs (cough me) to understand this?

Yep, this is indeed clarification now that we have multiple hashes. I will add some clarification here.

Sha256 and Hash256 are two of the five hash types supported by Script (the others are Sha1 (not supported by miniscript), Ripemd160 and Hash160). Meanwhile Hash is a trait from bitcoin_hashes which abstracts over all of these and allows feeding data into it.

(IIRC Hash is basically just a copy of io::Write but io::Write is std-only, for unclear reasons.)

@apoelstra , I think the Hash is @tcharding is talking about here is MiniscirptKey::Hash which is different from bitcoin_hashes::Hash.

The Hash represents the associated type corresponding to PkH fragment. Perhaps, with multiple hashes in the picture and considering #431 we should rename that to RawPkHash?

I think we should re-evaluate whether we even need a Pk::Hash type anymore. That can be useful for abstractions over RawPkH in the future when we have partial descriptors.

We can consider renaming that as a part of a separate PR.

tcharding · 2022-06-16T01:42:20Z

src/miniscript/hash256.rs

+//! Functionality similar to `sha256d` Hash type in bitcoin, but the FromStr
+//! `Display` are *NOT* REVERSE.


Suggested change

//! Functionality similar to `sha256d` Hash type in bitcoin, but the FromStr

//! `Display` are *NOT* REVERSE.

//! This module is _identical_ in functionality to the `bitcoin_hashes::sha256d` hash type

//! but the `FromHex` and `ToHex` implementations use `DISPLAY_BACKWARDS = false`.

Changed and also added a comment about FromStr and Display

tcharding · 2022-06-17T00:23:19Z

Hey man, I'm not sure what the force push means, did you think this is ready for re-review? The schemars stuff is still in there.

sanket1729

Sorry about the previous push. This is ready for review now @tcharding

sanket1729 · 2022-06-17T02:12:57Z

src/lib.rs

+    /// The associated [`hash256::Hash`] type for this [`MiniscriptKey`] type.
+    /// Used in the sha256 fragment
+    type Hash256: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash;
+


@apoelstra , I think the Hash is @tcharding is talking about here is MiniscirptKey::Hash which is different from bitcoin_hashes::Hash.

The Hash represents the associated type corresponding to PkH fragment. Perhaps, with multiple hashes in the picture and considering #431 we should rename that to RawPkHash?

I think we should re-evaluate whether we even need a Pk::Hash type anymore. That can be useful for abstractions over RawPkH in the future when we have partial descriptors.

We can consider renaming that as a part of a separate PR.

sanket1729 · 2022-06-17T02:16:08Z

src/test_utils.rs

@@ -43,6 +43,15 @@ impl Translator<String, bitcoin::PublicKey, ()> for StrKeyTranslator {
        .unwrap();
        Ok(hash)
    }
+
+    fn hash256(&mut self, _hash256: &String) -> Result<hash256::Hash, ()> {
+        // hard coded value


Removing the comment

sanket1729 · 2022-06-17T02:18:09Z

@apoelstra, let's try to merge #431 before this.

apoelstra · 2022-06-17T17:04:43Z

Yeah, renaming MiniscriptKey::Hash sounds like a good idea :)

sanket1729 · 2022-06-17T18:27:14Z

@apoelstra, we could also remove the MiniscriptKey::RawPkH completely. And make it fixed hash160::Hash. I don't see any specific reason why we would want to reason about raw things in an abstract.

But on the other hand, we already have the code. So part of me wants to keep it in case we need it in the future after partial descriptors are officially supported.

apoelstra · 2022-06-17T18:41:46Z

Yeah, let's keep it for now.

sanket1729 · 2022-06-17T19:08:48Z

@tcharding , @apoelstra , ready for review again.

apoelstra

ACK e49b6d8

sanket1729 · 2022-06-19T02:57:29Z

Rebased after #417 .

sanket1729 · 2022-06-19T03:00:59Z

Another problem that I am noting with merging the imports is that there are always conflicts when merging PRs that touch the same file.

PR1:

use crate::{A, B, SomethingFromPR1, T, U};

PR2:

use crate::{A, AnotherThingFromPR2, B, T, U};

These two PRs would conflict.

While previously it were no conflicts

use crate::SomethingFromPR1;

use crate::AnotherThingFromPR2;

tcharding · 2022-06-19T23:20:03Z

Another problem that I am noting with merging the imports is that there are always conflicts when merging PRs that touch the same file.

Yes you are correct. Its a trade off right, the more you merge the imports (not at all, merge last module, merge crate use foo::bar::{{baz::A, B}, C}) the more conflicts we get. The less we merge imports the more lines if code. There is no 'correct'. I personally like the imports_granularity = "Module" level, but its totally subjective.

Although my view was formed in projects that do not use the '2 explicit acks to merge' policy so perhaps the additional merge conflicts are not good in the rust-bitcoin stack?

sanket1729 · 2022-06-20T18:14:53Z

Although my view was formed in projects that do not use the '2 explicit acks to merge' policy so perhaps the additional merge conflicts are not good in the rust-bitcoin stack?

I tend to agree. I think we should have this in the rust-bitcoin/rust-bitcoin.

It has more open PRs
Requires 2 ACKs

Let us keep the merging import feature in rust-miniscript for a few more weeks and learn a few more things.

tcharding

ACK 9d83a8f

Implement a new hash type that does not displat/fromstr in backwards direction. We need support for support for double hashing, but if we use upstream sha256d::Hash, our FromString/Display requirements break

apoelstra

ACK 905c5dc

sanket1729 force-pushed the hash256 branch from f16927c to 4ffabb7 Compare June 13, 2022 18:53

apoelstra closed this Jun 15, 2022

apoelstra reopened this Jun 15, 2022

sanket1729 force-pushed the hash256 branch from 4ffabb7 to 616e3b9 Compare June 15, 2022 19:15

sanket1729 force-pushed the hash256 branch from 616e3b9 to c4f0b6d Compare June 15, 2022 19:45

sanket1729 force-pushed the hash256 branch 2 times, most recently from 74392eb to b9f7a63 Compare June 15, 2022 22:01

tcharding reviewed Jun 16, 2022

View reviewed changes

sanket1729 force-pushed the hash256 branch from b9f7a63 to 2c3119f Compare June 16, 2022 02:12

sanket1729 force-pushed the hash256 branch 2 times, most recently from f9f3e56 to 3685071 Compare June 17, 2022 02:16

sanket1729 commented Jun 17, 2022

View reviewed changes

sanket1729 force-pushed the hash256 branch from 3685071 to e49b6d8 Compare June 17, 2022 19:08

apoelstra previously approved these changes Jun 18, 2022

View reviewed changes

sanket1729 dismissed apoelstra’s stale review via 9d83a8f June 19, 2022 02:57

sanket1729 force-pushed the hash256 branch from e49b6d8 to 9d83a8f Compare June 19, 2022 02:57

SarcasticNastik mentioned this pull request Jun 19, 2022

Introducing Older/After Translation functionality #435

Open

tcharding mentioned this pull request Jun 21, 2022

Introduce rustfmt in a non-invasive manner rust-bitcoin/rust-bitcoin#1040

Merged

tcharding previously approved these changes Jun 21, 2022

View reviewed changes

sanket1729 added 3 commits June 20, 2022 22:34

Add Hash256 type

e22b081

Implement a new hash type that does not displat/fromstr in backwards direction. We need support for support for double hashing, but if we use upstream sha256d::Hash, our FromString/Display requirements break

Add Hash256 associated type

5d36f9b

Rename Pk::Hash to Pk::RawPkHash

905c5dc

sanket1729 dismissed tcharding’s stale review via 905c5dc June 21, 2022 05:35

sanket1729 force-pushed the hash256 branch from 9d83a8f to 905c5dc Compare June 21, 2022 05:35

apoelstra approved these changes Jun 21, 2022

View reviewed changes

sanket1729 merged commit e2e8e77 into rust-bitcoin:master Jun 21, 2022

	/// Used in the sha256 fragment
	/// The associated [`hash256::Hash`] for this `MiniscriptKey`, used in the hash256 fragment.

		//! Functionality similar to `sha256d` Hash type in bitcoin, but the FromStr
		//! `Display` are NOT REVERSE.

Add Hash256 associated type #434

Add Hash256 associated type #434

Conversation

sanket1729 commented Jun 13, 2022

apoelstra commented Jun 15, 2022

sipa commented Jun 15, 2022

apoelstra commented Jun 15, 2022

apoelstra commented Jun 15, 2022

apoelstra commented Jun 15, 2022

sanket1729 commented Jun 15, 2022

sanket1729 commented Jun 15, 2022

sanket1729 commented Jun 15, 2022

tcharding commented Jun 16, 2022 • edited Loading

sanket1729 commented Jun 16, 2022

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Jun 17, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 commented Jun 17, 2022

apoelstra commented Jun 17, 2022

sanket1729 commented Jun 17, 2022

apoelstra commented Jun 17, 2022

sanket1729 commented Jun 17, 2022

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 commented Jun 19, 2022

sanket1729 commented Jun 19, 2022 • edited Loading

tcharding commented Jun 19, 2022 • edited Loading

sanket1729 commented Jun 20, 2022

tcharding left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Jun 16, 2022 •

edited

Loading

sanket1729 commented Jun 19, 2022 •

edited

Loading

tcharding commented Jun 19, 2022 •

edited

Loading