3e520f9094 Use hex from internals rather than hashes (Martin Habovstiak)
Pull request description:
`bitcoin-internals` contains a more performant implementation of hex encoding than what `bitcoin_hashes` uses internally. This switches the implementations for formatting trait implementations as a step towards moving over completely.
The public macros are also changed to delegate to inner type which is technically a breaking change but we will break the API anyway and the consuers should only call the macro on the actual hash newtypes where the inner types already have the appropriate implementations.
Apart from removing reliance on internal hex from public API this reduces duplicated code generated and compiled. E.g. if you created 10 hash newtypes of SHA256 the formatting implementation would be instantiated 11 times despite being the same.
To do all this some other changes were required to the hex infrastructure. Mainly modifying `put_bytes` to accept iterator (so that `iter().rev()` can be used) and adding a new `DisplayArray` type. The iterator idea was invented by Tobin C. Harding, this commit just adds a bound check and generalizes over `u8` and `&u8` returning iterators.
While it may seem that `DisplayByteSlice` would suffice it'd create and initialize a large array even for small arrays wasting performance. Knowing the exact length `DisplayArray` fixes this.
Another part of refactoring is changing from returning `impl Display` to return `impl LowerHex + UpperHex`. This makes selecting casing less annoying since the consumer no longer needs to import `Case` without cluttering the API with convenience methods.
ACKs for top commit:
tcharding:
ACK 3e520f9094
apoelstra:
ACK 3e520f9094
Tree-SHA512: 62988cec17550ed35990386e572c0d32dc7107e1c36b7c9099080747e15167e6d66497fb300178afbd22481c0360a6b7a1228fd09402d4ce5d295a8594c02aa6
941083ec4e Remove rand-std dev-dependency from secp256k1 (Tobin C. Harding)
f71335f971 Add rand-std feature (Tobin C. Harding)
Pull request description:
This PR uncovered incorrect feature gating in `secp256k1`, fixed in https://github.com/rust-bitcoin/rust-secp256k1/pull/519
Currently we enable "secp256k1/rand-std" in the "rand" feature, this is incorrect because it means "rand" implies "std" which it does not.
Add a "rand-std" feature that turns on "seck256k1/rand-std" and make the "rand" feature turn on "seck256k1/rand".
Fix: #1384
ACKs for top commit:
sanket1729:
ACK 941083ec4e
apoelstra:
ACK 941083ec4e
Tree-SHA512: e1d12196766c2b3082b488fe7d0c03a865ebbfc70ffa6f128c57074df14da71e56c31ea92982991d376ac62fa86c6639049ce17aac93613b523ddcc99677c269
64f7d2549e Fix wrong newtype APIs (Martin Habovstiak)
Pull request description:
This fixes several API bugs:
* Use `TryFrom` instead of `From` for fallible conversions
* Move byte conversion methods from `impl_array_newtype` to `impl_bytes_newtype`
* Add missing trait impls like `AsRef`, `Borrow`, their mutable versions and infallible conversions from arrays
Closes#1336
Notably this change is much less bad than even I expected and some places actually get cleaner than before.
ACKs for top commit:
tcharding:
ACK 64f7d2549e
sanket1729:
code review ACK 64f7d2549e
apoelstra:
ACK 64f7d2549e
Tree-SHA512: 0c5ab19aa1a01ccf34ad1c2f3f66bd003a3142a840cb7cf57e1449e721f6eed1523555c62f0e3391012708767ff86315ff7edce9afd85017a67c61cfe57dddbc
ca471557a5 locktime: Add mutation testing (Tobin C. Harding)
26c0da41b4 locktime: Add inline to public functions (Tobin C. Harding)
Pull request description:
Add mutation testing to the `locktime` module and add unit tests to cover all mutants and ensure they are killed.
ACKs for top commit:
sanket1729:
ACK ca471557a5. How stable are the mutation tests?
apoelstra:
ACK ca471557a5
Tree-SHA512: 46a59c90fc25b0c803e96f7c5b98bd39055f7835e45ba137b2a01ad4221a676c54bc228b9ef7663b7300bb4260a6c2c80a0820c4f1bf0987650e1e2bd699f62d
cf9733d678 Verify and fix mul_u64 (Tobin C. Harding)
Pull request description:
Add kani verification for `U256::mul_u64`, doing so uncovered a bug in the current implementation due to overflow. Re-write the `mul_u64` method.
Fix: #1497
## Note
This PR now _only_ tests `mul_u64`, I will add testing div as a separate PR.
ACKs for top commit:
Kixunil:
ACK cf9733d678
apoelstra:
ACK cf9733d678
Tree-SHA512: c044fb385073bb068412195c079f26057ba560b2a7eca22693c360d324a7c0119cf10da2e70e544c69e05ca8269a81156418d307f4e11d8a4c1c1a7cafa32d0c
8aa5b7f081 Document test frameworks (Tobin C. Harding)
Pull request description:
Recently we started using various test frameworks; add documentation to the readme for running the various tests we now support (mutagen, kani, etc.)
### Note
This was on a different PR originally, pushing it up on its own.
ACKs for top commit:
Kixunil:
ACK 8aa5b7f081
apoelstra:
ACK 8aa5b7f081
Tree-SHA512: 13134ba2f6806a705f99af5b8d66b051e1e58da177a02ee46880f494e37c380fc4c28731cd42eabbd69ae884763dbc360902e0e8afa7f88e78483e8a37f614f5
56e4e53357 hashes: ci: Remove --all (Tobin C. Harding)
Pull request description:
Currently we are using the `--all` flag in `cargo` commands in the `hashes` CI script. This flag (the deprecated version of `--workspace`) causes cargo to run the command for the whole workspace, this is not what we want because we run test individually for each crate using a ci script per crate.
The effect of this patch is to reduce re-runs of tests i.e., reduce machine usage during CI runs with no reduction of coverage - PROFIT!
ACKs for top commit:
apoelstra:
ACK 56e4e53357
sanket1729:
utACK 56e4e53357
Tree-SHA512: 13134ba2f6806a705f99af5b8d66b051e1e58da177a02ee46880f494e37c380fc4c28731cd42eabbd69ae884763dbc360902e0e8afa7f88e78483e8a37f614f5
3372333865 Add a kani badge to the README (Tobin C. Harding)
3d2a62fdd5 Run kani daily on a schedule (Tobin C. Harding)
Pull request description:
Running kani takes ages, instead of running it on every pull request we can just run it daily.
ACKs for top commit:
sanket1729:
ACK 3372333865
apoelstra:
ACK 3372333865
Tree-SHA512: 63f71155eb3f2dd9bfbc3733c407c80b59a019d356127efc6d65cf53b517f15ddd8afd92d89f968734a508882eabbf720757d95c04d688438b762bb55a22f601
5a2a37d4be Allow dead_code/unused_imports when fuzzing (Tobin C. Harding)
Pull request description:
Littering the codebase with `#[cfg(not(fuzzing))]` is a bit messy just to quieten the linter during fuzzing. Instead just globally allow.
Done while debugging #1409
ACKs for top commit:
sanket1729:
ACK 5a2a37d4be
apoelstra:
ACK 5a2a37d4be
Tree-SHA512: fb84215a2b00ad6d3321b2781ba285af513ff8fd413c0997045a41c4f23028d2ef0fdf083839289d0c5108c990aa66bdae4430ad3ef32881eac5324b2e881b3b
4201612837 Use dtonlnay instead of actions-rs (Tobin C. Harding)
Pull request description:
Done on top of #1508
Currently we use the `actions-rs` GitHub action to run our tests. It seems the project is now unmaintained [0].
Well known Rust developer dtonlnay maintains a GitHub action that can be used instead.
Replace all uses of `actions-rs/toolchain` with `dtonlnay/rust-toolchain`. Note that with the new action there is no way to configure the toolchain, instead a different `uses` statement is required - this means we have to split our jobs up by toolchain. This is arguably cleaner anyways.
Note that with this patch applied the "no-std" tests are now _not_ run for MSRV since we explicitly support "no-std" only for the 1.47 and above toolchains - strange that this was working?
[0] https://github.com/actions-rs/toolchain/issues/216
ACKs for top commit:
sanket1729:
ACK 4201612837. Verified that we did not miss any checks in the translation.
elichai:
ACK 4201612837
Tree-SHA512: 117b35953c7e0d93ff1ea76fbff948d9a50aff9b3d0854beced540321f84eb83510af23067ff2ebc29b8d9c59b3ca205beeecde6e968bc619e21430b951f02cb
0aef1576fa Use cargo install cross `--locked` (Tobin C. Harding)
Pull request description:
`cross` currently fails to install, this has been reported already
https://github.com/cross-rs/cross/issues/1177
The workaround is to use `cargo install --locked`.
ACKs for top commit:
Kixunil:
ACK 0aef1576fa
apoelstra:
ACK 0aef1576fa
Tree-SHA512: 9c42cfa9d205df2c38aae651c104e5483ad7ac7098755040f1f12368f046375b824e663fadaaae11ca31d368700eea9a1783a02cb91cbd15a5a3cee5311cc2fe
e0bc50953a Make `Witness::tapscript()` return `Script` instead of raw bytes (Jiri Jakes)
Pull request description:
Since there is unsized `Script` now, this method can return it.
ACKs for top commit:
sanket1729:
utACK e0bc50953a
tcharding:
ACK e0bc50953a
Tree-SHA512: 32d4ca14f1b0fc1029f7376b1a43db90332b869a806609c82f660cb2690a4f0e1b91e1799fdac0d43c8a630aed0331f251d4159a662e86e5942c6fb698c42cd2
Add `#[inline]` to all public functions/methods excluding error types
and `Display` impls. Error paths do not need to be fast and presumably
`Display` is called on code paths that do IO so this also does not need
to be fast.
Currently we use the `actions-rs` GitHub action to run our tests. It
seems the project is now unmaintained [0].
Well known Rust developer dtonlnay maintains a GitHub action that can be
used instead.
Replace all uses of `actions-rs/toolchain` with
`dtonlnay/rust-toolchain`. Note that with the new action there is no way
to configure the toolchain, instead a different `uses` statement is
required - this means we have to split our jobs up by toolchain. This is
arguably cleaner anyways.
Note that with this patch applied the "no-std" tests are now _not_ run
for MSRV since we explicitly support "no-std" only for the 1.47 and
above toolchains - strange that this was working?
[0] https://github.com/actions-rs/toolchain/issues/216
Add kani verification for `U256::mul_u64`, doing so uncovered a bug in
the current implementation due to overflow.
Re-write the `mul_u64` method.
Props to Elichai for the algorithm.
Co-authored-by: Elichai Turkel <elichai.turkel@gmail.com>
920599da94 Add test for previous commit (Martin Habovstiak)
a7f3458c27 Fix bug in `ScriptBuf::extend` for short iterators (Martin Habovstiak)
Pull request description:
`ScriptBuf::extend` contained an optimization for short scripts that was
supposed to preallocate the buffer and then fill it. By mistake it
attempted to fill it from already-exhausted iterator instead of the
temporary array it drained the items into. This obviously produced
garbage (empty) values.
This was not caught by tests because the optimization is only active for
scripts with known maximum length and the test used `Instructions` which
doesn't know the maximum length.
ACKs for top commit:
sanket1729:
ACK 920599da94 . Tested that the bug is correctly fixed and tested in the new test
tcharding:
ACK 920599da94
Tree-SHA512: a80f5f262a840d8e77efd42d63c511224380ee3efa6c31855233e81c90332ac15db228e8d552d039d729d7b642e03c3939c8b6a92d3279001377515acb83abea
8ce928b8e7 Add testing section to readme (Tobin C. Harding)
2e79a0bdc4 Introduce mutation testing (Tobin C. Harding)
Pull request description:
Introduce mutation testing by way of mutagen [0] (see #1484 for context).
- Conditionally add the dev-dependency `mutagen` (using `do_mutate` flag)
This flag is not very well named but `mutagen` and `mutate` are already taken?
- Mutate all methods of the `U256` struct that do not require additional unit tests.
Uses `cfg(all(test, do_mutate), mutate)` - I cannot workout why we need to check on `test` as well i.e., I don't understand why we cannot use `cfg(do_mutate, mutate)`?
With this applied test can be run as usual with a stable toolchain. To run mutagen we use `RUSTFLAGS='--cfg=do_mutate' cargo +nightly mutagen` (doing so runs 29 mutants).
[0] https://github.com/llogiq/mutagen
ACKs for top commit:
Kixunil:
ACK 8ce928b8e7
apoelstra:
ACK 8ce928b8e7
Tree-SHA512: 024ba5d2dc983f7cd0444e09ba13280771157204999d2a44502e07a1d6436f571b75003c7cb543c943f17949b848d4070eda4e194bda774a3e41443ff79af0af
We now have a few different test harnesses in use, add a section to the
readme about each
- normal unit/integration tests
- benchmarks
- kani
- mutagen
e428486002 Add `from_bytes(Vec<u8>)` to `ScriptBuf` (Martin Habovstiak)
Pull request description:
This is useful when one already has bytes allocated in a vec that can be reused.
The change also documents that the mirror method `into_bytes()` doesn't allocate.
ACKs for top commit:
tcharding:
ACK e428486002
sanket1729:
ACK e428486002
Tree-SHA512: 257b9c2cae24fd5006b6ec0e4db4a8baac177e67308758fb6167ef90e79ce75d5553433557d0e887a3e18a8fc63036f5a2acd061528a4467f76f25b6f4852400
This fixes several API bugs:
* Use `TryFrom` instead of `From` for fallible conversions
* Move byte conversion methods from `impl_array_newtype` to
`impl_bytes_newtype`
* Add missing trait impls like `AsRef`, `Borrow`, their mutable versions
and infallible conversions from arrays
Closes#1336
`ScriptBuf::extend` contained an optimization for short scripts that was
supposed to preallocate the buffer and then fill it. By mistake it
attempted to fill it from already-exhausted iterator instead of the
temporary array it drained the items into. This obviously produced
garbage (empty) values.
This was not caught by tests because the optimization is only active for
scripts with known maximum length and the test used `Instructions` which
doesn't know the maximum lenght.
In order to get better test coverage we should not enable the secp26k1
feature "rand-std" in dev-dependencies but instead feature gate tests
that depend on this feature.
Currently we enable "secp256k1/rand-std" in the "rand" feature, this is
incorrect because it means "rand" implies "std" which it does not.
Add a "rand-std" feature that turns on "seck256k1/rand-std" and make the
"rand" feature turn on "seck256k1/rand".
Introduce mutation testing by way of mutagen [0]
- Conditionally add the dev-dependency `mutagen` (using `do_mutate`
flag)
This flag is not very well named but `mutagen` and `mutate` are already
taken?
- Mutate all methods of the `U256` struct that do not require additional
unit tests.
Uses `cfg(all(test, do_mutate), mutate)` - I cannot workout why we need
to check on `test` as well i.e., I don't understand why we cannot use
`cfg(do_mutate, mutate)`?
With this applied test can be run as usual with a stable toolchain. To
run mutagen we use `RUSTFLAGS='--cfg=do_mutate' cargo +nightly mutagen`.
[0] https://github.com/llogiq/mutagen
This is useful when one already has bytes allocated in a vec that can be
reused.
The change also documents that the mirror method `into_bytes()` doesn't
allocate.
`bitcoin-internals` contains a more performant implementation of hex
encoding than what `bitcoin_hashes` uses internally. This switches the
implementations for formatting trait implementations as a step towards
moving over completely.
The public macros are also changed to delegate to inner type which is
technically a breaking change but we will break the API anyway and the
consuers should only call the macro on the actual hash newtypes where
the inner types already have the appropriate implementations.
Apart from removing reliance on internal hex from public API this
reduces duplicated code generated and compiled. E.g. if you created 10
hash newtypes of SHA256 the formatting implementation would be
instantiated 11 times despite being the same.
To do all this some other changes were required to the hex
infrastructure. Mainly modifying `put_bytes` to accept iterator (so that
`iter().rev()` can be used) and adding a new `DisplayArray` type. The
iterator idea was invented by Tobin C. Harding, this commit just adds a
bound check and generalizes over `u8` and `&u8` returning iterators.
While it may seem that `DisplayByteSlice` would suffice it'd create and
initialize a large array even for small arrays wasting performance.
Knowing the exact length `DisplayArray` fixes this.
Another part of refactoring is changing from returning `impl Display` to
return `impl LowerHex + UpperHex`. This makes selecting casing less
annoying since the consumer no longer needs to import `Case` without
cluttering the API with convenience methods.
bae264d0c2 Add `tapscript_leaf_hash()` to `Script` (Jiri Jakes)
Pull request description:
Adds convenience method to `Script` for computing leaf hash of tapscript. Closes#1482.
The little test case is taken from `bip341_tests.json`.
ACKs for top commit:
Kixunil:
ACK bae264d0c2
apoelstra:
ACK bae264d0c2
Tree-SHA512: fb7a3a552017208decd56ca7d27eab1987a3a92aae5b8620896b3a02986c2fc13043c200ccfbadf9cfdd2d74af38b0bc25936338f55b7d318c1296acc88bf22a
6acf9ac8b8 Patch hashes and update the code (Martin Habovstiak)
Pull request description:
This patches `bitcoin_hashes` to use the version in the repository and fixes the code after removal of `Deref`.
ACKs for top commit:
tcharding:
ACK 6acf9ac8b8
apoelstra:
ACK 6acf9ac8b8
Tree-SHA512: b779fa79309f1d648020146b58e96346b67e9f76e29551cbd50251ea6bb7bcfc4c5d42f49cc7ad5660dcd0a5f6306efe96dfcd9530e4b32c62edf4af7076d830
67ca3463c0 Mention Script::is_v1_p2tr above Witness::tapscript (Casey Rodarmor)
Pull request description:
It seems useful to document that this check is also provided.
ACKs for top commit:
tcharding:
ACK 67ca3463c0
Kixunil:
ACK 67ca3463c0
apoelstra:
ACK 67ca3463c0
Tree-SHA512: b36ff89cd7bb4ffe48e1bc4fcbfe35116492f5b0f9fbce271abd83f266fdcb25f7faa459acc35f944c4bdfa2626c00c194400a11f32ad84a323a07bfa5ec1b0a
This patches `bitcoin_hashes` to use the version in the repository and
fixes the code after removal of `Deref`.
This also turns off `AS_DEPENDENCY` check with the intention to refactor
it later.
02c1cd6291 add some documentation clarifying the locktime ordering shenanigans in #1330 (Andrew Poelstra)
Pull request description:
Updates the CHANGELOG and also the doccomment on `Transaction`.
ACKs for top commit:
tcharding:
ACK 02c1cd6291
Kixunil:
ACK 02c1cd6291
sanket1729:
ACK 02c1cd6291
Tree-SHA512: e2d23a90fb1e53758449fe49a3db7ae1497a260ce7efcade4b50265fa70840db273609019590d9d0a69e1272607a6bcf37924b805b4f09909487eb0c3b91a3cd
b78ba730f2 hashes: Run clippy in ci (Tobin C. Harding)
5e67f7a7cb Remove the unnecessary explicit reference (Tobin C. Harding)
Pull request description:
Currently we only run the linter in `bitcoin/contrib/test.sh`, we should do the same in the `hashes` ci script.
- Patch 1: Fix current clippy issues in `hashes` crate
- Patch 2: Run clippy in CI for `hashes` crate
ACKs for top commit:
apoelstra:
ACK b78ba730f2
Kixunil:
ACK b78ba730f2
Tree-SHA512: f9fedcd8c1a06c715396cf6c7b752e2c9e32dbfde48c8b4bcb9ac5e701abe109ddeadc2e7466f6926f7c3ab74fa26e68b70473b4a5b5009cb4644d634707d4ea
8e428562cb Implemented unsized `Script` (Martin Habovstiak)
Pull request description:
This renames `Script` to `ScriptBuf` and adds unsized `Script` modeled
after `PathBuf`/`Path`. The change cleans up the API a bit, especially
all functions that previously accepted `&Script` now accept truly
borrowed version. Some functions that perviously accepted `&[u8]` can
now accept `&Script` because constructing it is no loger costly.
This is a more idiomatic alternative to #884 with two unavoidable lines of `unsafe` copied from `std`.
Closes#522Closes#949
(For 949, we allow users to use whichever they like but still use `ScriptBuf` in `Transaction`.)
ACKs for top commit:
apoelstra:
ACK 8e428562cb
tcharding:
ACK 8e428562cb
sanket1729:
ACK 8e428562cb. Let's get this in
Tree-SHA512: 2ff7f0b3abd1999261388b7c5075aa1caa17bdaf4538b443bd098c31565d9dc8423ae3f9a9b3cd2f0aee6a01591992dbe8c9592f9cf14ec0f7cc395f696b9a66
This renames `Script` to `ScriptBuf` and adds unsized `Script` modeled
after `PathBuf`/`Path`. The change cleans up the API a bit, especially
all functions that previously accepted `&Script` now accept truly
borrowed version. Some functions that perviously accepted `&[u8]` can
now accept `&Script` because constructing it is no loger costly.
Andrew Poelstra
sanket1729
Tobin C. Harding
Jiri Jakes
Martin Habovstiak
Byron Hambly
Daniela Brozzoni
Casey Rodarmor