-
Notifications
You must be signed in to change notification settings - Fork 36
Release 0.26.1 with some minor API extensions #250
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
Conversation
This type appears in the public API as part of the pset::Error type. However, the type itself is not exported, preventing users from naming or directly constructing it. This prevents construction of a single variant of pset::Error, and in particular prevents us from semver-tricking the Error type. It is bad practice to export the pset::Error type without also exporting its constituent types (except as an intentional form of sealing, which this is not). That is enough reason to accept this PR. But to give a more elaborate reason: 1. I would like to semver-trick 0.25.3 by re-exporting all the types from 0.26.0 except the ones that changed (`PartiallySignedTransaction`, `TxOut`, `pset::raw::ProprietaryKey`, and types that expose these in their public APIs: `Transaction`, `Block` and the sighash cache). 2. I need to re-export the `encode::Error` type, which fortunately did not change since 0.25.x, so that I can re-use the Encodable/Decodable logic for all the types that I'm re-exporting. (I can either re-export those traits directly, along with the error type, or implement a new one with impls that pass through to the 0.26.x one ... or retain a *ton* of explicit unreviewable code which probably won't even compile without changes to handle some types becoming foreign). 3. However, `encode::Error` has conversions to and from `pset::Error`. So if I re-export `encode::Error` I need to re-export `pset::Error`. (Also, I just really want to; everything I *don't* re-export is a bunch of extra explicit code that needs to stick around in this version of the library. See above.) 4. But I need to explicitly construct `pset::Error` in the `Map::insert_pair` impl for `pset::Output`. (This *can't* be re-exported because it's private, and I can't re-export the whole `pset::Output` type because it has a method which returns `TxOut`, whose API changed in 0.26.0.) 5. ...so I need a re-export of `PsetHash` to do the construction.
cc @RCasatta hopefully this isn't too bad to review. If you can ACK it I can merge it (and my merge script will cut the release automatically). |
Right now we have explicit impls of Vec<T> for a ton of specific types T. This is annoying for users, who are unable to implement Vec<T> themselves for their own types. In particular is super annoying for me trying to do a semver trick with elements 0.25 and 0.26, since I can't reexport the 0.26 Encodable trait since I would need to implement it for Vec<TxOut> for the 0.25 TxOut type. (But conversely, if I retain the full trait definition in 0.25, I can't blanket-impl this trait for all T: Encodable because of this Vec<T> crap; so I wind up needing to explicitly implement Encodable and Decodeable for every single type I re-export, and if I forget any then that's a semver violation. Brutal.) We do this is because we need a specialized impl for Vec<u8> and Rust has no specialization. However, Kixunil pointed out on rust-bitcoin that we can just do "runtime specialization" by checking TypeId::of::<T> and doing the efficient thing when T is u8. In practice, because the compiler knows T at codegen time, it will know how the check goes and simply delete the check and the untaken branch, so we have exactly the same result as we would if we had specialization. This works only for T: 'static, but it turns out that all of our types that we currently impl Encodable for Vec<T> on are 'static (and it's hard to imagine one that wouldn't be, since all bitcoin/elements primitive types are simple PODs). It also seems plausible that a user could construct a Vec<u8> which the compiler somehow doesn't realize is a Vec<u8> (e.g. if they had a Vec<dyn Encodable> or something, in some future Rust version that supports that). Such code will be sound and correct, but be slow because it won't use the fast path. But I don't think this is a practical concern because I can't even figure out how to do it..
a983454
to
60de8df
Compare
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.
On 60de8df successfully ran local tests
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.
utACK 45a6938 code review
double checked unsafe use, checked is similar to what is done in rust-bitcoin
Do we want to mention in the changelog that user can now implement consensus encoding with their own types?
Oh, yeah, I should add a changelog entry. |
60de8df
to
7a84a74
Compare
Let's just cut a release with these changes. Then I can do a followup PR which uses the semver trick in a new 0.25.3.
7a84a74
to
05a24ad
Compare
Done. I just described the change. Saying "now you can implement this on your own types" is a little strong. It was always true that you could implement it on your own types... it's just that if you had a vector of those types you'd have an annoying time. (But not a very annoying time; you just need to define a newtype wrapper.) |
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.
On 05a24ad successfully ran local tests
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.
utACK 05a24ad
Tagged and published. |
I want to do the dtolnay "semver trick" where I release 0.25.3 which re-exports most of the types and traits from 0.26. This will make migration easier for users since they could use elements 0.25 and 0.26 in the same crate and have compatible types. In particular ElementsProject/elements-miniscript#97 would compile without needing to update
simplicity
andelements
at the same time.However, when doing this I identified some gaps in the API which make this harder than it should be. See the commit messages for detailed justifications.
Also release 0.26.1 with these changes.