Dependency pinning is implemented incorrectly

[Kixunil]

schemars and dyn-clone are implemented by specifying <= $version in Cargo.toml.

In general pinning via Cargo.toml has these problems:

• Consumers who want to use newer version and are willing to use newer Rust can't do it even if they could otherwise
• Consumers who don't care much about version but already use newer Rust anyway together with some other dependnecy that depends on new version will have the crate duplicated
• The above may cause unfixable compilation failures because types from different versions are not considered same.
• If there is a critical bug the authors may decide to backport the fix. In this case consumers of bitcoin_hashes are stuck with buggy (maybe even vulnerable) version

The apprropriate way of pinning the dependencies is by doing cargo update -p $package --precise $version before compiling (or saving Cargo.lock somewhere)

In addition, specifically writing <= x here (as opposed to >= y, < x) says that the crate works with any version, including the lowest one. I didn't check if this is actually wrong but the chance of it being right is pretty much zero. So unless someone checked with -Z minimal-versions this is outright bug.

Pinging @tcharding who edited the line the last time. (Not to shit on you, I did various bad things myself; just to help you understand.)

[apoelstra]

The relevant change was in #167

Both Jeremy and I acked it, which is more than I can say for most things related to schemars. I'm not sure why we didn't want to approach this by using cargo update -p but I would imagine it was to minimize the amount of extra work that everybody else has to put up with in exchange for schemars being part of this crate.

[Kixunil]

Who actually uses schemars? If all people using it don't mind the issues I mentioned it's probably OK. If not and others don't want to do extra work maybe they should be responsible for fixing? If we know who uses it we should ping them.

[apoelstra]

ping @JeremyRubin

For context, schemars was a thing that @JeremyRubin wanted to use for reasons that AFAIK nobody else shares, and in doing so introduced an extra dependency that has repeatedly caused build breakage (at one point causing the entire project to be broken for about a year, unnoticed because we had a bad CI script).

I'm not sure how useful it is, given that his original intention was to introduce it throughout rust-bitcoin but it never made it past this crate, but it's barely tolerated even here. I tried to remove it once and he complained about not having enough warning (see #109 ) and he put it back #117. This effectively discouraged me from trying again as long as it doesn't cause too much trouble or effort to keep it. Of course, every issue like this changes the balance..

We have pinned it in the shitty way in the past, in #140.

[Kixunil]

Thanks for the context! Meanwhile I found that no Open Source project hosted on GitHub seems to use it.

I don't mind a bit of noise but I think @JeremyRubin should take care of the associated issues. Also if there's a weird hack in Cargo.toml which Jeremy doesn't mind we should at least have a comment saying "We know this is bad but the only person who uses it finds it OK. If you need it fixed contact Jeremy Rubin directly" Otherwise people like me will open issues like this one. :)

[tcharding]

Otherwise people like me will open issues like this one. :)

Lolz, made me laugh.

[tcharding]

The apprropriate way of pinning the dependencies is by doing cargo update -p $package --precise $version before compiling (or saving Cargo.lock somewhere)

Just checking I understand, so in future we work out what version to pin to, add a comment in README.md and then everyone who wants to build has to run the pinning command in order to build?

[Kixunil]

Yes. Most people use newer Rust anyways so it usually won't be too bad.

[JeremyRubin]

I gave up on upstreaming my sapio specific rust Bitcoin patches a while ago, could probably restart upstreaming but it seemed how negative folks were on schemars that it would be wasted effort. I think having jsonschema representations is really nice for all our types, and is something a lot of people could benefit from if they understood how it worked. Maybe I need to write a little tutorial? Rust-bitcoin is the only non forked repo, so I guess what's one more fork to manage if it's such a problem. I'll point out one benefit though, it did expose that our CI was broken 😂 Schemars is used by Sapio, which is open source. I think your grep might not be catching it because the dep exists in some forked crates. They're still open source though. Some other projects also use sapios Bitcoin forks iirc.

…

On Thu, Sep 1, 2022, 4:28 PM Martin Habovštiak ***@***.***> wrote: Yes. Most people use newer Rust anyways so it usually won't be too bad. — Reply to this email directly, view it on GitHub <#179 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGYN6Z2WNN2FYIUYIG4Q5TV4E33VANCNFSM6AAAAAAQCQ4LXQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Kixunil]

First of all, maybe I came off as more negative than intended. I worked for a company that used Json schema in the past and it was nice having it. I would've find it even better if the code was generated from it not the other way around (GRPC-style) but that's a different discussion. So I do like having it there, I just don't have a need for it right now.

What I wanted to say is either of these should be true:

• You're the only one using it and fine with potentially problematic code, that's OK for me, just have a comment about it to not cause noise.
• You want to make Json schema more widely used and accessible - then it shouldn't be a surprise that horrible hacks like this shouldn't be in the code because they detract people from using it. And if you are the only person attempting to do this you need to be the one putting the work into it. Note that as I said I kinda like Json schema so I am willing help a bit in the form of code review and giving advice on how to do things.

Also, when I review a crate and see horrible unexplained hacks I usually refuse to use it because I assume authors are either incompetent or don't care (big int crates from parity comes to mind). I think other people might act similarly.

[apoelstra]

@Kixunil the negativity toward schemars is because the dependency itself has caused us grief. The weird hacks are then downstream from that because neither Jeremy nor I are really taking care of the schemars-related code here.

Though as Jeremy rightly points out, it did make us aware that CI is broken, which we ought to count in its favor :P.

[JeremyRubin]

The issue is a "upstream" MSRV incompatible change (Arc::as_ptr), so we pin to before that.

It's fine to have any change made to make the pinning be such that it is only required when we are doing MSRV testing / for people who need the MSRV to be < 1.41. However, I don't know if MSRV consumers (e.g. @apoelstra / blockstream) would be OK with adding cargo update -p $package --precise $version to their build processes, which would be the way to clean up this hack. In theory they shouldn't even have to do this because the feature is cfg'd out though, for them?

Sapio does not have an MSRV Guarantee at this point, so we're happy with only post 1.41.

Maybe one thing we can do is have schemars also require a feature that indicates greater than 1.41 rust version?

[apoelstra]

At Blockstream we vendor all dependencies and don't even need to use cargo update -p to prevent cargo from fetching new broken packages from the Internet. This is also the approach I use for personal projects where I care about the MSRV.

Elsewhere (in rust-bitcoin) we've been providing cargo update -p instructions in the README for over a year now and I think this has worked well for us, in the sense that nobody has complained. My feeling is that users who care about their dependencies would actually prefer to have instructions they can run, rather than having our Cargo.toml protentially limiting their flexibility. Pinning is difficult to do reliably.

FWIW I do think we should move away from pinning toward just proving README instructions and some extra CI logic. It's just that (a) I don't remember why I didn't suggest this in #167 and I worry that there is some reason it didn't work; (b) I personally don't want to do the work.