There is no advantage in having `io::Read` as opposed to `Read` and
importing the trait. It is surprising that we do so.
Remove `io::` path from `io::Read` and `io::Write`. Some docs keep the
path, leave them as is. Add import `use io::{Read, Write}`.
Refactor only, no logic changes.
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
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.
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 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.
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
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.
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
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
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.
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
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.
c34e3cc7cc Re-write size/weight API (Tobin C. Harding)
73f7fbf520 Add code comments to transaction serialization (Tobin C. Harding)
29f20c1d0b Add segwit serialization constants (Tobin C. Harding)
Pull request description:
Audit and re-write the weight/size API for `Block` and `Transaction`. First two patches are trivial, patch 3 contains justification and explanation for this work, copied here:
```
Recently we introduced a bug in the weight/size code, while
investigating I found that our `Transaction`/`Block` weight/size APIs
were in a total mess because:
- The docs were stale
- The concept of weight (weight units) and size (bytes) were mixed up
I audited all the API functions, read some bips (141, 144) and re-wrote
the API with the following goals:
- Use terminology from the bips
- Use abstractions that mirror the bips where possible
```
Please note, this PR introduces panics if a sciptPubkey overflows the calculation `weight = spk.size() * 4`.
Fix#2049
ACKs for top commit:
apoelstra:
ACK c34e3cc7cc
sanket1729:
ACK c34e3cc7cc.
Tree-SHA512: 4944f652e6e362a282a5731140a9438a82d243a4c646b4627d9046a9f9cf13c476881750d432cfbc6b5fe5de1f0c4c9c44ed4569dac4bc11b55a5db28793803c
Recently we introduced a bug in the weight/size code, while
investigating I found that our `Transaction`/`Block` weight/size APIs
were in a total mess because:
- The docs were stale
- The concept of weight (weight units) and size (bytes) were mixed up
I audited all the API functions, read some bips (141, 144) and re-wrote
the API with the following goals:
- Use terminology from the bips
- Use abstractions that mirror the bips where possible
There is no logical default for the transaction version number, there is
only pre-bip68 (v1) and post-bip68 (v2). Uses should specify the version
they want not rely on us making the choice.
(I originally added this impl to support testing, this was in hindsight
the wrong thing to do, props to Sanket for noticing.)
One of our stated aims is to make it possible to learn bitcoin by using
our library. To help with this aim add to private consts for the segwit
transaction marker and flag serialization fields.
BIP-68 activated a fair while ago (circa 2019) and since then only
transaction versions 1 and 2 have been considered standard.
Currently in our `Transaction` struct we use an `i32`, this means users
can construct a non-standard transaction if they do not first look up
what the value should be. We can help folk out here by abstracting over
the version number.
Since the version number only governs standardness elect to make the
inner `i32` public (ie., not an invariant). The aim of the type is to
make life easy not restrict what versions are used.
Add transaction::Version data type that simply provides two consts `ONE`
and `TWO`.
Add a `Default` impl on `Version` that returns `Version::TWO`.
In tests that used version 0, instead use `Version::default` because the
test obviously does not care.
55e94b5dea Remove test from test names for Weight type (yancy)
142dde64c3 Use Weight type for stripped_size (yancy)
cb76f3ec43 Add scale_by_witness_factor to Weight type (yancy)
38c9e9947e Add witness scale factor to the Weight type (yancy)
77552987ab Add from_wu_usize to Weight type (yancy)
1a88c887f5 Rename strippedsize to stripped_size (yancy)
3369257c75 Fix grammar (yancy)
Pull request description:
Return Weight type for the strippedize function.
ACKs for top commit:
apoelstra:
ACK 55e94b5dea
tcharding:
ACK 55e94b5dea
Tree-SHA512: ad3e4bc29380f22e20a6302c1b24c201c772be759c655c62ba4717840a01fcaa36f0f8442c9a3ba71c6400d6af47a9a815e6d90877b5f14c6883fb950b9669fd
Throughout the codebase we cast values to `u64` when constructing a
`VarInt`. We can make the code marginally cleaner by adding `From<T>`
impls for all unsigned integer types less than or equal to 64 bits.
Also allows us to (possibly unnecessarily) comment the cast in a single
place.
2e3006a729 Add max standard tx weight constant to transaction (yancy)
Pull request description:
Add a constant for the max transaction weight. Similar to [max block weight](1b009b809b/bitcoin/src/blockdata/weight.rs (L35)). This value is pulled from core [here](44b05bf3fe/src/policy/policy.h (L27))
ACKs for top commit:
apoelstra:
ACK 2e3006a729
sanket1729:
ACK 2e3006a729
Tree-SHA512: 1583695f43387538f948be85ded7ff9a4bf9778169acb958debcbe1572a6dc8bfcd26ddfb8dbe0c030c98ab1f8a66d239a5bc663bf65ec3376a46d5f71e90894
50ada8298f Move EncodeSigningDataResult to sighash module (Tobin C. Harding)
1b7dc51ccb Remove deprecated code (Tobin C. Harding)
Pull request description:
We only keep deprecated code around for one release so we can now remove code deprecated in v0.30.0
Done in preparation as we gear up for v0.31.0 release.
ACKs for top commit:
apoelstra:
ACK 50ada8298f
sanket1729:
ACK 50ada8298f
Tree-SHA512: 40769258605563e2e12a6118306655fc9a012ae1f86509fca757ca411f0cef74480b7bb7b0db147f30a7d362b8494a077d5ec04f719351661ceb5a0697a5369d
3c0bb63423 Do trivial rustdoc improvements (Tobin C. Harding)
3225aa9556 Use defensive documentation (Tobin C. Harding)
80d5d6665a crypto: key: Move error code to the bottom of the file (Tobin C. Harding)
fe3b1e1140 Move From for Error impl (Tobin C. Harding)
5f8e0ad67e Fix docs on error type (Tobin C. Harding)
f23155aa16 Do not capitalize error messages (Tobin C. Harding)
ae07786c27 Add InvalidSighashTypeError (Tobin C. Harding)
baba0fde57 Put NonStandardSighashTypeError inside ecdsa::Error variant (Tobin C. Harding)
6c9d9d9c36 Improve error display imlps (Tobin C. Harding)
22c7aa8808 Rename non standard sighash error type (Tobin C. Harding)
Pull request description:
EDIT: The commit hashes below are stale but the text is valid still.
In an effort to "perfect" our error handling, overhaul the error handling in the `crypto` module.
The aim is to do a small chunk so we can bikeshed on it then I can apply the learnings to the rest of the codebase.
Its all pretty trivial except:
- commit `4c180277 Put NonStandardSighashTypeError inside ecdsa::Error variant`
- comimt `5a196535 Add InvalidSighashTypeError`
- commit `05772ade Use defensive documentation`
Particularly the last one might be incorrect/controversial.
Also, please take the time to check the overall state of error code in the `crypto` module on this branch in case there is anything else we want to do.
Thanks
ACKs for top commit:
apoelstra:
ACK 3c0bb63423
Tree-SHA512: 7e5f8590aec5826098d4d8d33351a41b10c42b6379ff86e5b889e73271b71921fc3ca9525baa5da53e07fa2e961e710393694e04658a8243799950b4604caf43
The `network` module deals with data types and logic related to
internetworking bitcoind nodes, this is commonly referred to as the p2p
layer.
Rename the `network` module to `p2p` and fix all the paths.
Fmt Bot
harshit933
Martin Habovstiak
Tobin C. Harding
Andrew Poelstra
Jonathan Underwood
conduition
Matt Corallo
Clark Moody
yancy
Daniela Brozzoni