Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: feature gate canonical macros #5004

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

djordon
Copy link

@djordon djordon commented Jul 24, 2024

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 #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 djordon requested review from a team as code owners July 24, 2024 18:50
@CLAassistant
Copy link

CLAassistant commented Jul 24, 2024

CLA assistant check
All committers have signed the CLA.

@djordon djordon changed the title bug: feature gate canonical macros fix: feature gate canonical macros Jul 24, 2024
@jcnelson
Copy link
Member

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

@djordon
Copy link
Author

djordon commented Jul 24, 2024

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
Copy link
Author

djordon commented Jul 25, 2024

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.

@jbencin
Copy link
Contributor

jbencin commented Jul 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants