stacks-core icon indicating copy to clipboard operation
stacks-core copied to clipboard

fix: feature gate canonical macros

Open djordon opened this issue 1 year ago • 5 comments

Description

We use several crates in this repository in the https://github.com/stacks-network/sbtc/ repo, and would like to forgo pulling in as many unnecessary dependencies as possible. Fortunately, things here are setup already to accommodate this by allowing users to specify the developer-mode feature (setting default-features = false doesn't quite work).

This PR makes two changes:

  1. Make sure that the impl_byte_array_rusqlite_only! macro isn't used whenever the canonical feature that defines is set.
  2. Remove the asm feature from the sha2 crate. It didn't appear to be used anywhere.

Change (2) was made because it was causing us issues. This issue would only happen during arm64 builds (and only with cargo-lambda extension) so it's unlikely to be an "actual" issue within this repo. But I figure we should remove an unused dependency if possible. It's not clear to me whether it's actually unused or is sneakily used somewhere.

Applicable issues

I haven't opened them yet, (1) seems like the only "bug". You can reproduce by pulling down the clean-up-stacks-core-dependencies branch in https://github.com/stacks-network/sbtc and running cargo build. I'll open an issue here sometime later.

Additional info (benefits, drawbacks, caveats)

Found https://github.com/stacks-network/stacks-core/pull/4435, but didn't add much color as to why sha2-asm needs to be here.

Checklist

  • [ ] Test coverage for new or modified code paths
  • [ ] Changelog is updated
  • [ ] Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • [ ] New clarity functions have corresponding PR in clarity-benchmarking repo
  • [ ] New integration test(s) added to bitcoin-tests.yml

djordon avatar Jul 24 '24 18:07 djordon

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jul 24 '24 18:07 CLAassistant

We use the asm feature because it's 40% faster, so I'm afraid it's going to have to stay.

jcnelson avatar Jul 24 '24 19:07 jcnelson

We use the asm feature because it's 40% faster, so I'm afraid it's going to have to stay.

Ahh I see, makes sense. Okay fair enough.

Are the other changes okay? Also, would you be okay with activating the sha2/asm feature via a crate features here? I'm thinking of something like the following in the stacks-common Cargo.toml:

[features]
default = ["canonical", "developer-mode"]
canonical = ["rusqlite", "sha2/asm"]
developer-mode = []
slog_json = ["slog-json", "sha2/asm"]
testing = ["canonical"]

[target.'cfg(all(any(target_arch = "x86_64", target_arch = "x86", target_arch = "aarch64"), not(any(target_os="windows"))))'.dependencies]
sha2 = { version = "0.10", default-features = false }

or something similar. Would something like that be acceptable?

djordon avatar Jul 24 '24 20:07 djordon

Also, I just want to make it clear that this isn't pressing for us. I wouldn't look at this until after 3.0 is released.

djordon avatar Jul 25 '24 12:07 djordon

Remove the asm feature from the sha2 crate. It didn't appear to be used anywhere.

Found #4435, but didn't add much color as to why sha2-asm needs to be here.

As the author of #4435, I've looked into this a bit, and I'll share what I know about it.

The sha2 crate has gone through some refactoring over time. It used to be that you needed to add the sha2-asm crate to get the optimized assembly. Later, sha2-asm became a transitive dependency via sha2 enabled by the asm feature flag. In the latest version (0.11.0) it looks like the asm feature flag has been removed entirely. Since we're still using 0.10.8, which supports the asm feature flag, we are going to keep it for now.

jbencin avatar Jul 26 '24 01:07 jbencin