fix: feature gate canonical macros
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:
- Make sure that the
impl_byte_array_rusqlite_only!macro isn't used whenever thecanonicalfeature that defines is set. - Remove the
asmfeature 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.yamlandrpc-endpoints.mdfor v2 endpoints,event-dispatcher.mdfor new events) - [ ] New clarity functions have corresponding PR in
clarity-benchmarkingrepo - [ ] New integration test(s) added to
bitcoin-tests.yml
We use the asm feature because it's 40% faster, so I'm afraid it's going to have to stay.
We use the
asmfeature 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?
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.
Remove the
asmfeature from thesha2crate. It didn't appear to be used anywhere.
Found #4435, but didn't add much color as to why
sha2-asmneeds 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.