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

Fixing hashes core dependency and fuzz feature #602

Merged
merged 1 commit into from May 7, 2021
Merged

Fixing hashes core dependency and fuzz feature #602

merged 1 commit into from May 7, 2021

Conversation

dr-orlovsky
Copy link
Collaborator

Caused by the latest bitcoin_hashes release

@dr-orlovsky dr-orlovsky added the bug label May 4, 2021
@dr-orlovsky dr-orlovsky added this to the 0.26.1 milestone May 4, 2021
@dr-orlovsky dr-orlovsky mentioned this pull request May 4, 2021
3 tasks
@@ -14,7 +14,7 @@ readme = "README.md"
[features]
default = [ "secp-recovery" ]
base64 = [ "base64-compat" ]
fuzztarget = ["bitcoin_hashes/fuzztarget"]
fuzztarget = []
Copy link
Member

Choose a reason for hiding this comment

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

We should drop this feature entirely now, its been replaced with an explicit --cfg=fuzzing

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any chance that people integrated this recusively into their fuzzing pipeline just as we did with bitcoin_hashes? If so this is a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

I would be very surprised if that were the case, though, indeed, its possible. Ultimately its still just in fuzzing pipeline(s), so hopefully not anything that would break any release builds anywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I assume we need to leave this as is for 0.26.1 and drop in 0.27?

Copy link
Member

Choose a reason for hiding this comment

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

If we're not going to yank bitcoin_hashes (which I'm fine with either way) then we can call it a consistent policy. Probably the only thing that will break is rust-Lightning's fuzz crates, and I'm fine dealing with that :).

Copy link
Member

Choose a reason for hiding this comment

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

Its a little bit confusing that we now have a fuzztarget feature which does nothing at all - you have to set cfg=fuzzing to get any behavior out of it. I think we should drop it.

Cargo.toml Show resolved Hide resolved
@@ -14,7 +14,7 @@ readme = "README.md"
[features]
default = [ "secp-recovery" ]
base64 = [ "base64-compat" ]
fuzztarget = ["bitcoin_hashes/fuzztarget"]
fuzztarget = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any chance that people integrated this recusively into their fuzzing pipeline just as we did with bitcoin_hashes? If so this is a breaking change.

@sgeisler
Copy link
Contributor

sgeisler commented May 4, 2021

From what I gather:

  • the bitcoin_hashes version bump actually belongs into another (Error derive) PR (we should just yank 0.9.5 to make sure nobody uses it instead)
  • removing the fuzz feature is breaking, but unless we also want to yank bitcoin_hashes 0.9.6 we better keep it in here to fix our fuzzing, consequences be damned
  • relying on core directly is a valid fix, either to be done here or when bumping bitcoin_hashes to 0.10.0 which will include the removal

I wouldn't be too opposed to just yanking 0.9.5 and 0.9.6 of bitcoin_hashes, do a proper SemVer release 0.9.7 to be used in the error derive PR and also a 0.10.0 release down the road with the breaking changes to be included in rust-bitcoin 0.27.0.

@sgeisler
Copy link
Contributor

sgeisler commented May 4, 2021

Some more research showed that there probably isn't much of a problem as cargo avoids versions with missing features automatically.

@apoelstra
Copy link
Member

I don't care about breaking fuzztarget users.

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.

This is fine, even if we don't completely remove the fuzztarget feature.

Copy link
Contributor

@devrandom devrandom left a comment

Choose a reason for hiding this comment

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

ACK, did something similar in #603

@TheBlueMatt TheBlueMatt merged commit 05046b5 into rust-bitcoin:master May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants