fb29aadc47 Remove bip158 types from crate root (Tobin C. Harding)
Pull request description:
BIP-158 (Compact Block Filters for Light Clients) is not so common as to require re-exorting its types at the crate root - remove them.
ACKs for top commit:
apoelstra:
ACK fb29aadc47367ba8579f0004e97cd1fd7fd8236f; successfully ran local tests
Kixunil:
ACK fb29aadc47
Tree-SHA512: 8a2edaad858b18feded8cc9e1d15f03a76980bd41524fa34b91b4055b236788c6d828940c6293e086a8c8e33baadc5765a1a60920513fdff2de22e9d94c0e541
280eb2239a io: Remove the impl_write macro (Tobin C. Harding)
Pull request description:
We have found calling macros from other crates to be error prone, especially when they involve feature gates. Furthermore the `impl_write` macro is trivial to write, users can just write it in the crate that needs it.
Lets just remove the macro. While we are at it point users to the examples in `bitcoin/examples/io.rs` for usage of the `io` crate.
Close: #3872
ACKs for top commit:
Kixunil:
ACK 280eb2239a
apoelstra:
ACK 280eb2239a2fcaa83a35ae6fb1ce62fe35a1e222; successfully ran local tests
Tree-SHA512: 62766ff6e4b5f3fae2d6214e87bd90c15b3103248effd4399a070ac831473b0288e599eed2377c08d2b7eca263220f172c8399a607756793477f82d3dc8f1b67
84bee2f7b0 Simplify `Witness` construction in tests (Martin Habovstiak)
3551ec2c69 Don't access internalls of `Witness` in tests (Martin Habovstiak)
c8078360d2 Impl `PartialEq` between `Witness` and containers (Martin Habovstiak)
587a66da47 Add a bunch of missing conversions for `Witness` (Martin Habovstiak)
Pull request description:
This is supposed to go in front of #4250
`Witness` lacked a bunch of APIs that were making it harder to use and test, so this also adds them in addition to cleaning up tests. (I only realized they are missing when I tried to clean up tests and got a bunch of errors.)
ACKs for top commit:
tcharding:
ACK 84bee2f7b0
apoelstra:
ACK 84bee2f7b06a7bd1f435aaad18fa76a15188326e; successfully ran local tests
Tree-SHA512: 7973f2a56b070babba7b4c632f45858154ccd00f8e77956ad2d28cb66e1fd18ff60d92c031ba3b76d0958e4acd34adfca10607fa26ec569dfd52ba1c1e2c79eb
3ae21d5111 Use impl_add/sub_assign for block interval (Tobin C. Harding)
9d55922952 Use impl_op_for_references for block height/interval (Tobin C. Harding)
f5e17914b6 Move Assign impls together (Tobin C. Harding)
cc66838e80 units: Remove unnecessary code comments (Tobin C. Harding)
Pull request description:
Improve the ops impls in the `block` module using the already present macros.
ACKs for top commit:
apoelstra:
ACK 3ae21d5111444f5e01f6cfb1a2b9b314f66418a3; successfully ran local tests; nice!
Tree-SHA512: 6565426a06bb47d337d21cf5c59acca43e69228dbec8319fc95373025d220d8ec6273c54f214f312c4229603c455d08e4c6a8c108663c6db5086df36266979de
e744347022 Make usage of Self and type uniform across both modules (Erick Cestari)
dfb49f014c Rename impl_try_from_array to impl_from_array (Erick Cestari)
Pull request description:
This PR makes two main changes:
1. Standardizes the function signatures in the `Amount` and `SignedAmount` implementations by consistently using `Self` as the return type instead of the concrete type names. This improves code consistency, maintainability, and follows Rust's idiomatic practices.
2. Renames `impl_try_from_array` to `impl_from_array` to better reflect its functionality.
### Changes
**Consistent usage of Self instead of concrete types**
- Replace all occurrences of `-> Amount` with `-> Self `in unsigned.rs
- Replace all occurrences of `-> SignedAmount` with `-> Self` in signed.rs
- Make similar replacements for Option/Result return types
- Use `Self::` instead of explicit type name for static method calls
**Function rename**
Renamed `impl_try_from_array` to `impl_from_array` for better clarity
### Related Issues
Closes#4210Closes#4241
ACKs for top commit:
Kixunil:
ACK e744347022
tcharding:
ACK e744347022
apoelstra:
ACK e744347022d1ad1e0ca0a83ec9350501af08297b; successfully ran local tests
Tree-SHA512: 3113f3ccf595b298afe6b23514f1de790284df7fcb55a13658aabe3ef4fcea0e401b65b0a2c67ac18da87a1bcd247bd1f1484856fe03470b98dfa2614958a3bb
069d2fd07e Add XOnlyPublicKey support for PSBT key retrieval and improve Taproot signing (Erick Cestari)
Pull request description:
The `bip32_sign_schnorr` function was previously only attempting to retrieve private keys using `KeyRequest::Bip32`, which limited the ability to sign Taproot inputs with key maps that don't support BIP32 derivation paths.
## Changes
- Added new `KeyRequest::XOnlyPubkey` variant to support direct retrieval using XOnly public keys
- Implemented `GetKey` for `HashMap<XOnlyPublicKey, PrivateKey>` for more efficient Taproot key management
- Modified `HashMap<PublicKey, PrivateKey>` implementation to handle XOnlyPublicKey requests by checking both even and odd parity variants
- Added comprehensive tests for both key map implementations
These improvements enable wallet implementations to store keys indexed by either `PublicKey` or `XOnlyPublicKey` and successfully sign PSBTs.
Closes#4150
ACKs for top commit:
Kixunil:
ACK 069d2fd07e
apoelstra:
ACK 069d2fd07e7d6dad1401fce6ab28ab1dc9f3c60f; successfully ran local tests
Tree-SHA512: 0ae07309b772f1a53e7da45073f7e2337cc332ab2335925d623d0e1ad1503aab77673bbbd64e5533ae7fc8d57f3577db0ae7ac3b05279de92d3b34ab8eeae90f
There's a restriction that for structs containing unsized types the
unsized type has to be the last field. `Address` is not an unsize type
but we are going to introduce a macro that will assume this order to
work equally well with both sized and unsized types. Thus we swap it
upfront here.
There's a restriction that for structs containing unsized types the
unsized type has to be the last field. `sha256t::Hash` is not an unsized
type but we are going to introduce a macro that will assume this order
to work equally well with both sized and unsized types. Thus we swap it
upfront here.
The `Witness`-related tests were constructing `Witness` in
over-complicated way by serializing `Vec<Vec<u8>>` and then
deserializing `Witness` even though they were not supposed to test
serialization but Taproot accessor methods. This was difficult to
understand and maintain.
This change simplifies them to just construct the `Witness` from array
of `Vec<u8>`s using the recently-added constructors. Note that we
already have serialization tests written separately so we're not losing
meaningful coverage here.
Accessing the internals of tested object is problematic because it makes
changes to layout harder, it makes tests harder to read and it checks
implementation details rather than semantics of the API (behvaior).
We had such tests in `primitives::witness` so this changes them to use
the newly added APIs instead. However, it still leaves
`from_parts__unstable` which needs to be dealt with separately.
Since `Witness` is semantically equivalent to `&[&[u8]]` they should
also be comparable. However we only had the impl to compare `Witness`
with itself. Being able to compare `Witness` with other containers is
particularly needed in tests.
`Witness` was missing conversions from arrays (and variations) which was
annoying when creating known-sized witnesses. These come up when
spending statically-known inputs and in tests.
1c13627cd8 Automated update to Github CI to rustc nightly-2025-03-21 (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 1c13627cd8
Tree-SHA512: 291dd7e0db15a8a3d187dc3e5a63105c8f0530cdf8dc0c87cb34f94f33766a1c8717bf7be400895d20a631537382de6aa281e0b6df303af62dfa2715e9c3fa75
This commit standardizes the function signatures in the Amount and SignedAmount
implementations by consistently using Self as the return type instead of the concrete
type names. This makes the code more consistent, easier to maintain, and follows Rust's
idiomatic practices.
Changes:
Replace all occurrences of -> Amount with -> Self in unsigned.rs
Replace all occurrences of -> SignedAmount with -> Self in signed.rs
Make similar replacements for Option/Result return types
Use Self:: instead of the explicit type name for static method calls
db9ec3bed8 Remove From<newtype> for $hash (Tobin C. Harding)
6b2b89c2f7 Remove From<hash> for not-general-hash types (Tobin C. Harding)
200ff47327 Use compute_merkle_root (Tobin C. Harding)
Pull request description:
The `hash_newtype` macro is explicitly designed to produce a hash that is not a general purpose hash type to try and prevent users hashing arbitrary stuff with it. E.g., `Txid` isn't meant to be just hash arbitrary data. However we provide a `From` impl that will convert any instance of the inner hash type into the new type. This kind of defeats the purpose. We provide `from_byte_array` and `to_byte_array` to allow folk to 'cast' from one hash type to another if they really want to and its ugly on purpose.
Also, it is becoming apparent that we may be able to remove the `hashes` crate from the public API of `primitives` allowing us to stabalise `primitives` without stabalising `hashes`.
For both these reasons remove the `From` impl from the `hash_newtype` macro. Note that deprecating doesn't seem to work so we just delete it.
ACKs for top commit:
Kixunil:
ACK db9ec3bed8
apoelstra:
ACK db9ec3bed8d6164a0345ba8db1e2162626db7cc5; successfully ran local tests
Tree-SHA512: 90bc325821cd2d72bbaef5b3cfef2d299192d1e7999cd4f96b6b69b8872e419964e431e91674c59bfdd2e9a5959dbc13ee89d5f87d03e96785044c616db19d72
This commit enhances PSBT signing functionality by:
1. Added new KeyRequest::XOnlyPubkey variant to support direct retrieval using XOnly public keys
2. Implemented GetKey for HashMap<XOnlyPublicKey, PrivateKey> for more efficient Taproot key management
3. Modified HashMap<PublicKey, PrivateKey> implementation to handle XOnlyPublicKey requests by checking both even and odd parity variants
These changes allow for more flexible key management in Taproot transactions.
Specifically, wallet implementations can now store keys indexed by either
PublicKey or XOnlyPublicKey and successfully sign PSBTs with Taproot inputs.
Added tests for both implementations to verify correct behavior.
Added test for odd parity key retrieval.
Closes#4150
437562e71c Add official BIP32 test vectors for invalid keys (Martin Habovstiak)
5dd0c9253d Remove a bunch of `try_into().expect()` (Martin Habovstiak)
Pull request description:
Previously we've used `try_into().expect()` because const generics wer
unavailable. Then they become available but we didn't realize we could
already convert a bunch of code to not use panicking conversions. But we
can (and could for a while).
This adds an extension trait for arrays to provide basic non-panicking
operations retutning arrays, so they can be composed with other
functions accepting arrays without any conversions. It also refactors a
bunch of code to use the non-panicking constructs but it's certainly not
all of it. That could be done later. This just aims at removing the
ugliest offenders and demonstrate the usefulness of this approach.
ACKs for top commit:
tcharding:
ACK 437562e71c
apoelstra:
ACK 437562e71cbb1e18179df3f5d01d2cc86961a150; successfully ran local tests
Tree-SHA512: 51384dd2e63a5bd0844828301c18a967d52b990b0f4caee2974d36db1185163af46cfa3abe558566caa6406eac996bcd11893dd4047dc7daabe27557b5cf64e8
36ebf6c186 Automated update to Github CI to rustc stable-1.85.1 (Update Stable Rustc Bot)
Pull request description:
Automated update to Github CI workflow `semver-checks.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK 36ebf6c186
Tree-SHA512: 1da59803e058dea232c77a752b8715220d29da615a0d502d5271a4888cf2439fcdc15fd216c9e8be735729bacf34c0605b9ce0f385d3792ecf17fc10cfd9532e
We provide the from/to_byte_array functions for casting between arrays.
We shouldn't be supporting calls to `into` to quickly do the cast.
We already removed the other direction, now remove this one.
The `hash_newtype` macro is explicitly designed to produce a hash that
is not a general purpose hash type to try and prevent users hashing
arbitrary stuff with it. E.g., `Txid` isn't meant to be just hash
arbitrary data. However we provide a `From` impl that will convert any
instance of the inner hash type into the new type. This kind of defeats
the purpose. We provide `from_byte_array` and `to_byte_array` to allow
folk to 'cast' from one hash type to another if they really want to and
its ugly on purpose.
Also, it is becoming apparent that we may be able to remove the `hashes`
crate from the public API of `primitives` allowing us to stabalise
`primitives` without stabalising `hashes`.
For both these reasons remove the `From` impl from the `hash_newtype`
macro. Note that deprecating doesn't seem to work so we just delete it.
These are defined in the BIP as invalid. The previous commit fixed a bug
where invalid key was parsed as valid and this bug can be caught by
these vectors. Therefore, if this commit is ordered before the last one
the test will fail.
Previously we've used `try_into().expect()` because const generics were
unavailable. Then they became available but we didn't realize we could
already convert a bunch of code to not use panicking conversions. But we
can (and could for a while).
This adds an extension trait for arrays to provide basic non-panicking
operations returning arrays, so they can be composed with other
functions accepting arrays without any conversions. It also refactors a
bunch of code to use the non-panicking constructs but it's certainly not
all of it. That could be done later. This just aims at removing the
ugliest offenders and demonstrate the usefulness of this approach.
Aside from this, to avoid a bunch of duplicated work, this refactors
BIP32 key parsing to use a common method where xpub and xpriv are
encoded the same. Not doing this already led to a mistake where xpriv
implemented some additional checks that were missing in xpub. Thus this
change also indirectly fixes that bug.
9ea2e9262f Don't use references to `TaprootMerkleBranchBuf` (Martin Habovstiak)
c528f52894 Change `Deref::Target` of `TaprootMerkleBranchBuf` (Martin Habovstiak)
04a4efbe63 Introduce unsized `TaprootMerkleBranch` (Martin Habovstiak)
370c2597c6 Add `as_mut_slice` to `TaprootMerkleBranchBuf` (Martin Habovstiak)
33d75659da Push `merkle_branch` module one level deeper. (Martin Habovstiak)
277045bad7 Add `Buf` suffix to `TaprootMerkleBranch` (Martin Habovstiak)
Pull request description:
This implements a bunch of changes needed to make `ControlBlock` alloc-free. In particular, this allows constructing `Witness` without the intermediate allocation. It is also a step towards having `P2TrSpend` public.
Closes#1614
This also intentionally does **not** address decoding of `ControlBlock` from `Witness` since I'm not sure about the API.
Rationale for doing the `Buf` rename: while doing it with `Script` was very painful it shouldn't be here since it's not used that often and also we can just backport the first commit with deprecated type alias. I was thinking of having `TaprootMerkleBr` but it'd be inconsistent and the name is silly.
(Also if anyone is wondering why I did this: I was too exhausted to do more important stuff but felt like doing something nice and easy like this.)
ACKs for top commit:
tcharding:
ACK 9ea2e9262f
apoelstra:
ACK 9ea2e9262fbc04ea6fad33047de0fc1ead999dc7; successfully ran local tests
Tree-SHA512: c5e3ea61d10fbe0cbce5e900943e3cef77a175a62043c500b3ff6df57a96f00692d80fb1c4dd75bca9a704201baab6ddfcc430b12c7ecabc43968198466fed9d
2958521117 amount: remove from_sat_unchecked (Andrew Poelstra)
d0d7a15604 amount: move MIN/MAX constants and constructors inside the privacy boundary (Andrew Poelstra)
004d073184 amount: return i64 from parse_signed_to_satoshi (Andrew Poelstra)
3370c14d74 amount: stop using from_sat_unchecked in tests (Andrew Poelstra)
05c8b043ff tests: replace Amount::from_sat_unchecked with from_sat.unwrap (Andrew Poelstra)
beaa2db7e5 amount: add from_sat_i32 and from_sat_u32 methods for small constants (Andrew Poelstra)
82d9d1aeea amount: rename `from_int_btc_const` unctions to hungarian ones (Andrew Poelstra)
2ec1c2a044 units: Make from_int_btc_const take a 16 bit integer (Tobin C. Harding)
Pull request description:
This does some followups to #4157.
It pulls in #4254, which changes the signature of `Amount::from_int_btc_const` and also replaces a `checked_mul` which can't ever fail with a `*`.
It then renames `from_int_btc_const` to `from_btc_u16`/`from_btc_i16` as appropriate, since these names reflect what the function does. It also adds an analogous method for making `Amount`s from sats rather than BTC.
Then it systematically removes every instance of `from_sat_unchecked`. There wind up being 7 instances in tests that I just added unwraps() to, plus one instance where we had `let sat = Amount::from_sat_unchecked` which I also added an unwrap() to.
In the real code, there was one instance (`Amount::to_signed`) where I needed to add an `expect`. Every other instance I was able to remove, by refactoring logic to use the error case as appropriate.
ACKs for top commit:
tcharding:
ACK 2958521117
Kixunil:
ACK 2958521117
Tree-SHA512: e2c2eb19acdd60da5524f6700e66c140b8e113bec7f2418a2505aeefdbf742eff216fb580891a3ea53255156786f5a6b365b8fb6553e60e42b18d4115d705dd2
7b114e3893 Increase test coverage in relative.rs (Jamil Lambert, PhD)
c97bebdcba Increase test coverage in absolute.rs (Jamil Lambert, PhD)
Pull request description:
Increase the test coverage in `primitives/src/locktime/*`.
Modify existing tests and add new ones where required to increase the test coverage to 100% for all features excluding `arbitrary` and `serde`.
ACKs for top commit:
tcharding:
ACK 7b114e3893
apoelstra:
ACK 7b114e38935f9c688c1259793e7ffda822f25c57; successfully ran local tests
Tree-SHA512: 9090565c358a23855f5bfa5c31202bfd78ed9dc36303210b64d502cacf692b010793ba0e5b2cc40c8da336429f6ffc07b6d4499cd8d03c0c5c7bb7bb868c4236
It's conceptually a bit tortured to have an `Amount` type defined in a
private module, with an _unchecked method allowing you to set values out
of range, which needs to be used outside of the module to *define* the
range and the constructors that check it.
Move the constants and constructors inside the privacy module, where they
can be written directly. This is easier to understand and eliminates a couple
_unchecked calls.
This private function is used for string-parsing an amount. It returns a
sign boolean and a u64, but its surrounding logic can be simplified if
it just returns a i64 (which is OK now since the range of `Amount` fits
into the range of i64).
Along the way we eliminate some instances of from_sat_unchecked.
Annoyingly we still need the bool to distinguish -0 from +0; when
parsing Amount we reject -0 (and we have tests for this).
This causes a couple error constructors to no longer be used outside of
unit tests. May be worth looking at whether we need them, or whether we
should be using them in parse_signed_to_satoshi.
We have a ton of calls to `from_sat_unchecked` for small constants which
were clearly in range, e.g. in fee.rs. Add a new constfn for these
cases. Don't bother making a generic Into<u32>/Into<u16> variant because
there isn't an obvious name for it.
There are 7 instances where we're using this method with values that are
out of range, which we leave as from_sat_unchecked for now.
We have `from_int_btc_const` on both `Amount` and `SignedAmount` because
the "real" `from_int_btc` is generic over the integer type it accepts,
which means that it cannot be a constfn. But we do want a constfn.
However, just because `from_int_btc_const` exists for the sake of
constfn doesn't mean that that's what it *is*. So rename these methods
to reflect what they *are*.
The new unsized type is more flexible and so are the references to it.
Just like we pass around `&str` instead of `&String` we should be
passing `&TaprootMerkleBranch` instead of `&TaprootMerkleBranchBuf`.
`TaprootMerkleBranchBuf` previously derefed to a slice which lost the
information about length being valid. This commit changes the type
which, while API-breaking, is not disruptive because the type has API
very similar to slice.
`TaprootMerkleBranchBuf` being a vec introduced intermediate allocation
when creating or decoding `Witness`. However the representation on the
wire is the same as in-memory (aside from `#[repr(transparent)]`) so
this allocation wasn't really needed.
This commit introduces `TaprootMerkleBranch` type which is unsized and
can be used in place of `TaprootMerkleBranchBuf` within `ControlBlock`.
Aside from removing the intermediate allocation, this improves the API a
bit: the conversion from array to other type is no longer needed because
it's performed by `ControlBlock` in its methods. Thus, consumers who
have an array can simply set it as `merkle_branch` field and then encode
the `ControlBlock` into witness. A convenience method is also provided
to push the `ControlBlock` along with other parts at the end of the
`Witness`.
merge-script
todaymoon
Jamil Lambert, PhD
jike
Martin Habovstiak
merge-script
Update Nightly Rustc Bot
Erick Cestari