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.
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
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.
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.
Since arrays better convey the intention than slices when parsing
fixed-sized bytes we're migrating to them. This deprecates the
`from_slice` method similarly to how we do it elsewhere.
Private keys have statically-known length of 32 bytes and we are
migrating types with known lenths to use `from_byte_array` methods. This
adds the method to `PrivateKey` as well and uses it to implement
`from_slice`.
During upgrade of `secp256k1` a number of function calls needed to be
rewritten to not use `from_slice` but `from_byte_array`. Unfortunately,
the conversions wasn't correct and caused panics on invalid inputs
rather than errors.
This fixes it simply by calling `try_into` on the entire slice and
converting the error.
f80cf2cb2a update secp256k1 to 0.30.0 (19年梦醒)
Pull request description:
ACKs for top commit:
apoelstra:
ACK f80cf2cb2aa318978da3a6c5df49d82c49344763; successfully ran local tests
tcharding:
ACK f80cf2cb2a
Tree-SHA512: 83b8bb72372025c4a4b81c2b7973a7808a4a1d9d6450adef8b60a890e128b2559b55832159c25fa91daac1856049b070cd910d87313fed2851ced9e72ae5ddf5
cf12ba262a Move taproot back to bitcoin crate (Tobin C. Harding)
Pull request description:
I don't know what I was thinking when I move the taproot hash types to `primitives`. As correctly pointed out by Kix we agreed to only have blockdata in `primitives`.
Move the taproot hash types back to `bitcoin::taproot` and remove the extension traits.
ACKs for top commit:
Kixunil:
ACK cf12ba262a
apoelstra:
ACK cf12ba262a646a6341098ee3f4c178a52fc90211; successfully ran local tests
Tree-SHA512: 0c5eabf395c05a93603a46b277c6ea2cc547f3894eef182fceb80f309123d67fe457936a388bac0249ec24cae7521eaef3bf8bd8facca5282e4ce2ea6fafd5f7
83bd83385e Update sighash.rs (leopardracer)
fc4ea87429 Update transaction.rs (leopardracer)
0e70e85a1a Update key.rs (leopardracer)
f7c28ab44d Update input_string.rs (leopardracer)
Pull request description:
This pull request contains changes to improve clarity, correctness and structure.
- Corrected typos in multiple files (e.g., `transacton` → `transaction`, `function panics` formatting).
- Improved readability of comments in `transaction.rs`, `key.rs`, and `input_string.rs.`
- No functional code changes, only documentation updates.
This message provides a clear summary of what was done and why the changes were made. Let me know if you need any modifications!
ACKs for top commit:
Kixunil:
ACK 83bd83385e
tcharding:
ACK 83bd83385e
apoelstra:
ACK 83bd83385e0bbad993f96f5abbe51f71d199aad1; successfully ran local tests
Tree-SHA512: ec0cae0357dd8c60e9286db71c3d18dfa6ba7acbaa27cce78c0586ab73c9631ea58ab453acd85b18d4edd72dcc578a4e08d69b9529e856837d5651efa288a4db
I don't know what I was thinking when I move the taproot hash types to
`primitives`. As correctly pointed out by Kix we agreed to only have
blockdata in `primitives`.
Move the taproot hash types back to `bitcoin::taproot` and remove the
extension traits.
For private WIF keys corresponding to a compressed address,
the last byte of the key needs to be 0x01, but the API
doesn't enforce this when using PrivateKey::from_wif(). So,
invalid keys can be accepted.
Thus we check if the last byte is equivalent to 0x01
if the key's length is 34 (which indicates it's
compressed).
We want to get rid of this constant, so we replace it in tests with 0
amount, empty script. Notably, the tests were already using it as a
dummy value - the exact amount was irrelevant, so this change doesn't
break anything.
The `encode_signing_data_to_inner` function previously constructed a
transaction internally, requiring a bunch of allocations, which it'd
then consensus-serialize into a writer (hasher). It also used a dummy
`TxOut::NULL` value which we want to get rid of.
To get rid of both allocations and the NULL value we serialize the
transaction on-the-fly. Because the encoding doesn't involve witnesses
it's not too complicated and the consensus encoding will never change so
there are no desync bugs possible. We may later change this to an
abstract transaction though.
In #3847 we added an `InvalidCharError` into one of the variants of
`ParsePublicKeyError` but we forgot to update the trait
implementations.
Fix the `error::Error` and `Display` implementations for
`ParsePublicKeyError`. While we are at it match on `*self` as is
typical in this codebase.
With this applied #3835 is fully resolved.
Close: #3835
316d8bcb01 Change all occurrences of "IO" to "I/O" (Jamil Lambert, PhD)
Pull request description:
Fixes#3871
ACKs for top commit:
tcharding:
ACK 316d8bcb01
apoelstra:
ACK 316d8bcb01504420a14854d2be122d1c8cffb4a9; successfully ran local tests; lgtm
Tree-SHA512: 437a95a1c36bcd4ae27aaacdfc5e0f3463e522a222c4a6ef2c3e234be4a24be2b600687bd58b300bf2b0a0d6596ab008f60903c91646458228eb34cf510908d6
a7c44cebf9 Use _unchecked to construct amounts (Tobin C. Harding)
09df951760 Use sat variable in tests (Tobin C. Harding)
4a5b2c60c6 Use ssat variable in tests (Tobin C. Harding)
Pull request description:
We have a `_unchecked` constructor now for both `Amount` and `SignedAmount`. Soon we would like to start enforcing the `MAX_MONEY` invariant in both amount types. To make that change easier do a few refactorings:
- Patch 1 and 2 introduce local variables for amount constructors.
- Patch 3 replaces the local variables introduce in (1) and (2) with macros
- Patch 4 uses `_unchecked` constructor for hard coded integers
The strange patch separation is done intentionally so we don't inadvertently reduce test coverage by using the wrong constructor. I made this mistake already in a previous PR, lesson learned.
Note please, the macro introduced in patch 3 is in preparation for enforcing `MAX_MONEY`. The macros allow us to panic (`from_sat().unwrap()`) instead of using the `_unchecked` version.
ACKs for top commit:
apoelstra:
ACK a7c44cebf9975c4eeba56a65c0ea65be90e5c7f3; successfully ran local tests
Tree-SHA512: 55c2428ae231882542a4cfa724675341f7b493d158f4bec26277d3eefb04d9597cc29b05dce859661a96855fa6f4bac250d53c3dfa9f86a9611d43387ee18667
We have a `_unchecked` constructor now for both `Amount` and
`SignedAmount`. In preparation for enforcing the `MAX_MONEY` invariant
use the `_unchecked` constructor throughout the codebase to construct
amounts from hard coded integer values.
Rust macros, while at times useful, are a maintenance nightmare. And
we have been bitten by calling macros from other crates multiple times
in the past.
In a push to just use less macros remove the usage of the
`impl_from_infallible` macro in the bitcoin, units, and internals crates
and just write the code.
945fcd0920 fix ParsePublicKeyError using hex::InvalidCharError (Innocent Onyemaenu)
Pull request description:
Replaced the InvalidChar variant u8 with hex::InvalidCharError
Resolves#3835
changed InvalidChar variant of the ParsePublicKeyError from `u8` to `hex::InvalidCharError`
```
pub enum ParsePublicKeyError {
...
/// Hex decoding error.
InvalidChar(hex::InvalidCharError),
...
}
Also,
modified the test cases to accommodate the new variant
Why:
- hex::InvalidCharError includes both the invalid character and its position.
- This improves debugging and makes error messages more actionable.
ACKs for top commit:
apoelstra:
ACK 945fcd09209120ef8869a2e4165e866328cc9bd5; successfully ran local tests; I like it
clarkmoody:
utACK 945fcd0920
Tree-SHA512: c13446c099cb02b4f253f9cc559a860aff3288a2cc5eac96d3cf910bf63e78957741bbdff69b936b16b36e46b366841a5c94876d16cbc0c41aea2a70866a6e45
What:
- Replaced the InvalidChar variant u8 with hex::InvalidCharError
Why:
- hex::InvalidCharError includes both the invalid character and its position.
- This improves debugging and makes error messages more actionable.
There is a loose convention in Rust to not use `test_` prefix. The
reason being that `cargo test` outputs 'test <test name>' using the
prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some
places and not others is confusing to new contributors and is leading me
to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
These lints are valuable, lets get at em.
Changes are API breaking but because the changes make functions consume
self for types that are `Copy` downstream should not notice the breaks.
The `Amount` and `SignedAmount` were not supposed to implement `serde`
traits by design because doing so implicitly uses sats. We provide two
modules `as_sat` and `as_btc` to allow users to explicitly serialize in
their preferred format.
In commit: `d57ec019d5 Use Amount type for TxOut value field` derives
were added for `serde` and we did not notice it during review.
For the `hashes` crate we would like to make `hex` an optional
dependency. In preparation for doing so do the following:
- Remove the trait bounds from `GeneralHash`
- Split the hex/string stuff out of `impl_bytelike_traits` into a
separate macro.
The `impl_bytelike_traits` macro is public and it is used in the
`hash_newtype` macro, also public.
Currently if a user calls the `hash_newtype` macro in a crate that
depends on `hashes` without the `serde` feature enabled and with no
`serde` dependency everything works. However if the user then adds a
dependency that happens to enable the `serde` feature in `hashes` their
build will blow up because `serde` code will start getting called from
the original crate's call to `hash_newtype`.
Pull the serde stuff out of `hash_newtype` and provide a macro to
implement it `impl_serde_for_newtype`.
1649b68589 Standardize wording to `constructs a new` (Jamil Lambert, PhD)
27f94d5540 Replace `creates` with `constructs` (Jamil Lambert, PhD)
Pull request description:
As discussed in issue #3575 there are various ways of saying a new object is created.
These have all be standardized to the agreed version.
Close#3575
ACKs for top commit:
apoelstra:
ACK 1649b68589834dfe9d5b63812da3e9f0e5930107; successfully ran local tests
tcharding:
ACK 1649b68589
Tree-SHA512: 0ed9b56819c95f1fc14da1e0fdbbe03c4af2d97a95ea6b56125f72913e8d832db5d2882d713ae139d00614e651f3834a4d72528bdf776231cceb6772bf2f9963
There is a range of different wordings used in the docs of constructor
type functions.
Change all to start with `Constructs a new` or `Constructs an empty`.
In functions that act like constructors there is a mixture of the usage
of `creates` and `constructs`.
Replace all occurrences of `creates` with `constructs` in the first line
of docs of constructor like functions.
Fmt Bot
merge-script
Martin Habovstiak
Erick Cestari
NinaLua
19年梦醒
Tobin C. Harding
leopardracer
Liu-Cheng Xu
Jose Storopoli
ndungudedan
Innocent Onyemaenu
Jamil Lambert, PhD
Shing Him Ng
Chris Hyunhum Cho