4446be6fc8 hashes: Add regression tests (Tobin C. Harding)
Pull request description:
We have regression tests spread out throughout the `hashes` module but they are not labelled as such. To give us more confidence and help debug when patching the `hashes` crate we can add a bunch of regression tests in a single place.
Add a module that does a single regression test for each type, simply hash some arbitrary data and check the hex display against a hard coded hex string.
ACKs for top commit:
apoelstra:
ACK 4446be6fc8 nice :)
Tree-SHA512: db643fdf799d23735934ad966ee66b40b3adb72db8f777112005f94d61ab4e382399a66e48d9fc9fdb0cd0abe17b3d4087fe6e6c494836d0c6fc713f4efc6413
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.
c8caee2b5e Document CompactTarget order/equality (Tobin C. Harding)
Pull request description:
Add documentation to the `CompactTarget` type explaining the nuance surrounding order/equality.
Close: #2110
ACKs for top commit:
apoelstra:
ACK c8caee2b5e
Tree-SHA512: c724b31ee620ff08d3c8b547250bc7067f875ef6cf4ce9efa082d5a9cfbd8b92620f86034e58caf573c479ce7aaa89bb7e9fa93dc356524663d3ecf583df3507
101378c4d0 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 units/src/ to make the files consistent
ACKs for top commit:
apoelstra:
ACK 101378c4d0
Tree-SHA512: 7f6e343ae48e3a5fe82f24c3b9cfe843b9c52458e164cd77e4d3cd3e455bc6636986305270b970b909c59bd241bc379f68912bb9a4b79b07b0638c193c1c8109
6c1a5401a7 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 internals/src/ to make the files consistent
ACKs for top commit:
apoelstra:
ACK 6c1a5401a7
Tree-SHA512: be86b35dc8cf0083dddfefde5b4d1195409523a48f0388285ed049fab8880dd7fbd570d5b15e4f5ed592fa336695130669c5041856da8a7314349f9200a6c66c
52bea9f6a4 Removed //! spare line at end of headers (jamil.lambert)
Pull request description:
Some of the headers had a //! spare line at the end but most didn't. They have all been removed in hashes/src/ to make the files consistent
ACKs for top commit:
apoelstra:
ACK 52bea9f6a4
Tree-SHA512: 66f9e89e1b11c4688b9d7b19e8ffb329f49aa5b32765b73540f90e12f1dd329de6307e8bdedcb9dffa527817dccbb46fa75912b7cad11bc0ab06a95632728f98
In a few cases a function header documents the parameters of the following function under the heading"Arguments", this has been changed to "Parameters"
We have regression tests spread out throughout the `hashes` module but
they are not labelled as such. To give us more confidence and help
debug when patching the `hashes` crate we can add a bunch of regression
tests in a single place.
Add a module that does a single regression test for each type, simply
hash some arbitrary data and check the hex display against a hard coded
hex string.
98d6ab3865 policy: Mention format of error variants (Tobin C. Harding)
Pull request description:
An error enum type should not use an `Error` prefix for its variants, mention as such in the policy docs.
ACKs for top commit:
apoelstra:
ACK 98d6ab3865
Tree-SHA512: 16375ff031af24e7b1a5e8206c9660512b26da86df4168ac4a18e48f118c455460d0fee78e1e1ce972a32daba9aff7fcc9d0043bfd21bae223f95d9e944330ca
We have a private function that makes use of the `Hash` trait to
generically hash map entries. This usage makes patching the `hashes`
module difficult. We can achieve the same thing by using a macro and
passing in the concrete type.
This is an internal change, no effect on logic or public API.
f6129317bd Run the formatter (Tobin C. Harding)
fa4d3d4417 Add whitespace (Tobin C. Harding)
Pull request description:
#2781 done by a human.
ACKs for top commit:
apoelstra:
ACK f6129317bd sad
Tree-SHA512: a72b507995a70ab2b5c2bb717d8bbc4b47f3190157e5d526d33e6a9fd2ad990295806a36e0bda0ac988b1e175d8356c6e6d277337d0ed0a5e0499ad3f336a930
61b428b2d9 CI: Fix workflow quotes (Tobin C. Harding)
Pull request description:
During the last month of CI changes a few anomalies have snuck into the main workflow file.
Make quotes uniform.
ACKs for top commit:
storopoli:
ACK 61b428b2d9
apoelstra:
ACK 61b428b2d9
Tree-SHA512: ec2c296aa0d2d4fe5d8b2747bf5812b27784b632241ccf889dde0d03fcc2e72fc7d2fb8614e6dbdbec49c0b81db9807933928f27d23b5eb11f6ba71b28b5c29e
7de02da9f5 policy: Introduce error type re-export policy (Tobin C. Harding)
Pull request description:
We have started introducing `error` sub-modules for modules that have a lot of error code. This is mainly a code organisation thing.
When importing a function it is annoying to have to go to another module to get the returned error type. Furthermore it leaks our module structure and introduces a maintenance burden because for modules that do not [yet] have an `error` sub-module the error types are in a different place to modules that do have an `error` sub-module.
Furthermore, if function users have to go to the `error` sub-module to read docs etc. they are bombarded with all the hider errors as well (see definitions below).
We can solve both problems by re-exporting all regular errors, making the `error` sub-module public (for niche users), and not re-exporting hider errors.
I believe the re-export should only be done in the module that holds `error` sub-module i.e., do not re-export `foo::error::SomeError` from `bar` module even if `bar::some_function` returns `SomeError`.
Definitions:
- hider error: An error type, often a struct, that is nested in another error just to hide the internals.
- regular error: An error type that is returned from a public function, most often an enum.
Please note that some times errors act as both hider errors and regular errors so the definitions are not perfect.
ACKs for top commit:
apoelstra:
ACK 7de02da9f5 LGTM
Tree-SHA512: 42b9d9b78c882d39553c3d72c188d80f5e305e1f7f1b9b9212760c75ff35ea71e868c93a17851ca0bf2104b01d5c6d316cd14c422c262ff39ad72922c562af4a
The formatter lines up comments if they are on consecutive lines even
if the second is supposed to be at the start of the collum and the
first is after code. Putting a line of whitespace between the two
lines stops this from happening.
Add whitespace to stop the formatter doing silly changes.
Whitespace only.
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
Currently we have to method names for the same thing "copy this object
into a vector". The library is easier to use if we are uniform and just
use one.
Elect to use `to_bytes`, for context see discussion in PR #2585.
Currently we have two macros used when creating array wrapper types,
one is in `internals` and the other in `bitcoin::internal_macros`. It
is not immediately obvious what is what and why there are two.
Improve the macros by:
- Move the inherent functions to `impl_array_newtype`
- Use `*_byte_array` for the names instead of `*_bytes` for functions
that return arrays
- Add `as_bytes` to return a slice
- Add `to_bytes` to return a vector
- Re-name the other macro to match what it now does
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
Andrew Poelstra
Tobin C. Harding
jamil.lambert
Rob N
Jose Storopoli