rust-bitcoin / rust-bitcoinconsensus

Bitcoin's libbitcoinconsenus.a with Rust binding. Built from Bitcoin sources with cargo.
Apache License 2.0
46 stars 34 forks source link

Depend on bitflags #60

Closed tcharding closed 2 years ago

tcharding commented 2 years ago

Currently we are manually creating C-like bitflags, there is a crate for this. In order to support our MSRV we cannot use the latest version of bitflags (which is v1.3.2 ATM), instead use v1.2.1.

FTR I tested the new flags during development using

    #[test]
    fn verify_flags() {
        assert_eq!(VERIFY_NONE, Flags::VERIFY_NONE.bits);
        assert_eq!(VERIFY_NONE, Flags::VERIFY_NONE.bits);
        assert_eq!(VERIFY_P2SH, Flags::VERIFY_P2SH.bits);
        assert_eq!(VERIFY_DERSIG, Flags::VERIFY_DERSIG.bits);
        assert_eq!(VERIFY_NULLDUMMY, Flags::VERIFY_NULLDUMMY.bits);
        assert_eq!(VERIFY_CHECKLOCKTIMEVERIFY, Flags::VERIFY_CHECKLOCKTIMEVERIFY.bits);
        assert_eq!(VERIFY_CHECKSEQUENCEVERIFY, Flags::VERIFY_CHECKSEQUENCEVERIFY.bits);
        assert_eq!(VERIFY_WITNESS, Flags::VERIFY_WITNESS.bits);
        assert_eq!(VERIFY_ALL, Flags::VERIFY_ALL.bits);
    }

Note also, that this patch removes the unit test for invalid flags - because invalid flags are now impossible, BOOM!

Fixes: #58

tcharding commented 2 years ago

Woops, forgot to add the pinning to CI, will add tomorrow - definitely works with 1.41.1

apoelstra commented 2 years ago

I'm pretty unconvinced that this is worth an extra dependency, especially one that's broken MSRV before we've even started using it.

apoelstra commented 2 years ago

And I would say that this is even worse than hex, as far as "crates that should obviously have been in the stdlib since 2015" go.

tcharding commented 2 years ago

Ha, I was surprised to see your positive response in the issue after our recent discussion, especially since the bit twiddling is super basic. Perhaps it was an old opinion and I should have clarified before doing this. Anyways, not to worry, I learned about the bitflags crate :)

apoelstra commented 2 years ago

Heh, well, it's only a month old ... but I didn't realize how trivial the diff would be. I think it actually increases the total complexity because now it's not obvious that the flags actually correspond to what the C code expects.

Kixunil commented 2 years ago

The diff looks trivial but the API improvement isn't. It creates a newtype with all relevant operator overloads.

Kixunil commented 2 years ago

Opened #61