fb29aadc47 Remove bip158 types from crate root (Tobin C. Harding)
Pull request description:
BIP-158 (Compact Block Filters for Light Clients) is not so common as to require re-exorting its types at the crate root - remove them.
ACKs for top commit:
apoelstra:
ACK fb29aadc47367ba8579f0004e97cd1fd7fd8236f; successfully ran local tests
Kixunil:
ACK fb29aadc47
Tree-SHA512: 8a2edaad858b18feded8cc9e1d15f03a76980bd41524fa34b91b4055b236788c6d828940c6293e086a8c8e33baadc5765a1a60920513fdff2de22e9d94c0e541
Previously we've used `try_into().expect()` because const generics were
unavailable. Then they became available but we didn't realize we could
already convert a bunch of code to not use panicking conversions. But we
can (and could for a while).
This adds an extension trait for arrays to provide basic non-panicking
operations returning arrays, so they can be composed with other
functions accepting arrays without any conversions. It also refactors a
bunch of code to use the non-panicking constructs but it's certainly not
all of it. That could be done later. This just aims at removing the
ugliest offenders and demonstrate the usefulness of this approach.
Aside from this, to avoid a bunch of duplicated work, this refactors
BIP32 key parsing to use a common method where xpub and xpriv are
encoded the same. Not doing this already led to a mistake where xpriv
implemented some additional checks that were missing in xpub. Thus this
change also indirectly fixes that bug.
ab4ea7c13d Enforce the MAX_MONEY invariant in amount types (Tobin C. Harding)
Pull request description:
Enforcing the `MAX_MONEY` invariant is quite involved because it means multiple things:
- Constructing amounts is now fallible
- Converting from unsigned to signed is now infallible
- Taking the absolute value is now infallible
- Integer overflow is eliminated in various places
Details:
- Update `from_sat` to check the invariant
- Fix all docs including examples
- Use the unchecked constructor in test code
- Comment any other use of the unchecked constructor
- Deprecate `unchecked_abs`
- Fail serde (using the horrible string error variant)
- Try not to use the unchecked constructor in rustdocs, no need to encourage unsuspecting users to use it.
- Use `?` in rustdoc examples (required by Rust API guidlines)
- Remove `TryFrom<Amount> for SignedAmount` because the conversion is now infallible. Add a `From` impl.
- Fix the arbitrary impls
- Maintain correct formatting
- Remove private `check_max` function as its no longer needed
Close#620
ACKs for top commit:
apoelstra:
ACK ab4ea7c13d08411044bd5f9c17457e926c80ed4d; successfully ran local tests
Tree-SHA512: bec963d8ea69e202f399cd19bca864b06f3e86323d376c2d2126d74093598f8bbbf19792b2327dba0862ef6f0201202778014a2be7a14991f02917d8ca312afb
Enforcing the MAX_MONEY invariant is quite involved because it means
multiple things:
- Constructing amounts is now fallible
- Converting from unsigned to signed is now infallible
- Taking the absolute value is now infallible
- Integer overflow is illuminated in various places
Details:
- Update from_sat to check the invariant
- Fix all docs including examples
- Use the unchecked constructor in test code
- Comment any other use of the unchecked constructor
- Deprecate unchecked_abs
- Fail serde (using the horrible string error variant)
- Try not to use the unchecked constructor in rustdocs, no need to encourage unsuspecting users to use it.
- Use ? in rustdoc examples (required by Rust API guidlines)
- Remove TryFrom<Amount> for SignedAmount because the conversion is now infallible. Add a From impl.
- Fix the arbitrary impls
- Maintain correct formatting
- Remove private check_max function as its no longer needed
We just re-named `Timestamp` to `BlockTime`. We have a `units::block`
module but it currently holds abstractions (`BlockHeight` and
`BlockInterval`) that are not onchain abstractions and therefore
somewhat different from the `BlockTime`. Instead of making `block` a
block 'utils' module instead re-name the `timestamp` module to `time`.
We just added a `Timestamp` type without knowing that there was a push
by OpenTimestamps to also create a timestamp and that our new type may
lead to confusion. Our timestamp is explicitly for the `time` field in a
block so we can call it `BlockTime`. This name change makes the module
name stale but we will change that in a following patch to ease review.
5da506b506 Add additional re-exports (Tobin C. Harding)
Pull request description:
As we do for other types add two new alias' at the crate root of `primitives` and mirror it in `bitcoin`:
- `BlockVersion`
- `TransactionVersion`
ACKs for top commit:
apoelstra:
ACK 5da506b506aa41b88aa7e8ecdffdcc0478ec72b6; successfully ran local tests
Kixunil:
ACK 5da506b506
Tree-SHA512: 5f91e3aae87b1128256b528d20d9aab562bf054131ed8028e0db789e2b487863ad4c89aaca080d0fbcf760dd160815a1843046c5fab0ed1483230fdedc66402e
b3f122b399 Add Timestamp newtype (Tobin C. Harding)
Pull request description:
Bitcoin block headers have a timestamp. Currently we are using a `u32`. While this functions correctly it gives the compiler no chance to enforce type safety.
Add a `Timestamp` newtype that is a thin wrapper around a `u32`. Document it and test the API surface in `api.rs`.
ACKs for top commit:
apoelstra:
ACK b3f122b3996c1a73479be2f95b7f2ae642c9c56f; successfully ran local tests
Kixunil:
ACK b3f122b399
Tree-SHA512: 6f4a4a588bc836243ae28f3d36be6c0ae264cb2b7a0061277910b107d05e5ca0e679497d2890208f5d8ec148f37bf263bcd0b0410f9e5e6370d8e763ff30b78a
852bcf6017 bitcoin: Remove hash type re-exports (Tobin C. Harding)
Pull request description:
The `{W}PubkeyHash` and `{W}ScriptHash` types are not likely to be used directly by consumers of the library because we have other function that return them and are more ergonomic to use. There is therefor no good reason to re-export them from the crate root.
ACKs for top commit:
apoelstra:
ACK 852bcf60178c9491ebcbf13243cf79857592ccec; successfully ran local tests; will one-ACK merge, still does not affect any public API
Kixunil:
ACK 852bcf6017
Tree-SHA512: fbc4a7e3e116e962dc4b65b8681343e6ecd485a2289c5798979ce8e6ead4c0b6bda04ae33af1bdd711e852f95896107f9fa3c63d52fd36a6f802de4a7b677073
The `{W}PubkeyHash` and `{W}ScriptHash` types are not likely to be used
directly by consumers of the library because we have other function that
return them and are more ergonomic to use. There is therefor no good
reason to re-export them from the crate root.
Duplicate `opcodes` in `bitcoin` and hide it in `primitives` so we do
not have to commit to the API.
We use opcodes in `impl fmt::Display for Script`.
Close: #4144
Bitcoin block headers have a timestamp. Currently we are using a
`u32`. while this functions correctly it gives the compiler no chance
to enforce type safety.
Add a `Timestamp` newtype that is a thin wrapper around a `u32`.
Document it and test the API surface in `api.rs`.
It has turned out that the `rust-ordered` crate and it's
`ArbitraryOrd` trait are only useful for locktimes and only marginally
useful for them at best.
Remove the `ArbitraryOrd` impls and the `rust-ordered` dependency.
This topic was discussed in various places including:
- #2500
- #4002
- #3881Close: #4029
Currently `InputString` is in the public API of `units` because of the
trait bound on `parse::int()`. We can just do the monomorphisisation
manually to remove it.
This patch renames `int` to have three different names, one for `&str`
one for `String`, and one for `Box<str>`.
a5993426fd bitcoin: Improve rustdocs on extern crates (Tobin C. Harding)
Pull request description:
Make slight improvement to the extern crates we re-export from `bitcoin`.
ACKs for top commit:
apoelstra:
ACK a5993426fddea85c29ac6cd5a0e3096988d72b03; successfully ran local tests
Tree-SHA512: bb857316fc2606aa553f15e1c9fcf8dd87374e170121fe30cf9e44ae190f873f94c113af49b096cb49a65fd68d1bf24732e3f1aba04861e2ae934414f30473c6
`rust-bitcoin` is a lot further from 1.0 than `bech32` is. We previously
removed the re-export of `bech32` because we thought it might hold us up
releasing `rust-bitcoin 1.0` but in hindsite this was incorrect.
Having a public re-export of dependencies helps users keep their deps in
sync with those in `rust-bitcoin` and is thus useful.
Close: #3650
On the way re-design the API by doing:
- Introduce `Checked` and `Unchecked` tags
- Rename the `txdata` field to `transactions`
- Make the `Block` fields private
- Add getters for `header` and `transactions` fields
- Move the various `compute_` methods to be free standing functions
- Make the `check_` functions private
- Introduce extension traits
These re-exports are hard to get right. We (I) recently made an attempt
to make our stack of crates easier to navigate using the idea that
exporting a type from the crate it is defined in adds some additional
information without any loss of clarity. Note however that the module
must be re-exported from the "highest" place possible because we at
times add additional functionality as we move up the stack e.g.,
`bitcoin::merkle_tree` has logic in it that `primitives::merkle_tree`
does not. In order to future proof the codebase default to always using
the highest module up the stack even when that module adds no additional
code e.g., re-export `blockdata::fee_rate` as opposed to
`units::fee_rate` in the event that we later add logic to
`bitcoin::blockdata::fee_rate`.
This patch adds an additional re-export: `sequence` (previously missing).
Is there any advantage trying to lay out the re-exports to give users an
idea of the crate structure?
We have the explicit aim that users who depend on `bitcoin` do not ever
need to reach directly into `primitives` (or `units`) however it is kind
of nice to know where things come from, saves jumping to multiple files
looking for them (for those of us that jump to files manually).
I do not know how all the re-exports interact with other folks IDEs, I
personally open files manually and just remember where stuff is.
For users who want to just grab stuff from the crate root it makes total
sense for there to be a `BlockHeader`.
Another nice thing, in the HMTL docs it makes BlockHeader be in the
struct list right along with `Block`, `BlockHash`, and `BlockHeight`.
This has been fixed and we use nightly to lint so we have access to the
merged fix.
Removing the attribute uncovers a bunch of real lint warnings, fix
them while we are at it.
Up until recently we were using wildcard re-exports for types moved to
`units` and `primitives`. We have decided against doing so in favour of
explicit re-exports.
Audit `units` and `primitives` using `git grep 'pub enum'` (and
`struct`) and explicitly re-export all types.
Remove all wildcards except for the re-exports from `opcodes`, there are
too many opcodes, explicitly re-exporting them does not aid clarity.
We use `TBD` in our `deprecated` string and it was discovered that there
is an exception on this string so as not to warn because it is used
internally by the Rust language. However there is a special lint to
enable warnings, lets use it.
Add `#![warn(deprecated_in_future)]` to the coding conventions section
of all crates except `fuzz`.
At some stage we named the compact encoding `VarInt` (which makes sense
because the compact size encoding is a variable length integer encoding).
However it turns out the term "varint" is used in Core for a different
encoding so this may lead to confusion.
While we fix this naming thing observe also that the `VarInt` type is
unnecessarily complicated, all we need to be able to do is encode and
decode integers in compact form as specified by Core. We can do this
simply by extending our `WriteExt` and `ReadExt` traits.
Add `emit_compact_size` and `read_compact_size` to emit and read compact
endcodings respectively.
Includes addition of `internals::compact_size::encoded_size_const`.
Patch originally written by Steven, Tobin cherry-picked and did a bunch
of impovements after the varint vs compact_size thing (#1016).
ref: https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer
Co-developed-by: Tobin C. Harding <me@tobin.cc>
Examples in documentation are not linted in the same way as other code,
but should still contain correctly written code.
unused_imports in docs have been removed in bitcoin, and a warn
attribute added to lib.rs.
c427d8b213 bitcoin: Compile time assert on index size (Tobin C. Harding)
49a6acc1a0 internals: Remove double parenthesis in const_assert (Tobin C. Harding)
2300b285ef units: Remove compile time pointer width check (Tobin C. Harding)
Pull request description:
3 patches in preparation for other size related work, this PR does not touch the `ToU64` issue which will be handled separately.
- Patch 1: Don't check pointer width in `units` because its not consensus code
- Patch 2: Modify internal macro `const_assert`
- Patch 3: Use index size to enforce not building on a 16 bit machine
ACKs for top commit:
Kixunil:
ACK c427d8b213 though I think the last commit was kinda a waste of time and it should have been adding the trait instead or leave it for later.
apoelstra:
ACK c427d8b213 successfully ran local tests; unsure if we want to merg this or wait for #3215
Tree-SHA512: 823df5b6a5af3265bce2422c00d287f45816faeb5f965685650ac974a1bd441cf548e25ac2962591732ff221bee91a55703da936382eb166c014ca5d4129edf8
Currently we enforce that our code only runs on machines with a
certain pointer width (32 or 64 by failing to compile if pointer size
width is 16). One of the underlying reasons is because of requirements
in consensus code in Bitcoin Core which requires containers with more
than 2^16 (65536) items [0].
We can better express our requirements by asserting on Rust's index
size (the `usize` type).
As a side benefit, there is active work [1] to make Rust support
architectures where pointer width != idex size. With this patch applied
`rust-bitcoin` will function correctly even if that work progresses.
- [0] https://github.com/rust-bitcoin/rust-bitcoin/pull/2929#discussion_r1659399813
- [1] https://github.com/rust-lang/rust/issues/65473
The version 1.63 satisfies our requirements for MSRV and provides
significant benefits so this commit bumps it. This commit also starts
using some advantages of the new MSRV, namely namespaced features, weak
dependencies and the ability to use trait bounds in `const` context.
This however does not yet migrade the `rand-std` feature because that
requires a release of `secp256k1` with the same kind of change - bumping
MSRV to 1.63 and removing `rand-std` in favor of weak dependency.
29b213daca Move validation module to consensus_validation (Tobin C. Harding)
Pull request description:
The `consensus` module is currently doing two things, validation and encoding. These two things are orthogonal.
Move the `consensus::validation` module to `consensus_validation`. Remove the function re-exports from `consensus`.
This was originally discussed here: https://github.com/rust-bitcoin/rust-bitcoin/issues/2779
ACKs for top commit:
Kixunil:
ACK 29b213daca
apoelstra:
ACK 29b213daca
Tree-SHA512: 3bd0e43c220b0d89a47e9df0e0c92b776ccc65f5f60d57f413db834acc8e86269379bc9fdd688f8c4f0138db22f8eb8983770afa2d7d53d51acf063f2302121c
The `consensus` module is currently doing two things, validation and
encoding. These two things are orthogonal.
Move the `consensus::validation` module to `consensus_validation`.
Remove the function re-exports from `consensus`.
The `Params` struct is currently defined in the `consensus` module which
has become a collection of orthogonal consensus-ish things. We would
like to put things in more descriptive places.
The `Params` struct defines constants that are network specific so it
makes sense to put it in the `network` module. As soft proof of this
argument note in this patch how often the `Params` type is imported
along with the `Network` type.
API break:
The type is no longer available at `bitcoin::consensus::Params` but
rather is re-exported at `bitcoin::network::Params`.
The `absolute` and `relative` locktimes as well as the `Sequence` are
all primitive bitcoin types.
Move the `Sequence`, and `locktime` stuff over to `primitives`.
There is nothing surprising here, the consensus encoding stuff stays in
`bitcoin` and we re-export everything from `blockdata`.
7fa53440dc Move serde_round_trip macro to internals (Tobin C. Harding)
Pull request description:
We currently duplicate the serde_round_trip macro in `units` and `bitcoin`, this is unnecessary since it is a private test macro we can just throw it in `internals`.
While we are at it lets improve the macro by testing a binary encoding also, elect to use the `bincode` crate because we already have it in our dependency graph.
Add `test-serde` feature to `internals` to feature gate the macro and its usage (preventing the transient dependency on `bincode` and `serde_json`).
ACKs for top commit:
Kixunil:
ACK 7fa53440dc
apoelstra:
ACK 7fa53440dc
Tree-SHA512: f40c78bf2539940b7836ed413d5fe96ce4e9ce59bad7b3f86d831971320d1c2effcd23d0da5c785d6c372a2c6962bf720080ec4351248fbbdc0f2cfb4ffd602c
4bb9240992 bitcoin: Add comment to manifest (Tobin C. Harding)
c6c70a721e bitcoin: Update feature docs (Tobin C. Harding)
7712e5d891 bitcoin: Use 100 colum width in crate level docs (Tobin C. Harding)
Pull request description:
Update stale crate level docs in `bitcoin`. Done as two separate patches so the last one is trivial to review, whitespace only.
ACKs for top commit:
Kixunil:
ACK 4bb9240992
apoelstra:
ACK 4bb9240992
Tree-SHA512: b9d5474f7e7a0576f535df428ea20084acbcb74d0576e7f2934c547dd2c54f4a939d73ec547b3b254105a45c2372113c65ce136be1eabd63701259a3a6de3737
We currently duplicate the serde_round_trip macro in `units` and
`bitcoin`, this is unnecessary since it is a private test macro we can
just throw it in `internals`.
While we are at it lets improve the macro by testing a binary encoding
also, elect to use the `bincode` crate because we already have it in
our dependency graph.
Add `test-serde` feature to `internals` to feature gate the macro and
its usage (preventing the transient dependency on `bincode` and
`serde_json`).
merge-script
Martin Habovstiak
Fmt Bot
Tobin C. Harding
Shing Him Ng
Casey Rodarmor
BinChengZhao
Steven Roose
Jamil Lambert, PhD