ade7ea5fcf psbt: replace the fee rate magic numbers with named constants in tests (frankomosh)
Pull request description:
Replace a hardcoded fee rate numbers in PSBT high-fee checks test with named constants.
Close#4378
ACKs for top commit:
tcharding:
ACK ade7ea5fcf
apoelstra:
ACK ade7ea5fcfc1d00a7cb3c35b4022a4543a7efe1d; successfully ran local tests
Tree-SHA512: fa02217940ae32f00164c762c82ecb6bd28da58e5ad44165124a7ad44367d7d0f752e7ab189b991edb2460c449bbd0a83df16a47b082f6755bd9e9d38e398cd8
Parsing and displaying strings is a PITA. `FeeRate` is likely not that
heavily used at the moment and users can always just call
`to_sat_per_kwu()` to format it.
So we can get the `units-1.0-alpha.0` release out the door just remove
the `Display` and `FromStr` impls for now. They can be added back in
later when we have time to get #4339 in.
c11772a768 Accept flexible input types for Taproot-related functions (Erick Cestari)
2a518d62e6 Wrap secp256k1::XOnlyPublicKey to improve error handling (Erick Cestari)
Pull request description:
This PR addresses issue #4361 by creating a wrapper type for XOnlyPublicKey instead of directly re-exporting it from the secp256k1 library.
### Key Changes
1. Created a new `XOnlyPublicKey` struct that wraps `secp256k1::XOnlyPublicKey`
2. Implemented custom error types:
- `ParseXOnlyPublicKeyError` for handling parsing errors
- `TweakXOnlyPublicKeyError` for tweaking an `XOnlyPublicKey`
3. Updated all imports and usage throughout the codebase
4. Implemented necessary traits and methods for compatibility
Closes#4361
ACKs for top commit:
apoelstra:
ACK c11772a768eefd89dcc0e3b1a369d535c191f94a; successfully ran local tests
tcharding:
ACK c11772a768
Tree-SHA512: c8da3486e7ffcab6c24cc08f9b2f964dd9158449ef2bd720e54d56176bc7027052314ea23cac3f673d217fa785238ea8a9b5323ba57f02199f20e56df5893965
Refactor Taproot functions to accept any type implementing `Into<XOnlyPublicKey>`,
instead of requiring `XOnlyPublicKey` directly. This improves ergonomics when working
with compatible types, avoiding unnecessary `.into()` conversions at call sites.
3bb6c73f2d Add methods to retrieve inner types (Shing Him Ng)
Pull request description:
Resolves#4345
ACKs for top commit:
tcharding:
ACK 3bb6c73f2d
apoelstra:
ACK 3bb6c73f2d3edd1165b7b7f3a833fa471786e166; successfully ran local tests; should backport to 0.32.x
Tree-SHA512: c89017bbc2126ec62c756c4ee9b49dcc8b94a3063a8155aadcf7c69a6f0bc9337baedffe7f52a4ab6f0b738302bea683391d394483c4c7eefbb622b97d34d26c
For TweakedKeypair, `to_inner` is also renamed to `to_keypair` to maintain
consistency. Similarly, `to_inner` is renamed to `to_x_only_pubkey` for
TweakedPublicKey
b9a12043b0 bip32: return error when trying to derive a path too deep (Andrew Poelstra)
73781e047b bip32: rename Error to ParseError (Andrew Poelstra)
a66ad97fb6 bip32: split InvalidChildNumber and InvalidChildNumberFormat out of error (Andrew Poelstra)
a891fb9b74 bip32: remove unused error variants (Andrew Poelstra)
f0a237c001 bip32: split out DerivationError from the main error enum (Andrew Poelstra)
32d96f6c33 bip32: make Xpriv::new_master be infallible (Andrew Poelstra)
0e5e021b69 bip32: change several cryptographically unreachable paths to expects (Andrew Poelstra)
Pull request description:
This PR makes a first pass at splitting the `bip32::Error` type into multiple distinct types -- one for derivation (which can fail if you try to derive a hardened child of an xpub, or if you try to derive too many layers), one for parsing child numbers or derivation paths, and one for parsing xkeys. Along the way it cleans up a ton of weird things and typos, e.g. the psbt `GetKeyError` having an unused `Bip32` variant whose display text references "bip 23".
Because all the error types get renamed, every part of this PR is an API break, but only the last commit is a "real" API break which uses the new `DerivationError::MaximumDepthExceeded` error variant to return an error when trying to derive a path of length 256 or longer. This means that `Xpriv::derive_xpriv` again returns an error result.
I will make a simpler version of this last commit suitable for backporting to 0.32.x. (In 0.32.x `Xpriv::derive_priv` returns an error, so we can change it to error out on max-depth-exceeded without breaking the API. Sadly most users are likely to be unwrapping the error because in 0.32.x currently the error path is cryptographically unreachable...but at least this way the panic will be in their control rather than ours.)
Fixes https://github.com/rust-bitcoin/rust-bitcoin/issues/4308
ACKs for top commit:
tcharding:
ACK b9a12043b0
Tree-SHA512: 688826126ff24066c6de9de3caa73db68c516ba8893d64d9226a498774a2fb9db7d7fd797375c6b3f820588c178632e1e6e8561022dfa7042a560582bf1509b4
The bip32::Error enum is now exclusively used for errors related to
parsing and decoding. It is still a little messy (mainly: it contains a
base58 variant which is used when parsing a string but not when decoding
from bytes) but much cleaner than it was.
I'm not sure why we do not use consensus encoding currently for encoding
and decoding scripts to/from hex strings. Many tests include hard coded
hex which do not include the length prefix.
- Add a pair of encoding functions to encode/decode to/from hex without
the length prefix.
- Make `to_hex` and `from_hex` expect the length prefix i.e., use
consensus encoding.
This makes the API easier to use because the various encoding APIs can
be use together now eg `consensus::encode_hex` and `ScriptBuf::from_hex`.
c009a42e60 Update internal_macros.rs (GarmashAlex)
a4253fa5d9 Update mod.rs (GarmashAlex)
604b095540 Update serialize.rs (GarmashAlex)
024f87e655 Update error.rs (GarmashAlex)
1af34f92c5 Update message_compact_blocks.rs (GarmashAlex)
f554b01e82 Update params.rs (GarmashAlex)
Pull request description:
This PR addresses several minor issues across the codebase, including:
- Fixing typographical errors in comments and documentation (e.g., "deserilaization" → "deserialization", "send" → "sent").
- Improving sentence clarity and grammar in doc comments (e.g., correcting sentence structure and word choice).
- Enhancing code readability without changing any logic or functionality.
These changes are purely cosmetic and aimed at improving maintainability and developer experience.
ACKs for top commit:
apoelstra:
ACK c009a42e60f0b4302506f5fc104af38a6c15be21; successfully ran local tests
Tree-SHA512: 915e2c9444d8f2810ba5cd51d3066685aea5a39d98303c793a854aea6da016cab2c457dd71c0b6549d29d6443db1292ebdb06d25f693741b2eca3979bf67cfab
d6296cd3d1 Remove usage of hex::test_hex_unwrap (Tobin C. Harding)
37035e20e8 Simplify and improve transaction benchmarks (Tobin C. Harding)
Pull request description:
We have the `hex_lit` dependency for converting a hex string literal to an array.
Currently we have a `test_hex_unwrap` macro in the `hex v0.3.0` release but not on either `master`
or the upcoming `v1.0.0-alpha.0` release. This is making PRs around releasing and depending on the
release more noisy than required.
Introduce a `test_hex_unwrap` macro in internals for usage when the input is not a string literal.
Use `hex_lit::hex` where possible (often needing an additional call to
ACKs for top commit:
apoelstra:
ACK d6296cd3d1989cf28d67a5329ad60da4f814ba92; successfully ran local tests
Kixunil:
ACK d6296cd3d1
Tree-SHA512: eab3573f6b7fee408ae11821b77e56cbaddf7cc4540bdc31ed7ef9eb3f25987f50e484f1553aaaa9709367e614eb77ed36250875d0faf5a51ab3fe709d4d4054
We have the `hex_lit` dependency for converting a hex string literal
to an array.
Currently we have a `test_hex_unwrap` macro in the `hex v0.3.0` release
but not on either `master` or the upcoming `v1.0.0-alpha.0` release.
This is making PRs around releasing and depending on the release more
noisy than required.
Use `hex_lit::hex` where possible (often needing an additional call to
`to_vec()`) and where not possible use `Vec::from_hex`.
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.
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
95ad91cdb6 hashes: Remove the GeneralHash trait (Tobin C. Harding)
6426e59c63 Remove unused trait import (Tobin C. Harding)
791501eabc io: Use function in place of GeneralHashExt (Tobin C. Harding)
2b6ef31469 hashes: Add hash_byte_chunks function to modules (Tobin C. Harding)
d3846895d7 hashes: Add hash function to modules (Tobin C. Harding)
e1bac7da55 Bound HmacEngine on HashEngine (Tobin C. Harding)
ab63b7a0ff Add Hash type and finalize method to HashEngine (Tobin C. Harding)
84623ffaf9 Add hash_again regression test (Tobin C. Harding)
Pull request description:
This is the done as part of #4051.
Requires some surgery on the `Hmac` and `Hkdf` types as well as a few other patches to maintain the logic that is currently provided by the trait. Final patch is a pure red diff - enjoy.
ACKs for top commit:
Kixunil:
ACK 95ad91cdb6
apoelstra:
ACK 95ad91cdb64c8870d3eb992090bab7a70d1369b9; successfully ran local tests
Tree-SHA512: bfd215de51c115144c6f5b3430b17dad5d770a8c876fe3775af2828ec620a1f4e4155e63bb18dac244a82c3840413a615d55a0fef91b6949d3de319aa5bb8c2f
Add a standalone `hash` function that is a drop in replacement for
`GeneralHash::hash`. Do not add it to `hmac` - this is in parity with
the current code because `Hmac` does not implement `GeneralHash::hash`.
Use the new function in `bitcoin` removing all occurrences of
`GeneralHash` from `bitcoin`.
In `hashes` replace usage of `GeneralHash::hash` with the new `hash`
function.
ecc5791930 Add test checking `XOnlyPublicKey::deserialize` (Martin Habovstiak)
bbe87eccf2 Fix bug in PSBT `Deserialize` for `XOnlyPublicKey` (Martin Habovstiak)
8efacd4dda Deprecate `PrivateKey::from_slice` method (Martin Habovstiak)
0d5cd7af43 Add `from_byte_array` to `PrivateKey`. (Martin Habovstiak)
1778fea66e Add a test checking `PrivateKey::from_slice` (Martin Habovstiak)
b87ddc0043 Don't panic in `PrivateKey::from_slice` (Martin Habovstiak)
Pull request description:
This fixes the bug introduced in #4154 and deprecates `from_slice` method in favor of `from_byte_array` (see commits).
Closes#4191
ACKs for top commit:
jamillambert:
ACK ecc5791930
apoelstra:
ACK ecc5791930b88581fcbc8f2417b0221486bd1031; successfully ran local tests
Tree-SHA512: 05117c68db7b46605dba6104ee7696220416f4efaef1fff01843a910037e4c96bfebc45fcdd16f875e5e800bb33af17193c4aa9b0b1593807df5153e7e935c22
During upgrade of `secp256k1` a number of function calls needed to be
rewritten to not use `from_slice` but `from_byte_array`. Unfortunately,
the conversion 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.
a4b9c196b1 Manually update nightly version (Tobin C. Harding)
Pull request description:
While trying to use the `macro_use_imports` lint I found that there is a bug in last weeks nightly. It has been fixed already so lets update.
Update to todays nightly compiler. Doing so causes some new linter warnings, for now we just allow them.
ACKs for top commit:
apoelstra:
ACK a4b9c196b13a2029ef0de198114a29c71a192d03; successfully ran local tests
Tree-SHA512: a982ba05713c214af3b9375ecf37b66aea07ec0330392c30d2dbe3c9b40e1219b587ee43222f2355292b9b374a6a1d8a19d18b559bb47d33e544ed78509a51b8
While trying to use the `macro_use_imports` lint I found that there is a
bug in last weeks nightly. It has been fixed already so lets update.
Update to todays nightly compiler. Doing so causes some new linter
warnings, for now we just allow them.
We would like to return an error when doing math ops on amount types.
We cannot however use the stdlib `Result` or `Option` because we want to
implement ops on the result type.
Add an `AmountOpResult` type. Return this type from all math operations
on `Amount` and `SignedAmount`.
Implement `core::iter::Sum` for the new type to allow summing iterators
of amounts - somewhat ugly to use, see tests for example usage.
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.
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.
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.
The Amount type provides better type safety and is more appropriate in
this context than u64. Currently the checked arithmetic operations for
Amount and u64 are identical in behavior. Therefore, this refactor does
not result in any behavior change and is purely cosmetic.
Tobin C. Harding
merge-script
frankomosh
Fmt Bot
Erick Cestari
Shing Him Ng
GarmashAlex
Fallengirl
jike
Martin Habovstiak
healthyyyoung
19年梦醒
yancy