Calling `wpubkey_hash` on a key that is uncompressed is flat out an
error, really it is a programmer error at build time because a segwit
key should never be compressed, however, for historical reasons we do
not enforce this in the type system. As a step towards clarity make it
an error to call `wpubkey_hash` on a an uncompressed pubkey. This adds
documentation and potentially might assist debugging for newer devs.
Currently the feature enabling is different for "std" and "no-std",
which is again different to the order in the dependencies section. These
two things make reading the manifest harder than it needs to be.
Put the dependencies in alphabetic order in the dependencies section as
well as when enabling them.
Refactor only, no logic changes.
b1870656c9 Combine the two Error impl blocks together (Tobin C. Harding)
82ea4ff31d Move error code to submodule (Tobin C. Harding)
67511ed03f Move no_std above comment (Tobin C. Harding)
f83d68f246 Add vertical whitespace (Tobin C. Harding)
Pull request description:
Do various refactorings to the `io` crate. These are all trivial to review except the bigish commit: `d3d3e50f Move error code to submodule` which is code move only.
ACKs for top commit:
Kixunil:
ACK b1870656c9
apoelstra:
ACK b1870656c9
Tree-SHA512: 391347614121ea8ebe55af705e9688a9a668222cb8f1eb0a3a5275970e67ffb9dd6dfbd436080ab72592f0c86f2b8b36e0ff0322ca25dcd926d0ce80fb8b942f
Move error code to a private `error` submodule and re-export it at the
crate root.
(Puts private `sealed` module at the bottom of the file i.e., this patch
is strictly a code move but we re-order the code while moving it.)
6a9713a0cc Add author for the format bot (Tobin C. Harding)
Pull request description:
The format bot defaults to using the person who triggered the workflow. We can use the `author` option to configure a dummy bot author.
ACKs for top commit:
Kixunil:
ACK 6a9713a0cc
apoelstra:
ACK 6a9713a0cc
Tree-SHA512: 8b2c419767d217ddb499032497f46588f939d92a01694b611ad44d872541871da28cfe3e9edd0c917bfdea02e86f8ed3a4fdfef60b59d0be5b7c0c2814a5db7b
c27f101840 Introduce usage of TBD instead of NEXT-RELEASE (Tobin C. Harding)
Pull request description:
There is an ecosystem-wide solution to our custom usage of `NEXT-RELEASE` in deprecation attributes - use
`#[decprecated(since = "TBD", note = "use bar instead")]`
Lets use it.
ACKs for top commit:
apoelstra:
ACK c27f101840
Kixunil:
ACK c27f101840
Tree-SHA512: 00cbb09f41249402f90e8b2da8c930adca9eec2e4e2684fb26eb844f738245a113788fde392b0b3bf4b5f278057e111ece7a805d0b19e93a4997873ba8051d2f
There is an ecosystem-wide solution to our custom usage of
`NEXT-RELEASE` in deprecation attributes - use
`#[decprecated(since = "TBD", note = "use bar instead")]`
Lets use it.
f764a607ac Use conventional import path for io crate (Tobin C. Harding)
5c0759a390 Inline io module in io crate root (Tobin C. Harding)
80fe9b99b2 Move public macros to a separate module (Tobin C. Harding)
Pull request description:
Its not immediately obvious why we nest the whole `io` code in an `io` submodule within `lib.rs`. As far as I can tell we can inline it and re-export from `rust-bitcoin` same as we do for our other dependencies.
This change would effect other users of the crate but since the `io` crate is unreleased this effects no-one except us.
After doing this it might be because `crate::io::Foo` looks good when near `std::io::Foo`?
ACKs for top commit:
apoelstra:
ACK f764a607ac
Kixunil:
ACK f764a607ac
Tree-SHA512: 38888b0c23d5f2cd874f77dd332fe4fa4b9acb90e3a2dac19e62ed3d98151acd7480c719aa85434e1a3de987af2c4f565528a914a14d5fd3f0f0e410cbdf5d40
We have a convention in `rust-bitcoin` to use external crates directly
when importing them not via `crate::foo`.
Update all the import paths for `io` to use this form.
fcc4c40a1c Rename from_vb_const (yancy)
Pull request description:
The new function is more clear because the purpose of the function is to return a value that doesn't need to be unwrapped. The current MSRV does not allow unwrap() in const context.
ACKs for top commit:
apoelstra:
ACK fcc4c40a1c
Kixunil:
ACK fcc4c40a1c
Tree-SHA512: 62bbd61800e9f29768d884dbe3bca14fea9b51b8f413131e0f29bfc0f0d0e20631d30489e078cc948f2dba0c5d7d9d7c229b4bb7187faef23083a88337efb6a6
The new function is more clear because the purpose of the function is to
return a value that doesn't need to be unwrapped. The current MSRV does
not allow unwrap() in const context.
Its not immediately obvious why we nest the whole `io` code in an `io`
submodule within `lib.rs`. As far as I can tell we can inline it and
re-export from `rust-bitcoin` same as we do for our other dependencies.
This change would effect other users of the crate but since the `io`
crate is unreleased this effects no-one except us.
In preparation for inlining the `io` molule, move the public macros to a
private `macros` module.
Includes removal of the public re-export of `std` as `_std` - flaggin
this because I do not understand why it is here in the first place, we
can use `std::io::Write` in code that is feature gated on "std".
761de886be Remove imports of TryFrom and TryInto (Tobin C. Harding)
4d5415f835 Add rust-version to the workspace manifests (Tobin C. Harding)
a41e978855 Update to edition 2021 (Tobin C. Harding)
d9cc724187 Bump MSRV to Rust version 1.56.1 (Tobin C. Harding)
Pull request description:
Rust version 1.56.0 introduced edition 2021. Shortly afterwards, on October 21 2021 Rust version 1.56.1 was released.
Debian stable is currently shipping `rustc 1.63.0`. Our stated MSRV policy is: In Debian stable and at least 2 years old. Therefore our MSRV policy is met by Rust version 1.56.1 and we can strat to bump our MSRV org wide. Start by bumping the `rust-bitcoin` and `hashes` MSRV to Rust 1.56.1
Start by bumping the `rust-bitcoin` and `hashes` MSRV to Rust 1.56.1, includes:
- Update docs.
- Update CI and remove pinning.
- Update the build files and remove now stale cfg attributes rust_v_1_x for values less than the new MSRV.
- Use new `IntoIterator` for arrays so we no longer need to allocate a vector to iterate.
Links:
- https://blog.rust-lang.org/2021/11/01/Rust-1.56.1.html
- https://blog.rust-lang.org/2021/10/21/Rust-1.56.0.html
- https://packages.debian.org/stable/rust/rustc
ACKs for top commit:
Kixunil:
ACK 761de886be
apoelstra:
ACK 761de886be
Tree-SHA512: 3a81c8bfa37d8cec0ec794f516f014da67ae8e437decf149c9681aa547885acac0ee07ea2c0f42e4f6bfd6f7ed1695fcf4747f53cc50e5f4e70ce3fe7bcba4e9
fa104aefa5 bitcoin: Add signing examples (Tobin C. Harding)
Pull request description:
Add two signing examples to showcase signing a simple one input two output transaction using both segwit v0 outputs and taproot outputs.
This patch is the result of the recent rust-bitcoin TABConf workshop, with bug fix by Sanket, updated to use APIs from tip of master branch.
This code, depending on v0.30.0 is what was added to the cookbook.
ACKs for top commit:
realeinherjar:
ACK fa104aefa5
apoelstra:
ACK fa104aefa5
Tree-SHA512: ce0d5b8291c94387c68b5e1cf740d3267fc00c997af5b96f5be525f348140d9a9af17ab66d556990f09bf081a5a812374cb633ea276100c7c21f218b85eae3fd
f41ebc2149 Add test for input weight predictions (conduition)
4514a80a23 Fix the InputWeightPrediction constants for DER signatures (conduition)
b5ce219c62 add weight method to InputWeightPrediction (conduition)
Pull request description:
The `P2WPKH_MAX` constant assumed DER signatures in the witness have a max length of 73. In practice, their maximum length is 72, because [BIP62](https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki) forbids nodes from relaying transactions which contain non-canonical ECDSA signatures (i.e. TX sigs must have an $s$ value of less than $\frac{n}{2}$).
This means $s$ is never encoded with a leading zero byte, and the signature as a whole never exceeds 72 bytes in total encoded length. The `ground_p2wpkh` function was already correct; only the constant needed to be corrected.
Technically 73 bytes *is* the upper limit for signatures, as nothing forbids miners from including such non-standard transaction signatures in blocks, but for the purposes of fee estimation and input weight prediction, 72 is the number which 99.9% of implementations should use as their ceiling. We already use it as the ceiling for the `ground_p2wpkh` function - `ground_p2wpkh(0)` returns a prediction which uses a witness signature of length 72.
Reference:
- https://bitcoin.stackexchange.com/questions/77191/what-is-the-maximum-size-of-a-der-encoded-ecdsa-signature
- https://bitcoin.stackexchange.com/questions/106435/are-high-s-ecdsa-signatures-forbidden-in-segwit-witnesses
- https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki
To enable testing, I added a `weight()` method to `InputWeightPrediction` and made it public but i'm not sure whether it has a use-case. Let me know if I should make it private instead.
ACKs for top commit:
tcharding:
ACK f41ebc2149
apoelstra:
ACK f41ebc2149
Tree-SHA512: 10e837bad9881c0efebb0598eaefd4ab039f2a6ececead75a68e253d84f5e85cb30496a6069eee8dfe9714773f3aa23cfe373f5d88d1c5609e1b1be1ff142e37
Rust version 1.56.0 introduced edition 2021. Shortly afterwards, on
October 21 2021 Rust version 1.56.1 was released.
Debian stable is currently shipping `rustc 1.63.0`.
Our stated MSRV policy is: In Debian stable and at least 2 years old.
Therefore our MSRV policy is met by Rust version 1.56.1 and we can strat
to bump our MSRV org wide.
Start by bumping the `rust-bitcoin` and `hashes` MSRV to Rust 1.56.1,
includes:
- Update docs.
- Update CI and remove pinning.
- Update the build files and remove now stale cfg attributes rust_v_1_x
for values less than the new MSRV.
- Use new `IntoIterator` for arrays so we no longer need to allocate a
vector to iterate.
Links:
- https://blog.rust-lang.org/2021/11/01/Rust-1.56.1.html
- https://blog.rust-lang.org/2021/10/21/Rust-1.56.0.html
- https://packages.debian.org/stable/rust/rustc
75c490c60f hashes: Remove default features from schemars dep (Tobin C. Harding)
1105876423 Remove whitespace character from string (Tobin C. Harding)
a6d7d542ab bitcoin:: Remove dev dependency serde_derive (Tobin C. Harding)
Pull request description:
Done while investigating removal of `serde_derive` dependency.
- Patch 1: Do trivial dev-dep removal
- Patch 2: Manually implement `JsonSchema` and remove default dependencies from "schemars" dependency (transitively depends on `serde_derive`)
ACKs for top commit:
apoelstra:
ACK 75c490c60f
Tree-SHA512: aab5bd622a76fc24259933af2f20f863d20c8ccf6e69e68246c374266c540e483ced8a769532582a184b922996857db7320a6b08ae9b5b95503eac752ef9d301
321d3923b8 Add from_vb_const function (yancy)
Pull request description:
This function is can be used to construct a Weight type from_vb in const context. Note I don't think it's possible to test the panic case since it's a compile time error work around currently to panic.
ACKs for top commit:
tcharding:
ACK 321d3923b8
apoelstra:
ACK 321d3923b8
Tree-SHA512: dc11409f0e3079400da261a8c9f580ef0527b77643ce1a5dda65c0975db19c2f2da46ac693e6a2bf49e0105b8b096e1ee51f09f5d1c78d634e4e274d7467ee05
The P2WPKH_MAX constant assumed DER signatures in the witness have
a max length of 73. However, their maximum length in practice is 72,
because BIP62 forbids nodes from relaying transactions whose ECDSA
signatures are not canonical (i.e. all sigs must have an s value of
less than n/2). This means s is never encoded with a leading zero
byte, and the signature as a whole never exceeds 72 bytes in total
encoded length. The ground_p2wpkh function was already correct;
only the constant needed to be corrected.
Currently we derive `impl Debug for PrivateKey` for "std" builds and
manually implement an obfuscated version for "no-std" builds. Since
we enable the `hashes` feature of `rust-secp` this is unnecessary
because secp takes care of obfuscating the secret for us.
We are trying to get rid of the `serde_derive` dependency from our
dependency graph.
Stop using default features for the `schemars` dependency which includes
`schemars_derive` which depends on `serder_derive`.
Manually implement `schemars::JsonSchema` instead of deriving it.
We do not need this dependency because we can get the serde derives
directly from `serde`.
diff --git a/bitcoin/Cargo.toml b/bitcoin/Cargo.toml
index 3868bd08..db7fb322 100644
--- a/bitcoin/Cargo.toml
+++ b/bitcoin/Cargo.toml
@@ -53,7 +53,6 @@ actual-serde = { package = "serde", version = "1.0.103", default-features = fals
[dev-dependencies]
serde_json = "1.0.0"
serde_test = "1.0.19"
-serde_derive = "1.0.103"
bincode = "1.3.1"
[target.'cfg(mutate)'.dev-dependencies]
add371d263 Remove `core2` dependency entirely (Matt Corallo)
b7dd16da99 [IO] Use our own io::Error type (Matt Corallo)
c95b59327a Explicitly use `std::io::Error` when implementing `std` traits (Matt Corallo)
9e1cd372cb Use `io::Error::get_ref()` over `std::error::Error::source()` (Matt Corallo)
3caaadf9bb [IO] Replace the `io::Cursor` re-export with our own `Cursor` (Matt Corallo)
141343edb4 [IO] Move to custom `Read` trait mirroring `std::io::Read` (Matt Corallo)
7395093f94 Stop relying on `Take`'s `by_ref` method (Matt Corallo)
2364e1a877 Stop relying on blanket Read impl for all &mut Read (Matt Corallo)
6aa7ccf841 [IO] Replace `std::io::Sink` usage with our own trivial impl (Matt Corallo)
7eb5d65bda [IO] Provide a macro which implements `io::Write` for types (Matt Corallo)
ac678bb435 [IO] Move to custom `Write` trait mirroring `std::io::Write` (Matt Corallo)
5f2395ce56 Add missing `?Sized` bounds to `io::Write` parameters (Matt Corallo)
2348449d2a Stop relying on `std::io::Write`'s `&mut Write` blanket impl (Matt Corallo)
5e0209569c Use `io::sink` rather than our custom `EmptyWrite` utility (Matt Corallo)
a0ade883b6 [IO] Move io module into selected re-exports (Matt Corallo)
27c7c4e26a Add a `bitcoin_io` crate (Matt Corallo)
Pull request description:
In order to support standard (de)serialization of structs, the
`rust-bitcoin` ecosystem uses the standard `std::io::{Read,Write}`
traits. This works great for environments with `std`, however sadly
the `std::io` module has not yet been added to the `core` crate.
Thus, in `no-std`, the `rust-bitcoin` ecosystem has historically
used the `core2` crate to provide copies of the `std::io` module
without any major dependencies. Sadly, its one dependency,
`memchr`, recently broke our MSRV.
Worse, because we didn't want to take on any excess dependencies
for `std` builds, `rust-bitcoin` has had to have
mutually-exclusive `std` and `no-std` builds. This breaks general
assumptions about how features work in Rust, causing substantial
pain for applications far downstream of `rust-bitcoin` crates.
This is mostly done, I'm still finalizing the `io::Error` commit at the end to drop the `core2` required dep in no-std, but its getting there. Would love further feedback on the approach or code-level review on these first handful of commits.
ACKs for top commit:
tcharding:
ACK add371d263
apoelstra:
ACK add371d263
Kixunil:
ACK add371d263
Tree-SHA512: 18698ea8b1b65108ee0f695d5062d2562c8df2f50bf85d93442648da3b35a4184a5d5d2a493aed0adaadc83f663f0cd2ac735c34941cc9a6fa58d826e548e091
Tobin C. Harding
Andrew Poelstra
apoelstra
yancy
conduition
Vojtěch Toman