52f9c13358 Implement Copy for Address and AddressInner (tushar994)
Pull request description:
solves #4237
ACKs for top commit:
apoelstra:
ACK 52f9c13358c97c358543f3302b325f37ac49392f; successfully ran local tests
Tree-SHA512: 007092c57b893cfa62a6185c21b8839daf946a5b8ca505fc7d5f3b78c056fc5ce44cfb32936469f1955d535f4798506ea20a936218f0bf6808b17506e87836f2
ebaf162a96 Add push_relative_lock_time() and deprecate push_sequence() (Erick Cestari)
Pull request description:
This pr improves the script builder API to better align with Bitcoin semantics when working with relative timelocks:
- Add `push_relative_lock_time()` method that takes a `relative::LockTime` parameter, which correctly represents the semantic meaning when working with CHECKSEQUENCEVERIFY
- Deprecate `push_sequence()` in favor of `push_relative_lock_time()` to avoid confusion between sequence numbers and relative timelocks
This addresses a potential confusion point in the API where developers might incorrectly push raw sequence numbers in scripts when what they actually need is to push a relative locktime value that will be checked against the transaction's sequence numbers by CHECKSEQUENCEVERIFY.
Closes#4301
ACKs for top commit:
apoelstra:
ACK ebaf162a962494329c6cb5f6d375a6a4a97fe83b; successfully ran local tests
tcharding:
ACK ebaf162a96
Tree-SHA512: 52c37b6e8bbcaa3f9346c5fd5db26eba69169bce13f915906df95fdc65204067fd75f803f8b5adad76978c9baad553c99281628736db4d1d317b149ab257d81f
157fe48dfd minor docstring fixups (planetBoy)
Pull request description:
ACKs for top commit:
apoelstra:
ACK 157fe48dfdc4029a0db63b393d8d9fd32a197e30; successfully ran local tests
Tree-SHA512: 29fe6168ff729f0f65f32a2c6ad28d45e36e0761cac4455b57b891f9c0bd2622db51a21b4961d33fa5a8934302eefca4a77c20732bf047e2721a5bc5d655c340
This commit improves the script builder API to better align with Bitcoin
semantics when working with relative timelocks:
- Add push_relative_lock_time() method that takes a relative::LockTime
parameter, which correctly represents the semantic meaning when working
with CHECKSEQUENCEVERIFY
- Deprecate push_sequence() in favor of push_relative_lock_time() to avoid
confusion between sequence numbers and relative timelocks
This addresses a potential confusion point in the API where developers
might incorrectly push raw sequence numbers in scripts when what they
actually need is to push a relative locktime value that will be checked
against the transaction's sequence numbers by CHECKSEQUENCEVERIFY.
51d3a83891 updated and corrected links CHANGELOG.md (Bilog WEB3)
Pull request description:
Hey , I read through the whole `CHANGELOG.md` and fixed all the links + adjusted some
ACKs for top commit:
apoelstra:
ACK 51d3a83891395195fb89ae8590addded7efb4871; successfully ran local tests
Kixunil:
ACK 51d3a83891
Tree-SHA512: d36f38dd4b4fb83e615a3cdfe5efdbec5418283b53b09a0e7f288d88711626123c3e1dd5d3dac151186a4592dfbbaa26c90e4d409ed9f1cbf2116292979c3e20
7b193b5125 fix err P2WPKH to P2WSH (planetBoy)
Pull request description:
The correction is important because “P2WPK” is not a valid name. In the BIP141 specifications, the correct terms are “P2WPKH” and “P2WSH”.
ACKs for top commit:
Kixunil:
ACK 7b193b5125
apoelstra:
ACK 7b193b5125336263f672f2e2c69447cc3ae58926; successfully ran local tests
Tree-SHA512: 951bcde2c28e2086a69043c1ed27bde0935df0918f418c5f6f89ed476ba9e182e99eec545a438f79ca4e1704ce496d443b5bc9e368a53dd583a884f1da405865
492073f288 Strengthen the type of `taproot_control_block()` (Martin Habovstiak)
e8a42d5851 Unify/reduce usage of `unsafe` (Martin Habovstiak)
d42364bd9d Swap around the fields in `Address` (Martin Habovstiak)
7a115e3cf1 Make `Address` obey sanity rules (Martin Habovstiak)
bc6da1fe07 Swap around the fields in `sha256t::Hash` (Martin Habovstiak)
8ee088df74 Make `sha256t` obey sanity rules (Martin Habovstiak)
Pull request description:
Well, I thought this PR will be just the last commit... 😅
Anyway, this implements a bunch of changes to allow returning `ControlBlock` from `Witness` method(s). One cool side effect is that this PR also reduces the number of `unsafe` blocks.
ACKs for top commit:
apoelstra:
ACK 492073f28876406f8fe5a07a8a2495c8e0ba1fb3; successfully ran local tests
Tree-SHA512: 11979517cc310abf25644fc93a75deccacae66af8ba2d9b4011fdc3f414b15fac7e748399c7eef492ca850c11b7aacc3f24ec46fccf95e6d57a400212979637e
The type returned by `Witness::taproot_control_block()` was just `&[u8]`
which wasn't very nice since users then had to manually decode it which
so far also required allocation. Thanks to previous improvements to
`ControlBlock` it is now possible to return a `ControlBlock` type
directly.
To avoid expensive checks, this change adds a new type
`SerializedXOnlyPublicKey` which is a wrapper around `[u8; 32]` that is
used in `ControlBlock` if complete checking is undesirable. It is then
used in the `ControlBlock` returned from
`Witness::taproot_control_block`. Users can still conveniently validate
the key using `to_validated` method.
It then uses this type in the recently-added `P2TrSpend` type. As a side
effect this checks more properties of `Witness` when calling unrelated
methods on `Witness`. From correctness perspective this should be OK: a
witness obtained from a verified source will be correct anyway and, if
these checks were done by the caller, they can be removed.
From performance perspective, if the `Witness` was obtained from a
verified source (e.g. using Bitcoin Core RPC) these checks are wasted
CPU time. But they shouldn't be too expensive, we already avoid
`secp256k1` overhead and, given that they always succeed in such case,
they should be easy to branch-predict.
Since the introduction of `Script` `unsafe` started slowly creeping in
as more types with similar semantics were added. The `unsafe` in these
cases is just for trivial conversions between various pointer-like
types. As such, it's possible to move these into a single macro that
takes care of the conversions at one place and avoid repeating the same
`unsafe` code in the codebase. This decreases the cost of audits which
now only need to happen in `internals`, focuses any changes to happen in
that single macro and decreases the chance that we will mess up
similarly to the recent `try_into().expect()` issue (but this time with
UB rather than panic).
The new macro accepts syntax very similar to the already-existing struct
declarations with these differences:
* The struct MUST NOT have `#[repr(transparent)]` - it's added by the
macro
* If the struct uses `PhantomData` it must be the first field and the
real data must be the second field (to allow unsized types).
* The struct must be immediately followed by an impl block containing at
least on conversion function.
* If the struct has generics the impl block has to use the same names of
generics.
* The conversion functions don't have bodies (similarly to required
trait methods) and have a fixed set of allowed signatures.
* Underscore (`_`) must be used in place of the inner type in the
conversion function parameters.
The existing code can simply call the macro with simple changes and get
the same behavior without any direct use of `unsafe`. This change
already calls the macro for all relevant existing types. There are still
some usages left unrelated to the macro, except one additional
conversion in reverse direction on `Script`. It could be moved as well
but since it's on a single place so far it's not really required.
9a572dabde refactor: use path dependencies for workspace members in bitcoin/Cargo.toml (Eval EXEC)
Pull request description:
This PR want to:
1. make all workspace members use `workspace = true` syntax to import dependencies.
2. use `path = [balabala]` to define dependencies, instead of useing `[patch.crates-io.balabala]` , fix: https://github.com/rust-bitcoin/rust-bitcoin/issues/4283
ACKs for top commit:
Kixunil:
ACK 9a572dabde
apoelstra:
ACK 9a572dabdeb077f96b2ab66be1a80fcec3e805e3; successfully ran local tests
Tree-SHA512: 834ef881ed3fd324a9ecca440e8e591984a7e474eb6aeab86a0301cbd08b6dc96ecdc34b306ad146b11b50f7488024c289b8f8c7c6de1a2bdba7aec515b722ee
e966335447 chore: remove explicit into iteration (jike)
Pull request description:
```
warning: it is more concise to loop over containers instead of using explicit iteration methods
--> bitcoin/src/psbt/serialize.rs:209:21
|
209 | for cnum in self.1.into_iter() {
| ^^^^^^^^^^^^^^^^^^ help: to write this more concisely, try: `&self.1`
```
ACKs for top commit:
Kixunil:
ACK e966335447
apoelstra:
ACK e9663354476b0b9b2ec52ce034de5ab9223f9d29; successfully ran local tests
Tree-SHA512: 32fced1a5aba2caa6e48a2e586bf00dcf3d4b82b6b530d9c7f535b530737a40e24d5c05a7cea40747df4c7c4698ec141683b9416583b185b1ff6461f3c8f667f
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
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
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.
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.
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
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
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.
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`.
Maxim Evtush
merge-script
tushar994
Bilog WEB3
Fmt Bot
fuder.eth
lfgtwo
planetBoy
Erick Cestari
Martin Habovstiak
VolodymyrBg
Eval EXEC
jike
Tobin C. Harding