The tests for both `Script` and `ScriptBuf` `as_ref` and `as_mut` only
covered the `u8` impl.
Expand the tests to cover the `Self` impl as well.
The allow useless_asref is required to remove a clippy lint. Removing
the "useless" as_ref also removes the test coverage of the as_ref or
as_mut impl.
35ca48a85c primitives: Document script to/from bytes (Tobin C. Harding)
Pull request description:
The byte slice/vector in the to/from methods for the script types does not include the length prefix that is used when consensus encoding.
Add docs to make this more clear.
ACKs for top commit:
apoelstra:
ACK 35ca48a85cd90fc9c1b13da439177a4772c1c330; successfully ran local tests; yeah, I like this
Tree-SHA512: 6efeddfd8fe9e5bb32a6d40ca1a0fa3aa686047ad9f4cb865e3c70f8339cd39c18daa01441a1a5898300b1e73150b39c53d507af528830bbd68904148b65ef35
a5f904559d primitives: Make hex optional (Tobin C. Harding)
Pull request description:
Make the `hex` dependency optional. This means not implementing
`Display` for some types if `hex` is not enabled and only implementing
`Debug`.
Also without `hex` enabled:
- We loose the ability to parse an `OutPoint` from string because we
can no longer parse `Txid`.
- We loose the hex formatting of witness elements.
Note also that `primitives` builds with the `serde` feature even if
`hex?/serde` is excluded from the `serde` feature. I found this
surprising.
Close: #4183
ACKs for top commit:
apoelstra:
ACK a5f904559d3b5d2dfbcbed8b1746305e32103fee; successfully ran local tests
Tree-SHA512: c105e089508036af8251cb923f3eda163b8f2d6151ea043aa1489edb6ce396975d1f598a240f4ca6e48f6ef780774f7d86cb70ec9399f3d2ff87c0ac53ceee91
7e1369b3f1 Cover all TryFrom implementations in Script (Jamil Lambert, PhD)
6dfa3d8f4d Update test to increase coverage (Jamil Lambert, PhD)
Pull request description:
Update test in borrowed to cover the `default` implementation of `Script`.
Add tests to cover all `TryFrom` implementations in `Script`
ACKs for top commit:
apoelstra:
ACK 7e1369b3f1a7789b2757951c2e3ff58937442200; successfully ran local tests
tcharding:
ACK 7e1369b3f1
Tree-SHA512: 766b8eb63abbb1f10670f42b3789a8a4d4d73470ade13f14a1140d4cdd93b49d79865645d49f404774d8ff7aeb22b68059275902dc063d2baaf79fc8a89dca95
The byte slice/vector in the to/from methods for the script types does
not include the length prefix that is used when consensus
encoding.
Add docs to make this more clear.
Make the `hex` dependency optional. This means not implementing
`Display` for some types if `hex` is not enabled and only implementing
`Debug`.
Also without `hex` enabled:
- We loose the ability to parse an `OutPoint` from string because we
can no longer parse `Txid`.
- We loose the hex formatting of witness elements.
Note also that `primitives` builds with the `serde` feature even if
`hex?/serde` is excluded from the `serde` feature. I found this
surprising.
The `default` implementation is missed in the test coverage.
Change calling `new()` to using the `default` implementation, which then
calls `new()` anyway.
492073f288 Strengthen the type of `taproot_control_block()` (Martin Habovstiak)
e8a42d5851 Unify/reduce usage of `unsafe` (Martin Habovstiak)
d42364bd9d Swap around the fields in `Address` (Martin Habovstiak)
7a115e3cf1 Make `Address` obey sanity rules (Martin Habovstiak)
bc6da1fe07 Swap around the fields in `sha256t::Hash` (Martin Habovstiak)
8ee088df74 Make `sha256t` obey sanity rules (Martin Habovstiak)
Pull request description:
Well, I thought this PR will be just the last commit... 😅
Anyway, this implements a bunch of changes to allow returning `ControlBlock` from `Witness` method(s). One cool side effect is that this PR also reduces the number of `unsafe` blocks.
ACKs for top commit:
apoelstra:
ACK 492073f28876406f8fe5a07a8a2495c8e0ba1fb3; successfully ran local tests
Tree-SHA512: 11979517cc310abf25644fc93a75deccacae66af8ba2d9b4011fdc3f414b15fac7e748399c7eef492ca850c11b7aacc3f24ec46fccf95e6d57a400212979637e
Since the introduction of `Script` `unsafe` started slowly creeping in
as more types with similar semantics were added. The `unsafe` in these
cases is just for trivial conversions between various pointer-like
types. As such, it's possible to move these into a single macro that
takes care of the conversions at one place and avoid repeating the same
`unsafe` code in the codebase. This decreases the cost of audits which
now only need to happen in `internals`, focuses any changes to happen in
that single macro and decreases the chance that we will mess up
similarly to the recent `try_into().expect()` issue (but this time with
UB rather than panic).
The new macro accepts syntax very similar to the already-existing struct
declarations with these differences:
* The struct MUST NOT have `#[repr(transparent)]` - it's added by the
macro
* If the struct uses `PhantomData` it must be the first field and the
real data must be the second field (to allow unsized types).
* The struct must be immediately followed by an impl block containing at
least on conversion function.
* If the struct has generics the impl block has to use the same names of
generics.
* The conversion functions don't have bodies (similarly to required
trait methods) and have a fixed set of allowed signatures.
* Underscore (`_`) must be used in place of the inner type in the
conversion function parameters.
The existing code can simply call the macro with simple changes and get
the same behavior without any direct use of `unsafe`. This change
already calls the macro for all relevant existing types. There are still
some usages left unrelated to the macro, except one additional
conversion in reverse direction on `Script`. It could be moved as well
but since it's on a single place so far it's not really required.
9a572dabde refactor: use path dependencies for workspace members in bitcoin/Cargo.toml (Eval EXEC)
Pull request description:
This PR want to:
1. make all workspace members use `workspace = true` syntax to import dependencies.
2. use `path = [balabala]` to define dependencies, instead of useing `[patch.crates-io.balabala]` , fix: https://github.com/rust-bitcoin/rust-bitcoin/issues/4283
ACKs for top commit:
Kixunil:
ACK 9a572dabde
apoelstra:
ACK 9a572dabdeb077f96b2ab66be1a80fcec3e805e3; successfully ran local tests
Tree-SHA512: 834ef881ed3fd324a9ecca440e8e591984a7e474eb6aeab86a0301cbd08b6dc96ecdc34b306ad146b11b50f7488024c289b8f8c7c6de1a2bdba7aec515b722ee
84bee2f7b0 Simplify `Witness` construction in tests (Martin Habovstiak)
3551ec2c69 Don't access internalls of `Witness` in tests (Martin Habovstiak)
c8078360d2 Impl `PartialEq` between `Witness` and containers (Martin Habovstiak)
587a66da47 Add a bunch of missing conversions for `Witness` (Martin Habovstiak)
Pull request description:
This is supposed to go in front of #4250
`Witness` lacked a bunch of APIs that were making it harder to use and test, so this also adds them in addition to cleaning up tests. (I only realized they are missing when I tried to clean up tests and got a bunch of errors.)
ACKs for top commit:
tcharding:
ACK 84bee2f7b0
apoelstra:
ACK 84bee2f7b06a7bd1f435aaad18fa76a15188326e; successfully ran local tests
Tree-SHA512: 7973f2a56b070babba7b4c632f45858154ccd00f8e77956ad2d28cb66e1fd18ff60d92c031ba3b76d0958e4acd34adfca10607fa26ec569dfd52ba1c1e2c79eb
Accessing the internals of tested object is problematic because it makes
changes to layout harder, it makes tests harder to read and it checks
implementation details rather than semantics of the API (behvaior).
We had such tests in `primitives::witness` so this changes them to use
the newly added APIs instead. However, it still leaves
`from_parts__unstable` which needs to be dealt with separately.
Since `Witness` is semantically equivalent to `&[&[u8]]` they should
also be comparable. However we only had the impl to compare `Witness`
with itself. Being able to compare `Witness` with other containers is
particularly needed in tests.
`Witness` was missing conversions from arrays (and variations) which was
annoying when creating known-sized witnesses. These come up when
spending statically-known inputs and in tests.
db9ec3bed8 Remove From<newtype> for $hash (Tobin C. Harding)
6b2b89c2f7 Remove From<hash> for not-general-hash types (Tobin C. Harding)
200ff47327 Use compute_merkle_root (Tobin C. Harding)
Pull request description:
The `hash_newtype` macro is explicitly designed to produce a hash that is not a general purpose hash type to try and prevent users hashing arbitrary stuff with it. E.g., `Txid` isn't meant to be just hash arbitrary data. However we provide a `From` impl that will convert any instance of the inner hash type into the new type. This kind of defeats the purpose. We provide `from_byte_array` and `to_byte_array` to allow folk to 'cast' from one hash type to another if they really want to and its ugly on purpose.
Also, it is becoming apparent that we may be able to remove the `hashes` crate from the public API of `primitives` allowing us to stabalise `primitives` without stabalising `hashes`.
For both these reasons remove the `From` impl from the `hash_newtype` macro. Note that deprecating doesn't seem to work so we just delete it.
ACKs for top commit:
Kixunil:
ACK db9ec3bed8
apoelstra:
ACK db9ec3bed8d6164a0345ba8db1e2162626db7cc5; successfully ran local tests
Tree-SHA512: 90bc325821cd2d72bbaef5b3cfef2d299192d1e7999cd4f96b6b69b8872e419964e431e91674c59bfdd2e9a5959dbc13ee89d5f87d03e96785044c616db19d72
We provide the from/to_byte_array functions for casting between arrays.
We shouldn't be supporting calls to `into` to quickly do the cast.
We already removed the other direction, now remove this one.
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.
7b114e3893 Increase test coverage in relative.rs (Jamil Lambert, PhD)
c97bebdcba Increase test coverage in absolute.rs (Jamil Lambert, PhD)
Pull request description:
Increase the test coverage in `primitives/src/locktime/*`.
Modify existing tests and add new ones where required to increase the test coverage to 100% for all features excluding `arbitrary` and `serde`.
ACKs for top commit:
tcharding:
ACK 7b114e3893
apoelstra:
ACK 7b114e38935f9c688c1259793e7ffda822f25c57; successfully ran local tests
Tree-SHA512: 9090565c358a23855f5bfa5c31202bfd78ed9dc36303210b64d502cacf692b010793ba0e5b2cc40c8da336429f6ffc07b6d4499cd8d03c0c5c7bb7bb868c4236
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
Jamil Lambert, PhD
merge-script
Tobin C. Harding
Fmt Bot
Martin Habovstiak
Eval EXEC
jike
AM
Shing Him Ng