Introduce a primitive types crate #1818
Conversation
tcharding
force-pushed
the
04-27-primitives
branch
from
70d0771
to
37a007d
Compare
|
Flagging that this needs the name to be agreed upon: #1809 (comment) |
NACK this name (unless it's temporary). |
| repository = "https://github.com/rust-bitcoin/rust-bitcoin" | ||
| documentation = "https://docs.rs/rust-bitcoin-primitives/" | ||
| description = "Primitive types used by the rust-bitcoin eccosystem" | ||
| categories = ["cryptography::cryptocurrencies"] |
There was a problem hiding this comment.
Should have also no-std and no-alloc
primitives/src/witness_version.rs
Outdated
| } | ||
|
|
||
| impl FromStr for WitnessVersion { | ||
| type Err = ConversionError; |
There was a problem hiding this comment.
I assume you copied this so I'd accept this as a stepping stone but this is not great. We should have ParseError != ConversionError. ParseError may internally store ConversionError but not vice-versa.
There was a problem hiding this comment.
I didn't copy it I wrote this, I'll have to come back and read this again (just going through my notifications quickly), I can't solve parse vs convert nuance right now. Will re-work the error though, thanks.
|
Just curious, what's the advantage of including this crate as another workspace member instead of part of the rust-bitcoin organization? |
|
@yancyribbens mainly allows us to update the API and then immediately use the new API in downstream crate. This way we can start testing out the new API sooner. It also has its own problems though - see: #1553 |
|
@yancyribbens in general, I think life is easier in Rust-land if we have a monorepo, at least for the "core" crates. rust-secp is the only real exception because it has such a different review/maintainership model ... though we may wind up pulling it in nonetheless because of what a PITA it is to have it separate. A similar strategy is taken by the |
|
@apoelstra if merging is an option I think I'm quite capable of dealing with "weird C crap, alignment issues and synchronization and FFI" as you put it if it helps. But I suspect the main obstacle is the other maintainers don't want all the notifications from |
|
@Kixunil hmm, good point. I was strongly considering bring rust-secp in because, as you say, I trust you (and the other rust-bitcoin maintainers) to be able to deal with the C crap ... or at least to avoid it when it's sufficiently obscure. But if merging the repos would cause a shitload of notifications for e.g. elichai or bluematt, which they could otherwise avoid, that's a harder burden to take. Another observation is that rust-secp has a one-ACK policy while rust-bitcoin has a two-ACK policy, because of the lack of maintainers on rust-secp. I worry that if rust-secp were part of this repo, we'd need two ACKs for everything but often be in a position where there weren't two people available who were comfortable ACKing. Especially if this led to bluematt or elichai being even less active on the crate.. |
|
Not sure about the last part but the theoretical number of total maintainers increasing should improve with merging so two ACKs might improve quality without significantly degrading progress. And the C crap there isn't that much of "rocket science" actually the only thing I found confusing is double panicking supposedly being UB across FFI. |
|
I'm happy to hear it, but I think everybody except you finds the conversation around rust-bitcoin/rust-secp256k1#539 (comment) pretty confusing :) Kidding aside, I think you're probably right. 99% of what goes into rust-secp is pretty straightforward. Even the C vendoring stuff is nicely wrapped up so you can just use the vendoring scripts. You don't even need to know C. |
Not sure I agree Network is a primitive. But yeah agree on the crate name being annoying. I contacted the bitcoin-rs guy to see if he'd be willing to discuss handing over crate names if we want them. His project seems stale. |
Also because rust-secp256k1 is not bitcoin related.. I mean it's used in bitcoin but in principle it's way more general. (Subtle hint at hex crate 😅 ) |
greetings folks -- I am happy to transfer crate names where it is helpful to you. I am currently bringing up a c++ to rust transpiler which will be able to expedite the rest of the translation. (This is why the project seems stale. It is not abandoned or stale, but awaiting the transpiler.) When this bringup is done, we should be able to zip pretty quickly through any remaining translation work. Are you open to the possibility of merging? For example, On my end, the transpiler is already good enough to do the remainder of both I suppose that when it is finished, the transpiler may be helpful to you folks as well -- is this statement correct? On my end, at least, it is one of the only remaining bottlenecks. |
|
@klebs6 that sounds great! I'm not sure which parts can be meaningfully merged but I'd be happy to look into it. Especially for primitives that could be shared between projects. Also transpiler sounds very interesting, you might get recognition in wider Rust community. |
|
Agreed that this sounds great! Though I also agree that I'm unsure, specifically, where there's an opportunity for unification :). In general this crate is trying to move toward a "pure Rust" model of Bitcoin and associated types -- so for things users or wallets might care about we provide our own data structures that increasingly differ from the internal libbitcoinconsensus/Core ones, and for consensus-critical things we try to avoid providing any functionality at all. So I'm unsure that we'll be able to directly use a C++ transpiler. But it's an interesting thought. It would be magical if we could somehow have a bitcoinconsensus library that was "written" in pure Rust and would compile to WASM etc.. |
|
It looks like Some benefits in creating an empty crate:
|
|
I'd prefer waiting until we have a stronger reason. Especially if we move address/bitcoin-specific stuff to |
|
|
Thanks man! |
|
On ice until we need it. |
|
It's too late now I guess, but for your project, I think it would have been better if you had prefixed all your crate with something more specific to your effort. Like |
Test the soon-to-be-released `hex` crate.
Built depending on this branch:
rust-bitcoin/hex-conservative#64
Currently does not work because of the recent changes we did to the
`DisplayArray` type in PR rust-bitcoin#52
rust-bitcoin/hex-conservative#52
I'm throwing this up so others can see the error without having to run
it locally.
Fixes a gap in the API of the taproot module. Callers can now use TapTree::root_hash or NodeInfo::node_hash to extract the taproot tree merkle root hash for fast validation without any ECC overhead.
Make the `From` impls conform to our convention. Refactor only, no logic changes.
We do not need to expose the hex error types in our public API, doing so is just asking for a headache later on. We add duplicate types to `bitcoin` and `hashes` to achieve the same thing in each crate. Fully encapsulate all `hex` module errors.
We don't currently use the `OperationError` type, remove it.
We need this error type if we are to move absolute/relative `Height` and `Time` types to `units`. Move the `ParseIntError` type to the `units` crate. Add re-expots to rust-bitcoin including a public re-export of the whole `units crate. Remove `parse` module because it was private, keep the `errors` re-export but deprecate it because it was public.
The `relative` module has a single general error type, we are moving away from this style to specific error types. Split the `relative::Error` up into three error structs. I forget the policy on public inner fields.
Move the bitcoin relative and absolute height and time types over to the units crate. This patch introduces a regression because it removes the ability to parse height or time types from hex. This is because the `hex` crate is being actively worked on to handle the prefix stuff that is currently in `bitcoin::string` and which was used by the height and time types.
Everything is now in place to move these two types across to units.
Create an empty crate that will hold primitive types used by the `rust-bitcoin` ecosystem.
tcharding
force-pushed
the
04-27-primitives
branch
from
37a007d
to
adcfd59
Compare
github-actions
bot
added
C-bitcoin
Pull Request Test Coverage Report for Build 7896620507Details
💛 - Coveralls |
|
Just a demo to see if we have the current Re-done on top of #2477, was pretty easy to do. The only complexity was the |
|
The crate is supposed to contain |
|
I just did locktimes as a quick and dirty check that it would work. But yes there are loads more things, I can do more if it adds value to do it now, probably need to do something that depends on weight and fee rate once those are in units. |
|
Sure wait for other changes. |
Create a crate that holds primitive types used by the
rust-bitcoinecosystem.bitcoin-primitivesalready exists on crates.io, userust-bitcoin-primitivesinstead.Just the lock times at the moment.