3615410d21 api: Run just check-api (Tobin C. Harding)
a3d2d1a184 Make Address:p2sh_from_hash public (Tobin C. Harding)
Pull request description:
We previously made this function Private and added a comment that doing so was somehow better to remove the footgun of hashing the wrong length script. However in hindsight this was a bad idea and users want the functionality.
Make the `Address:p2sh_from_hash` public and document it as we do for `Address::p2sh`.
This is an additive change and is expected to be backported to `v0.32`, as part of the fix to #2784. Please note it introduces the footgun that is described in the function rustdoc. This will be improved as a separate patch and added to the current release.
ACKs for top commit:
apoelstra:
ACK 3615410d21
Tree-SHA512: 535bb7894eeef8ecb5afb7bf6e5c483cd42c6a4282d1c116e5bf86cd1364a8327bbec1efb8634a578f07ad2832c1e5daf7fe7e844574b88b1ad355a443627bef
11bb1ff6ff Standardize function doc Safety, Returns and Parameters (jamil.lambert)
df83016c98 Standardize function doc Errors (jamil.lambert)
d219ceb68e Standardize function doc Examples (jamil.lambert)
233a9133d8 Standardize function doc Panics (jamil.lambert)
Pull request description:
The subheadings in the rustdocs have been standardized according to [./CONTRIBUTING.md](https://github.com/rust-bitcoin/rust-bitcoin/blob/master/CONTRIBUTING.md):
```rust
impl FooBar {
/// Constructs a `FooBar` from a [`Baz`].
///
/// # Errors
///
/// Returns an error if `Baz` is not ...
///
/// # Panics
///
/// If the `Baz`, converted to a `usize`, is out of bounds.
pub fn from_baz(baz: Baz) -> Result<Self, Error> {
...
}
}
```
ACKs for top commit:
apoelstra:
ACK 11bb1ff6ff
tcharding:
ACK 11bb1ff6ff
Tree-SHA512: 163af3cd1cfb47cea3e55eddeaeb6843ff7ec89c57354e3247d6bae85e756b183e8045c2555cfcf87e8c23c1388ff9d7592cfb6a951a37a9ec41d27263e5a2e4
830a6e1b0c fuzz: delete CBOR test (Andrew Poelstra)
91eb50b2db fuzz: add lint to generate-files.sh (Andrew Poelstra)
Pull request description:
We were using an outdated CBOR crate for MSRV reasons. But this old crate is causing suprious test failures. So delete it. (Sadly, updating the crate doesn't fix the issue, replacing it with ciborium breaks our MSRV tests because it needs a more recent `half` dependency, and replacing it with `minicbor` doesn't work because minicbor is not based on serde. So we don't really have any options.)
In general, I am suspicious of this decode-then-reencode test. CBOR has some ambiguity in integer encoding. Empirically it has seemed to work for a long time, but this seems more like an indictment of our test than a positive result.
Also, round-trip testing serde encoding of a byte vector is probably not a great use of our fuzz resources. I don't believe we have ever had a problem with this.
Fixes#2801
ACKs for top commit:
tcharding:
ACK 830a6e1b0c
Tree-SHA512: f207b68da2f0910542cd8b6a35bb2364462030bdf08ac1e954fd9dcdbef47b2035ac85f964adb9590078dfc2151e8fc7fe2ed41ec0919ff937723c5954612a47
cf3e1eb198 api: Run just check-api (Tobin C. Harding)
98bf213c52 bitcoin: Remove error module (Tobin C. Harding)
a5b93cb159 Flesh out hex unit parsing API (Tobin C. Harding)
Pull request description:
Add to `units::parse` the complete suit of hex unit parsing functions:
- remove prefix
- assert without prefix
- parse with or without prefix
- parse with prefix
- parse without prefix
- parse prefix unchecked
Refactor `bitcoin` to use the exact function we need, removing code duplication.
This is a breaking change to `units`, it does however keep the current re-exports from the public, now empty, `bitcoin::error` module.
ACKs for top commit:
apoelstra:
ACK cf3e1eb198
Tree-SHA512: 1778108d4364e290e8956cfea6f23fcdd82c835844d034a00b4cf5cab5552e3efbe853dfbf8a3e0a4bd53a8e3da9d6f7c7408d332d18cd7090aec16fc1f02fe7
We previously made this function Private and added a comment that doing
so was somehow better to remove the footgun of hashing the wrong length
script. However in hindsight this was a bad idea and users want the
functionality.
Make the `Address:p2sh_from_hash` public and document it as we do for
`Address::p2sh`.
726ff25c46 Hard code genesis script bytes instead of hex (Tobin C. Harding)
6e5592db77 Use test_hex_unwrap in bench code (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
apoelstra:
ACK 726ff25c46
Tree-SHA512: 96110332fc24dd5b251150b32737fa198113244c3b51b35453c8c1fcc8386c5a2f68dddb30d78cf2f9e1762550099fdb4109dc550f4c144625795ce60b86e574
7f29313d36 Update API (Tobin C. Harding)
4f29adf163 Enable getting the witness program from an address (Tobin C. Harding)
Pull request description:
We have getters for the pubkey hash and script hash but we forgot one for the witness program - add it.
Done as part of fixing #2784, this is an additive change and is expected to be backported to `v0.32`
ACKs for top commit:
apoelstra:
ACK 7f29313d36
Tree-SHA512: 365aba572eaacb789f5424c233be067500ceff18dc27e28fc3be123c49d27333e95bbf4527469a9caf8cc2fe54f8e13a0fc83bc3fc7c3356aea876c9dd2fa5f0
We were using an outdated CBOR crate for MSRV reasons. But this old
crate is causing suprious test failures. So delete it. (Sadly, updating
the crate doesn't fix the issue, replacing it with ciborium breaks our
MSRV tests because it needs a more recent `half` dependency, and
replacing it with `minicbor` doesn't work because minicbor is not based
on serde. So we don't really have any options.)
In general, I am suspicious of this decode-then-reencode test. CBOR has
some ambiguity in integer encoding. Empirically it has seemed to
work for a long time, but this seems more like an indictment of our test
than a positive result.
Also, round-trip testing serde encoding of a byte vector is probably not
a great use of our fuzz resources. I don't believe we have ever had a
problem with this.
Fixes#2801
9bb75703a1 Header change from arguments to parameters (jamil.lambert)
Pull request description:
In a few cases a function header documents the parameters of the following function under the heading "Arguments", this has been changed to "Parameters".
Since the description is at the level of the function definition and not where it is being called parameters seems the more accurate term.
ACKs for top commit:
apoelstra:
ACK 9bb75703a1
tcharding:
ACK 9bb75703a1
Tree-SHA512: aa24af3fd6e086c09f5e2605fa58289969fc7188f63d7f53c0e325315644f9704d51d4cf526ebfc51b2cf9216155fc3d48cc6bca759dc14bae15e4770de5116e
The `error` module is empty except for public re-exports. We are still
in the "break everything and get the API right" stage so this module
adds no value - remove it.
Add to `units::parse` the complete suit of hex unit parsing functions:
- remove prefix
- assert without prefix
- parse with or without prefix
- parse with prefix
- parse without prefix
- parse prefix unchecked
Refactor `bitcoin` to use the exact function we need, removing code
duplication.
This is a breaking change to `units`, it does however keep the current
re-exports from the public, now empty, `bitcoin::error` module.
30a482504b bump nightly-version (Andrew Poelstra)
5ad7c245e3 cargo: whitelist all cfgs used in this repo (Andrew Poelstra)
814786b0a6 crypto: enable and fix accidentally disabled unit test (Andrew Poelstra)
Pull request description:
https://github.com/rust-lang/rust/issues/124800 has been fixed and we can update our nightly version by whitelisting all cfgs that are used.
There was one place where we had an old `cfg(feature = "no-std")` despite having removed the feature. By removing that cfg check we re-enabled a previously disabled test.
ACKs for top commit:
tcharding:
ACK 30a482504b
Tree-SHA512: d25bed819091db74b9d47cb2c23caa3ceb0d7be323b37831326e2ec1608cb1577d41aad2e1cdf59d66df69397537bc3e17a3c2872935d5a4f46f4dc55b5e613c
f3d364ef1d reduce two-ACK requirement to one-ACK requirement (Andrew Poelstra)
Pull request description:
For the last few months it has only been myself and tcharding working on this project actively. Prior to that we had Kixunil as well, which allowed us to move forward requiring two ACKs on every PR (though it put a minimum 24-hour delay on everything since we are pairwise 8 hours separated from each other in timezones).
The other listed maintainers, especially Sanket and Matt, are intermittently available, which is awesome but insufficient to get large refactors or long-term work through.
We have increasingly depended on the "one-ACK carveout", an ever-increasing list of exceptions to our two-ACK rule. Most silly, one exception is that if something stays open for two weeks, we don't need the second ACK. So the result is a "one ACK plus 2 weeks" policy which is grating and demoralizing.
Meanwhile, we are not finished our crate-smashing project or our API overhaul, and we want to continue moving at a fast rate. (Though of course, we will take breakage seriously; we do not want to make gratuitous changes or changes that replace previous changes, and the changes we *do* make we will do our damnedest to do via a year-long deprecation cycle. And we welcome suggestions to improve our policy or process on this front. But requiring extra maintainers/reviewers has not been helpful for us.)
I'll let this PR sit open for 7 days (til Wed May 22) to accumulate ACKs or NACKs, and assuming no strong opposition, will merge it.
ACKs for top commit:
sanket1729:
ACK f3d364ef1d
Tree-SHA512: 6d1ea41ce79b038304c59d574b239fc855cc178dc61b4dd8e20dc9e0493278df339950a7604592dd20d7b25c8c56f6d4b5c13b103771a8b920ec10d4050578ae
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
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
Andrew Poelstra
Fmt Bot
Tobin C. Harding
jamil.lambert