fd6fedc3ad Improve API for max target threshold calculation (Tobin C. Harding)
6e47d57744 Rename difficulty transition threshold functions (Tobin C. Harding)
4121c9a09f Rename Params::pow_limit to max_attainable_target (Tobin C. Harding)
f0f6d3f162 Take Params instead of Network in difficulty function (Tobin C. Harding)
104dee9376 Debug assert that target != zero in difficulty calc (Tobin C. Harding)
c1ba496a07 Document current behaviour of difficulty_float (Tobin C. Harding)
3d01146374 Allow needless-borrows-for-generic-args (Tobin C. Harding)
2a6821b426 Use link to CompactTarget in rustdoc (Tobin C. Harding)
Pull request description:
When computing the maximum difficulty transition threshold we forgot to check that the returned `Target` is not bigger than the maximum. This value is network specific so keep the original logic but with `_unchecked` on the function name.
This was noted in the discussion on #2161
ACKs for top commit:
apoelstra:
ACK fd6fedc3ad
sanket1729:
ACK fd6fedc3ad
Tree-SHA512: 520ee2a07edb251c84b5ce8b48ed6e5a5c1945126dc7bcdb5570e97101ec4a3dc63fa7992725194869e22b21ee4f5955579d5e2499fcb48167637fd1fb3ae74d
The maximum target threshold has a network dependant upper bound.
Currently we are not checking this bound. One complication is that there
is currently heated open debate around the `Network` type.
We can bypass the `Network` issue by using `AsRef<Params>` instead.
Add a function that does the checks based on the `Params` type as well
as an unchecked version.
These two functions calculate the min/max threshold transition which is
a _target_ not a "difficulty" number. Using "difficulty" in the function
name is unnecessarily confusing.
Rename and deprecate the functions.
The maximum "attainable" target is a `rust-bitcoin` thing, Core use max
unattainable.
Deprecated the `Params::pow_limit` field and add a new field
`max_attainable_target`.
The `Params` type is `non_exhaustive` so this is not an API breaking
change.
What we really want is the maximum target, but since this is a const in
`Params` use an `AsRef<Params>` argument in the `difficulty` functions.
Requires implementation of `AsRef<Params> for Params`.
The `difficulty` calculation requires dividing a target value by `self`.
Add an assertion that `self` is not zero to help devs debug this.
Note that this should never really be hit, but its possible there is a
bug somewhere causing the target to be set to zero - so this may help
debugging.
Also, add panics section to rustdocs.
This lint triggers when parsing a reference to a large struct as a
generic argument, which is wrong.
Allow it crate wide because [subjectively] this lint never warns for
anything useful.
d91cdd20bf docs: Document ordered feature (Tobin C. Harding)
3520f550f0 Implement ArbitraryOrd for relative::LockTime (Tobin C. Harding)
Pull request description:
TL;DR As we do for `absolute::LockTime` and for the same reasons; implement `ArbitraryOrd` for `relative::LockTime`.
locktimes do not have a semantic ordering if they differ (blocks, time) so we do not derive `Ord` however it is useful for downstream to be able to order structs that contain lock times. This is exactly what the `ArbitraryOrd` trait is for.
Fix: #2566
ACKs for top commit:
sanket1729:
ACK d91cdd20bf
apoelstra:
ACK d91cdd20bf
Tree-SHA512: 52ace9222e765dfa266d003b4aff3e93e35d1414c9fd579c4a4a36998d6d1b08bf6d4964a6f1c1d769068d65e47a882495daa4aacf254909a35dce8e01c99a9e
af6dc1db02 internals: Bump version to 0.3.0 (Tobin C. Harding)
Pull request description:
In preparation for release add a changelog and bump the version number.
Please note, the changelog is pretty terse.
ACKs for top commit:
apoelstra:
ACK af6dc1db02
sanket1729:
ACK af6dc1db02
Tree-SHA512: b70d4b9de7de90aba3cbff90dd7f25c5ac801d020dbdfe3e64af4c079347cba726aa783a94fc777e7bf177db8402b54948c2dfd4a766d90c1a7a7a6bdfd36136
a565db9fdd 2024-03-31 automated rustfmt nightly (Fmt Bot)
Pull request description:
Automated nightly `rustfmt` changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
apoelstra:
ACK a565db9fdd checked that it matches `cargo fmt` locally but did not actually read the diff because it is too big
Tree-SHA512: 48f55c043a39f496b954d646c3ffe4f90942bb02b4dfe0cc3ed44f4bc09cc44f12ee8abaabeb25a524c5a4f8c82aa85562cc247ec39d5cf878f97984735e2a00
9b70c65f5d Introduce new one-ack carve out rule (Tobin C. Harding)
42d02fbd66 Merge Refactor and One ACK carve outs (Tobin C. Harding)
ebf5b670d4 Update test script mention (Tobin C. Harding)
Pull request description:
Update merge carve-out policy and introduce new rule.
- Patch 1: Fix stale test script mention
- Patch 2: Merge current carve-outs into a single carve-out with multiple rules
- Patch 3: Introduce new carve-out rule
From patch 3:
```
Introduce new one-ack carve out rule
Our merge process is being artificially slowed down because of a
combination of:
- Using merge-commit merging means PRs often have to be rebased with no
changes but a different merge base (and force pushed).
- Trivial changes, like fixing nits, are often force pushed also.
- Force pushes invalidate ACKs
- Our devs are spread around the world working at different times
What this means is trivial force pushes often cause multi day delays in
merging. To try and alleviate this problem introduce an additional rule
to the One ACK carve-out so that Andrew can merge PRs that have
previously been ack'ed by another dev and have only minimal changes.
The definition of "trivial" is subjective which introduces a burden on
Andrew to not merge stuff willy-nilly but also allows simple changes to
the original PR (eg fixed nits that the original reviewer suggested).
```
ACKs for top commit:
apoelstra:
ACK 9b70c65f5d confirmed via range-diff that the commit everyone ACKed and this one differ only in `as` vs `has`
Tree-SHA512: 41898e71e013ac70e41bb4624ce5e5055dc3e7a405dd73d3988f5b02ece104d7fad746203ce8d26a6a33f98b745010fc39e9a4bddb9bcf22267c942a4dac2028
Our merge process is being artificially slowed down because of a
combination of:
- Using merge-commit merging means PRs often have to be rebased with no
changes but a different merge base (and force pushed).
- Trivial changes, like fixing nits, are often force pushed also.
- Force pushes invalidate ACKs
- Our devs are spread around the world working at different times
What this means is trivial force pushes often cause multi day delays in
merging. To try and alleviate this problem introduce an additional rule
to the One ACK carve-out so that Andrew can merge PRs that have
previously been ack'ed by another dev and have only minimal changes.
The definition of "trivial" is subjective which introduces a burden on
Andrew to not merge stuff willy-nilly but also allows simple changes to
the original PR (eg fixed nits that the original reviewer suggested).
22daea86ad Remove stale changelog (Tobin C. Harding)
Pull request description:
Somehow a stale version of the `io` crate's changelog got put in the repo root directory. The current version is in `io/`.
Remove the changelog from the repository root.
ACKs for top commit:
sanket1729:
ACK 22daea86ad
apoelstra:
ACK 22daea86ad
Tree-SHA512: 17d2a7e9740afda7e601e5d58baf546f91a73f4fe0d6f0b211e4ff76ab96e9e15ed7a845b2aa6a6811f072143356f4bb7d8c134c9b514da0628a0aec836dc50d
Somehow a stale version of the `io` crate's changelog got put in the
repo root directory. The current version is in `io/`.
Remove the changelog from the repository root.
35687c84fc CI: Fix Manage PR job (Tobin C. Harding)
Pull request description:
In #2635 we broke the Manage PR CI job but for some reason CI didn't run on that PR so we merged the breakage.
Fix the script by setting default variables when they are not set. Done with ChatGPT.
ACKs for top commit:
apoelstra:
ACK 35687c84fc
Tree-SHA512: 707ad8872468a9eb6292ae3e8d7896a19baf044eaa28d6c45119d4367a7b14a73923a86cf68d64799df43febb35ecccb39422ce0d28f37100c6120a4316240d1
In #2635 we broke the Manage PR CI job but for some reason CI didn't run
on that PR so we merged the breakage.
Fix the script by setting default variables when they are not set. Done
with ChatGPT.
3fa3d37c21 Add set -euo pipefail (Tobin C. Harding)
Pull request description:
Add `euo pipefail` to all non-trial shell scripts, note if `x` is already set we maintain it.
ACKs for top commit:
sanket1729:
utACK 3fa3d37c21
apoelstra:
ACK 3fa3d37c21
Tree-SHA512: 24c0da96bea44ea4f550847c8f73bbe1d9ccd1bfee1714ef8a0c23075e3a6d438b6006e351cce0df7a1cb0b9d1b50096270f65a62d0def05c84c9573ffefacba
6ab0110964 Run fuzzer daily (Tobin C. Harding)
Pull request description:
Waiting for the fuzzer slows down the dev feedback loop because it makes CI slow. We still want fuzz coverage but not in a way that slows down devs.
Instead of running the fuzzer on every PR just run it once a nightly.
As we do for other daily jobs, rename the yaml file to make explicit what it does.
Note also we only get 20 jobs, currently there are 18 fuzzing jobs. This means for this hour any other CI runs will only have access to 2 jobs (if i understand GitHub resource usage correctly).
### Open questions
Docs: https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration
> Concurrent jobs - The number of concurrent jobs you can run in your account depends on your GitHub plan,
What is the definition of "account" - is that a user account, or a repository? Which account is running the cron job?
ACKs for top commit:
apoelstra:
ACK 6ab0110964 though if this proves too onerous we should try 30 or even 15 minutes
Tree-SHA512: d6d07d16b550fe27258c97cb4da305f0bec16d39788da3cf061e9db7ca4a039897dc1a91b394ccc03cd4cd5427d3da94dd7c0ec4e0a80410bd48d029d4112d4e
Add `euo pipefail` to all non-trial shell scripts, note if `x` is
already set we maintain it.
Note we have a pipe in `run_task.sh` that relies on grep not finding
anything i.e., failing, so we cannot use pipefail there. Disable it and
re-enable it after the pipe.
Waiting for the fuzzer slows down the dev feedback loop because it makes
CI slow. We still want fuzz coverage but not in a way that slows down
devs.
Instead of running the fuzzer on every PR just run it once a nightly.
As we do for other daily jobs, rename the yaml file to make explicit
what it does.
Open questions:
- This should run for 30 minutes but I can't work out why the current
set up only runs for a shorter time than that?
- Is this less fuzzing i.e., is 30 minutes once a day better or worse that 1
minute 30 times?
TL;DR As we do for `absolute::LockTime` and for the same reasons;
implement `ArbitraryOrd` for `relative::LockTime`.
locktimes do not have a semantic ordering if they differ (blocks, time)
so we do not derive `Ord` however it is useful for downstream to be able
to order structs that contain lock times. This is exactly what the
`ArbitraryOrd` trait is for.
Update the rustdocs in `relative` and mirror the docs changes in
`absolute`.
Fix: #2566
0ca5a43ce5 hashes: Bump version to v0.14.0 (Tobin C. Harding)
Pull request description:
In preparation for release add a changlelog entry and bump the version.
Note the hashes 0.13.0 dependency stays in the dependency graph because of secp, we can update secp after releasing `hashes` then update the secp dependency in `rust-bitcoin` thereby removing the `hashes v0.13.0` dependency - phew.
Note we are right to release this immediately, the two open PRs (#2337 and #2541) that touch `hashes` only add a clippy attribute so can safely be ignored.
ACKs for top commit:
apoelstra:
ACK 0ca5a43ce5
sanket1729:
ACK 0ca5a43ce5
Tree-SHA512: d1d26acb8fbf13f785b25add3f1dac05bb392b5bdbad16ead2bc5dd26f3d668824c4b653c373f88c3562a37e775146766680606cedd19db40e0f197b26ca86b8
c16c1be946 base58: Add changelog (Tobin C. Harding)
Pull request description:
In preparation for release add a minimal changelog to the `base58ck` crate.
This crate is currently unreleased and has the version number correctly set to `v0.1.0` - as of today, the crate name `base58ck` is available on crates.io.
ACKs for top commit:
sanket1729:
utACK c16c1be946
apoelstra:
ACK c16c1be946 Let's do it!
Tree-SHA512: 8c66e7823dd058257c65e40e67f4386f045cd6292f11356f80bf5e95c0fa28d573b52c57f8ba1504893e236e8124916311fa0c2af1807fdb48e3123bb3d826fb
`require_network` is typically called as part of parsing, often in the
same line of code. Counter to our normal errors, it makes
`require_network` more ergonomic to use if we just return a `ParseError`
variant.
Close: #2507
Done in preparation for adding the `NetworkValidationError` as a variant
of `ParseError`.
Move the `NetworkValidationError` type to beneath `ParseError`.
Code move only, no other changes.
In preparation for release add a minimal changelog to the `base58ck`
crate. This crate is currently unreleased and has the version number
correctly set to `v0.1.0` - as of today, the crate name `base58ck` is
available on crates.io.
fd040f5e38 Replace TBD with 0.32.0 (Tobin C. Harding)
Pull request description:
We are gearing up for the 0.32.0 release; replace all instances of TBD with the version number of the upcoming release.
ACKs for top commit:
sanket1729:
ACK fd040f5e38
apoelstra:
ACK fd040f5e38
Tree-SHA512: fe73fd47a794557742f618b21434cd3cc18cde0e861216716723bfcc9135accf63590e1ea60bfeda066acec7312c8b9f1bf09e7454e7161ccaba5ebe60af66fd
af49841433 Hide base58::Error internals (Tobin C. Harding)
4f68e79da0 bitcoin: Stop using base58 errors (Tobin C. Harding)
669d5e8fc6 base58: Add InvalidCharacterError for decoding (Tobin C. Harding)
ec8609393b base58: Add error module (Tobin C. Harding)
42fabbab03 base58: Run the formatter (Tobin C. Harding)
Pull request description:
Improve the error code in the new `base58` crate.
ACKs for top commit:
apoelstra:
ACK af49841433
sanket1729:
ACK af49841433
Tree-SHA512: c05479f02a9a58c7c98fd5987e760288562372e16cceeeb655f0a5385b4a8605945a3b6f7fcf473a7132a40f8dc90d204bc5e9e64fd2cc0bdc37dbcabb4ddc5c
c17db32df3 Pub back in deprecated dust_value (Tobin C. Harding)
Pull request description:
When we renamed `dust_value` to `minimal_non_dust` we forgot to keep the original and deprecated it, doing so assists with the upgrade path.
Put back in deprecated `dust_value`, linking to the rename.
Renamed in #2255, found while testing upgrade of downstream software.
ACKs for top commit:
tcharding:
> ACK [c17db32](c17db32df3) I _think_ this matches the behavior of the old version
apoelstra:
ACK c17db32df3 I *think* this matches the behavior of the old version
sanket1729:
ACK c17db32df3
Tree-SHA512: 28e1bd2e1a0fd13c78c70ad2667b72b3bf649c293201b79c86c00f09d0126389ebaeb430b8dd32aeeec3d60cbd8761ae949f5784a5ea7756b1b9ae77ec96ce61
dec05b63e9 Refactor witness_version and is_witness_program (Tobin C. Harding)
dac552b436 Add unit tests for shortest/longest witness program (Tobin C. Harding)
Pull request description:
Refactor `witness_version` and `is_witness_program`.
- Patch 2 adds a couple of preparatory unit tests.
- Patch 2 does the refactor
Fix: #2618
ACKs for top commit:
apoelstra:
ACK dec05b63e9
sanket1729:
ACK dec05b63e9
Tree-SHA512: 3db0a1d8175cbb2fd18f3254854d02db3ad7efa2620b12f08d9727ef6bb5854f0a015917e57023cd2196a36d13276e80536a0e96318c44a1173da4f6793ca370
04715e3e60 absolute: make is_* methods uniform with the ones from relative (Andrew Poelstra)
878b865f85 relative locktime: introduce is_* methods to check units (Andrew Poelstra)
c2f87c7ab3 relative locktime: add is_implied_by method for sequences (Andrew Poelstra)
319e102fed relative locktime: use From/TryFrom to convert between relative locktimes and Sequence (Andrew Poelstra)
0ed26915f6 relative locktime: add conversions to/from sequence (Andrew Poelstra)
5c8fb5c11b relative locktime: add consensus encode/decode functions (Andrew Poelstra)
ac968e02b6 relative locktime: constify a bunch of constructors (Andrew Poelstra)
f27e675e1e relative locktime: add "obvious" constructors (Andrew Poelstra)
f02b1dac5b relative locktime: copy comments and PartialOrd impl from absolute locktimes (Andrew Poelstra)
2ff5085e70 locktimes: run cargo fmt (Andrew Poelstra)
Pull request description:
While implementing https://github.com/rust-bitcoin/rust-miniscript/pull/654 I ran into a number of limitations of the `relative::LockTime` API. This fixes these by
* Copying a ton of functions from `absolute::LockTime` to `relative::LockTime`, adjusting comments and functionality accordingly.
* Adding conversion functions to/from `Sequence` numbers, as well as a method to check whether a locktime is satisfied by a given sequence number.
Fixes#2547Fixes#2545Fixes#2540
ACKs for top commit:
tcharding:
ACK 04715e3e60
sanket1729:
ACK 04715e3e60
Tree-SHA512: 70740eaa3a83dc1e7312b99e907ccdcef4eeb6191ae881d81712707ad6fb949c4e28183ab6f9258c6cde1ef8fdd5dc6476439e705a9e02a939b7832430a608d4
The "One ACK carve-out" has 3 rules and then there is a separate
"Refactor carve-out" that covers things that are not only refactoring -
this makes it hard to reference the carve-outs in github because its a
bit confusing.
Merge the carve-outs into a single "one ACK carve-out" with multiple
rules. Use rule 0 for the original refactor carve-out stuff because it
makes the diff smaller and all good lists start with 0.
Also remove mention of the refactor carve-out from rule 3.
When we renamed `dust_value` to `minimal_non_dust` we forgot to keep the
original and deprecated it, doing so assists with the upgrade path.
Pub back in deprecated `dust_value`, linking to the rename.
8bd0394b0a Document how to write commits (Tobin C. Harding)
Pull request description:
Reviewers often find themselves linking to blog posts to encourage newer devs to improve their commit logs, we can save everyones time by putting the links in the contributing docs, then we can just point devs there.
ACKs for top commit:
apoelstra:
ACK 8bd0394b0a
Tree-SHA512: b2713c2882c3153152091bd2a72a473e151cf1e3e93288ae17b773c9f2944805a78c991437c9951d9fe5478c76a31d5c0f2ac0212bda6cbca531681a7b41f988
Adds constructors to allow directly creating locktimes from time or
block counts; adds a flooring constructor to Time to match the ceiling
one; adds an explicit constructor to Height since the From<u16> was not
very discoverable.
Andrew Poelstra
Tobin C. Harding
Fmt Bot