impl WriteBase32 for Vec<u5> show cannot fail

[Ericson2314]

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

[sgeisler]

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]

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]

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]

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]

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]

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]

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]

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

[RCasatta]

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]

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]

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

[Ericson2314]

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

[clarkmoody]

I would rather bump MSRV than add a dependency.

[Ericson2314]

OK that is done instead.

[apoelstra]

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

[tcharding]

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

[tcharding]

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]

Updated!

[apoelstra]

utACK cc50e7d

[tcharding]

ACK cc50e7d

[Ericson2314]

Thanks!