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.

Sign up for GitHub

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

Jump to bottom

impl WriteBase32 for Vec<u5> show cannot fail #43

Merged
merged 1 commit into from
Sep 28, 2022
Merged

impl WriteBase32 for Vec<u5> show cannot fail #43

merged 1 commit into from
Sep 28, 2022

Conversation

Ericson2314
Copy link
Contributor

Use void::Void, which in uninhabited, to do this.

@Ericson2314 Ericson2314 force-pushed the use-void-for-infalliblity branch from 3d01211 to f9000a4 Compare March 11, 2020 22:44
@Ericson2314 Ericson2314 mentioned this pull request Mar 11, 2020
Closed
@sgeisler
Copy link
Contributor

Hi, great idea! Afaik constructing a void type is quite straight forward, maybe we could implement it ourselves instead of relying on a dependency? Otherwise this would increase the auditing effort/attack surface.

@Ericson2314
Copy link
Contributor Author

Ericson2314 commented Mar 12, 2020 •
edited
Loading

Implementing ones own Void does mean that one looses out from From instances in other crates, could we just pin it down really hard with a version bound? I generally don't like inlining things for auditability, it doesn't scale for downstream consumers which may have the same dependency.

! has finally been stabilized, so hopefully this problem goes away soon.

@sgeisler
Copy link
Contributor

sgeisler commented Mar 13, 2020 •
edited
Loading

Pinning the version tightly without a cryptographic hash of the crate is even worse imo. You loose out on bug fixes while it's still possible to get pwned (not by the author, but crates.io or AWS). So how badly is this feature needed irl (I totally see the theoretical appeal)? Can we just wait for ! instead?

The only drawback seems to be unnecessary unwraps and less compiler optimization. If there is an argument to be made that either of these is a major inconvenience I could see this being merged. Otherwise we try hard to minimize dependencies.

@clarkmoody what do you think? It's kind of a borderline case imo since we'd use a big part of a very small crate, so the additional auditing effort would be small compared to inlining it.

@Ericson2314
Copy link
Contributor Author

Pinning the version tightly without a cryptographic hash of the crate is even worse imo.

Yes, sorry, that is absolutely correct. Would be ideal if there is a tool to go over a lockfile with some sort of audit whitelist.

@apoelstra
Copy link
Member

Yeah, unfortunately by implementing this ourselves we'd lose a ton of interop benefits.

I'm pretty tempted by this, even though I'm generally pretty dependency-averse.

@clarkmoody
Copy link
Member

If we can wait on stabilization of ! and get this for free when we bump minimum version down the line, I think that might be better than adding a dependency.

@sgeisler
Copy link
Contributor

That might be a very long time to wait :( If it got stabilized in the next version that doesn't mean we can use it the next time we bump our min version (e.g. if we want to target debian stable/oldstable that could take some time to propagate there).

@Ericson2314
Copy link
Contributor Author

What about using core::convert::Infallible, which is intended to become !?

@RCasatta
Copy link
Contributor

What about using core::convert::Infallible, which is intended to become !?

I am afraid Infallible is introduced in 1.34 which is still after our MSRV 1.29

@TheBlueMatt
Copy link
Member

I think we can just bump the MSRV. There was some previous agreement to move the rust-bitcoin ecosystem to 1.36 once it was 2 years old (which it is now). rust-bitcoin/rust-bitcoin#510 (comment)

@Ericson2314
Copy link
Contributor Author

RIght, it's small extra dep vs bump MSRV. I am fine either way, just please pick your poison :).

@Ericson2314
Copy link
Contributor Author

If you do end up going the bumping MSRV route, I will happily fix up #44 to switch to newer edition too.

@clarkmoody
Copy link
Member

I would rather bump MSRV than add a dependency.

@Ericson2314
Copy link
Contributor Author

OK that is done instead.

@apoelstra
Copy link
Member

apoelstra commented Sep 14, 2021 •
edited
Loading

Heads up that rust-bitcoin is at least one major version away from also bumping its MSRV, so if we merge this now, it may complicate our dependency management for a bit.

See discussion in rust-bitcoin/rust-bitcoin#510

@apoelstra apoelstra mentioned this pull request Sep 14, 2021
Closed
20 tasks
@tcharding
Copy link
Member

The changes to lib.rs in this PR can be directly applied on top of #57.

@tcharding
Copy link
Member

We now have an MSRV of 1.41.1 so can use Infallible. The changes to lib.rs in this PR can be directly applied on top of master.

@Ericson2314
impl WriteBase32 for Vec<u5> show cannot fail
cc50e7d 
Use void::Void, which in uninhabited, to do this.
@Ericson2314 Ericson2314 force-pushed the use-void-for-infalliblity branch from 82cfc37 to cc50e7d Compare September 23, 2022 00:53
@Ericson2314
Copy link
Contributor Author

Updated!

apoelstra
apoelstra approved these changes Sep 24, 2022
View reviewed changes
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.

utACK cc50e7d

@apoelstra apoelstra mentioned this pull request Sep 24, 2022
Closed
tcharding
tcharding approved these changes Sep 28, 2022
View reviewed changes
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK cc50e7d

@apoelstra apoelstra merged commit 3f92d7d into rust-bitcoin:master Sep 28, 2022
@Ericson2314 Ericson2314 deleted the use-void-for-infalliblity branch September 28, 2022 22:10
@Ericson2314
Copy link
Contributor Author

Thanks!

@tcharding tcharding mentioned this pull request Mar 21, 2023
Merged
apoelstra added a commit that referenced this pull request Mar 22, 2023
@apoelstra
Merge #93: Run fuzzer in CI
1645683 
37d285c Run fuzzer in CI (Tobin C. Harding)
bd4e8e2 Rename travis-fuzz.sh to fuzz.sh (Tobin C. Harding)
18214b2 Do not manually check for ascii range (Tobin C. Harding)

Pull request description:

  As we do in `rust-bitcoin` run the fuzzer for each PR and each push.

  Fix: #43

ACKs for top commit:
  apoelstra:
    ACK 37d285c

Tree-SHA512: d98fcc970f46d34e0b9331643648684ef77787e3e21d25884c093d9f049f8a60b09b6d4771f73230ced57db93fd92810e58f342bcca2ed36dcbb9bab7abd79e3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

@tcharding tcharding tcharding approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Ericson2314 @sgeisler @apoelstra @clarkmoody @RCasatta @TheBlueMatt @tcharding