Currently we have a dependency on `hex_lit` and it is used in exactly
one place outside of test code, if we instead use a hardcoded array
instead we can move the `hex_lit` dependency to `dev-dependencies`.
Hard code the genesis block script bytes as an array of hex digits, link
to the blockstream explorer for those interested and comment the bytes
liberally since it took me a while to work out what they were.
Move the `hex_lit` dependency and update the lock files.
We would like to move the dependency on `hex_lit` to be a
dev-dependency but currently are using it in bench code. The bench
code is enabled if any downstream crate tries to build with
`--cfg=bench` and during such a build our dev-dependencies are not
available.
We also have the `test_hex_unwrap` macro in the `hex` crate and since
the bench code is more or less test code (and the macro call is not
being benchmarked) we can use that macro instead.
76331aeee3 Add a just cmd to check for API changes (Tobin C. Harding)
b222f40f99 CI: Add job to check for API changes (Tobin C. Harding)
9e7cd97c25 Add a script to check the public API (Tobin C. Harding)
Pull request description:
This PR is #1880 re-opened.
Add a script that checks the public API of `hashes` and `bitcoin`. Document how to use it during development. Call it in CI. Do not add it to githooks because the githooks because its expected to be run per PR not per commit.
Includes a `just` command to run the script: `just check-api`
### Implied workflow change
This PR imposes workflow changes.
Explicitly: all PRs that change the public API of `bitcoin`, `base58`, `hashes`, `io`, or `units` must contain changes to the api text files.
Suggestion: We add the patch updating api text files as a separate patch at the end of each PR so we can haggle over it separately from the actual code changes.
Fix: #1875
ACKs for top commit:
apoelstra:
ACK 76331aeee3 normally would complain about the whitespace but I would like to ACK/merge this quickly since most other PR that gets merged will force it to be rebased. also will one-ACK merge as "no NACKs or comments from maintainers in 2 weeks"
Tree-SHA512: 67b20e2ce0c22aa67be931c4da0b591bc351ccb1aa620003c60bfb4b10d5c292edceca929bf6318993f2d16f9f58374aac336adf0f8234c5e2f16e3857b7901b
c934d03fcf p2p: Cleanup test imports (Tobin C. Harding)
Pull request description:
Clean up the test imports in the `p2p` module:
- Use `use super::*` as is conventional.
- Use `sha256d::Hash` as is conventional.
Refactor, no logic changes.
ACKs for top commit:
apoelstra:
ACK c934d03fcf
Tree-SHA512: 35538f35706df8982625f2e1764bc66eea9636fc9073ebf61476097e7ad5f45288c1450244e45ddb5bac56f342fe48c31e88456698e8fd93367c28d3bbc37f2e
We would like to check for API changes during development and in CI so
that such changes can be discussed separately from the code.
Add tooling and documentation for creating API listings for the public
crates within the workspace (i.e., not `internals`).
Add API text files from an initial run of the script.
Clean up the test imports in the `p2p` module:
- Use `use super::*` as is conventional.
- Use `sha256d::Hash` as is conventional.
Refactor, no logic changes.
6d0d0fe51f Fix rustdoc header format (jamil.lambert)
Pull request description:
By convention rustdoc headers should not include a colon.
Removed colon from rustdoc headers.
ACKs for top commit:
apoelstra:
ACK 6d0d0fe51f
Tree-SHA512: 36d5822e80cc86139ce07a1c86681998e1d27549421b9b50589fe4a9a5c860ed47f145d5af9501957f199d0f77df069c2b295d43ed4a15e9b6d98c0edc12abd0
802af8e417 Removed //! spare line at end of headers (jamil.lambert)
Pull request description:
Some of the headers had a //! at the end but most didn't. They have all been removed in bitcoin/src/ to make the files consistent
ACKs for top commit:
apoelstra:
ACK 802af8e417
Tree-SHA512: a1eb0dda76af68cb96352f6b31231fa5391d49e11df924065e76871f82231ec0d5751190663f142240e5d757975937387243d1fdac3684d9bdbd7e2362dbd0a7
f3c80ea820 Use concrete type for all_zeros call (Tobin C. Harding)
Pull request description:
Currently we use the `Hash` trait in a bunch of places to call `all_zeros`. We are attempting to improve the `hashes` API and this usage is both unnecessary and also hindering that effort.
Use the concrete type (e.g. `BlockHash`) instead of calling through the trait method.
Refactor only, no logic changes.
ACKs for top commit:
apoelstra:
ACK f3c80ea820 I contend that this meets one-ACK carve-out #4 "code moves that do not change the API"
Tree-SHA512: a8d7ba48cf6816b722d626ed0a9a7ccfeee2dff19ef689c78661e9afff1f9053a53752562c70c201e33f8e979a2ea9d14660b36d3f732c0f37c327a062514919
Currently we use the `Hash` trait in a bunch of places to call
`all_zeros`. We are attempting to improve the `hashes` API and this
usage is both unnecessary and also hindering that effort.
Use the concrete type (e.g. `BlockHash`) instead of calling through the
trait method.
Refactor only, no logic changes.
47dc4a3180 feat(pow): add difficulty adjustment calculation (Rob N)
Pull request description:
Hi, I hit a roadblock with the current `pow` API. As far as I can tell, the only workaround to calculate the next work required similar to `bitcoin/src/pow.cpp` is to use a general big integer library, convert the `Target` to bytes, do the math, and convert back to `Target` from bytes. I have also been working with [Floresta](780ea8d0b0) and their [solution](780ea8d0b0/crates/floresta-chain/src/pruned_utreexo/consensus.rs (L187)) was to fork off and exposed the `U256` struct publicly on their branch. I think these home brewed difficulty adjustment solutions will continually pop up, so I created a `from_next_work_required` method to return a `Target`. My work veers significantly from #2180, as I only provided a single method to do so, without further guidance on when exactly this retarget occurs.
I am happy to add tests once I get further direction from maintainers if this as a likelihood of being accepted or not. Thanks.
ACKs for top commit:
tcharding:
ACK 47dc4a3180
apoelstra:
ACK 47dc4a3180 used range-diff
Tree-SHA512: 6d627ce698361afed61c8f2a12a1a96371a7a93118e08a91dae250de4f23d65c615d2654d37d2699c88b7c22f6e4bc2a1195f963c15512d7c0d041498f02dc41
46e0ce59a3 bitcoin: Set version number (Tobin C. Harding)
Pull request description:
During the recent release cycle we left `bitcoin` on the last rc version.
Set the version number to `v0.33.0-unreleased` to make it obvious what it is.
Close: #2724
ACKs for top commit:
apoelstra:
ACK 46e0ce59a3
sanket1729:
ACK 46e0ce59a3
Tree-SHA512: 8a1fa062cf0240c5a7e1c05f0378097464423c29827f14068d01ac562d1be571bec2d267a5a12d0828c5a2809ef743f03ef29ccea9a31a5c9e3eefce66b3d30d
d353be4546 bip32: derive_xpriv should not return a Result (Jose Storopoli)
Pull request description:
We discussed in #2752 that `derive_priv` never fails.
This PR addresses that issue.
ACKs for top commit:
apoelstra:
ACK d353be4546
tcharding:
ACK d353be4546
sanket1729:
ACK d353be4546
Tree-SHA512: 3a3d09027c6079581636b51a506fac2b325e592d182167c0a3d8676b41fb2bef59a85a404ad12b2e14d73e58fd4b6d8f5923a3dc76a4b9724033097fc08b36ac
021bea89bb ci: shellcheck checks (Jose Storopoli)
Pull request description:
Closes#2739.
I am proposing that we use this GitHub Shellcheck action:
[`ludeeus/action-shellcheck`](https://github.com/ludeeus/action-shellcheck)
since it has most stars (and eyes on it).
I also did all fixes that I could find with
```bash
shellcheck **/*.sh
```
If I've missed any please let me know.
ACKs for top commit:
apoelstra:
ACK 021bea89bb
tcharding:
ACK 021bea89bb
Tree-SHA512: 67e37da9ae3ea0c5551af57b928016a2d9e76761af5558b3057ac47e773189629dd20eea9e659b4323c8568fb48dcdbe9ebd5c730f2c6266fb0db52886c9835f
4646690521 fix clippy lint by using resize instead of push (Riccardo Casatta)
deeb160b86 remove SmallVec (Riccardo Casatta)
e4b707ba83 add bench for base58::encode_check (Riccardo Casatta)
Pull request description:
In a downstream app I've seen printing a descriptor is not a cheap operation, analyzing the flamegraph it seems base58 encoding of the xpub is the culprit
This PR adds benches for the `encode_check` function, and add the changes gaining more boost, which is also good cause it removes code.
Other attempts didn't provide enough benefit for inclusion but I report them here for knowledge.
```
## baseline
running 2 tests
test benches::bench_encode_check_50 ... bench: 8,760 ns/iter (+/- 113)
test benches::bench_encode_check_xpub ... bench: 19,982 ns/iter (+/- 109)
## remove smallvec
running 2 tests
test benches::bench_encode_check_50 ... bench: 7,935 ns/iter (+/- 129)
test benches::bench_encode_check_xpub ... bench: 18,076 ns/iter (+/- 184)
## increase smallvec to 128 (fits xpub)
test benches::bench_encode_check_50 ... bench: 8,786 ns/iter (+/- 738)
test benches::bench_encode_check_xpub ... bench: 20,006 ns/iter (+/- 2,611)
## avoid char-to-str by keeping str map
test benches::bench_encode_check_50 ... bench: 7,895 ns/iter (+/- 88)
test benches::bench_encode_check_xpub ... bench: 17,883 ns/iter (+/- 118)
```
Gains are good (~10%), but I don't think they explains the 3ms to print a descriptor in wasm env,
probably is the sha256 for the checksum is fast in cargo bench but slow in wasm env, but I didn't research on the topic.
ACKs for top commit:
apoelstra:
ACK 4646690521
tcharding:
ACK 4646690521
Tree-SHA512: 978bbe22c99bb455028d90532d59a076321e0c19105fc8335bd44cd84fbedda109083380df5c658b85121242c88d442994cfc58d141f3fc5daa66c27b1499329
6def5bc974 CI: Use run_task from maintainer tools (Tobin C. Harding)
c5af52847b CI: Add docs and document start (Tobin C. Harding)
62ba10503a CI: Use correct spacing (Tobin C. Harding)
0c0e88165e CI: Add README file (Tobin C. Harding)
44cb2255d3 CI: Add sanitizer script (Tobin C. Harding)
3407257936 CI: Add WASM script (Tobin C. Harding)
cc14edf63f CI: Run the schemars job directly using cargo (Tobin C. Harding)
1fb12e1917 CI: Remove recent from schemars job (Tobin C. Harding)
8d7117bb0e CI: Use original name (Tobin C. Harding)
Pull request description:
First we do some clean up, then we pull the stuff that is specific to this repo out into separate tests, then as the last patch we switch over to use the new script for `rust-bitcoin-maintainer-tools` that was introduced in https://github.com/rust-bitcoin/rust-bitcoin-maintainer-tools/pull/4.
ACKs for top commit:
apoelstra:
ACK 6def5bc974
Tree-SHA512: 70105d455294d49deb43461958435096d074d19ea8d9adc7036e15d74dfdab466f04b1d9e934574077ca0c32a6fe77d0e5aa11068d8c4d51acef634591227d33
Add a document start and comment to help try to stop the readme from
going stale.
This removes a `yamllint` warning.
While we are clearing lint warnings disable the one causing
warning truthy value should be one of [false, true] (truthy)
This is a know issue with GitHub actions yaml because `on` is a yaml
keyword, or something like that.
As we did for the wasm job.
In preparation for using the `run_task` script from maintainer tools we
want to have all the things that are particular to `rust-bitcoin` out of
the current `run_task` script.
The address/memory sanitizer test is specific to `hashes`. Add a script
in `hashes/contrib` and call it from the ASAN job.
No test coverage change.
In preparation for using the `run_task` script from maintainer tools we
want to have all the things that are particular to `rust-bitcoin` out of
the current `run_task` script.
The wasm test is specific to `hashes`. Add a script in `hashes/contrib`
and call it from the wasm job.
No test coverage change.
I've been a bit confused lately about pinning and the locks, at some
stage recently I changed 'Set dependencies' to 'Copy lock file'. Its no
biggy but the original that Kix wrote was correct and descriptive - it
was me who was confused. Revert it back to the original.
7685461e62 Document the sha256t_hash_newtype direction (Tobin C. Harding)
30e91cc766 Default to forward for tagged hashes (Tobin C. Harding)
5ecc69cd28 Add forward/backward unit test (Tobin C. Harding)
9aee65d1ba Refactor tagged hash tests (Tobin C. Harding)
216422dffc Remove schemars impl for test type (Tobin C. Harding)
Pull request description:
First three patches are preparation, improvements to the units tests in `sha256t`.
From the final patch:
Displaying backward is an anomaly of Bitcoin Core's early days and the
double SHA256 hash type. We should not let this unfortunate beast leak
out into other places.
Default to displaying forward when creating a new tagged hash and remove
all the explicit attributes from `bitcoin` that just clutter the code.
This is an API break and may quietly break some users downstream - eventually we should stop doing that sort of thing.
ACKs for top commit:
apoelstra:
ACK 7685461e62
Tree-SHA512: cb8a41b207aa68ecf63cb7af7f39f7d7c8a3a27f38595867949b288a81a20bff0c17aa4c17bb099e2ecf85194d83bad23c9c9792f511b6c4cd625ff27c1affaa
Since the default display direction is now forward, use
`#[hash_newtype(backward)]`
in the rustdocs on the macro. Also add an example usage to the changelog
in case someone downstream is relying on the old default behaviour of
displaying backwards (unlikely).
d094350230 hashes: Modify trait bounds (Tobin C. Harding)
Pull request description:
Currently we require indexing trait bounds as well as `Borrow` on the `Hash` trait. We also already implement `AsRef`.
It was observed that `Borrow<[u8]>` does not best describe what we want from the `Hash` trait implementor but rather `AsRef<[u8]>` does.
Remove all the inexing trait bounds. Remove the `borrow::Borrow<[u8]>` trait bound. Add a `convert::AsRef<[u8]>` trait bound.
This leaves the `Borrow<[u8]>` implementation for hashes created with `hash_newtype`, I'm not sure if this should be removed or not.
ACKs for top commit:
apoelstra:
ACK d094350230 thanks! FWIW I think we might want to return the indexing traits one day, at least `[..]`, but we can do that post-1.0 and we have not gotten any complaints after removing them from the non-HMAC hashes, so maybe people are good with it as is.
Tree-SHA512: 2704a7e850083e4eae5844c401c6ac6fe32e0939bbe6e0fb5e21faf4745f9c98634e210f522199ceacd663be60685d7f30df6c89a7787ca11ea8294d104d813e
cf19e37785 CI: Update nightly semi-weekly (Tobin C. Harding)
Pull request description:
Updating the nightly toolchain every day is overly arduous, lets just update it twice a week.
ACKs for top commit:
apoelstra:
ACK cf19e37785 CI-only, will one-ACK merge
Tree-SHA512: c827a1805efea43b945c254775cde7d3e064ccebbcca7cf0d9ca92447e54a43ec0f41d324b3e39d70ff060620ef005e867578461d1eb4b69996d2416f69f6a05
Currently we require indexing trait bounds as well as `Borrow` on the
`Hash` trait. We also already implement `AsRef`.
It was observed that `Borrow<[u8]>` does not best describe what we want
from the `Hash` trait implementor but rather `AsRef<[u8]>` does.
Remove all the inexing trait bounds. Remove the `borrow::Borrow<[u8]>`
trait bound. Add a `convert::AsRef<[u8]>` trait bound.
This leaves the `Borrow<[u8]>` implementation for hashes created with
`hash_newtype`, I'm not sure if this should be removed or not.
b705ac62e6 Automated update to Github CI to rustc nightly-2024-05-04 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK b705ac62e6
Tree-SHA512: e43188c0ba5d5b38c7bdb192e5ade7f612e1867bb9825676412237200b54d4d4417c4f306339b745e1af08907d9f8a377bddabe52e6bce8af0925149506e6fa5
0a5d8b07a8 Automated update to Github CI to rustc nightly-2024-05-03 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK 0a5d8b07a8
Tree-SHA512: 98d9cfb0cffe94b78a5939e0967d73f3c5ab382527dd66bc4f4823d8327d1afe6bb2d779c467ed9e815bcf25d7ca7940d9799c189b355cc642db000e633ceb1f
Tobin C. Harding
Andrew Poelstra
jamil.lambert
Rob N
Jose Storopoli
Fmt Bot
Riccardo Casatta