When signing a Taproot input (in a PSBT) using a key path spend we
currently return the pubkey associated with key that signs. However it
is common to think of the internal key as being the one that signs even
though this is not technically true. We also have the internal key in
the PSBT so matching against it is less surprising.
When using the `Psbt` type to sign a Taproot input using a key path
spend return the internal key.
There is no need to panic if input index is out of bounds because we
have a function to check the validity of the `input_index` argument and
use it in other places already.
f6467ac98d Minimize usage of Network in public API (Tobin C. Harding)
3ec5eff56e Add Magic::from_params (Tobin C. Harding)
Pull request description:
Minimize usage of the `Network` enum in the public API.
See #2225 for context, and https://github.com/rust-bitcoin/rust-bitcoin/pull/1291#discussion_r1492993788 for an interpretation of that long discussion.
Close: #2169
ACKs for top commit:
sanket1729:
reACK f6467ac98d.
apoelstra:
ACK f6467ac98d
Tree-SHA512: f12ecd9578371b3162382a9181f7f982e4d0661915af3cfdc21516192cc4abb745e1ff452649a0862445e91232f74287f98eb7e9fc68ed1581ff1a97b7216b6a
a05da2294e units: Add an initial changelog (Tobin C. Harding)
36ef4a62cf units: Improve re-exports (Tobin C. Harding)
Pull request description:
In preparation for release do two things:
- Patch 1: Attempt to improve the re-exports and HTML docs
- Patch 2: Add an initial changelog file
Note the version number is set already and does not conflict with the current `v0.0.0` version: https://crates.io/crates/bitcoin-units
ACKs for top commit:
sanket1729:
utACK a05da2294e
apoelstra:
ACK a05da2294e
Tree-SHA512: ab78f89d2e29e58e840d64889f0b7e0e57335e610b8759490478511854f1a7606e8203e4201872eaeb16a96fc527ecdca35e13b39027218d8f71290591e68101
In preparation for the initial release add a changelog. Note the version
number is already set to `v0.1.0` and this does not conflict with the
release currently on crates.io `v0.0.0`.
1bb32febbd Use manual docs attributes (Tobin C. Harding)
19f70959e1 Document private from_hex_internal function (Tobin C. Harding)
81a704302c Improve rustdocs on U256 type (Tobin C. Harding)
Pull request description:
Follow up to #2514, improve docs in the `pow` module. Done as separate patches because there are 3 distinct improvements. The first two are trivial the last can be verified by running:
`RUSTDOCFLAGS="--cfg docsrs" cargo +nightly rustdoc --all-features -- -D rustdoc::broken-intra-doc-links`
ACKs for top commit:
apoelstra:
ACK 1bb32febbd
sanket1729:
ACK 1bb32febbd
Tree-SHA512: 13b9db4b184328fdea1f212b9d049c76b0b824bae822cd9f6cf9460a38f3ca3d2f7270d4c4ee2b9301a5a4049a30d82a706b8b5c8415573fa6724d5105093d97
A release or so ago we added `non_exhaustive` to the `Network` enum,
this turned out to make usage of the enum un-ergonomic for downstream
users. After much debate we decided that a way forward was to just
minimize the usage of the enum in the public API by instead use
`AsRef<Params>` so that downstream could define their own network enum
based on the networks they support.
Minimize usage of `Network` by using `AsRef<Params>` as a parameter type
instead. "minimize" because the `Network` still appears in some places.
Make an attempt to improve the ergonomics and docs clarity of the
`units` crate.
- Don't inline error type re-exports, this keeps them up in the
"Re-exports" section and saves cluttering the other inlined docs.
- Re-export and inline the docs for `FeeRate` and `Weight` same as we do
for `Amount`. This makes the "Structs" section of the docs nice except
for the exclusion of the locktime types (which cannot be helped).
Currently we are using `Self` (in backticks) in the docs to functions
defined by the `do_iml` macro, this is a bit lazy, we can do better than
that.
Use `doc` attribute and the `$ty` macro variable to construct the docs
to use the type name.
Use a code comment to document the calling restrictions of private
function `from_hex_internal`. (Code comment because comment is not well
formed as per convention in this codebase.)
Improve the rustdocs on the private `U256` type by doing:
- Remove link to self within constructors, just use backticks
- Use `U256` instead of `Self` or `self`
- Fix incorrect usage of `CompactTarget` [0]
[0] We knew this was wrong when we merged it but because the docs are
private we elected to do this follow up patch.
288ed33afa just: Use pinned nightly (Tobin C. Harding)
Pull request description:
Use the nightly version that we use in CI when running `just` commands that use the nightly toolchain.
ACKs for top commit:
apoelstra:
ACK 288ed33afa
sanket1729:
utACK 288ed33afa
Tree-SHA512: f19f8e41025a9c78d1d86ef13b2924321e1fad7e11c815960ea197d127a79423ca5f6bb447e878fc69c8bf9d34e278ca27f9d1bd5a75ca0a8a7539a7fa22c2c2
e1869340be Upgrade secp dependency (Tobin C. Harding)
Pull request description:
Upgrade `rust-secp256k1` to the latest version `v0.29.0`. This removes the duplicate deps as well.
Includes removal of usage of `ThirtyTwoByteHash` and enables usage of `Message::from(sighash)`.
ACKs for top commit:
sanket1729:
ACK e1869340be
apoelstra:
ACK e1869340be
Tree-SHA512: d2ae5f7fcac5f189d9a333025985d90a73af6cfcf7a55cf3a21e8b56c15e6327171ef3f38904fdd102ac1f39c567742bdf295133a52a506c39ca8c78500f2448
f019e24f1f Add hex parsing to pow types (Tobin C. Harding)
d33625f6e2 units: Introduce public hex_u128 function (Tobin C. Harding)
9705d51782 docs: Use backticks on stdlib type (Tobin C. Harding)
cf65bf035f Introduce local variable (Tobin C. Harding)
1269722770 Move helper function (Tobin C. Harding)
dca054c680 test: Add unit tests for hex_u32 (Tobin C. Harding)
Pull request description:
The `pow` types implement `fmt::LowerHex` but do not implement hex parsing.
Add inherent methods `from_hex` and `from_prefixed_hex` to the `pow` types - as we did for locktime types.
ACKs for top commit:
apoelstra:
ACK f019e24f1f
sanket1729:
ACK f019e24f1f
Tree-SHA512: d682e1259db1c2c0abe24a8ca137fc49abe0ed7ccce90de4d6058c7a4986d26c86a84289ec7377b9209648a57e0af6735c1eb3d39c9de6770fde1936f596dfd2
Currently `Magic` has per network consts but no way to dynamically get
the magic bytes for a network. Note also that we are currently trying to
reduce the usage of `Network` in the public API.
Add a public constructor to the `Magic` type that accepts a `Params`
parameter to determine the network to use.
The `pow` types implement `fmt::LowerHex` but do not implement hex
parsing.
Add inherent methods `from_hex` and `from_prefixed_hex` to the
`pow` types.
Move the `strip_hex_prefix` helper function to below where it is called.
Note that I was the original author of this helper so there is no excuse
for it being above - bad Tobin no biscuit.
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
Tobin C. Harding
Andrew Poelstra
Fmt Bot