Generic types can be single letters, and a writer is conventionally, in
this codebase at least, called `W`.
Use `W` instead of `Write` with no loss of clarity.
1ee989a3af Remove private fmt_internal function (Tobin C. Harding)
923ce7402d Remove Network from AddressInner (Tobin C. Harding)
3490433618 Return error from wpubkey_hash (Tobin C. Harding)
f7ab253ce4 Remove stale comment (Tobin C. Harding)
Pull request description:
An `AddressInner` struct (contains `Network` field) is created when parsing address strings however address strings do not map 1:1 to `Network` because signet and testnet use the same bech32 prefix "tb".
We can fix this by inlining the `Payload` variants into `AddressInner` and adding prefix enums for legacy addresses and an `Hrp` for bech32 addresses.
Fix: #1819
ACKs for top commit:
Kixunil:
ACK 1ee989a3af
apoelstra:
ACK 1ee989a3af
Tree-SHA512: 1c2749dc929a1e9ad9b9feb01bec5c96b5aec07c6d646d88652deca7abe485907403116e9e29a0ab7dc06223254c4b49a384043284ec0a68fd76f9ab551e9e8a
396e049a7a Use InputString instead of String (Tobin C. Harding)
acacf45edf Add ParseDenominationError (Tobin C. Harding)
69e56a64ed Add bitcoin-units crate (Tobin C. Harding)
4ecb1fe7da internals: Add docs to InputString (Tobin C. Harding)
fa8d3002cd internals: Fix docs typo (Tobin C. Harding)
Pull request description:
Create a new `bitcoin-units` crate as described [here](https://github.com/rust-bitcoin/rust-bitcoin/issues/550#issuecomment-1012103022).
Only the `amount` module is currently included.
I've resolved the `Encodale/Decodable` issue by keeping the `amount` module in `bitcoin`.
ACKs for top commit:
Kixunil:
ACK 396e049a7a
apoelstra:
ACK 396e049a7a
Tree-SHA512: caf5e9da0458435ab19d00d4506896257e898525a4472d435fdac1d1a37bb747befd56993b106673f938475e5777d952a13ba04a2d3cb710d7afe7f5faebb7b8
e1cc98986c Put `#[inline]` on trivial functions (Martin Habovstiak)
e531fa612b Move `TaprootMerkleBranch` and impl `IntoIterator` (Martin Habovstiak)
9d23c1d0a8 Implement std traits for `TaprootMerkleBranch` (Martin Habovstiak)
93b415589d Rename `inner` to `slice`/`vec` (Martin Habovstiak)
bb0f839c2f Lint with nightly (Martin Habovstiak)
Pull request description:
This contains several improvements to `TaprootMerkleBranch` that make the API more idiomatic.
ACKs for top commit:
tcharding:
ACK e1cc98986c
apoelstra:
ACK e1cc98986c
Tree-SHA512: b2bf52b027e7c1f8588c54e8b8d7a5fa54011dc521bd917995011d5fcc16c50a486eb89c0cdae2557a58adbe7708a4f2bc8f4c492e3d88c679f2abf85b1e7c83
1b23220d10 Fix: TxOut::minimal_non_dust and Script::dust_value (Jonathan Underwood)
Pull request description:
Fixes#2192
TxOut::minimal_non_dust has 3 problems.
1. There is an invisible dependency on Bitcoin Core's default minrelaytxfee value. It has been made explicit.
2. There is an off by one error. The dust limit comparison uses < and therefore `+ 1` was not needed. It has been fixed.
3. It was not returning 0 amount for OP_RETURN outputs.
Script::dust_value has 2 problems.
1. The dust amount depends on minrelaytxfee which is configurable in Bitcoin Core. This method was not configurable.
2. The division operation was done before multiplying the byte amount, which can cause small differences when using uncommon scripts and minrelaytxfee values.
ACKs for top commit:
Kixunil:
ACK 1b23220d10
apoelstra:
ACK 1b23220d10
Tree-SHA512: eafd5112fbf773d86e094e3a69c519dd32f5074f5c9c63a8d69b1c9796579a8f2c2d11ad0995d8252c25b7fed5cd7c968ab88a70588986981a0a63649d43e197
c7c553ebc0 Remove impossible InvalidParity error variant (Martin Habovstiak)
Pull request description:
Since we do `& 1`, only 0 and 1 are possible values, so the error return there can never happen. I made this explicit by manually setting the parity.
This is a rebase of Steven's change #2163 with a rewrite of `match` to not panic.
ACKs for top commit:
tcharding:
ACK c7c553ebc0
apoelstra:
ACK c7c553ebc0
Tree-SHA512: 5591fda686295f330b6da757a0052687eddec135ac947a2801343f1681bc4fd9f6cfb722ac1339ae6187a8784e7629b5f12cca32b33a81ffc8791b4407b29f85
3d17031725 Derive Debug for PrivateKey for no-std builds (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
apoelstra:
ACK 3d17031725
Kixunil:
ACK 3d17031725
Tree-SHA512: 0ce394c6517c51e8964290a980cddd20186d19bcc6cbb8c71aa09b7485d6a0df373960798418184971e1c6e5a6b8f725dd44ebfa7184e31b63faf105dea69725
801c72e056 Add deprecation comment to hash_types module (Tobin C. Harding)
61351c917f Move impl_asref_push_bytes to internal_macros (Tobin C. Harding)
2b4b66dee3 Move impl_hashencode to internal_macros (Tobin C. Harding)
2a0ac1258a Move the bip158 filter hash types (Tobin C. Harding)
3107f80aac Move transaction hash types (Tobin C. Harding)
61c02ff202 Move block hash types (Tobin C. Harding)
Pull request description:
Move hash types out of `hash_types` and into the modules where they are primarily used. Adds deprecated re-export so this is not a breaking change.
Is an alternate solution to #2072Resolves: #2072
ACKs for top commit:
apoelstra:
ACK 801c72e056
Kixunil:
ACK 801c72e056
Tree-SHA512: 4ccac63553de3f7d417213429c0f5c2b7ebc3c2d77a9feb6d4a7daa233565fc62617edf6426a421d251eadc0841235a719bd7fd3f980302c7a2bf3dacb8b4a61
Since we do `& 1`, only 0 and 1 are possible values, so the error return
there can never happen. I made this explicit by manually setting the
parity.
This is a rebase of Steven's change with a rewrite of `match` to not
panic.
Since the iterator created by `IntoIterator` should be called `IntoIter`
we move the whole `TaprootMerkleBranch` to its own module which contains
the type to avoid confusion. This has an additional benefit of reducing
the scope where the invariant could be broken. This already uncovered
that our internal code was abusing access to the private field (although
the code was correct).
To implement the iterator we simply delegate to `vec::IntoIter`,
including overriding the default method which are likely to be
implemented by `Vec` more optimally. We avoid exposing `vec::IntoIter`
directly since we may want to change the representation (e.g. to
`ArrayVec`).
The type is naturally a collection of hashes so make it behave that way
by implementing `Deref`, `AsRef`, `Borrow` and their mutable versions as
well as `IntoIterator` for its reference. `IntoIterator` for itself is
not yet implemented because it's a bit more complicated.
While `clippy` now allows `TBD` to be used in `since` parameter of
`deprecated` attribute it is only available in the newest, nightly,
version. Switch `clippy` version to nightly to enable the `TBD` value.
TxOut::minimal_non_dust has 3 problems.
1. There is an invisible dependency on Bitcoin Core's default minrelaytxfee value. It has been made explicit.
2. There is an off by one error. The dust limit comparison uses < and therefore `+ 1` was not needed. It has been fixed.
3. It was not returning 0 amount for OP_RETURN outputs.
Script::dust_value has 2 problems.
1. The dust amount depends on minrelaytxfee which is configurable in Bitcoin Core. This method was not configurable.
2. The division operation was done before multiplying the byte amount, which can cause small differences when using uncommon scripts and minrelaytxfee values.
In the 0.31.0 release we renamed the bip32 extended key types without
leaving the originals in there marked as deprecated. This makes for a
bad experience for devs, add them back in.
98ce46c009 Update docs on witness_mut (Tobin C. Harding)
Pull request description:
Recently during the rust-bitcoin workshop at TABConf devs were thrown off by the example on `witness_mut`. We have some work going on to add examples and a cookbook that all demonstrate usage of `witness_mut`.
Remove the docs on `witness_mut` and direct devs to the `examples/sign-tx-*` files.
ACKs for top commit:
apoelstra:
ACK 98ce46c009
Kixunil:
ACK 98ce46c009
Tree-SHA512: e662213db4cbdaa53f6927cc1b10c1b6276f538cc6ad0d4bfff6dfcbf042f287a14bf5bfc88eeba7a32646c3d6741c5e09d11bb76666572a12a2043db55a2f38
Recently during the rust-bitcoin workshop at TABConf devs were thrown
off by the example on `witness_mut`.
Attempt to improve the docs on `witness_mut`.
0ac9ad16ce Add `taproot::SerializedSignature` (Martin Habovstiak)
dffa51e735 Move taproot module to a subdirectory (Martin Habovstiak)
Pull request description:
Previously `taproot::Signature` could be only serialized into `Vec<u8>`
which forced allocation. This adds a `SerializedSignature` type which
acts like `Box<u8>` but is on stack.
Note: the code was copied from `secp256k1::ecdsa::serialized_signature`
with minimal changes.
ACKs for top commit:
apoelstra:
ACK 0ac9ad16ce
tcharding:
ACK 0ac9ad16ce
Tree-SHA512: e3d24f4ddd8074d477c25f5378c3f57dd266405380cc54596104effbad96e7abdb201e032b4b70cdbbaac595d9ae9c4043fca79d275ce62f9f6b221e6783956e
Previously `taproot::Signature` could be only serialized into `Vec<u8>`
which forced allocation. This adds a `SerializedSignature` type which
acts like `Box<u8>` but is on stack.
Note: the code was copied from `secp256k1::ecdsa::serialized_signature`
with minimal changes.
Add a feature matrix section to the `bitcoin` CI script as we do in
`hashes`. This means:
- test with no default features
- test with all individual features
- test with all combinations of two features
Note, with this applied all features and optional dependencies are
included in `FEATURES` (excluding `secp-lowmemory`).
The feature matrix test gets run for stable and MSRV toolchains.
Currently `bitcoin` cannot be built with no features enabled, it must
have either "no-std" or "std" enabled. This is an artifact from when
we depended on `core2` for "no-std", now that we have our own `io` crate
and we unconditionally depend on it we can remove the "no-std" feature.
We are emptying the `hash_types` module. `impl_asref_push_bytes!` is an
internal macro, as such it can live in the `internal_macros` module.
While we are at it import the macro and call it without any qualifying
path, this is typical for our usage of other internals/internal_macros
usage.
We would like all the various hash types to be defined where they
rightly live instead of in the `hash_types` module.
Move the BIP-158 filter hash types to the `bip158` module.
We would like all the various hash types to be defined where they
rightly live instead of in the `hash_types` module.
Move transaction hash types to the `transaction` module.
We would like all the various hash types to be defined where they
rightly live instead of in the `hash_types` module.
Move the block hash types to the `block` module. While moving, add full
stops to the rustdoc of each hash.
Re-export _all four_ types from lib.rs (previously `WitnessMerkleNode`
was not re-exported).
An `AddressInner` struct is created when parsing address strings however
address strings do not map 1:1 to `Network` because signet and testnet
use the same bech32 prefix "tb".
We can fix this by inlining the `Payload` variants into `AddressInner`
and adding prefix enums for legacy addresses and a `KnownHrp` for bech32
addresses.
Also enables removing the `AddressEncoding` struct as we can display the
`AddressInner` struct directly. (The `Display` impl is on `AddressInner`
and not directly on address to ignore the `NetworkValidation` wrapper,
may be able to be simplified still further.)
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.
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.
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 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
9282cc4dad Implement standard conversions `Network`->`Params` (Martin Habovstiak)
9a8694fae5 Add `params` method to `Network` (Martin Habovstiak)
Pull request description:
Writing `network.params()` is less annoying than `Params::network()`, so this adds it. Making it return a static could also improve performance.
Didn't do `Params` -> `Network` conversions because of #2173
ACKs for top commit:
tcharding:
ACK 9282cc4dad
apoelstra:
ACK 9282cc4dad
Tree-SHA512: 6455956fd2c937b7212c9bab6ac7cfa05fb99b5da955f4f6690d7056cbe3902a3dadf94352c76b6866655b2e34a936191362a1cc81b33a5b252dd21dbc84d7b6
c745c97e5f add input weight predictions for p2pkh outputs (conduition)
Pull request description:
Adds input weight prediction constant and `ground_p2pkh_*` methods, mirroring those for `P2WPKH`. This seemed to be missing.
ACKs for top commit:
tcharding:
ACK c745c97e5f
apoelstra:
ACK c745c97e5f
Tree-SHA512: 69b4484686ecf761b766d2a7d7408c784981a9ba8c4aa8abd9d8655bd9421eb882e346ece232530edf9edff602d2fe1570992a5eb7b420b928d8b13397d2f60f
Adds missing prediction constants and const fns for
predicting the weights for P2PKH transaction inputs,
covering both compressed and uncompressed public keys.
7d695f6b41 Improve public re-exports (Tobin C. Harding)
33774122e0 Remove public re-exports from private module (Tobin C. Harding)
Pull request description:
Improve the public exports in two ways:
1. Inline re-exports into the docs of the module that re-exports them.
2. Separate public and private use statements
Recently we discussed a way to separate the public and private import statements to make the code more clear and prevent `rustfmt` joining them all together.
Separate public exports using a code block and `#[rustfmt::skip]`. Has the nice advantage of reducing the number of `#[doc(inline)]` attributes also.
1. Modules first, as they are part of the project's structure.
2. Private imports
3. Public re-exports (using `rustfmt::skip` to prevent merge)
Use the format
```rust
mod xyz;
mod abc;
use ...;
pub use {
...,
};
```
This patch introduces changes to the rendered HTML docs.
ACKs for top commit:
apoelstra:
ACK 7d695f6b41
Tree-SHA512: dc9121c0fe282e3035d862beadb89e2d5a374a7dab6b1c3147a9b5960f8bc2f5af49892f0f713f55c645c46f53464c32daf390c11d85c75553b3ea7e0efc8246
In order to move towards our own I/O traits in the `rust-bitcoin`
ecosystem, we have to slowly replace our use of the `std` and
`core2` traits.
Here we take the second big step, replacing
`{std,core2}::io::Read` with our own `bitcoin_io::io::Read`. We
provide a blanket impl for our trait for all `std::io::Read`, if
the `std` feature is enabled, allowing users who use their own
streams or `std` streams to call `rust-bitcoin` methods directly.
Since we are no longer relying on the blanket `io::Write` impl for
`&mut io::Write`, we should now ensure that we do not require
`Sized` for our `io::Write` bounds, as its unnecessarily
restrictive and can no longer be worked around by simply adding an
`&mut`.
`std::io::Write` is implemented for all `&mut std::io::Write`. This
makes it easy to have APIs that mix and match owned `Write`s with
mutable references to `Write`s.
However, in the next commit we add our own `Write` trait which we
intend to implement for all `std::io::Write`. Sadly, this is
mutually exclusive with a blanket implementation on our own
`&mut Write`, as that would conflict with an `std::io::Write`
blanket impl.
Thus, in order to use the `Write for all &mut Write` blanket impl
in rust-bitcoin, we'd have to bound all `Write`s by
`std::io::Write`, as we're unable to provide a blanket
`Write for &mut Write` impl.
Here we stop relying on that blanket impl in order to introduce the
new trait in the next commit.
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.
Here, we add a new `bitcoin_io` crate, making it an unconditional
dependency and using its `io` module in the in-repository crates
in place of `std::io` and `core2::io`. As it is not substantial
additional code, the `hashes` io implementations are no longer
feature-gated.
This doesn't actually accomplish anything on its own, only adding
the new crate which still depends on `core2`.
12d615d900 Use network when calculating difficulty (Tobin C. Harding)
62af5b54f3 Improve difficulty rustdocs (Tobin C. Harding)
Pull request description:
The difficulty is a ratio of the max and current targets, since the max is network specific the difficulty calculation is also network specific.
We already have network specific maximum target constants, use them when calculating the difficulty.
Patch 1 is a trival docs improvement to `block::Header::difficulty`.
ACKs for top commit:
Kixunil:
ACK 12d615d900
apoelstra:
ACK 12d615d900
Tree-SHA512: 8b414c975306667309b0918109b3e5e8774496fc4c0f3413709e95ad7499bebf1a017def4c180a2bb5f1750c69bb505d94c738a28525b7ccc8b36e5e42514000
01e2233f6c Remove deprecated since NEXT-RELEASE (Tobin C. Harding)
Pull request description:
Not sure what happened here but our release job didn't catch this? We should have updated this to "since = 0.31.0" before release. Since we only deprecate for one release lets go ahead and remove this.
ACKs for top commit:
apoelstra:
ACK 01e2233f6c
clarkmoody:
ACK 01e2233f6c
Kixunil:
ACK 01e2233f6c
Tree-SHA512: ad362058371e5e8ac7b577c3e32a0c65ce29e5723ff5efbccbcc57684fd61364f3caf7a4c8f0ee8c24bcb7a765bad2539a862860a902e5cec495ed9972379c2d
7f75447c1d Make Payload private and inline functionality (Tobin C. Harding)
b12bf07232 Make the AddressEncoding type private (Tobin C. Harding)
Pull request description:
The `AddressEncoding` and `Payload` types are implementation details and should never have been public. Make them private.
Fix: #1908
ACKs for top commit:
Kixunil:
ACK 7f75447c1d
apoelstra:
ACK 7f75447c1d
Tree-SHA512: 37083bc759f32e5187126c4f8d39c9c9cb39bd80a92b2479128da39f4db7672fe0be24a58756a387fe944c63efb4ffacc58c1dac071f3314e882ed0f0e9f5a23
Currently we have functions on `Address` that call through to a public
`Payload` type. The `Payload` type is an implementation detail and
should never have been public. In preparation for modifying the
`AddressInner` and removing `Payload` altogether lets move all the
functionality from `Payload` into `Address` - this is basically just
code moves so it is feasible to review with some confidence.
This is an API breaking change because it makes `Payload` private and
also removes from the pubic `Address` API functions that accept and
return `Payload`. Apart from that the changes can be seen as
refactoring.
The `AddressEncoding` type exists solely to assist us in implementing
`Display` on `Address`, it may have been used in the past by alt-coins
back when we had a more tolerant outlook on supporting them. Nowadays
we explicitly do not support alts.
Not sure what happened here but our release job didn't catch this? We
should have updated this to "since = 0.31.0" before release. Since we
only deprecate for one release lets go ahead and remove this.
The difficulty is a ratio of the max and current targets, since the
max is network specific the difficulty calculation is also network
specific.
We already have network specific maximum target constants, use them when
calculating the difficulty.
e21ee381bc Split Prevouts errors out into specific error types (Tobin C. Harding)
Pull request description:
Done as part of the great error clean up.
Currently we are returning a general `Error` from `Prevouts` functions, this is un-informative, we can do better by returning specific types that indicate the exact error path.
ACKs for top commit:
Kixunil:
ACK e21ee381bc
apoelstra:
ACK e21ee381bc
Tree-SHA512: 2a4900f9e31584ad2b6faafa17ea98742fff9206ee1bf77ed29624e0c7b05e655b3b6bf3710e2da26b0b2b8bd5eb36fdd81decbb1f55b41f153f0fbcc4a9165e
d6298fe711 Use capital B for Bitcoin in rustdoc (Tobin C. Harding)
bcfabc3556 Fix typo, missing word (Tobin C. Harding)
Pull request description:
In an effort to make review merge quicker push these two changes up as a separate PR.
Totally trivial.
ACKs for top commit:
Kixunil:
ACK d6298fe711
apoelstra:
ACK d6298fe711
Tree-SHA512: 34635ecd87c918f106694a81f50e69dda233000ac616557744466eb58422a2742f35cadbb059f0f81449efd2f61a37ceb71840bf216009914ba2162834e13fc6
fde6479c6a Create uniform build script (yancy)
Pull request description:
Previously, each unique compiler cfg attribute that appeared in the codebase was hard coded and emitted to stdout at compile time. This meant keeping the file up to date as different compiler cfg attributes changed. It's inconsequential to emit a compiler version that's not used, so this change just emits all possibilities to reduce the maintenance burden of the build script.
Note that there is a bit of diff noise in one of the `build.rs` since I simply did a copy of one to the other to make them uniform.
ACKs for top commit:
Kixunil:
ACK fde6479c6a
tcharding:
ACK fde6479c6a
apoelstra:
ACK fde6479c6a
Tree-SHA512: 401134c168a604a092b72c3980fae6d20adda761273ea47a887cf4c01838536241a45f310cbc2b5941311d533e1d10c848062198af70c0ed619a57973e842840
Improve the public exports in two ways:
1. Inline re-exports into the docs of the module that re-exports them.
2. Separate public and private use statements
Recently we discussed a way to separate the public and private import
statements to make the code more clear and prevent `rustfmt` joining
them all together.
Separate public exports using a code block and `#[rustfmt::skip]`. Has
the nice advantage of reducing the number of `#[doc(inline)]` attributes
also.
1. Modules first, as they are part of the project's structure.
2. Private imports
3. Public re-exports (using `rustfmt::skip` to prevent merge)
Use the format
```rust
mod xyz;
mod abc;
use ...;
pub use {
...,
};
```
This patch introduces changes to the rendered HTML docs.
Done as part of the great error clean up.
Currently we are returning a general `Error` from `Prevouts` functions,
this is un-informative, we can do better by returning specific types
that indicate the exact error path.
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,
wit bug fix by Sanket, updated to use APIs from tip of master branch.
This code, depending on v0.30.0 is what is being introduced to the
cookbook at the moment.
Previously, each unique compiler cfg attribute that appeared in the
codebase was hard coded and emitted to stdout at compile time. This
meant keeping the file up to date as different compiler cfg attributes
changed. It's inconsequential to emit a compiler version that's not
used, so this change just emits all possibilities to reduce the
maintenance burden of the build script.
2ecab31f94 Remove stale comment and map_err (yancy)
b166442fb0 Replace hex_psbt macro with test helper function (yancy)
9e4a784b8b Move psbt macro to the psbt test module (yancy)
Pull request description:
Remove `#[cfg(test)]` and the macro `psbt_with_values` from macros.rs and place it in the tests module for psbt.
ACKs for top commit:
apoelstra:
ACK 2ecab31f94
tcharding:
ACK 2ecab31f94
Tree-SHA512: 06a55056e864befac8b33968bf4e469c3c7bc20e651ad5bb3b80aa76749169af1266e1d4101d3e9e9bbffe7c860e8b9fcd675a78ca7ae67dc09892c75fba0dd0
875545517d Add clippy exceptions for needless_question_mark lint (Steven Roose)
Pull request description:
This lint forces you to write semantically different code that is in most cases inferior, just to save you 5 characters.
The reason why the code is inferior is because it doesn't do error conversion so it would break when either of the two function signatures changes while in the original code using the `?` operator, nothing would break if the inner error can be converted into the outer error.
ACKs for top commit:
apoelstra:
ACK 875545517d
tcharding:
ACK 875545517d
Tree-SHA512: 8429e0fb7d759a3d19231e7bcaed61b0988172d931e758a9522d7c994854fd403408bb93b06778a5c09746cd38b6a96d3d2e0a862fb4516f2dbfffffe8735ce0
b163d9b59a Replace hex_script macro with a helper function (yancy)
7f26439e20 Add track_caller to test helper functions (yancy)
bf08ee4499 Replace helper macro with helper function (yancy)
Pull request description:
Remove the macro hex_psbt and replace with a function. Using track_caller to accurately report the test name and line number during a panic is used in place of a macro.
This is a refactor commit to the test suit.
ACKs for top commit:
apoelstra:
ACK b163d9b59a
tcharding:
ACK b163d9b59a
Tree-SHA512: 6955c966d1c37020515ae990e5e0ec154f591ce025321dee71264e4e53cbab38afdb681ca193e626efdb7be4b4e84763cd8baf41b2a1258d34c38acd58713254
Remove the macro hex_script and replace with a function. Using
track_caller to accuretly report the test name and line number
during a panic is used in place of a macro.
The test helper files can panic when calling hex_psbt(). hex_psbt()
will report the calling function, instead of the offending test. Adding
track_caller to functions that call hex_psbt will report the line number
of the failing test.
Remove the macro hex_psbt and replace with a function. Using
track_caller to accuretly report the test name and line number
during a panic is used in place of a macro.
38960ab5a5 Bump version to 0.31.0-rc2 (Tobin C. Harding)
79dfe8d270 justfile: Add update-lock-files command (Tobin C. Harding)
178069c13e Add changelog entries for v0.31.0 (Tobin C. Harding)
Pull request description:
Prepare for the v0.31.0-rc2 release.
- Add changlog entries
- Add a `just update-lock-files` command
- Bump the version ready for next RC release
ACKs for top commit:
apoelstra:
ACK 38960ab5a5
clarkmoody:
ACK 38960ab5a5
Tree-SHA512: d2d459666117992689ea2bde2ba4a5d9a9cd36a85c5f0df443ceca3bf9d4ec351baffb4be09fbe25bf0767145d11a59b97031e7c8f9182fa9348a6883420b8fe
b108ffa2ec Implement manual fmt::Debug for BlockHeader to include block hash (Steven Roose)
Pull request description:
I think it makes sense for a block header to have the block hash shown in the debug print.
ACKs for top commit:
tcharding:
ACK b108ffa2ec
vincenzopalazzo:
ACK b108ffa2ec
apoelstra:
ACK b108ffa2ec
Tree-SHA512: 37b77f8b2d8da3903bc5ff329abd45dcb5f189ba2ac38fc309fa477b0eb569b01dd85c41f160e073bb56b4a599b7c4774fbfbd8da4b82ddb41540d60bfeafea3
hex_psbt was added as a macro so that a panic would reveal the line
number of the failing test by expanding the macro at the test location.
However, a stack trace can be used to reveal the test that caused the
failure using RUST_BACKTRACE=1. Furthermore, the track_caller macro is
added to the helper methods which will reveal the line number of the
calling function (the offending test). More detailed information for
debugging has been added to hex_psbt() so that the offending string
will be included in the panic message.
The macro psbt_with_values is used by the psbt test module. Since
there is no pre-processing required, there is no reason to use
metaprogramming here, so this commit moves the logic from a
macro to common function in the test module.
38005f6aa7 Use Target for pow_limit (Tobin C. Harding)
Pull request description:
The `Params::pow_limit` field is currently a `Work` type, this is incorrect. The proof of work limit is the highest _target_ not the lowest work (even though these have a relationship).
Note that we use the highest _attainable_ target, this differs from Bitcoin Core and the reasoning is already documented in the code.
Add new consts and document where they came from as well as how they differ to Core.
Use the new consts in the various network specific `Params` types.
Fix: #2106
ACKs for top commit:
junderw:
ACK 38005f6aa7
apoelstra:
ACK 38005f6aa7
Tree-SHA512: 5e71f69cdd555fd95a52fc1479e27b5e11226772f6432758c1364107a068bd1271486da6db1ece694da0287ce10cfbc18d28d6e3dbb0b9d387ff968eea43ab18
The `Params::pow_limit` field is currently a `Work` type, this is
incorrect. The proof of work limit is the highest _target_ not the
lowest work (even though these have a relationship).
Note that we use the highest _attainable_ target, this differs from
Bitcoin Core and the reasoning is already documented in the code.
Add new consts and document where they came from as well as how they
differ to Core.
Use the new consts in the various network specific `Params` types.
We have a new API function available with recent version of `secp256k1`
to create a `Message` directly from a sighash byte array.
Use `Message::from_digest(sighash.to_byte_array())` to construct
messages ready to sign.
Upgrade the `secp256k1` dependency to the newly released `v0.28.0`.
FTR this includes two simple changes:
- Use `Message::from_digest_slice` instead of `Message::from_slice`.
- Use `secp256k1::Keypair` instead of `secp256k1::KeyPair`.
In preparation for updating the secp dependency to v0.28.0, which
includes a change of `KeyPair` to `Keypair`, change our identifier usage
to indicate that "keypair" is a single word.
Deprecate the old forms.
Update the `bech32` dependency to use the newly release beta version.
The main fix here is silent, a bug fix in `bech32` that was being hit by
our fuzzing suite.
6b5d06f23e ci: fix the byteorder to 1.4.3 for edition 2018 (Vincenzo Palazzo)
98513ef151 clippy: more worning fixes (Vincenzo Palazzo)
05d3dc5d72 Remove redundant guard (Tobin C. Harding)
4537634e7e ci: bump rustc to 1.60 for fuzz test (Vincenzo Palazzo)
Pull request description:
Ci looks like broken, so this should fix
it
ACKs for top commit:
apoelstra:
ACK 6b5d06f23e
Tree-SHA512: bfa0eaf8cbc02a671237d99221db8c21264ce9df91301818c95c41dcc5ad4935e91254b0b3fa8f36738a9d71b6541fb8784ac8280d67057960a3d20e385a9f17
8eff4d0385 Remove private hex test macro (Tobin C. Harding)
Pull request description:
We have this macro in `hex-conservative` now, remove the version here.
This patch does not change the public API and only touches test code.
ACKs for top commit:
apoelstra:
ACK 8eff4d0385
clarkmoody:
ACK 8eff4d0385
Tree-SHA512: 93a08fff778930071cd1a28c19202e4a94ca8881b2e873538de2e942b71c2cd6184ed6364c572538a8a699295a71761c6f836accaf251a15683138b71f148fab
10374af75c Make error types uniform (Tobin C. Harding)
43d3306822 Use explicit error::Error impl instead of the default (Tobin C. Harding)
2512dbafc2 Remove impl_std_error macro (Tobin C. Harding)
6933ca4fc2 Add suffix to HiddenNodes error type (Tobin C. Harding)
2b40ea24fb Add suffix to IncompleteBuilder error type (Tobin C. Harding)
f41416a0ea Add suffix to UnknownMagic error type (Tobin C. Harding)
5658dac024 Add suffix to UnknownChainHash error type (Tobin C. Harding)
2fb71dd943 Move p2p error types to bottom of file (Tobin C. Harding)
39314ad52f Move error code to match conventional layout (Tobin C. Harding)
Pull request description:
PR aims to achieve two things:
- Make error code brain dead easy to read
- Get error code closer to being ready for v1.0
The first 8 patches are pretty basic, and are broken up into really small changes. The last patch is much bigger, it has a long git log to explain it but reviewing should not take too much brain power.
This PR does not introduce anything new, it just applies what we have been doing recently with errors. Before v1.0.0 others will likely want to re go over all the error types. As such I believe this PR can be merged under the one ack carve-out.
### TODOs (future PRs)
We have a few errors that still need splitting up:
- Split up `merkle_tree::block::MerkleBlockError`
- Split up `psbt::error::Error`
- Split up `IncompleteBuilderError`
Also, all error From's should probably have `#[inline]`, I noticed late in the process and did not have the heart to visit every error again.
ACKs for top commit:
apoelstra:
ACK 10374af75c
clarkmoody:
ACK 10374af75c
Tree-SHA512: 4f4f3533f42dc11af8e7978f3272752bb56d12a68199752ed4af0c02a46a87892b55c695b7007bc3d0bdf389493068d068e2be1780e8c3008815efec3a02eedf
On our way to v1.0.0 we are defining a standard for our error types,
this includes:
- Uses the following derives (unless not possible, usually because of `io::Error`)
`#[derive(Debug, Clone, PartialEq, Eq)]`
- Has `non_exhaustive` unless we really know we can commit to not adding
anything.
Furthermore, we are trying to make the codebase easy to read. Error code
is write-once-read-many (well it should be) so if we make all the error
code super uniform the users can flick to an error and quickly see what
it includes. In an effort to achieve this I have made up a style and
over recent times have change much of the error code to that new style,
this PR audits _all_ error types in the code base and enforces the
style, specifically:
- Is layed out: definition, [impl block], Display impl, error::Error impl, From impls
- `error::Error` impl matches on enum even if it returns `None` for all variants
- Display/Error impls import enum variants locally
- match uses *self and `ref e`
- error::Error variants that return `Some` come first, `None` after
Re: non_exhaustive
To make dev and review easier I have added `non_exhaustive` to _every_
error type. We can then remove it error by error as we see fit. This is
because it takes a bit of thinking to do and review where as this patch
should not take much brain power to review.
In a further effort to make the code brain-dead easy to read; use an
explicit implementation of `std::error::Error` that returns `None`
instead of relying on the default trait implementation.
We would like the codebase to be optimized for readability not ease of
development, as such code that is write-once-read-many should not use
macros.
Currently we use the `impl_std_error` macro to implement
`std::error::Error` for struct error types. This makes the code harder
to read at a glance because one has to think what the macro does.
Remove the `impl_std_error` macro and write the code explicitly.
Tobin C. Harding
Andrew Poelstra
Fmt Bot
Martin Habovstiak
Jonathan Underwood
apoelstra
yancy
conduition
Vojtěch Toman
Matt Corallo
lateminer
Steven Roose
Vincenzo Palazzo
sanket1729