8b47068a2e feat(locktime): implement MtpAndHeight structure and validation logic (aagbotemi)
Pull request description:
This PR fixes#4299
- Computed MtpAndHeight structure
- Checked if relative time and height is satisfied by MtpAndHeight
- Compared the Ordering of MtpAndHeight with time and height
- Checked MtpAndHeight satisfaction and comparison in Locktime
- Added unit tests for all the implementation
I've reviewed and adhered to the contribution guidelines
ACKs for top commit:
apoelstra:
ACK 8b47068a2efada30aec21c61ae4be0da4d8e8fc8; successfully ran local tests
Kixunil:
ACK 8b47068a2e
tcharding:
ACK 8b47068a2e
Tree-SHA512: b00d1384d5deaa038b486ca9d77ad33cfa6cd8c987e08407863f2be8d540014bdcc971cd9d46acb51a2d105341accc04ba151e5cccb276e8352a5d45b33097eb
Add the length prefix when formatting hex strings by way of `LowerHex`
and `UpperHex`.
This looses formatting options because I can't remember right now how
not to - again.
- Add MtpAndHeight for relative locktime checks
- Include unit tests for time/height comparisons
- Fix API design for mtp_as_time() error handling
- Update documentation and dependencies
- Fix BlockTime, CI, remove Ordering, and PR discussion fixed
- Fix UTXO height and timestamps
- Fix: chain_state and utxo_state handled seperately for is_satisfied_by
- Fix: panic on overflow fixed with check_add
- Fix: documentation updated and trailing whitespaces removed
- docs(mtpheight): documentation updated
- used accessors to_height and to_mtp over From impl
c4d9c1b9f8 Use a consistent rustdoc heading level of H1 `#` (Jamil Lambert)
6325a7cdea Change rustdoc heading level of references (Jamil Lambert)
f22e997587 Use parameters instead of arguments in rustdocs (Jamil Lambert)
e2c7be6d2f Fix typo (Jamil Lambert)
Pull request description:
In the rustdocs both `# Parameters` and `# Arguments` are used to mean the same thing. In a previous PR #2792 it was decided to go with Parameters everywhere. Since then there have been a few additions of "Arguments" into the rustdocs.
There is also a mix in the usage of `#`, `##` or `###` for headings. Noticed here https://github.com/rust-bitcoin/rust-bitcoin/pull/2792#issuecomment-2125775974.
- Fix a typo found when looking into this.
- Change all occurances to use `# Parameters` in rustdocs.
- Change all heading levels to H1 `#`
- Change all subheading levels to H3 `###` to make the small difference in the rendered font size noticable
Closes#4380
ACKs for top commit:
apoelstra:
ACK c4d9c1b9f8e59bf795812c42bd1eee68d97b9bbd; successfully ran local tests
Tree-SHA512: c8cc77ccf7e2003dd2dd1d309268624576e3bf390cd8ac61b0a7bb1141ca05377c83627576b0b7ff258b8e51c2d255097a4363fbdd1b368db7d32ac32ece58a1
9eaaf114b0 fix(witness): remove explicit type (aagbotemi)
5b572c5976 docs(witness): use constructs instead of creates (aagbotemi)
Pull request description:
This PR fixes#4379
- Removed explicit type
- updated documentation
- PR is in two seperate patches
ACKs for top commit:
apoelstra:
ACK 9eaaf114b0d6afb810cec8e9dac603ecadf64ff4; successfully ran local tests
Tree-SHA512: 65a6ca880c64aa81d83dffbfcd21a0427c874237a606d82ab70f226839adb164c9ee7fd1ecd5d8cee662660efcd1c458c523a27964cd1d00704d40a3bb6a1deb
6370aac7e1 Make Version::maybe_non_standard and Version::is_standard const (Shing Him Ng)
Pull request description:
Closes#4206
ACKs for top commit:
apoelstra:
ACK 6370aac7e119042d8b6b40021bbcd6324ad02bbb; successfully ran local tests
Tree-SHA512: aaf6afbe39d6568e0d05776a2a2f58a9a657f28cd3942391b599390bb4db974684e7d76cdf8db51a0fbcb948cdc4e19f22ba4b8ca39537e162cbd5bb2d4cd40e
cf42c511d8 primitives: replace error propagation with `expect` (Andrew Poelstra)
Pull request description:
This error path cannot happen, and if it could, failing formatting is not the correct source of action, because the std docs say that formatting may only fail if the formatter says so.
Fixes comment in #4269
ACKs for top commit:
tcharding:
ACK cf42c511d8
Kixunil:
ACK cf42c511d8
Tree-SHA512: 0ad47bb210dc71568bde426b8fd7782a4d48415fbf27133783975117d174b58eb791e68290e53f150a7e65236efda75032852bb6ed30bf4a182f18b9440cfcf9
There was and inconsistent usage of `#`, `##` and `###` in rustdoc
headings. The difference in the rendered rustdocs is a minimal font
size change.
Change all headings to be H1 `#`.
Change all subheadings to be `###` to have a noticeable difference in
font size in the rendered docs.
`Bitcoin Core References` used H3 `###` in rustdocs, and one occurrence
had another `References` heading above it.
Change them to be H1 `#` like other headings.
Remove the redundant `References` heading. NB. there are no other
occurrences of `# References` in this crate.
This error path cannot happen, and if it could, failing formatting is
not the correct source of action, because the std docs say that
formatting may only fail if the formatter says so.
Fixes comment in #4269
e0b627ea81 deserialize witness from a list of hex strings - ci(primitives): enable hex feature in CI build - from_hex() implemented more efficiently (aagbotemi)
Pull request description:
This PR implements `from_hex` function for deserialize `Witness` from a list of hex strings. Added unit test.
This PR fixes#4350
ACKs for top commit:
apoelstra:
ACK e0b627ea816a730949cdb200105598600fcac094; successfully ran local tests
tcharding:
ACK e0b627ea81
Tree-SHA512: deec3f9e5f67a0915b11a811c40c341dd9f24d0394d6cfbd6a09f765ce3fc0dcce2740949c264d4aa2d2db748a5ce81416b4dac15b1b64475a7c024b205e40ab
6323867c65 Run the formatter (Tobin C. Harding)
d5c08aef32 units: Update column width (Tobin C. Harding)
f7f1a0be8c Add privacy boundary to BlockTime (Tobin C. Harding)
b3dfe0df3f Add privacy boundary to Weight (Tobin C. Harding)
e1a14b3c2d Add privacy boundary to FeeRate (Tobin C. Harding)
4b733d4dad Run the formatter (Tobin C. Harding)
Pull request description:
Add privacy boundary for `FeeRate`, `BlockTime`, and `Weight`. The lock times are a bit curly so just doing these ones for now.
ACKs for top commit:
apoelstra:
ACK 6323867c65cdbf0f1b25d252fded6873acd35a3a; successfully ran local tests
Tree-SHA512: b6cf5eb032e055414f48f3469ec1748635b9f53ca64c158bd861d1ae6e2cfbe7b4d82de98f7dc90913d98429e6ae6991a4a61b6c438574fcbef9b3bc7ebafb4b
b4b61cb884 Add tests to increase coverage (Jamil Lambert, PhD)
6bcc6b703d Expand display test to cover debug and OP_0 (Jamil Lambert, PhD)
8a096b77d5 Expand as_ref and as_mut tests (Jamil Lambert, PhD)
afa51f7bfa Add missing empty line, and remove extra one (Jamil Lambert, PhD)
Pull request description:
Expand existing tests and add new ones to increase the test coverage in `primitives/src/script/` to 100% excluding `serde` and `arbitrary` features.
ACKs for top commit:
apoelstra:
ACK b4b61cb8849d5d536532474691c5d9f10c146672; successfully ran local tests
tcharding:
ACK b4b61cb884
Tree-SHA512: 6c37a194e5a94fe04bb26d4e904bd98efc75bd433b59aa09b1c102df96b753602d3ad6540ed9941827f03c7d38e7bbb8b10bbcc8c6ab52b8f96b83743eec4519
e3b059cebf Implement Display for block::Header (Tobin C. Harding)
Pull request description:
Not all the fields within `block::Header` implement `Display` however a block header can reasonably be displayed by using 160 hex characters.
Implement `Display` for `block::Header` by printing the header in hex in the same layout as we hash it in `block_hash`.
Close: #3658
ACKs for top commit:
apoelstra:
ACK e3b059cebfb917c3a876cc5a39d942d2dead475c; successfully ran local tests
Tree-SHA512: c1ff0d73562c9e00e93659f385a471c2d7912e1f09faf6c646bd7864d1a98e6e2baddebfcd1a032bf5adc3e27586022927261c8867098f7e7bf166bf35a9ffbd
Add to the test to cover the `fmt::Debug` impl without checking the
actual text.
Change one of the opcodes so that the display of the special case OP_0
is checked.
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
Not all the fields within `block::Header` implement `Display` however
a block header can reasonably be displayed by using 160 hex characters.
Implement `Display` for `block::Header` by printing the header in hex in
the same layout as we hash it in `block_hash`.
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
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.
merge-script
Tobin C. Harding
Fmt Bot
aagbotemi
Jamil Lambert
Shing Him Ng
Martin Habovstiak
Eval EXEC
jike
AM
Erick Cestari
jrakibi