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
c707b959b7 Rename timestamp module to time (Tobin C. Harding)
e2dee4900f Re-name Timestamp to BlockTime (Tobin C. Harding)
Pull request description:
Done in two patches so we can bikeshed the name of the type and separately the name of the module.
- Rename type: `Timestamp` to `BlockTime`
- Rename module: `timestamp` to `time`
ACKs for top commit:
apoelstra:
ACK c707b959b72dd89ca6df581a6102f32daedb8368; successfully ran local tests
Tree-SHA512: de3855b38445a58b6767a6081919eecb81c6c12aee3f6699f3bfa10efaf5770b54fb412da23991a9ee734e14dfb642af670f0218d1886cdc8c8d3f393ef65d7e
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
199f57849a Remove references to cfg(mutate) from lint allow - no longer allowed (AM)
a65d1d8b95 docs: Update README to replace use of mutagen with cargo-mutants (AM)
Pull request description:
Hey there!
I am just getting up to speed with the project and in following the README discovered that there are still references to the previous mutation testing tool `mutagen`. I updated the README to refer to the new tool, `cargo-mutation`.
I'm suggesting the user use the same command, `cargo mutants --in-place --no-shuffle`, as is run in the weekly CI workflow.
I noticed that there are still references to the old `mutate` attribute in the following files. I removed these as well as per [feedback](https://github.com/rust-bitcoin/rust-bitcoin/pull/4228#issuecomment-2709407253).
`primitives/Cargo.toml`:
```
[lints.rust]
unexpected_cfgs = { level = "deny", check-cfg = ['cfg(mutate)'] }
```
and
`bitcoin/Cargo.toml`:
```
unexpected_cfgs = { level = "deny", check-cfg = ['cfg(bench)', 'cfg(fuzzing)', 'cfg(kani)', 'cfg(mutate)'] }
```
Update to PR: removed incorrect understanding of logic in PR description as per [feedback](https://github.com/rust-bitcoin/rust-bitcoin/pull/4228#issuecomment-2709408598) and removed `cfg(mutate)` from above 2 files.
ACKs for top commit:
tcharding:
ACK 199f57849a
apoelstra:
ACK 199f57849acd9845902a8090ad6490a61ee03d24; successfully ran local tests
Tree-SHA512: e154c504aa5283f1da05d0120ea8dda97d1159389e692b0d57d7d864032ecb2b48c496054ede5500477367bc732dc34b0870f2709b8bd6e7b3f5c18a10f7a29e
0428554585 primitives: Feature gate import (Tobin C. Harding)
Pull request description:
Feature gate the `Infallible` import. Found with `clippy`.
ACKs for top commit:
apoelstra:
ACK 0428554585de668e2a59fa61e44b473f4085d717; successfully ran local tests
Tree-SHA512: 23bf7d62d81af87b67395ddee251cef82140895be76026c43c774997c29f2ed2707bf42e82d0a155a5cff83412538cb2ed5251f10c0e0cb746434a8d7db8e554
df500e9b71 primitives: Enable pedantic lints (Tobin C. Harding)
Pull request description:
Draft to check the subjective ones please, then I'll squash.
ACKs for top commit:
apoelstra:
ACK df500e9b71187fe658da76adafdb3300a51de2ef; successfully ran local tests
Tree-SHA512: 8cc8c9b369a63c1b2b26461e288a818e3b74e0f9b7359c964c1650028d3161db1d79369c74f18e79958873bf4d223ee72fa481708600f0297d79377d97a84dda
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.
90d909becc Kill mutants in primitives and units (Shing Him Ng)
Pull request description:
This kills 15 mutants found with the mutants workflow. Ran `cargo mutants` locally to confirm
Closes#4156Closes#4106
ACKs for top commit:
jamillambert:
ACK 90d909becc
tcharding:
ACK 90d909becc
apoelstra:
ACK 90d909becc4638c03003845154e9cc1eb5f3ad41; successfully ran local tests
Kixunil:
ACK 90d909becc
Tree-SHA512: e5c95a1c4054cf1c60c940ea605eec84dffcbff292f9c7c4d96813c6389e807c318f6c5f8f69ceeb9ffcab3c3e45aa0d5a8fda7335d540c6f070aab92bae7a0f
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
Pull request description:
Enhance Witness struct element access methods:
- Rename `nth()` to `get()` for clearer slice-like element retrieval
- Introduce `get_back()` method for flexible reverse indexing
- Remove redundant `second_to_last()` and `third_to_last()` methods
- Add `#[track_caller]` to index implementation for better error tracking
- Update all references to use new method names
- Improve documentation with usage examples
The changes provide a more intuitive and consistent approach to accessing witness elements.
Close#4098
ACKs for top commit:
Kixunil:
ACK 3ca3218c23
tcharding:
ACK 3ca3218c23
apoelstra:
ACK 3ca3218c236c63a9b006047524e2b47e310f07d9; successfully ran local tests
Tree-SHA512: 163e7457f3fe5141373e27a6df5fe1da6f2f05f02e877ef96243510d030d832c0fa86ade781e015a3c392f004651170b60438a83d330f1059457e5ade6478af7
d1d483491f Make `hex` in `internals` optional (Martin Habovstiak)
Pull request description:
The `hex` crate is not always desirable - e.g. when the consumer wants to work with raw data only. We already had this optional in `hashes` but if `hashes` is going to depend on `internals` it would break this property.
This change makes `hash` optional, since it's easy: there's just one struct that depends on it.
ACKs for top commit:
tcharding:
ACK d1d483491f
apoelstra:
ACK d1d483491f5f0181e60778e557fadd99b44b5d30; successfully ran local tests; nice
Tree-SHA512: c20091e6febb49b9114273c280580ddcdafc91893f3c73288b374f51990f09f035a044806dd26777148f2e4341ad082ab05f1b49f8ceb3bcd24eb210ffa5fd5f
Enable all the pedantic lints and fix warnings.
Notable items:
- `enum_glob_used` import types with a single character alias
- `doc_markdown`: add a whitelist that includes SegWit and OpenSSL
a74393324b Move opcodes back to bitcoin (Tobin C. Harding)
Pull request description:
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
ACKs for top commit:
apoelstra:
ACK a74393324bd47f89fd47281d567ab15ab6bcb2ba; successfully ran local tests; sure
Kixunil:
ACK a74393324b
Tree-SHA512: 738685b9cd2288a581daa6219e3b21bd48bb4845ea627bf6b8085e0e48f5649ac5ec616a3421d10cd37543f76b66d31f94fd55bf94effc2fb8f91d1ecf5c8611
Enhance Witness struct element access methods:
- Rename `nth()` to `get()` for clearer slice-like element retrieval
- Introduce `get_back()` method for flexible reverse indexing
- Remove redundant `second_to_last()` and `third_to_last()` methods
- Add `#[track_caller]` to index implementation for better error tracking
- Update all references to use new method names
- Improve documentation with usage examples
The changes provide a more intuitive and consistent approach to
accessing witness elements.
b656d7a16c Inline small functions (Jamil Lambert, PhD)
Pull request description:
Functions that fit the below criteria should be inline:
> Basically, if a function jut delegates into another function and passing through arguments, it should be inline. Also when doing assignment of `u32` or similarly trivial operation. Also if it checks the validity of argument(s) and then performs something simple (like a call to `_unchecked`).
_Originally posted by Kixunil in https://github.com/rust-bitcoin/rust-bitcoin/pull/4099#discussion_r1966156399_
Add `#[inline]` to all functions in `primitives` that fit the criteria.
ACKs for top commit:
tcharding:
ACK b656d7a16c
apoelstra:
ACK b656d7a16c2d0cdd6d8a94fffeac842ffc0fabf4; successfully ran local tests
Tree-SHA512: 0059aa25252e634e55482b201a9414d19a01435fea801a20dc38b4b701a52f4a14565a1cfb0ff4ed7a771a25629eb192fd69cb4ee81e78f6a0ef79d56db0ef5b
The `hex` crate is not always desirable - e.g. when the consumer wants
to work with raw data only. We already had this optional in `hashes` but
if `hashes` is going to depend on `internals` it would break this
property.
This change makes `hash` optional, since it's easy: there's just one
struct that depends on it.
I took a look at the rendered HMTL of `bitcoin`, `primitives`, `units`,
`serde`, and `tokio` and picked a header style that I thought looked
good.
Use it for `primitives` and `units`.
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
43ae9d7516 primitives: Hide script error internals (Tobin C. Harding)
2d8227f091 Hide relative locktime error internals (Tobin C. Harding)
Pull request description:
Make the struct fields private and add getters.
ACKs for top commit:
apoelstra:
ACK 43ae9d751622c7bef548a469466d74cf01284129; successfully ran local tests; nice! Way easier to understand these types with the new incompatible / expected names
Tree-SHA512: cfe67d60ea61a2a4c27b09071a6b11739ca281bf0b4a655121f90215ce38c3a637acf53a6e01aa2ef26fa80004cd919bf3b3334dbd9566ee2f594cab7750b563
I don't know what I was thinking when I move the taproot hash types to
`primitives`. As correctly pointed out by Kix we agreed to only have
blockdata in `primitives`.
Move the taproot hash types back to `bitcoin::taproot` and remove the
extension traits.
As part of the 1.0 effort and forward maintainability hide the internals
of the two error types in the `script` module. Add getters to get at the
invalid size.
As part of the 1.0 effort and forward maintainability hide the internals
of the two error types in the `relative` locktime module. Doing so
allows us to remove the `non_exhaustive` attribute. Add getters to get
at the error innards.
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`.
5680b4e870 Refer to `Script{Buf}` as `Self` where relevant (Martin Habovstiak)
ce55dd5b70 Make `ScriptHash` & `WScriptHash` obey sanity rule (Martin Habovstiak)
9ec9adc71d Add a note about Electrum's script hashes (Martin Habovstiak)
82f553aada Expose `ScriptBuf`'s `capacity` (Martin Habovstiak)
6b9d439dc1 Remove stale FIXME comments (Martin Habovstiak)
0567e6fe1d Put `#[inline]` on most `Script{Buf}` methods (Martin Habovstiak)
b7e2af1b6b Implement `Arbitrary` for `&'a Script` (Martin Habovstiak)
bca2864084 Move `Deref{Mut}` from common module to `owned` (Martin Habovstiak)
3b15e900f0 Add `const` to some `Script` methods (Martin Habovstiak)
277223da6a Make `Script` and `ScriptBuf` obey sanity rules (Martin Habovstiak)
Pull request description:
This implements various improvements related to `Script`. Please refer to the individual commits for details.
This is a part of #4059
ACKs for top commit:
tcharding:
ACK 5680b4e870
apoelstra:
ACK 5680b4e870ba3b7340432256c24d37d2b6ead15a; successfully ran local tests
Tree-SHA512: 5daa8bf6c0b439a579d31d23944077e4a7fa89e14052003d2b81c745f225147f8f6f693d068e0567830027cefea7dda2516596da632bc817199352fa29af0a9b
cb270eae8e Make transaction::Version field private (jrakibi)
6c69b66b0d Use Version constant (jrakibi)
Pull request description:
This commit addresses #4041 by making the `transaction::Version` field private.
This forces people to either use the associated constants (`Version::ONE/TWO/THREE`) or the `non_standard`/`from_consensus` methods for any other transaction version.
This aligns with our approach to `block::Version`
ACKs for top commit:
tcharding:
ACK cb270eae8e
Kixunil:
ACK cb270eae8e
Tree-SHA512: dcf5b50dfeda04e56fec350acd735dcb7099989b552afce4261d559a8a846c0eb3705369ad635ef9bbbfb2373d203a2c3641178925de6685426aa91245db9a8c
This commit addresses #4041 by making the transaction::Version field private
Changes:
- Make the `Version` field private with `pub(crate)`
- Rename `non_standard` to `maybe_non_standard` for clarity since it accepts both standard and non-standard versions
- Add `#[inline]` attributes to small, frequently used methods:
- `as_u32`
- `maybe_non_standard`
Users now must use either:
- Constants (`Version::ONE/TWO/THREE`) for standard versions
- `maybe_non_standard` method for any version (standard or non-standard)
These were re-implementing hashing after the check rather than calling
the `_unchecked` method, so this replaces the manual implementation with
the method.
The Electrum protocol uses hashes of `script_pubkey` that might look
similar to the ones we have in the crate and could be confused. This
change notes that to hopefully avoid the confusion.
Resolves https://github.com/rust-bitcoin/rust-bitcoin/discussions/3997
There are already several methods referring to capacity but none to
retrieve it. Those methods also promise certain behavior that mandates
having *a* capacity field inside the struct, so no changes in layout
will ever remove it. Thus it's OK to expose the field.
Aside from exposing the field, this also fixes up the tests to obey the
sanity rules.
These methods are either newtype casts that should compile to no-ops or
directly calling into some other function with at most some pointer
adjustments. As such making them `#[inline]` is definitely benefitial.
There are also methods that check length and then call some other
function. These are also worth inlining since the length could be known
at compile time and the check could be eliminated.
We have several trait implementations for `Script` and `ScriptBuf` in a
common module so that it's easy to verify that they are same but `Deref`
and `DerefMut` should *not* be implemented for `Script` so having them
in the common module is not helpful. This moves them to the appropriate
`Owned` module.
The newtype sanity rules (a name I came up with):
* Newtypes should have at most one constructor that directly references
the inner field.
* Newtypes should have at most three accessor methods that directly
reference the ineer field: one for owned access, the second for
borrowed and the third for mutably borrowed.
* All other methods should use the methods above to perform operations
on the newtype and not directly access the fields.
This commit makes `Script` and `ScriptBuf` obey these except for
`reserve` and `reserve_exact` since we don't have `as_mut_vec` method.
As a side effect it also adds `const` to `ScriptBuf::from_bytes`.
This commit introduces `WrapDebug` in `internals` and updates `Witness`
debug implementation to use it. The previous `DebugElements` struct has
been removed in favor of an ad-hoc closure inside `WrapDebug`, which
formats witness elements as a debug list of hex-encoded values.
By abstracting out the "debug-print hex fields" pattern, we reduce
code duplication and improve maintainability.
4259dab93a Remove rust-ordered dependency (Tobin C. Harding)
d392cdbd7d Remove PartialOrd from locktimes (Tobin C. Harding)
Pull request description:
These two things are related so we remove them both in the same PR. Please see commit logs for full explanation.
For more context see discussion below and also:
- #2500
- #4002
- #3881Close: #4029
ACKs for top commit:
Kixunil:
ACK 4259dab93a
apoelstra:
ACK 4259dab93ab52795305db4b889b9151595bcee51; successfully ran local tests
Tree-SHA512: 7526d4faaa9edf8017d2af412c41a33f33d851ad5130c9a745bba86d9c71dc1db7f20d07377aaf3a25fec2c0de79f3ffabc2c538a5a366e415c7a6eaa730153c
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 it is possible to write
```rust
if this_locktime < that_locktime {
do_this();
}
```
with the hope that this code means if a locktime is satisfied by the
value in the other locktime. This is a footgun because locktimes are
incommensurate.
We provide the `is_satisfied_by` API to help users do locktime
comparisons.
Remove the `PartialOrd` implementation from both locktime types.
8d8edd2c77 make Debug representation of Witness to be slice of hex-encoded bytes strings (Erick Cestari)
Pull request description:
This PR updates the Debug implementation for the Witness type to improve its readability by displaying the witness data as a slice of hex-encoded strings rather than a concatenated blob or list of raw u8 values. The changes include:
- Improved Output:
The debug output now shows pseudo-fields such as the number of elements and the total length of all elements, making it easier to understand the underlying data without exposing internal indices like indices_start.
- Hex-Encoding:
Each witness element is displayed as a hex-encoded string, similar to Bitcoin Core's output style, which enhances clarity during debugging sessions.
These changes should provide a more developer-friendly view of the witness data and align with similar patterns used elsewhere in the ecosystem.
Closes#4023.
Example display:
```
Witness {
num_elements: 3,
total_bytes: 5,
elements: [
0b,
1516,
1f20,
],
}
```
```
Witness { num_elements: 3, total_bytes: 5, elements: [0b, 1516, 1f20] }
```
ACKs for top commit:
tcharding:
ACK 8d8edd2c77
Kixunil:
ACK 8d8edd2c77
apoelstra:
ACK 8d8edd2c77de9b0423533fc70802171803761fcd; successfully ran local tests
Tree-SHA512: ffcdf67542049f405317eecd74876b51972d27ec552eec8e9c7b6324f18f31f4721fc4d2be1e596232c39af90a8d169c082f9b0636e5aa1a80fe1b063d645456
ab2f709181 Implement Default for Script (jrakibi)
Pull request description:
This PR implements Default for `Script` to return an empty script.
*Note: ScriptBuf already has `#[derive(Default)]` in the code so it's already handled*
Resolves#3735
ACKs for top commit:
tcharding:
ACK ab2f709181
Kixunil:
ACK ab2f709181
apoelstra:
ACK ab2f7091814333b20669d41f1f78e0e52795df08; successfully ran local tests; neat!
Tree-SHA512: c06ba98d9bf8568e323ef9082a7f06756586360d6bef2b93721db7f6e28a777852e494c86319c97b0fd5444a0010d6c679625753534c0e1c8116e452ce8fa9cc
6cecc40ae4 Test LockTime PartialOrd (Jamil Lambert, PhD)
Pull request description:
Add tests to kill the mutants in both relative and absolute PartialOrd.
ACKs for top commit:
tcharding:
ACK 6cecc40ae4
apoelstra:
ACK 6cecc40ae4d52b711f58998315155bc8c6b19d7b; successfully ran local tests; thanks!
Tree-SHA512: dba7d90e3f6e62f0d3417bacc09d38145dd29bf654f84c2d3bc68af30c0e65b105146466a384bd35ef4326913ca414fd31f92daa3d7ffe3ff409c49bd1c05d96
7e66091e1e Add from impl tests (Jamil Lambert, PhD)
2f95064cfd Add from_parts test (Jamil Lambert, PhD)
3ee66c5bb8 Modify push test (Jamil Lambert, PhD)
Pull request description:
Add tests to kill the mutants in `primitives/src/witness.rs`
ACKs for top commit:
tcharding:
ACK 7e66091e1e
apoelstra:
ACK 7e66091e1e8b6cdd3e40d001ea1824125f7175e7; successfully ran local tests
Tree-SHA512: 57b2b0e4dbd93023d1a6a9709a02fa843e3ef9b25e7293ad641726b9c335e220a4ed87b717ec5dda999217677a916b86ac7daa9aaaec077afbfee4789836344e
435750f292 Add a parse_vout test (Jamil Lambert, PhD)
957be3c978 Add OutPoint test (Jamil Lambert, PhD)
a4ef027134 Add tests of transaction functions (Jamil Lambert, PhD)
Pull request description:
Add tests to kill the mutants in `primitives/src/transaction.rs`
ACKs for top commit:
tcharding:
ACK 435750f292
Tree-SHA512: 78baf40ad6ed1cd5b3a33346b4702c3a7efbb03ae6eec9a802d3dea99910373cf4e8053c73e9fdc02ce54852d3ee43253f2ff0c149c86870ecfed2fc909e5bcf
Cargo mutants found mutants in witness.
Add to the existing test `push` to kill the mutants from `is_empty` and
`third_to_last`.
Change the dummy values to make the progression of `elements` down the
test easier to follow.
Currently in order to release `hashes v1.0` we need to 1.0 `io` as well.
For multiple reasons, many out of our control, the `io` crate may not
stabalise any time soon.
Instead we can invert the dependency between the two crates.
This is an ingenious idea, props to Kixunil for coming up with it.
Notes
- `io` does not currently re-export the `hashes` crate.
- This work highlights that we cannot call `hash_reader` on a siphash.
- The `Hmac::hash_reader` uses the default key which may not be obvious.
Signed-off-by: Tobin C. Harding <me@tobin.cc>
Currently the `primitives` crate does not use the `io` dependency. I
don't know if this is just a mistake of if it used to and the manifest
is just stale.
963dba2bdf Test sequence formatting (Jamil Lambert, PhD)
6c3d7f6443 Test Sequence properties (Jamil Lambert, PhD)
Pull request description:
Add tests to kill the mutants found in Sequence.
ACKs for top commit:
tcharding:
ACK 963dba2bdf
apoelstra:
ACK 963dba2bdf102b2015393014d06cc56b8473520a; successfully ran local tests
Tree-SHA512: f03a02cd49df2b29232bf4f9baf663a7b51b0de504de88ae4d62de96d4e672800ac2387d0675aa48280dcbdfc0738f80484842f98b58324a09462c01956566cf
e5c74c38a2 Add missing re-exports to primitives crate root (Jamil Lambert, PhD)
Pull request description:
There are re-exports in the bitcoin root from primitives that were not re-exported in the primitives crate root.
Add the missing re-exports to primitives crate root.
First mentioned here https://github.com/rust-bitcoin/rust-bitcoin/pull/4012#discussion_r1943895283
ACKs for top commit:
tcharding:
ACK e5c74c38a2
apoelstra:
ACK e5c74c38a20e153f0919b43755d58dff8f2c8da2; successfully ran local tests
Tree-SHA512: c3355981ccd3a5db32423f6f44a6e169062c8cbce1f5e358ae2791e24801459a9db22065b83e54950dd3fd745ac4af4dcb1e1bd4625b5f25a9dd4f8fd4e6b860
b2d0737acc Add tests to CompactTarget (Jamil Lambert, PhD)
Pull request description:
Add tests to pow.rs to kill the mutants found in CompactTarget.
ACKs for top commit:
tcharding:
ACK b2d0737acc
apoelstra:
ACK b2d0737accb998071e0f19cda6f988956b6c097a; successfully ran local tests
Tree-SHA512: 8245b2342fabb7142cc0369cc53f4213f8f685dd0ed9d357214b2f692237b3484236462a96315d2c9409625fcf59484fc505674859542311893c23e50da6ffdf
486d55f042 Add test to opcodes (Jamil Lambert, PhD)
Pull request description:
Cargo mutant found a mutant in opcodes.
Add a test to kill it.
ACKs for top commit:
tcharding:
ACK 486d55f042
apoelstra:
ACK 486d55f0421a05c0aaa8f883bc15cacd212dcc19; successfully ran local tests
Tree-SHA512: 8bd28d56d6e5d79a3531f0a3391ac26021f8351a77e632dcceb8a57b2f1cce05aa02ec5e37d5b5771c3a42a2a7010a241d8f232521678604cc6ecb4bdb3327a5
There are re-exports in the bitcoin root from primitives that were not
re-exported in the primitives crate root.
Add the missing re-exports to primitives crate root.
a4fe67645a Use relative::* or absolute::* in locktime example (ndungudedan)
Pull request description:
Aims to resolve#3976
Apart from the examples in the `relative.rs` file, I see others in the **examples** folder. If this patch is good, I can continue with them.
I will happily appreciate a review.
ACKs for top commit:
tcharding:
ACK a4fe67645a
jamillambert:
ACK a4fe67645a
Tree-SHA512: a9551c77bf91fdc212126d387adb110a7dc71635620f4efa25dc1e4306a9953968a4ad0e1528c990d5083bd9833702e62ae819db474a49f1284327fd042fb1c2
Users should be encouraged to explicitly use relative::* or
absolute::* instead of just LockTime, Time, or Height so that
it is clear when looking at the code if it is a relative or
absolute locktime.
12a1c3c4b7 Add `script` tests (Jamil Lambert, PhD)
Pull request description:
Together with #3971 this fixes all of the mutants in `primitives/src/script` except for those related to `serde`.
ACKs for top commit:
tcharding:
ACK 12a1c3c4b7
apoelstra:
ACK 12a1c3c4b7bc097553fbfb05a0e92320ae42975c; successfully ran local tests
Kixunil:
ACK 12a1c3c4b7
Tree-SHA512: c0b757ae672999799e105befb6a2907b8cecb54c3e6d86dbf3e877ddf54e6fd102ebb2604381d0fa89bce0b3b4f70db6cb9ed3b5c85076e13de8369e81431926
c16eab7723 Add examples to `absolute::LockTime` (Jamil Lambert, PhD)
2d73746ad1 Improve `from_consensus` example (Jamil Lambert, PhD)
25a6742573 Add examples to `relative::LockTime` (Jamil Lambert, PhD)
Pull request description:
To conform to API guidelines add examples to both `relative` and `absolute` `LockTime`.
As discussed in #3967 add an example that doesn't round trip in `relative::LockTime::from_consensus`.
ACKs for top commit:
tcharding:
ACK c16eab7723
apoelstra:
ACK c16eab772375a0b8f82789a3453f7e983523fd50; successfully ran local tests
Tree-SHA512: 898db2dbcccfc54971c65bb72030505dbcfffb035aa74eb376cc1eb8d3619c205338899a58fb9f267fca192c11e7ae48fc17a2d9019c5ff4bd34bf029fa9a48f
We've stopped using `TxOut::NULL` in the code and we want to restrict
`Amount` to 21M BTC, so we are now deleting this constant without
deprecation. Deprecation can be backported if needed.
6cde537d9b Add `ScriptBuf` tests (Jamil Lambert, PhD)
566a6e5da6 Add `Script` tests (Jamil Lambert, PhD)
Pull request description:
Add tests to both `primitives/src/script/borrowed.rs` and `primitives/src/script/owned.rs` to kill the mutants found by running `cargo mutants`.
ACKs for top commit:
tcharding:
ACK 6cde537d9b
apoelstra:
ACK 6cde537d9ba9bd56495ca47afb3f9a560e9b4358; successfully ran local tests
Kixunil:
ACK 6cde537d9b
Tree-SHA512: fc36c1e9249753ebcb0f72e8195c85a7eccd3a0b8b3e4e753102d405494e1f72cd1cdfb6f12af41a9aa6f2ff10142b95827039fa428997b96510a5e262007b25
The existing example at first look seemed to contradict the doc above
that the function would not round trip.
Update the example to show the useage for both a time and height based
lock time.
Replace unwrap() with ?
cb5ffde9ee Change `#[must_use]` to be the same as stdlib (Jamil Lambert, PhD)
Pull request description:
Change the iterator `#[must_use]` message to be the same as stdlib uses.
Message taken from https://doc.rust-lang.org/src/core/iter/traits/iterator.rs.html#38Close#3962
ACKs for top commit:
Kixunil:
ACK cb5ffde9ee
tcharding:
ACK cb5ffde9ee
Tree-SHA512: 4866d9bddccdc28e2ba2b15cb4041851abb1512cea0317f20d2e2306df6dcd1addb828d61a84a2083d3ef47abb5cf9276e857cf098de2d28b4706ecd0a1f24fa
Back in 2022 we elected to use `mutagen` for mutation testing. Since
then `cargo mutants` has progressed to a point where we would now like
to use it instead.
Remove all the `mutagen` stuff and update the lock files.
Close: #2829
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>`.
85e04315d5 Remove test_ prefix from unit tests (Tobin C. Harding)
Pull request description:
There is a loose convention in Rust to not use `test_` prefix. The reason being that `cargo test` outputs 'test <test name>' using the prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some places and not others is confusing to new contributors and is leading me to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
ACKs for top commit:
shinghim:
ACK 85e04315d5
apoelstra:
ACK 85e04315d5eb90075ce55bf18fab8876a4583def; successfully ran local tests
Tree-SHA512: d90ae5ef75cc5e5a8f43f60819544f1a447f13cbe660ba71e84b8f27bfcc04a11d3afde0ed56e4eea5c73ebc3925024b800a1b995f73142cab892f97a414f14a
0d8e9ef096 Remove usage of impl_from_infallible in leaf crates (Tobin C. Harding)
Pull request description:
Rust macros, while at times useful, are a maintenance nightmare. And we have been bitten by calling macros from other crates multiple times in the past.
In a push to just use less macros remove the usage of the `impl_from_infallible` macro in all the leaf crates and just write the code.
ACKs for top commit:
apoelstra:
ACK 0d8e9ef096fd60fcdb7928697c8f554bf428b754; successfully ran local tests; this is a great change
Tree-SHA512: c4cff4517f7846d91142f26d451c2bcafc014a0921d11ac1487eab087449f12023c3b4fc57e1bc72ed3ea58406b4c3d24bbd846df21353f5d7ecb4a4b8bfb0b2
fd8d563b87 Remove macro debug_from_display (Tobin C. Harding)
Pull request description:
Rust macros, while at times useful, are a maintenance nightmare. And we have been bitten by calling macros from other crates multiple times in the past.
In a push to just use less macros remove the `debug_from_display` macro and just write the code.
This is an API breaking change to `internals` but an internal change only to any of the _real_ crates.
ACKs for top commit:
apoelstra:
ACK fd8d563b873c87a996d82062285169e16e3f0c13; successfully ran local tests; this is a great change
Tree-SHA512: 26faa6645d010c1b5873d584c36d0fc52b73553d88be3226937431210ccc2479548757593b8a151936e9fa280cb3c604b241511f24ef0aa5cbf3f424d7ecf215
79791e7444 primitives: Add core re-exports (Tobin C. Harding)
Pull request description:
This is not strictly needed but we would like to copy the public macros from `units` to primitives and they use `$crate::_export` paths.
ACKs for top commit:
apoelstra:
ACK 79791e7444326d284bdf42c40926f6b05a6b65f7; successfully ran local tests; sure, lgtm
Tree-SHA512: cdf683041dca07bc53ae8e70806cfb74ef79e94b8699112774e3bfb3efd29cc270bd24ab03a448197edd1d143dec07d18d9a3a92d74097d87478d40954392ee4
There is a loose convention in Rust to not use `test_` prefix. The
reason being that `cargo test` outputs 'test <test name>' using the
prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some
places and not others is confusing to new contributors and is leading me
to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
Rust macros, while at times useful, are a maintenance nightmare. And
we have been bitten by calling macros from other crates multiple times
in the past.
In a push to just use less macros remove the usage of the
`impl_from_infallible` macro in all the leaf crates and just write the
code.
Rust macros, while at times useful, are a maintenance nightmare. And
we have been bitten by calling macros from other crates multiple times
in the past.
In a push to just use less macros remove the `debug_from_display`
macro and just write the code.
This is an API breaking change to `internals` but an internal change
only to any of the _real_ crates.
I just went to town on the `rust-ordered` crate to get it ready for
releasing a `1.0` version. None of the changes effect our usage here in
`rust-bitcoin`.
Upgrade to the latest version of `rust-ordered` - `v0.3.0`.
e56f461916 Make capitalization of SegWit uniform in strings (Jamil Lambert, PhD)
3520e832ac Make capitalization of SegWit uniform in rustdocs (Jamil Lambert, PhD)
Pull request description:
Fixes#3770
- Searched and replaced all occurrences of `//` * `segwit` (case insensitive) with `//` * `SegWit`
- Searched and replaced all occurrences of `"` * `segwit` (case insensitive) with `"` * `SegWit`
ACKs for top commit:
apoelstra:
ACK e56f461916b85928139950074d0df52caf4f919b; successfully ran local tests
Tree-SHA512: c56704102d8e86f26378bb302d5fdc7ea21d41fd49f55606e589ec32ff896f1adfb1960d8fb29abccfbf298b90fc357ed609d80fd0163dcb4ff01573646b02c3
We need to do a quick release because it turns out I was wrong in
thinking that making `hex` an optional dependency is not a breaking
change.
```
the package `bitcoin` depends on `bitcoin_hashes`, with features: `hex`
but `bitcoin_hashes` does not have these features. It has a required
dependency with that name, but only optional dependencies can be used as
features.
```
Add a changelog, bump the version, depend on the new version repo wide,
and update the lock files - like its our job.
6e01383f16 api: Run just check-api (Tobin C. Harding)
3855d3cc83 Move script hashes to primitives (Tobin C. Harding)
d1dd63d6d4 Remove wildcard in script re-exports (Tobin C. Harding)
Pull request description:
Woops, this should have been done before v0.101.0 was released.
Move the `ScriptHash` and `WScriptHash` types to `primitives`.
Requires moving constants and error types as well. We re-export the errors because they are in the `mod.rs` file so they should appear in both `primitives::script::FooError` and `bitcoin::script::FooError`.
ACKs for top commit:
apoelstra:
ACK 6e01383f162307a573c3454733ccae0596d3eaf6; successfully ran local tests
Tree-SHA512: 4ef838ee9c4cb3eb308ffa855ea5d6f5ad46f81c17b9413faa493a46a262afc18b47f28a0fdd5fc675eea31b895d0eb0e2505a523e820504ec88d9334d6874b4
463fbd16f1 Update to arbitrary v1.4 (Tobin C. Harding)
Pull request description:
Out with the old in with the new, update to the latest minor version of arbitrary.
No real reason to do this except that I wanted to add the minor version instead of using just `1` so that we don't accidentally pull a new minor version in during a patch release (after we 1.0).
(I'm unsure if this is necessary and did not test it against MSRV.)
ACKs for top commit:
apoelstra:
ACK 463fbd16f1431febe7e4fc50abd7415886542109; successfully ran local tests
Tree-SHA512: 88913f993b97abba224ba94ee9d5057894bb7142fef07e557de03a423da314a9f21632932fb64c4c27c676051bbb6e90d9b90bfa944af810e834bf42707d3cff
Out with the old in with the new, update to the latest minor version of
arbitrary.
No real reason to do this except that I wanted to add the minor version
instead of using just `1` so that we don't accidentally pull a new minor
version in during a patch release (after we 1.0).
Woops, this should have been done before v0.101.0 was released.
Move the `ScriptHash` and `WScriptHash` types to `primitives`.
Requires moving constants and error types as well. We re-export the
errors because they are in the `mod.rs` file so they should appear in
both `primitives::script::FooError` and `bitcoin::script::FooError`.
adaf4ac086 Set avoid-breaking-exported-api to false (Tobin C. Harding)
Pull request description:
These lints are valuable, lets get at em.
ACKs for top commit:
apoelstra:
ACK adaf4ac086553674803fadfde776d6dd013a7964; successfully ran local tests; will probably be a problem repeatedly for the next few days until we get through all currently-open PRs
Tree-SHA512: 56617a2f4aeafceef72008232cee817d45b62c040d7f1938631291c5d73627851cfbefc4b4000cd380ecb5c7e1379d1022f6cc90a4b68c819c78fb883bee0b3a
549be547ac primitives: Add must_use (Tobin C. Harding)
Pull request description:
Enable lint `clippy::return_self_not_must_use` and add attribute `must_use` as required.
Also run the linter with `clippy::must_use_candidate` enabled and manually check every warning site.
Done as part of #3185
ACKs for top commit:
apoelstra:
ACK 549be547accdb13037bf3ef60310ca30d045dce0; successfully ran local tests; nice! will one-ACK merge
sanket1729:
ACK 549be547ac
Tree-SHA512: b22a6849fd0f4a7b65e1a9816efd47d411dcf2a5d5d46ae75b2b4d2389d3c9f46ab271314b112de9cd6fdc52cac7b53a632642e9bd90092d7065a8646e1362ec
These lints are valuable, lets get at em.
Changes are API breaking but because the changes make functions consume
self for types that are `Copy` downstream should not notice the breaks.
Enable lint `clippy::return_self_not_must_use` and add attribute
`must_use` as required.
Also run the linter with `clippy::must_use_candidate` enabled and
manually check every warning site.
While we are at it change the current `must_use` usages to have no
message. We can always add a message later if needed.
Recently we reduced the `alloc` requirements in `units` but we did not
propagate these changes up to `primitives`.
Remove a bunch of `alloc` feature gating from `primitives`.
The `Amount` and `SignedAmount` were not supposed to implement `serde`
traits by design because doing so implicitly uses sats. We provide two
modules `as_sat` and `as_btc` to allow users to explicitly serialize in
their preferred format.
In commit: `d57ec019d5 Use Amount type for TxOut value field` derives
were added for `serde` and we did not notice it during review.
ec06028f63 hashes: Make hex dependency optional (Tobin C. Harding)
9dce0b4b8c Remove hex string trait bounds from GeneralHash (Tobin C. Harding)
766f498b33 Pull serde stuff out of impl_bytelike_traits macro (Tobin C. Harding)
Pull request description:
This is done in 3 parts:
1. Pull the `serde` stuff out of `impl_bytelike_traits` to fix the bug described here: https://github.com/rust-bitcoin/rust-bitcoin/issues/2654#issuecomment-2470716693
2. Prepare the `hashes` trait by removing string/hex trait bounds from `GeneralHash` and also pull the hex/string stuff out of `impl_bytelike_traits`
3. Make hex optional, including adding custom debug logic when `hex` feature is not enabled
Patch 3 is tested in `hashes/embedded`, by the new `debug` unit test, and there is a `Midstate` unit test as well that covers the `Debug` impl.
Close: #2654 - BOOM!
ACKs for top commit:
apoelstra:
ACK ec06028f63ba591a14c3a15cdfd410bb5ff1c09b; successfully ran local tests; nice!
Tree-SHA512: 85eb10d36a4581af6cd700f7ff876585bcc114c60e9864906e65659f3b3ee550fe6d9f40ca4230d870a9e23f0720723e11443ec329f16e40259a259b9be57466
In preparation for releasing `primitives v0.101.0` bump the version
number, add a changelog entry, update the lock files, and depend on the
new version in all crates that depend on `primitives`.
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
The only reason we need `hex-conservative` is to parse strings and
format them as hex. For users that do not require this functionality we
can make the `hex-conservative` crate an optional dependency.
The `serde` feature requires `Display` so we enable `hex` from the
`serde` feature.
If `hex` feature is not enabled we still need to be able to debug so
provide `fmt::Debug` functionality by way of macros.
Close: #2654
be553217f1 transaction: Add plural field getters (Tobin C. Harding)
Pull request description:
The `Transaction` type was created way back when, and the field names were named using singular (`input` and `output`). In hindsite plural probably should have been used since its more idiomatic Rust but the fields have been around so long now that re-naming them is going annoy a whole bunch of people.
As a compromise add getters that use plural, immutable and mutable versions, for both the `input` and `output` fields.
Close: #822
ACKs for top commit:
apoelstra:
ACK be553217f19069dd9ca28c3ba5953059df9caa1b; successfully ran local tests; though will let this sit a couple days before merging
sanket1729:
ACK be553217f1
Tree-SHA512: 4df9db179a69c9eea2940008c9eca9f9b9b9d9f27e2a174db39bfb3d5c32a9d06b26e5349e5baf5af78600ae128d1b534559baf7335aec6df1238de96a508764
For the `hashes` crate we would like to make `hex` an optional
dependency. In preparation for doing so do the following:
- Remove the trait bounds from `GeneralHash`
- Split the hex/string stuff out of `impl_bytelike_traits` into a
separate macro.
The `impl_bytelike_traits` macro is public and it is used in the
`hash_newtype` macro, also public.
Currently if a user calls the `hash_newtype` macro in a crate that
depends on `hashes` without the `serde` feature enabled and with no
`serde` dependency everything works. However if the user then adds a
dependency that happens to enable the `serde` feature in `hashes` their
build will blow up because `serde` code will start getting called from
the original crate's call to `hash_newtype`.
Pull the serde stuff out of `hash_newtype` and provide a macro to
implement it `impl_serde_for_newtype`.
In #2646 we introduced a bug in the taproot witness stack getter
functions, of which we have three:
- `tapscript`
- `taproot_control_block`
- `taproot_annex`
Each returns `Some` if a possible bytes slice is found (with no other
guarantees).
Use `taproot_annex` combined with getters from `primitives` to implement
the other two getters. This simplifies the code and fixes the bug.
Add an additional getter to `primitives` `Witness::third_from_last`.
Fix: #3598
We are trying to make it so that what ever crate a user uses they see
the same module/type tree (if the module or type exists).
E.g., one can do either of these.
If they use `bitcoin`:
```
use bitcoin::{
amount, block, fee_rate, weight, merkle_tree, opcodes,
pow, script, sequence, transaction, witness,
};
```
Or if they use `primitives`:
```
use bitcoin_primitives::{amount, block, fee_rate, weight};
```
The above imports were tested and `amount` was found to be missing.
There is a range of different wordings used in the docs of constructor
type functions.
Change all to start with `Constructs a new` or `Constructs an empty`.
In functions that act like constructors there is a mixture of the usage
of `creates` and `constructs`.
Replace all occurrences of `creates` with `constructs` in the first line
of docs of constructor like functions.
The `Transaction` type was created way back when, and the field names
were named using singular (`input` and `output`). In hindsite plural
probably should have been used since its more idiomatic Rust but the
fields have been around so long now that re-naming them is going annoy a
whole bunch of people.
As a compromise add getters that use plural, immutable and mutable
versions, for both the `input` and `output` fields.
Close: #822
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.
85942c355d Re-export block::Header as BlockHeader (Tobin C. Harding)
Pull request description:
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`.
Close: #3548
ACKs for top commit:
apoelstra:
ACK 85942c355dfdcf9ddf9a46202f4db56794dcc85d; successfully ran local tests
jamillambert:
ACK 85942c355d
Tree-SHA512: 6fbaf7936062323d31940179ffcbc62951b21f53955a42dca2e28476bda0cebeb041db0e1b4f1ceeac32e43d94fe5476ef055595ae88dc6cfc266d32082bdf4d
Jamil Lambert, PhD
Fmt Bot
merge-script
Tobin C. Harding
AM
Shing Him Ng
Erick Cestari
Martin Habovstiak
jrakibi
Andrew Toth
ndungudedan