Rust 2018 & MSRV bump 1.41.1 tracking issue

[dr-orlovsky]

    I wonder if we should have a Rust 2018 tracking issue and start tagging things that are blocked on it. 
    Would be good to have a clear picture of what we're missing by being on 1.29.

— @apoelstra #373 (comment)

• No async support (not sure we need it at the level of this library)
• We can not rename packages, which creates a lot of ambiguity with serde crate/feature and prevents from using normal solutions like Replace feature serde with use-serde #373 (comment)
• No able to use simple derive proc macros with syn 1.0 support like in amplify_derive instead if hard-to-read impl_ macro and multiple impl From... and impl Display... blocks (and there are wrapper types as well https://docs.rs/amplify_derive/2.0.6/amplify_derive/derive.Wrapper.html)
• Not able to do #[non_exhaustive] to solve the problem of breaking API changes with gated enum veriants

Checklist

• [-] async - probably only for Read and Write async equivalents This is a nice to have, leave for another day.
• revive Replace feature serde with use-serde #373 - WIP in PR Feature use-serde renamed to serde #1006
• [-] use amplify_derive? note that this will blow up compile times for non-serde uses This is a nice to have, leave for another day.
• Implement TryFrom and use TryInto (Implement TryFrom #1007 does TryFrom but not the TryInto bit)
• sort_by_cached_key impl PartialOrd/Ord for PublicKey #524 Add PublicKey::to_sort_key method for use with sorting by key #1084
• [-] Use alloc? Is this still relevant?
• Use Infallible (Remove Uninhabited #1075)
• Use contains() on ranges Add OP_CHECKSIGADD and OP_SUCCESSxxx #644 (comment) (Remove MSRV todo comments #952)
• Use chunks_exact() instead of chunks (Remove MSRV todo comments #952)
• use Trait as _ (Remove unnecessary Write as _fmtWrite #965)
• [-] Exhaustive integer matching instead of comparing with zero (improves readability) (Match on specific integer values #1012)
• Use iter.copied() in Change EcdsaSig hash type deser in psbt #776 (comment) (Clear Clippy warnings #1004)
• Use (;)? instead of (;)* in macros (Add amount::Display - make formatting configurable #716) (Match against an optional single trailing colon #1010)
• Use literal instead of expr in macros (Add amount::Display - make formatting configurable #716) (Use fragment-specifier literal #1014)
• Usetrim_start_matches instead of trim_left_matches (Remove MSRV todo comments #952)
• Use Vec::from([foo]) instead of Vec::new followed by v.push(). (Remove MSRV todo comments #952, I checked all instances of mut foo = Vec::new() and also the same for BTreeMap)
• Change cause() to source() in impl of std::error::Error
• Use u16::to_be_bytes instead of swap_bytes (Remove MSRV todo comments #952)
• Use #[non_exhaustive] (Add non_exhaustive compiler directive to AddressType #1011 and Add non_exhaustive to all error enums #1026)

non_exhaustive types:

• Error (Add non_exhaustive to all error enums #1026)
• AddressType (Add non_exhaustive compiler directive to AddressType #1011)
• address::Payload (Add non_exhaustive compiler directive to AddressType #1011)
• LeafVersion (Add non_exhaustive compiler directive to AddressType #1011)

PR list:

• [-] Edition 2018 #635 closed in favour of 983
• Edition 2018 #983

[apoelstra]

• TryInto, see Derivation path improvements, closes #473 #498 and probably many others (this is Rust 1.34, not just 2018)

[RCasatta]

• sort_by_cached_key, see impl PartialOrd/Ord for PublicKey #524 (Rust 1.34)

[dr-orlovsky]

• stable alloc crate requiring 1.36 no_std support #528

[stevenroose]

• CommandString::try_into (see CommandString refactor as part of Make Inventory and NetworkMessage enums exhaustive #496) can use TryInto (Rust 1.34)

[apoelstra]

1.36 is also the (current, may change without major version bump) MSRV of rand 0.8, whose non-optional dep tree now consists of rand-core and nothing else.

[apoelstra]

Possibly this crate could be interesting for us https://crates.io/crates/standback

[dpc]

mrust can compile rustc 1.39 now. https://www.reddit.com/r/rust/comments/mjxbaz/mrustc_upgrade_rustc_1390/

[elichai]

mrust can compile rustc 1.39 now. https://www.reddit.com/r/rust/comments/mjxbaz/mrustc_upgrade_rustc_1390/

Nice!

[apoelstra]

Any opposition to moving to 1.39 then for rust-bitcoin 0.27? I can open another discussion issue. cc @TheBlueMatt 1.29 is really old now and is causing us a lot of grief especially with respect to nostd/alloc.

[TheBlueMatt]

Hmm, in general I think I'm fine with 1.39. It is less than two years old, though, so not sure if we don't want to use 1.34 (which is about two years old) or 1.36 (for alloc). I do admit it would be cool to go right to 1.39 and use async in a few places.

[apoelstra]

I think we could wait for the beginning of July and then jump to 1.36 (which aside from being "the one with alloc" also felt to me like a more important/well-tested release than 1.34 or 1.39).

[apoelstra]

As for using async I think that'd be a major change, though I guess if it were localized maybe not. And the upcoming API overhaul + transition to Rust 2018 would already be major changes, and it might be good to stagger them.

[TheBlueMatt]

Right, I was mostly thinking of (optionally) using async in rust-bitcoincore-rpc by porting the lightning-block-sync stuff to there.

[elichai]

yeah I don't think rust-bitcoin itself should be async, maybe when we have AsyncRead/AsyncWrite in libstd then we might want to add support for these in the Encode/Decode traits

[elichai]

As for bumping, I can come up with a summary of what features we might want and what versions they exists in,
the first things that come to my mind are mostly dependencies (latest rand: 1.36, cc: 1.34, serde_derive: 1.31)

[TheBlueMatt]

Alright, lets plan on 1.36 Soon (tm), then. We may actually go ahead and do it in RL because we're already at 1.30 (due to cargo bug that prevented us from using 1.29) and I'm tired of trying to make cc work right.

[dr-orlovsky]

Upgrade to 1.36 actually means we can use syn with its MSRV 0.31, and finally do derives instead of macros for many occasions.

I just completed development of a 1.31-compatible library simplifying creation of complex derivation macros to a big degree: https://docs.rs/amplify_syn/. You can see a sample of what it can be done with it here: https://github.com/LNP-BP/rust-amplify/blob/master/derive/src/lib.rs#L480-L553

Upon MSRV increase I will be happy to refactor rust-bitcoin code to introduce derivation macros using it (we already had one attempt in the past by @elichai, but then we were blocked by MSRV requirement).

[TheBlueMatt]

As of July 4 1.36 was two years old, so I think we can do this whenever now. https://blog.rust-lang.org/2019/07/04/Rust-1.36.0.html

[tcharding]

I've checked off all the basic ones crew, many of the others I either do not understand how to do or I do not understand the motivation behind doing them. If you put one on the list, and would like to see it implemented without having to do it yourself, please throw a comment here with a bit more context and I'm happy to do it. Thanks.

[Kixunil]

Adding #[non_exhaustive] attribute to the listed types would be great. It ensures forward compatibility.

$(,)? in macros is cosmetic.

[tcharding]

I had a think about the non_exhaustive task. I have a question: Why only do Error, if we are justified in doing one error why not do all of the error types?

[tcharding]

Does anyone remember where Infallible was supposed to be used?

[tcharding]

I scratched alloc from the list, flagging it here because I'm not 100% sure that is correct.

[Kixunil]

@tcharding I think by Error I meant all error types. :)

$ rg --multiline 'enum.*\{[ \t\n]*\}'
src/blockdata/script.rs
173:enum Uninhabited {}

Uninhabited should be removed and replaced with Infallible

We can skip alloc because the library is mostly useless without it and it'd be easier to do with Amount & co in a separate crate.

[tcharding]

Cool, cheers.

[tcharding]

There are only a few things to go on the list.

• The Infallible/Uninhabited thing
• The two checklist items at the top that do not have PRs, neither of which I'm know if we still want, anyone?
  • "async - probably only for Read and Write async equivalents"
  • "use amplify_derive? note that this will blow up compile times for non-serde uses"

[dr-orlovsky]

amplify_derive is a crate which I did to simplify deriving Errors, Froms and Displays all around, which may be useful here. That crate has very few dependencies (comparing to other error/display derive crates), and matches MSRV after its bump. So it's up to rust-bitcoin maintainers to decide whether to use it here or continue maintaining larger codebase for each error.

Me and @Kixunil are maintainers of that repo hosted in a dedicated github org (@Kixunil contributed to other parts of the amplify lib family) and I can add more maintainers from here to ensure safety of the use of the library & reviews requirement for any codebase changes.

[dr-orlovsky]

async - that was about adding support to async functions, for instance for implementing non-blocking consensus encoding/decoding with io::Read/Write - this may be useful, for instance when they are used in serializing large structures (blocks) to files by projects like electrum server (personally I do not need it, since I am using different approach to a non-blocking architecture with ZMQ and microservices - and not rust async which I do not like).

[Kixunil]

Exactly what @dr-orlovsky said. Those two are definitely not required for the issue to be complete and are more "nice to have" things that could be added at any time in the future. IMO it's even better to skip them to release more quickly.

@tcharding I belive we already did Infallible. I checked also wrong uses of () and while there is one in AddressType it's not infallible, just a bad error type.

[tcharding]

Thanks fellas, Uninhabited still exits and my memory fails me right now, its tied in with non_exhaustive, I meant to come back and work it out but now you've comment I'll just own up and say I can't remember :)

The long and the short of it is then, to sum up, checklist is done after:

• merge open PRs
• Tobin to investigate Uninhabited Done

[Kixunil]

Ah, I got confused because I remembered that the PR was opened but forgot it was closed without replacement.

[tcharding]

My bad.

[tcharding]

The checklist is complete - closing.