Start crate smashing #751
Start crate smashing #751
Conversation
|
Any reason to switch to |
|
Awesome, I just hit issues with @DON-MAC-256 @apoelstra said he likes |
|
Thinking about this again wouldn't it be easier to start with separating-out primitives? |
|
We have to start at the bottom, right? |
|
Not sure what you consider bottom but thinking about it again:
How I would approach it:
|
9c3e016 to
040598f
Compare
I took the bottom to be
Did I misunderstand the thread? Do you still have some concern on the ordering of primitives vs encoding ( |
040598f to
d062632
Compare
|
Actually, you're right. |
9a98934 to
1aa9164
Compare
The `base58` module should not be encapsulating a secp256k1 error, these things are un-related. Removing the error variant is an API breaking change but is necessary in preparation for splitting the `base58` module out into the about-to-be-created `bitcoin-str` crate.
f7f2f65 to
320cc45
Compare
Add a workspace to the top level directory. Create a directory `bitcoin` and move into it the following as is with no code changes: - src - Cargo.toml - test_data - examples Add a workspace to the repository root directory. Add the newly created `bitcoin` crate to the workspace. Fix `contrib/test.sh`, fuzz, embedded, and CI so everything still passes.
320cc45 to
b037c7b
Compare
Create a new crate `bitcoin-str` and add it to the newly created workspace. Extract `base58.rs` and `endian.rs` modules out of `bitcoin` and into the new crate. Depend upon and re-export `bitcoin_hashes` and `base64-compat` from the new crate. Fix manifest files as required. Re-exports in `bitcoin` of all types and functions from `bitcoin-str` allow for minimal changes to the code base.
b037c7b to
dce4ff7
Compare
|
@tcharding IDK if you noticed that this was originally planned for version 0.30. Maybe it'll cause many rebases of this. However I would agree with change to get this in 0.29 |
|
Oh I did not realize that, I thought it was wanted for |
| @@ -0,0 +1,41 @@ | |||
| [package] | |||
| name = "bitcoin-str" | |||
| version = "0.27.0" | |||
There was a problem hiding this comment.
| version = "0.27.0" | |
| version = "0.1.0" |
| [workspace] | ||
| members = ["bitcoin", "bitcoin-str"] | ||
| exclude = ["embedded", "fuzz"] |
There was a problem hiding this comment.
You can reduce the diff here by not moving the main crate into a subdirectory.
Regular crate manifests can also have a [workspace] table, meaning you can introduce bitcoin-str and move all the stuff in without touching anything in bitcoin.
There was a problem hiding this comment.
Oh cool, I did not know that. Cheers man!
|
|
||
| [dependencies] | ||
| bech32 = { version = "0.8.1", default-features = false } | ||
| bitcoin_hashes = { version = "0.10.0", default-features = false } |
There was a problem hiding this comment.
Do we need the hashes as well or are we just taking hex from this? Would be nice to move hex into bitcoin-str and have bitcoin-hashes depend on bitcoin-str. AFAIK, hex only lives in bitcoin-hashes because bitcoin-hashes was already a leaf dependency.
There was a problem hiding this comment.
concept ACK that dependency inversion.
Yes, the hex stuff was put into bitcoin-hashes purely because it was the smallest crate in the ecosystem. (I might've put hex into bech32 if it'd been around at the time :P.)
There was a problem hiding this comment.
There are three functions in the base58 module that depend on hashing for checksum'ing, e.g.
/// Obtain a string with the base58check encoding of a slice
/// (Tack the first 4 256-digits of the object's Bitcoin hash onto the end.)
pub fn check_encode_slice(data: &[u8]) -> String {
let checksum = sha256d::Hash::hash(data);
encode_iter(
data.iter()
.cloned()
.chain(checksum[0..4].iter().cloned())
)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping to convey that with the comment in bitcoin-str/src/lib.rs
pub mod hex {
//! Re-exports hex de/serialization code from `bitcoin_hashes`.
//!
// There is a chicken and the egg problem with putting _all_ the en/decoding code in a single crate.
// Hashing cannot be separated totally from en/decoding because base58 -> hashing -> hex.
Or is there a way around this?
There was a problem hiding this comment.
Why we don't use the hex crate?
There was a problem hiding this comment.
After reading your argument I no longer feel comfortable with accepting option 3.
I believe we should really revive it. But could the crate be without "bitcoin" in its name or docs? I'd like it being neutral general purpose 1.36-compatible crate. hex-compat similarly to base64-compat sounds sensible.
There was a problem hiding this comment.
There is already amplify_num crate I made for this purpose, when work on bitcoin_hex has stalled. I will be ok with moving it to this org and even renaming it. Additionally to hex it has big and small integers (u1-u7, u256 taken from rust-bitcoin ecosystem and u512-u1024). u256 as multiple improvements over type in rust-bitcoin, providing arithmetics, overflow wraping/checkings, octal/hex etc formating – but the code base is still quite small.
Currently it is a part of the repo managed by me and @Kixunil: https://github.com/LNP-BP/rust-amplify
There was a problem hiding this comment.
I'm unconvinced that "changes will happen" to hex encoding code which require synchronized updates, but concept ACK having a separate hex-compat crate.
Agree that it shouldn't have bitcoin anywhere in the name.
I'm also OK in concept with talking to the hex maintainer but I have no interest in doing this myself.
There was a problem hiding this comment.
I'm unconvinced that "changes will happen" to hex encoding code which require synchronized updates, but concept ACK having a separate hex-compat crate.
That was my thinking as well. Very likely, the hex-encoding needs within bitcoin-hashes are not going to change any time soon and hence we might be able to reduce it down to 2-3 private functions that have exactly the signatures we need because the don't need to account for any other usage.
Contrary to that, a public hex module within bitcoin-str will likely need to offer much more flexible APIs, see more usage and thus require more maintenance.
That was the thinking when I proposed option 3 above. I am not fussed about extracting a dedicated crate though.
There was a problem hiding this comment.
OK, I've mentioned there that I wish for MSRV to be no newer than 1.41.1 and noticed they also have 1.0 milestone which is very nice. I can see that the code being short and unlikely to change can be a good argument for duplication being fine but still prefer a separate crate. It could be a fork of hex with changes to make it compilable on 1.41.1.
I'm expecting this to take a few iterations.
Not sure if this is wanted still as part of 0.28 release. I've taken it off draft because its ready for review although clearly not ready for merge.I need input from others for this one, both opinions and also some technical things, primarily I cannot work out how to re-exportbitcoin_hashesfrombitocoin-strand get macros to work in Rust 1.29 (that should be the only CI failure), it builds locally with newer versions of Rust.Make an attempt to start the crate smashing described in #550. This is a long thread and its not super easy to tease out what has consensus and what does not but this PR will try to do that.
The stated aims of this work are:
rust-bitcoin.At a high level this PR does:
bitcoinbitcoin-str(also referred to asbitcoin-encodingin the issue thread)I've tried to only do changes required to make code build when extracting code into
bitcoin-str.bitcoin-strincludes:hashes::hex::*from modulebitcoin_str::hexfor ergonomicsQuestions/TODOs
bitcoin-str/src/hex.rs(CI failure)?bitcoin_hashesinbitcoin.