Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use new hex-conservative crate #1833

Merged
merged 3 commits into from Jul 27, 2023

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented May 4, 2023

Use the newly released hex-conservative crate, by doing the following:

  • Depend on hex-conservative in bitcoin and hashes
  • Re-export hex-conservative as hex from both crate roots.
  • Remove all the old hex code from hashes
  • Remove all the old hex code from internals
  • Remove the now unused internals::prelude
  • Fix all the import statements (makes up the bulk of the lines changes in this patch)

@apoelstra
Copy link
Member

I think we should rename it to Iter and yeah, we should export it at the crate root.

apoelstra added a commit to rust-bitcoin/hex-conservative that referenced this pull request May 5, 2023
fb37f96 Rename HexIterator to Iter (Tobin C. Harding)

Pull request description:

  A few observations:

  - The `HexIterator` is an adaptor iterator, iterating over a hex string yielding bytes.
  - The `HexIterator` is passed to a function called `from_bytes_iter`.
  - It is unlikely that we will ever have an adaptor iterator iterating over a bytes an yielding two hex digits.

  The name `Iter` seems like a good candidate, if re-exported at the crate root this allows usage as `hex::Iter::new("deadbeef")`.

  Rename `HexIterator` to `Iter` and re-export it at the crate root.

  Inspired by rust-bitcoin/rust-bitcoin#1833 (comment)

ACKs for top commit:
  apoelstra:
    ACK fb37f96

Tree-SHA512: f201b42f93240a115ae5e96f753eaf16b49bd183b0838aabbb29156a513e305529d8ebc2293621391d08838063495de15ee7d886ede9675adeef15658e703d56
@apoelstra
Copy link
Member

Nice! This looks good now.

@tcharding
Copy link
Member Author

Thanks man, looks sweet huh.

@tcharding
Copy link
Member Author

This will need the lock files updating still, hopefully #1916 will be in by then so msrv pinning stuff is resolved.

@apoelstra
Copy link
Member

I think we should do the hex stuff before any other releases.

@tcharding tcharding force-pushed the 05-04-use-hex-conservative branch 2 times, most recently from 2403c0c to ae87bd6 Compare July 13, 2023 01:56
@tcharding
Copy link
Member Author

Something is wrong with the core2 build and I can't seem to see it, does anything cry out?

git diff @~ hashes/Cargo.toml
diff --git a/hashes/Cargo.toml b/hashes/Cargo.toml
index 0516af97..ac8591b9 100644
--- a/hashes/Cargo.toml
+++ b/hashes/Cargo.toml
@@ -14,9 +14,10 @@ exclude = ["tests", "contrib"]
 
 [features]
 default = ["std"]
-std = ["alloc", "internals/std"]
-alloc = ["internals/alloc"]
+std = ["alloc", "internals/std", "hex/std"]
+alloc = ["internals/alloc", "hex/alloc"]
 serde-std = ["serde/std"]
+core2 = ["actual-core2", "hex/core2"]
 
 [package.metadata.docs.rs]
 all-features = true
@@ -24,12 +25,15 @@ rustdoc-args = ["--cfg", "docsrs"]
 
 [dependencies]
 internals = { path = "../internals", package = "bitcoin-internals", version = "0.1.0" }
+hex = { package = "hex-conservative", version = "0.1.0", default-features = false }
 
-core2 = { version = "0.3.0", default_features = false, optional = true }
 schemars = { version = "0.8.3", optional = true }
 # Only enable this if you explicitly do not want to use "std", otherwise enable "serde-std".
 serde = { version = "1.0", default-features = false, optional = true }
 
+# Do NOT use this feature! Use the `core2` feature instead.
+actual-core2 = { package = "core2", version = "0.3.0", default-features = false, optional = true }
+

@tcharding tcharding marked this pull request as ready for review July 13, 2023 01:58
@apoelstra
Copy link
Member

@tcharding did you update Cargo-recent.lock and Cargo-minimal.lock after making those changes to Cargo.toml?

@tcharding
Copy link
Member Author

Yeah I did, I couldn't get hashes to build even with cargo check locally so I figured it was something brain dead and I was just blind to it. I'll play more with it today.

@tcharding tcharding force-pushed the 05-04-use-hex-conservative branch 2 times, most recently from 6813ac1 to 91583ad Compare July 14, 2023 02:10
@tcharding
Copy link
Member Author

FTR the solution was

extern crate actual_core2 as core2;

But yesterday I hade extern crate core2;

@apoelstra
Copy link
Member

Awesome, glad CI is passing.

I'm temped though to get hex-conservative 0.1.1 out first (with the new hex! macro and fixing the fmt impls to respect {:8})

@tcharding
Copy link
Member Author

Agree, mind if I call it v0.2.0? I've given up understanding Rust semver rules for under v1.0, just feels too trivial increasing only the patch number :)

@apoelstra
Copy link
Member

LGTM. May be a bit easier to review split into multiple commits, one for each crate, and a final one which deletes the hex stuff from -internals.

apoelstra
apoelstra previously approved these changes Jul 20, 2023
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bd8a74b

@tcharding
Copy link
Member Author

Can do!

@tcharding
Copy link
Member Author

tcharding commented Jul 21, 2023

I had a go but we can't split it up without removing the workspace patch of hashes because of the hash_newtype! macro. I could remove that as an initial patch, do each crate separately, then add it back in as a final patch, that kind of defeats the purpose of having each patch build though because git bisecter's would not expect such a change half way through bisecting.

@tcharding
Copy link
Member Author

I pulled the internals changes out into a separate patch, its all red :)

We do not expect users of `rust-bitcoin` to ever activate the "core2"
dependency directly, add a comment explaining that.
@tcharding
Copy link
Member Author

And I removed the unnecessary actual-core2 dependency trick from bitcoin because its unneeded since we only ever expect users to use "no-std" to activate the "core2" dependency - added an initial patch to this PR that comments the "core2" feature as such.

We have just released the `hex-conservative` crate, we can now use it.

Do the following:

- Depend on `hex-conservative` in `bitcoin` and `hashes`
- Re-export `hex-conservative` as `hex` from both crate roots.
- Remove all the old hex code from `hashes`
- Fix all the import statements (makes up the bulk of the lines changed
  in this patch)
Remove the now unused `hex` module from internals, this functionality is
now provided by the `hex-conservative` crate.
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bb8bd16

@tcharding tcharding marked this pull request as draft July 25, 2023 23:11
@tcharding
Copy link
Member Author

Draft because we may be able to make the hex-conservative library available as hex after all - @stevenroose prompted me to look into it just now.

@tcharding tcharding marked this pull request as ready for review July 26, 2023 21:09
@tcharding
Copy link
Member Author

The hex lib rename thing got closed so this is good to go.

@tcharding
Copy link
Member Author

@sanket1729 would you mind taking a look at this if you get a chance please? The green in the diff is almost all import statements and error type renames. The red is 1000+ lines of mostly removing all the stuff that has been released already in hex-conservative - I don't think you need to put much thought into those lines.

One thing stand out as non-obvious, the implementation of FromHex in internal_macros has a lot of code removed and we call through to FromHex which has exactly the same code as that removed, might have to open the editor to confirm that.

Thanks man.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK bb8bd16

@tcharding
Copy link
Member Author

Oh my goodness we can merge this now, this has been a loooooong time coming :)

@apoelstra apoelstra merged commit 04976ed into rust-bitcoin:master Jul 27, 2023
29 checks passed
@tcharding tcharding deleted the 05-04-use-hex-conservative branch August 1, 2023 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants