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
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`.
The `from_int_btc_const` constructors are specifically designed for
easily creating amount types in const context but currently they return
an error which is annoying to handle in const context. If we make the
`whole_bitcoin` parameter a 16 bit integer this gives us a nicer const
constructor with the downside that it can only create values upto a
maximum of
- unsigned: 65_536
- signed: 32_767
That is plenty high enough for most use cases.
Then use the new `from_int_btc_const` in associated consts.
Note that because `from_sat` checks max (and min) values we must
define max and min from sats directly.
f4f79f88eb Enable getting the network kind from an address (Tobin C. Harding)
Pull request description:
Users may wish to ask of an address 'what kind of address is this?' We have the `NetworkKind` struct that abstracts over the answer but currently no API to ask the question.
The address may have been parsed or constructed and weather the network has been checked already is immaterial. Hence we add the function for both `NetworkChecked` and `NetworkUnchecked` addresses.
Fix: #4247
ACKs for top commit:
apoelstra:
ACK f4f79f88eb2c6c80c46c95c69fcc43b17d306be2; successfully ran local tests
Kixunil:
ACK f4f79f88eb
Tree-SHA512: 57bdf7a0f2ae8bf599b3830d10201af3f6312a802ab72c0d86e346af660cbc4f430954e46d6698032a062514ec3ee1ee7edc732beff79af99a84ce718a519afa
Users may wish to ask of an address 'what kind of address is this?' We
have the `NetworkKind` struct that abstracts over the answer but
currently no API to ask the question.
The address may have been parsed or constructed and weather the network
has been checked already is immaterial. Hence we add the function for
both `NetworkChecked` and `NetworkUnchecked` addresses.
Fix: #4247
cacf734ccf Automated update to Github CI to cargo-semver-checks version-0.40.0 (Update cargo-semver-checks 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 cacf734ccf
Tree-SHA512: 7fb8c6b5c5d8614d39ee14b961f29371b135596cbbe6930183495baf81fa6dcaf53866f853986d5f924c0e2fbd0e7c06193bb65e5249d64d82c373e27644c064
058e8285f0 Automated update to Github CI to rustc nightly-2025-03-14 (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 058e8285f0
Tree-SHA512: d3211e78dbb5717b9369a7d331f07d7c2c3560e24de8ffadf6c60170f60ffbb4a9991e5dba174ddb8e0cb11bbbfed91737b2fda04b7d199f383050a26ff442e1
ab4ea7c13d Enforce the MAX_MONEY invariant in amount types (Tobin C. Harding)
Pull request description:
Enforcing the `MAX_MONEY` invariant is quite involved because it means multiple things:
- Constructing amounts is now fallible
- Converting from unsigned to signed is now infallible
- Taking the absolute value is now infallible
- Integer overflow is eliminated in various places
Details:
- Update `from_sat` to check the invariant
- Fix all docs including examples
- Use the unchecked constructor in test code
- Comment any other use of the unchecked constructor
- Deprecate `unchecked_abs`
- Fail serde (using the horrible string error variant)
- Try not to use the unchecked constructor in rustdocs, no need to encourage unsuspecting users to use it.
- Use `?` in rustdoc examples (required by Rust API guidlines)
- Remove `TryFrom<Amount> for SignedAmount` because the conversion is now infallible. Add a `From` impl.
- Fix the arbitrary impls
- Maintain correct formatting
- Remove private `check_max` function as its no longer needed
Close#620
ACKs for top commit:
apoelstra:
ACK ab4ea7c13d08411044bd5f9c17457e926c80ed4d; successfully ran local tests
Tree-SHA512: bec963d8ea69e202f399cd19bca864b06f3e86323d376c2d2126d74093598f8bbbf19792b2327dba0862ef6f0201202778014a2be7a14991f02917d8ca312afb
93c6c8cef5 Use impl_op_for_references macro in fee module (Erick Cestari)
Pull request description:
This pr replaces the individual operator implementations in the fee module with the impl_op_for_references macro to handle reference operations. This removes the need to manually implement reference combinations for operands, simplifying the code and improving consistency.
### Changes:
- Replaces direct implementations of operators with macro usage
- Adds tests to verify that reference operations work correctly
- Maintains the same semantics as the original implementation
Closes#4173
ACKs for top commit:
tcharding:
ACK 93c6c8cef5
apoelstra:
ACK 93c6c8cef59ceed56932d62daeb212c2b40fc4a1; successfully ran local tests; yeah, I think the docs loss is fine -- the docs are hard to find and say exactly what users expect of the / operator
Tree-SHA512: 51d7643c2cecd16a0cb16afcd195fd87bc9eca9116e16518d888ba61a8edb5684162af987ea52611c9463f5299810f92a057dedc3fa8e89cdef21ef40528bca1
c707b959b7 Rename timestamp module to time (Tobin C. Harding)
e2dee4900f Re-name Timestamp to BlockTime (Tobin C. Harding)
Pull request description:
Done in two patches so we can bikeshed the name of the type and separately the name of the module.
- Rename type: `Timestamp` to `BlockTime`
- Rename module: `timestamp` to `time`
ACKs for top commit:
apoelstra:
ACK c707b959b72dd89ca6df581a6102f32daedb8368; successfully ran local tests
Tree-SHA512: de3855b38445a58b6767a6081919eecb81c6c12aee3f6699f3bfa10efaf5770b54fb412da23991a9ee734e14dfb642af670f0218d1886cdc8c8d3f393ef65d7e
c1ea20b958 Increase test coverage in block.rs (Jamil Lambert, PhD)
3e750d7ba1 Create test helper function to create a header (Jamil Lambert, PhD)
Pull request description:
Add tests to `primitive::block` module to increase test coverage.
Coverage tested with `cargo-tarpaulin`
`block.rs` coverage:
before 13.79%
after 100%
ACKs for top commit:
apoelstra:
ACK c1ea20b958f448a5a35073ce3d5bd5394918eb9b; successfully ran local tests
tcharding:
ACK c1ea20b958
Tree-SHA512: 70bbdce9c45e9bdcea76026b7a27582a32b796099329c429527d7dfe574788604020e53e417afd3cf53886ceb87df37b6dcb34a7ff40260b9d8a3811e22a478c
c0e20dbf2e test: add coverage for ServiceFlags::P2P_V2 (Bruno Garcia)
Pull request description:
Include `P2P_V2` on `service_flags_test`
ACKs for top commit:
tcharding:
ACK c0e20dbf2e
apoelstra:
ACK c0e20dbf2e1406a3f3df5538d1397b233078cd7c; successfully ran local tests
Tree-SHA512: 6ebb2f493bcc8fc6643cec67945a81692f03ceb976d75e10ce6052e775901846b9795f31817439b4484ef2d1eaae122d7da270cac2b59d7bb41cb2015593d1f5
Enforcing the MAX_MONEY invariant is quite involved because it means
multiple things:
- Constructing amounts is now fallible
- Converting from unsigned to signed is now infallible
- Taking the absolute value is now infallible
- Integer overflow is illuminated in various places
Details:
- Update from_sat to check the invariant
- Fix all docs including examples
- Use the unchecked constructor in test code
- Comment any other use of the unchecked constructor
- Deprecate unchecked_abs
- Fail serde (using the horrible string error variant)
- Try not to use the unchecked constructor in rustdocs, no need to encourage unsuspecting users to use it.
- Use ? in rustdoc examples (required by Rust API guidlines)
- Remove TryFrom<Amount> for SignedAmount because the conversion is now infallible. Add a From impl.
- Fix the arbitrary impls
- Maintain correct formatting
- Remove private check_max function as its no longer needed
8f74b823ab Add validation for private key format and master key constraints (Erick Cestari)
Pull request description:
This PR addresses issue #4195 by adding proper validation when decoding extended private keys:
### Changes
- Add validation to ensure byte 45 is zero as required by BIP-32 specification for private keys
- For master keys (depth=0), add validation to ensure parent fingerprint is zero
- For master keys (depth=0), add validation to ensure child number is zero
- Add corresponding error types to handle these validation failures
- Add unit tests to verify each validation rule
### Validation Rationale
These checks improve security by rejecting malformed extended keys that could potentially lead to unexpected behavior. As noted in the issue discussion, these validations are explicitly required by the BIP-32 specification.
### Testing
Added three new unit tests to verify each validation rule:
- test_reject_xpriv_with_non_zero_byte_at_index_45
- test_reject_xpriv_with_zero_depth_and_non_zero_index
- test_reject_xpriv_with_zero_depth_and_non_zero_parent_fingerprint
Fixes#4195
ACKs for top commit:
jrakibi:
ACK 8f74b823ab
tcharding:
ACK 8f74b823ab
apoelstra:
ACK 8f74b823ab8ef44bde7d003f8ba43fbe44dbef3e; successfully ran local tests
Tree-SHA512: 6a013e4917f83cfd7e39a2a18f7491853d791ab1d981a99eeea6204e1dab723fed7a168ff2a89e8850d512c3c381bfa1afef7fa32e5a0d246d949a46b01a3023
This commit replaces the individual operator implementations in the fee
module with the impl_op_for_references macro to handle reference operations.
This removes the need to manually implement reference combinations for
operands, simplifying the code and improving consistency.
The change:
- Replaces direct implementations of operators with macro usage
- Adds tests to verify that reference operations work correctly
- Maintains the same semantics as the original implementation
e4513bf925 feat: add MAX_BLOCK_SERIALIZED_SIZE existing in core (ChrisCho-H)
Pull request description:
fad0d9ea2d1e807806fa141238e279fddea6ae99: add `MAX_BLOCK_SERIALIZED_SIZE` as constant, which also exists in [bitcoin-core](59ff17e5af/src/consensus/consensus.h (L13)).
I originally thought it would be better to use this value for checking limit of push_bytes [here](0870cd1660/bitcoin/src/blockdata/script/push_bytes.rs (L31)), as it's the actual limit(`OP_PUSHDATA4` semantic says it could allow up to 4GB though). However, I'm not sure whether there might be need to push_bytes larger than `MAX_BLOCK_SERIALIZED_SIZE`, so just let developer use this constant to check the actual limit rather than enforcing it.
ACKs for top commit:
tcharding:
ACK e4513bf925
apoelstra:
ACK e4513bf9250799bc18a10728af184d6c86a561a4; successfully ran local tests
Tree-SHA512: 44c5a4882666ad286c1e1c40b9738929e2a8ad4bb44aaf48865fc395291185ae5aae351d26ac9334671e47a11e844bd037bd251a921b6b028a116d1b442b9183
5d851f1c3e Remove deprecated amount methods (Tobin C. Harding)
76a2d70b28 Make mul weight by fee return NumOpResult (Tobin C. Harding)
f9eb307953 Remove panic in dust value functions (Tobin C. Harding)
13595fbe7d Fix amount whole bitcoin constructors (Tobin C. Harding)
ac71680202 Pick one - MAX or MAX_MONEY (Tobin C. Harding)
6d70c77cf9 Enforce newtype sanity rules for amount types (Tobin C. Harding)
e6f7b26d80 Use _unchecked in amount const types (Tobin C. Harding)
ef0af8d62e Use sat/ssat constructors throughout tests (Andrew Poelstra)
8ecdc7c275 Use den_ prefix for local Denomination variable (Tobin C. Harding)
938461cc65 psbt: Use Amount::ZERO in unit test (Tobin C. Harding)
Pull request description:
We want to start enforcing MAX_MONEY as an invariant in the amount types. There are a few more steps we can do first to make that change easier to review.
ACKs for top commit:
jamillambert:
ACK 5d851f1c3e
apoelstra:
ACK 5d851f1c3e98d7d426e5897b2d734b77a299ccfb; successfully ran local tests
Tree-SHA512: 9e28b273d41fc143656e3a84736b6abe477fae5721b02bce7436551bd489cc235dc7e9fc68ffafa98f75a61065470ac514570a42bea94e90fedbb31f3cd61031
Erick Cestari
merge-script
merge-script
Update Stable Rustc Bot
Martin Habovstiak
Jamil Lambert, PhD
planetBoy
healthyyyoung