Remove hashbrown dependency #564

tcharding · 2023-07-13T04:54:11Z

We can use BTreeSet instead of HashSet and BTreeMap instead of HashMap to remove the hashbrown dependency.

tcharding · 2023-07-13T05:47:57Z

Bother, will try to fix tomorrow.

tcharding · 2023-07-19T00:30:10Z

Putting on ice until #569 merges so I don't have to work out the pinning.

apoelstra · 2023-07-25T19:04:14Z

Looking good. The CI failure suggests that this is simply incomplete.

apoelstra · 2023-07-27T17:30:00Z

concept ACK. I actually think the duplicated code is short enough that we could just duplicate it rather than using a macro. Unsure.

tcharding · 2023-07-27T20:04:14Z

So the readability thing is definitely real. On the other hand, and what I tried to assist with the comment

// You can ignore the complexity of this and similar macros below, all they do is implement
// `Satisfier` on both `BTreeMap` and `HashMap` feature guarding the `HashMap` impl on "std"
// feature.

is that there is a trade off between the readability of the macro and the readability of the duplicate code. One thing I'm struggling with, as a newcomer to the miniscript codebase, is what code is duplicate and what code is slightly different. There is so much duplicate code that my eyes skim over things thinking they are duplicate and missing the details. I've attempted refactorings only to find that I missed some difference - perhaps its just me but the code duplication seems to make me more lazy at reading.

tcharding · 2023-07-27T20:06:36Z

Would putting this on top of each macro help?

/// Implements `Satisfier<Pk> for $map<(Pk, TapLeafHash), taproot::Signature>`.

apoelstra · 2023-07-27T20:18:18Z

Ok, this makes sense. And it is true that this library has a ton of duplicate code, some of which is almost-duplicate, and some of which should be duplicate but isn't.

Maybe if we made the macro contents look like impl Satisfier<Pk> for BTreeMap<(Pk, TapLeafHash), taproot::Signature>; so it's obvious what the code "should" be doing, that'd be fine with me.

tcharding · 2023-07-27T21:15:09Z

Oh that is kinda nice, hows this?

macro_rules! impl_satisfier_for_map_key_hash_to_taproot_sig {
    ($(#[cfg($attr:meta)])* impl Satisfier<Pk> for $map:ident<$key:ty, $val:ty>) => {
        impl<Pk: MiniscriptKey + ToPublicKey> Satisfier<Pk> for $map<$key, $val>
        {
            fn lookup_tap_leaf_script_sig(
                &self,
                key: &Pk,
                h: &TapLeafHash,
            ) -> Option<bitcoin::taproot::Signature> {
                // Unfortunately, there is no way to get a &(a, b) from &a and &b without allocating
                // If we change the signature the of lookup_tap_leaf_script_sig to accept a tuple. We would
                // face the same problem while satisfying PkK.
                // We use this signature to optimize for the psbt common use case.
                self.get(&(key.clone(), *h)).copied()
            }
        }
        
    };
}
impl_satisfier_for_map_key_hash_to_taproot_sig! {
    impl Satisfier<Pk> for BTreeMap<(Pk, TapLeafHash), bitcoin::taproot::Signature>
}
impl_satisfier_for_map_key_hash_to_taproot_sig! {
    #[cfg(feature = "std")]
    impl Satisfier<Pk> for HashMap<(Pk, TapLeafHash), bitcoin::taproot::Signature>
}

apoelstra · 2023-07-31T14:10:45Z

Yeah, that looks great to me! This way when you're looking for impls you can still find it via grep, and once you arrive at the macro call, the actual invocation is nearby and almost entirely quoted code.

tcharding · 2023-08-01T05:28:56Z

Cool, I'll hack it up.

sanket1729 · 2023-08-09T05:53:07Z

concept ACK. I like the current code, but I don't think we need the pinning commit anymore

tcharding · 2023-08-09T06:48:23Z

I forgot I'd only done one of the macros with the new style. All done now, and squashed into a single commit although this has made the diff a bit hard to view. I can split it all back up if needed.

apoelstra · 2023-08-09T13:45:16Z

The CI failure looks legit (and annoying :))

tcharding · 2023-08-10T00:47:39Z

The problem was I forgot to add the passed in attribute inside the macro call.

apoelstra · 2023-08-10T15:22:57Z

The first commit doesn't compile with cargo test --no-default-features --features no-std anymore (and I suspect the latter commit only compiles because HashMap is no longer used).

tcharding · 2023-08-10T21:24:16Z

My bad, will investigate. Thanks

Define a marco that when called looks reasonably similar to the original code to implement `Satisfier<Pk> for map<Pk, bitcoin::ecdsa::Signature` and call it with `BTreeMap` and `HashMap`. This is an API addition, `HashMap` is imported from `hashbrown` for no-std builds.

As we did for `map<Pk, bitcoin::ecdsa::Signature>`, define a macro that implements `Satisfier<Pk>` for `map<(Pk, TapLeafHash), bitcoin::taproot::Signature>` And call it with `BTreeMap` and `HashMap`.

As we did for `map<Pk, bitcoin::ecdsa::Signature>`, define a macro that implements `Satisfier<Pk>` for `map<hash160::Hash, (Pk, bitcoin::ecdsa::Signature)>` And call it with `BTreeMap` and `HashMap`.

As we did for `map<Pk, bitcoin::ecdsa::Signature>`, define a macro that implements `Satisfier<Pk>` for `map<(hash160::Hash, TapLeafHash), (Pk, bitcoin::taproot::Signature)>` And call it with `BTreeMap` and `HashMap`.

The `BTreeMap` is a drop in replacement for the `HashMap` with the benefit that it is available in no-std environments. Use a `BTreeMap` for the `despcriptor::KeyMap` instead of a `HashMap`. Done in preparation for removing the `hashbrown` dependency.

The `BTreeMap` is a drop in replacement for the `HashMap` with the benefit that it is available in no-std environments. We have a few uses of `HashMap`s internally within functions, we can use `BTreeMap` instead. Done in preparation for removing the `hashbrown` dependency.

At some stage, it appears, we have changed from using a `HashMap` to a `BTreeMap` but the variable names and comments still refer to hash map. Note also that these comments are "what" comments (as opposed to "why" comments) so they are not adding an value anyways. Fix stale code by doing: - Remove the "what" comments. - Rename local variables from `hash_map` to plain old `map`

The `BTreeSet` is a drop in replacement for the `HashSet` with the benefit that it is available in no-std environments. We have a few uses of `HashSet`s internally within functions, we can use `BTreeSet` instead. Done in preparation for removing the `hashbrown` dependency.

Instead of `HashMap` we can use a `BTreeMap` in unit test code, the functionality is exactly the same although `BTreeMap` does not have a `with_capacity` constructor.

tcharding · 2023-08-10T22:24:39Z

Ok, there offender turned out to be one little usage of HashMap in a unit test. The PR is now broken up into very small digestible chunks.

apoelstra · 2023-08-11T18:51:12Z

In 986ed86

cargo test --no-default-features --features='no-std compiler'

fails

We can use BTreeMap as a drop in replacement in the `compiler` modules test code.

Feature guard implementations of `Satisfier` on `HashMap` on the "std" feature. We then no longer require the `hashbrown` dependency sine we use `BTreeMap` and `BTreeSet` everywhere else.

tcharding · 2023-08-11T21:38:20Z

Thanks, feels a bit like you are my CI again.

apoelstra

ACK 49fd7b9

49fd7b9ad80064de42b2ff2ca63fe750b609d478 Remove hashbrown dependency (Tobin C. Harding) 4e174e1432f1a23d418adcc8c6cf41a4bd42ce43 Use BTreeMap in compile test code (Tobin C. Harding) eefd3377d33057d3576acc94f847993804d0722e Use BTreeMap in test code (Tobin C. Harding) 13b9ec9a0843da4a5b894d937e08d83fa0dae878 Use BTreeSet internally (Tobin C. Harding) 023f13b95576d94de9e1eef5cf4c2175973d281a Fix stale variable names and comments (Tobin C. Harding) cef9d461a2716b2362ffabf10ee99bd1bb13268f Use BTreeMap internally (Tobin C. Harding) 4e2ee866a082bb26d5977fdad5da2fdcc2fc218e Use BTreeMap for descriptor::KeyMap (Tobin C. Harding) 49a2bdfb4e93bf901f93798ab7b5175220966e66 Add Satisfier macro for ((hash160, leaf hash), (pk, tap sig)) (Tobin C. Harding) df11203e4f01e7ca90d3b1f3c4bef04ce802588b Add Satisfier macro for (hash160, (pk, ecdsa sig)) (Tobin C. Harding) 42f559f9613c978ca413188378c36619ba24e01f Add Satisfier macro for ((pk, leaf hash), taproot sig) (Tobin C. Harding) 20ca6a5d6f2760d27432d27194bf0af27841235c Implement Satisfier for map<Pk, bitcoin::ecdsa::Signature> (Tobin C. Harding) Pull request description: We can use `BTreeSet` instead of `HashSet` and `BTreeMap` instead of `HashMap` to remove the `hashbrown` dependency. ACKs for top commit: apoelstra: ACK 49fd7b9ad80064de42b2ff2ca63fe750b609d478 Tree-SHA512: 73d0e29812b91c18a10e6dd57363a7b188985c62d74500134b41879539ce2ecc6cf8d638ed27ba4986ddbe3e70f7f5bc0d3cf70491da521b5f724b818cb5334c

tcharding force-pushed the 07-13-rm-hashbrown-dep branch from 4ae1229 to 5be647f Compare July 13, 2023 05:26

tcharding force-pushed the 07-13-rm-hashbrown-dep branch from 5be647f to 78c67f7 Compare July 18, 2023 02:25

tcharding marked this pull request as draft July 19, 2023 00:30

tcharding force-pushed the 07-13-rm-hashbrown-dep branch from 78c67f7 to dfd7f1f Compare July 25, 2023 08:37

tcharding force-pushed the 07-13-rm-hashbrown-dep branch from dfd7f1f to f5736cf Compare July 27, 2023 03:31

tcharding marked this pull request as ready for review July 27, 2023 04:57

tcharding force-pushed the 07-13-rm-hashbrown-dep branch from f5736cf to adf4630 Compare August 9, 2023 06:47

tcharding force-pushed the 07-13-rm-hashbrown-dep branch from adf4630 to e5023b7 Compare August 9, 2023 06:49

tcharding force-pushed the 07-13-rm-hashbrown-dep branch from e5023b7 to b90cea5 Compare August 10, 2023 00:11

tcharding added 5 commits August 11, 2023 07:39

Add Satisfier macro for ((pk, leaf hash), taproot sig)

42f559f

As we did for `map<Pk, bitcoin::ecdsa::Signature>`, define a macro that implements `Satisfier<Pk>` for `map<(Pk, TapLeafHash), bitcoin::taproot::Signature>` And call it with `BTreeMap` and `HashMap`.

Add Satisfier macro for (hash160, (pk, ecdsa sig))

df11203

As we did for `map<Pk, bitcoin::ecdsa::Signature>`, define a macro that implements `Satisfier<Pk>` for `map<hash160::Hash, (Pk, bitcoin::ecdsa::Signature)>` And call it with `BTreeMap` and `HashMap`.

Add Satisfier macro for ((hash160, leaf hash), (pk, tap sig))

49a2bdf

As we did for `map<Pk, bitcoin::ecdsa::Signature>`, define a macro that implements `Satisfier<Pk>` for `map<(hash160::Hash, TapLeafHash), (Pk, bitcoin::taproot::Signature)>` And call it with `BTreeMap` and `HashMap`.

tcharding added 4 commits August 11, 2023 08:01

Use BTreeMap in test code

eefd337

Instead of `HashMap` we can use a `BTreeMap` in unit test code, the functionality is exactly the same although `BTreeMap` does not have a `with_capacity` constructor.

tcharding force-pushed the 07-13-rm-hashbrown-dep branch from b90cea5 to 986ed86 Compare August 10, 2023 22:23

tcharding added 2 commits August 12, 2023 07:37

Use BTreeMap in compile test code

4e174e1

We can use BTreeMap as a drop in replacement in the `compiler` modules test code.

Remove hashbrown dependency

49fd7b9

Feature guard implementations of `Satisfier` on `HashMap` on the "std" feature. We then no longer require the `hashbrown` dependency sine we use `BTreeMap` and `BTreeSet` everywhere else.

tcharding force-pushed the 07-13-rm-hashbrown-dep branch from 986ed86 to 49fd7b9 Compare August 11, 2023 21:37

apoelstra approved these changes Aug 12, 2023

View reviewed changes

apoelstra merged commit 65a004c into rust-bitcoin:master Aug 12, 2023

tcharding deleted the 07-13-rm-hashbrown-dep branch August 18, 2023 22:44

Remove hashbrown dependency #564

Remove hashbrown dependency #564

Uh oh!

Conversation

tcharding commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jul 13, 2023

Uh oh!

tcharding commented Jul 19, 2023

Uh oh!

apoelstra commented Jul 25, 2023

Uh oh!

apoelstra commented Jul 27, 2023

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

apoelstra commented Jul 27, 2023

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

apoelstra commented Jul 31, 2023

Uh oh!

tcharding commented Aug 1, 2023

Uh oh!

sanket1729 commented Aug 9, 2023

Uh oh!

tcharding commented Aug 9, 2023

Uh oh!

apoelstra commented Aug 9, 2023

Uh oh!

tcharding commented Aug 10, 2023

Uh oh!

apoelstra commented Aug 10, 2023

Uh oh!

tcharding commented Aug 10, 2023

Uh oh!

tcharding commented Aug 10, 2023

Uh oh!

apoelstra commented Aug 11, 2023

Uh oh!

tcharding commented Aug 11, 2023

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tcharding commented Jul 13, 2023 •

edited

Loading