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.
To prevent rounding errors converting to and from f64 change
`Amount::MAX` to `MAX_MONEY` which is below the limit in f64 that has
issues.
Add checks to `from_str_in`, `checked_add` and `checked_mul` that the
result is below MAX, where previously a u64 overflow was relied on.
Change tests to account for new lower MAX that is within the range of
SignedAmount and does not overflow so easily
Remove overflow tests
`Amount::MAX` is now below `u64::MAX` and within the range of values for
`SignedAmount`. These tests therefore do not overflow.
In effective_value there is no error with `Amount::MAX` and the correct
value is returned.
In psbt the removed test is effectively the same as the previous test.
Modify `Amount` tests to work with new `MAX`
Tests need to be changed that checked values above the new `MAX` or
`Amount::MAX` was out of range for `SignedAmount` which it isn't anymore
9aebb96fb9 Fix psbt fuzz crash (Sanket Kanjalkar)
Pull request description:
Fixes: https://github.com/rust-bitcoin/rust-bitcoin/issues/3628
This occurs when combining two PSBTs with different xpub key sources. Added a length check before indexing into slices to prevent out-of-bounds access.
For some reason, the precommit hook complained about non-ascii files. I don't think any of the names here are non-ascii
ACKs for top commit:
apoelstra:
ACK 9aebb96fb99e8e9e019663659c6eff851a62f2ce; successfully ran local tests; thanks!
tcharding:
ACK 9aebb96fb9
Tree-SHA512: b61274c594bc1f2ea4d04c8a7ace673a7632bb9ea31f59b56779a008c35e61281ea4f6b869990d886779e3e556932a3b2e8b015733ef18e236f12ca77e211c26
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.
Deprecate the `Script::to_bytes` function in favour of `to_vec` as we
are doing elsewhere.
Note that `ScriptBuf` has `into_bytes` because it does not copy.
Potentially this should be deprecated in favour of `into_vec`?
Note that in regards to the `to_` prefix this naming as valid according
to convention because the `Script` type is borrowed and `to_vec` copies
the underlying bytes.
During this release cycle we deprecated `to_vec` in favour of
`to_bytes`, we have since reversed our position on the name.
Remove the deprecation of `to_bytes` from the three types that had it
and use `to_vec`.
WARNING: This is not like all the other extension traits.
Because of the use of generics on various `Transaction` methods it is
not easily possible to use the `define_extension_trait` macro.
Manually create the extension traits (public and private) for the
`Transaction` type. This is quite ugly but c'est la vie
(Includes two in the `transaction` module and one in the
`consensus_validation` module.)
c89b816437 psbt: Fix bug in Subtype consensus_encode (Tobin C. Harding)
Pull request description:
In #2906 we switched from using a `u8` for type keys to using a `u64` and encoding as a compact int (inline with the spec). Note that a `u8` encodes to the same bytes as a `u64` when the value is < 252.
In that patch, I introduced a bug because the length returned by `PoprietaryKey::consensus_encode` uses a hard code 1 for the length of the encoding (because of single byte) instead of the variable length for the new compact encoding.
Bug showed up in fuzzing, and was isolated by Jamil - mad props.
Fix: #3501
ACKs for top commit:
jamillambert:
ACK c89b816437
apoelstra:
ACK c89b8164377123eb20476636f2f5271c6a687406; successfully ran local tests
Tree-SHA512: 1b61b6a9ece197d74038ceedb447fd3ca21db8e2a6a96c9281a99ac232c18c3ca55da8e3f46930401714d3575e9a406a36e4f44929ca963208a5df4be6b46cfb
025a8773bf Use fully qualified path in macro (Tobin C. Harding)
Pull request description:
Using fully qualified paths in macros reduces maintenance burden. We have one macro where we use relative path to access the `psbt` module.
Refactor only, no external change.
ACKs for top commit:
apoelstra:
ACK 025a8773bf63aacdaca011ef000f41a85a961567; successfully ran local tests; will one-ACK merge
Tree-SHA512: eb5923a48ae4d82499679a58375ef7d2e8ba85c91671e350f7be19f0372750a269f44dd2f05f4a70ed0c7f277b160400eb41ff1d42b90e6057f1344be7e11a89
Using fully qualified paths in macros reduces maintenance burden. We
have one macro where we use relative path to access the `psbt` module.
Refactor only, no external change.
In #2906 we switched from using a `u8` for type keys to using a `u64`
and encoding as a compact int (inline with the spec). Note that a `u8`
encodes to the same bytes as a `u64` when the value is < 252.
In that patch, I introduced a bug because the length returned by
`PoprietaryKey::consensus_encode` uses a hard code 1 for the length of
the encoding (because of single byte) instead of the variable length for
the new compact encoding.
Bug showed up in fuzzing, and was isolated by Jamil - mad props.
merge-script
GarmashAlex
Fallengirl
Tobin C. Harding
Fmt Bot
jike
Erick Cestari
Martin Habovstiak
healthyyyoung
19年梦醒
Shing Him Ng
yancy
Jamil Lambert, PhD
Sanket Kanjalkar