3b8164139f primitives: Add docs section for script hex API (Tobin C. Harding)
6b90e42e78 Finalize the script hex APIs (Tobin C. Harding)
Pull request description:
In #4316 we made some 'improvements' to what script functions and trait implementations do and do not include the length prefix. Iterate again on it as described here: https://github.com/rust-bitcoin/rust-bitcoin/pull/4316#issuecomment-2847710436
- Patch 1 does the changes
- Patch 2 adds some more docs, requires a grammarian to check my Aussie lingua
ACKs for top commit:
apoelstra:
ACK 3b8164139f6ecebc97b66a299b4a87c2288d8a76; successfully ran local tests
Tree-SHA512: 481db88ae1b6f5751e81e4cd126f545cfc34bef6dcfcf857f1c7464aeb41f5de95fc4582c015abde04372fe025efa13cdf2906e75517f62cff3ddec05c4d9711
Recently we made an attempt at making the hex APIs for scripts easier to
use, better documented, and shown via an example.
After that work we decided it would be better if `LowerHex`/`UpperHex`
did not have the prefix. We also wanted to further clarify the inherent
function names to make the all explicit.
See GitHub issue #4316 for the thread of discussion.
Note that this PR does not require changes to the serde regression test
which were non changed in the original work either.
Name the type exactly what it is. This used to be `Time`, then we tried
`MtpInterval`.
Note that this makes some of the original function names overly verbose
e.g., `NumberOf512seconds::from_512_second_intervals()` but given the
curlyness of locktimes too verbose is better than too terse. Also this
type, along with `NumberOfBlocks` is not going to be in very wide use so
the ergonomic hit is worth the additional clarity.
Name this type exactly what it is. Note for the error we just use
'height' even though this is a bit stale but the general concept is ok
in the error type because the name is long already.
47c77afaac units: delete MtpAndHeight type (Andrew Poelstra)
d82b8c0bcb primitives: stop using MtpAndHeight (Andrew Poelstra)
72d5fbad73 units: stop using MtpAndHeight in locktime::relative is_satisfied_by methods (Andrew Poelstra)
d933c754f5 units: change type of MtpHeight::to_mtp to BlockMtp (Andrew Poelstra)
dcbdb7ca8a units: add checked arithmetic to Block{Height,Mtp}{Interval,} (Andrew Poelstra)
4300271f0c units: add constructor for absolute::Mtp from timestamps (Andrew Poelstra)
4e4601b3d5 units: rename BlockInterval to BlockHeightInterval (Andrew Poelstra)
cb882c5ce1 units: add global `BlockMtpInterval` type (Andrew Poelstra)
4e3af5162f units: add global `BlockMtp` type (Andrew Poelstra)
a3228d4636 units: pull u32 conversions for BlockHeight/BlockInterval into macro (Andrew Poelstra)
Pull request description:
This is a more involved PR than I'd expected but hopefully the individual commits make sense and are well-motivated. Essentially, my goal was to replace `MtpAndHeight` as used by relative locktimes with a pair of `Mtp` and `Height`.
However, relative locktimes, when given a MTP/Height for the UTXO creation and the chain tip, are roughly modeled as "take a diff of MTPs to get a `relative::MtpInterval`, a diff of heights to get a `relative::HeightInterval`, and compare to the locktimes". *However*, we have no standalone MTP type to "take a diff of", and also there are failure modes when creating the diffs (e.g. if the diff would exceed the range of `MtpInterval` or `HeightInterval`).
So I backed up and decided to use the existing `BlockHeight`/`BlockInterval` as the type to "take a diff of". I needed to introduce a `BlockMtp`/`BlockMtpInterval` to work with MTPs. These types have full-u32 range, unlike the similarly-named types in `units::locktimes::absolute`. I then needed to add some conversion methods. Along the way, I cleaned up the APIs and documentation, added checked arithmetic, etc., as needed.
See the individual commit messages for more detail.
I believe the resulting API is much more consistent and discoverable, even though it has more surface than the old API.
I considered splitting this into 2 PRs but I think the first half of the changes aren't well-motivated with out the second half. Let me know.
ACKs for top commit:
tcharding:
ACK 47c77afaac
Tree-SHA512: ebe19a5b1684db8c2d913274347c994026aaa0dcdd79349c237920a82fe55560777278efdbbc7f1b1424c9391d9bbd891ae844db885deea75288000437a8a287
13cbead947 Use NumOpResult instead of Option (yancy)
002a0382aa Mark function constant (yancy)
Pull request description:
Prefer the more descriptive NumOpResult return type over Option where return types are fallible.
Closes https://github.com/rust-bitcoin/rust-bitcoin/issues/4419
ACKs for top commit:
apoelstra:
ACK 13cbead94766987f59482b1fbc1d0ebd0799737c; successfully ran local tests
tcharding:
ACK 13cbead947
Kixunil:
ACK 13cbead947
Tree-SHA512: 1a870962dcafe901a07abd93bd8075e41696341c1a4b3efef615493c73d5e5728bbc2326f8c2c95b9034ab001d0b3c668c9d64793ab03486d3a19f31df907c96
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
7ecef176f9 Fix documentation error for `TweakedPublicKey::serialize` (Daniel Roberts)
Pull request description:
Fixes an ancient copy/paste error in documentation ( `secp256k1::schnorrsig::PublicKey::serialize()` docs copied from ECDSA docs, which was copied into rust-bitcoin)
Is there a threshold beneath which a PR is too trivial?
ACKs for top commit:
apoelstra:
ACK 7ecef176f9523cd8fece7e8e71040507f46fb9c2; successfully ran local tests; thanks!
tcharding:
ACK 7ecef176f9
Tree-SHA512: 9b7469d34eadfcabc93264c114f292c415d2dbb09b41ec05de4ac399677d5c80f1d09ecd0c382680996450824f1fd60503d3e3d3ec8bdd8135cebdf7ef82fe0d
5ba763f1a2 Update Github CI to rustc nightly-2025-05-02 (Jamil Lambert, PhD)
09132b80e1 Fix rustdoc compile_fail example (Jamil Lambert, PhD)
282434d4bd Use variable directly in format! string (Jamil Lambert, PhD)
2fbbc825c9 Allow uninlined format args (Jamil Lambert, PhD)
Pull request description:
There is a new lint error on nightly-2025-04-25 "variables can be used directly in the `format!` string".
The existing syntax `format!("{}", x)` is more commonly used than `format!("{x}")` therefore allow it in existing code.
Also the rustdoc example in #4259 now causes the new nightly to fail CI because of the unused variable.
Patches in the PR:
- Exclude the lint to allow the existing syntax in `format!` strings in all crate `lib.rs`, `build.rs.` and test files.
- Use the variables in the `format!` string for all cases in `bitcoin/examples/` since there are no other allowed lints in examples.
- Correct the function names in the rustdoc example and prefix the unused variable with an underscore.
- Update rustc to nightly-2025-05-02 (2025-04-25 had a bug which is fixed in 2025-05-02).
ACKs for top commit:
apoelstra:
ACK 5ba763f1a2ebea2cb80ee50a80228e6bda11936f; successfully ran local tests
Kixunil:
ACK 5ba763f1a2
Tree-SHA512: 20b97d2bedc631715c2b541285559a6ab84bbdb8f2f11d7282bdfecadba0cc8781a1973f0c01c25432aaceaad09e3ddbf59afe54c0bba54768e93ed9d5e50d5a
51fe619fe0 Set deprecation to released date of to_inner (Tobin C. Harding)
Pull request description:
In #4373 we added a couple new conversion methods and deprecated the `to_inner` ones. During that the deprecation date was set to `0.33.0`.
We have backported the changes and will deprecate in `0.32.6` so set the version number now so we don't forget later.
ACKs for top commit:
apoelstra:
ACK 51fe619fe0c9e154a7979a3a71c56707bfa73e9c; successfully ran local tests
shinghim:
ACK 51fe619fe0
Tree-SHA512: 9f8badee92684204966107013b272ff51b88bee632bd6779ec6ecf2c78221c2228428a3c180200db5ae3205d042e58a6919dc9e621e153171e200e9b81c628d6
For our relative locktime API, we are going to want to take differences
of arbitrary MTPs in order to check whether they meet some relative
timelock threshold.
However, the `locktime::absolute::Mtp` type can only represent MTPs that
exceed 500 million. In practice this is a non-issue; by consensus MTPs
must be monotonic and every real chain (even test chains) have initial
real MTPs well above 500 million, which as a UNIX timestamp corresponds
to November 5, 1985.
But in theory this is a big problem: if we were to treat relative MTPs
as "differences of absolute-timelock MTPs" then we will be unable to
construct relative timelocks on chains with weird timestamps (and on
legitimate chains, we'd have .unwrap()s everywhere that would be hard to
justify). But we need to treat them as a "difference of MTPs" in *some*
sense, because otherwise they'd be very hard to construct.
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.
There is a new lint error on nightly-2025-04-25 "variables can be used
directly in the `format!` string".
Use the variables in the `format!` string for all cases in
`bitcoin/examples/`.
There is a new lint error on nightly-2025-04-25 "variables can be used
directly in the `format!` string".
Exclude the lint to allow the existing syntax in `format!` strings.
53d32c9e4f bitcoin: remove torv2 support (Bruno Garcia)
Pull request description:
This PR removes support to TorV2 since it's deprecated and no longer useful to have it.
ACKs for top commit:
apoelstra:
ACK 53d32c9e4f0ef0f3b2c7d4dcba42e3ac5344f78a; successfully ran local tests
tcharding:
ACK 53d32c9e4f
Tree-SHA512: 69a2ba399d5eac7f132519ab83362fbd8739d9e975795e441cefa75896ddbf4041db2125ffde51316f9ad69aa0b62c8b226ccff042b0dae6d3c615826bc339f4
In #4373 we added a couple new conversion methods and deprecated the
`to_inner` ones. During that the deprecation date was set to `0.33.0`.
We have backported the changes and will deprecate in `0.32.6` so set the
version number now so we don't forget later.
44d01d0ce4 Fix broken changelog links (GarmashAlex)
Pull request description:
Fixed malformed markdown links in bitcoin/CHANGELOG.md that were causing errors. Specifically, removed double parentheses from links to primitives and units CHANGELOG.md files, ensuring proper rendering and accessibility of the documentation.
ACKs for top commit:
apoelstra:
ACK 44d01d0ce468549c64f8362c552c434eb1c3503b; successfully ran local tests
Tree-SHA512: e2d2598070c3df8e4dfa59c019d8ab9a1f86731a70624ceef80b68849454bd3828c558ef8eb30bb9d56c648fcacccea459d2a7a536021d65c2fbcbe5595916ce
Prefer the more descriptive NumOpResult return type over Option where
return types are fallible.
The returned type NumOpResult is already annotated as must_use per the
lint, so must_use can be removed after changing the method return types
to NumOpResult.
Fixed malformed markdown links in bitcoin/CHANGELOG.md that were causing errors.
Specifically, removed double parentheses from links to primitives and units
CHANGELOG.md files, ensuring proper rendering and accessibility of the documentation.
826acb8273 units: rename relative::HeightInterval constructors (Andrew Poelstra)
39b4f7670d units: rename relative::Height to HeightInterval (Andrew Poelstra)
d3619cc1bc units: deprecate relative::MtpInterval::to_consensus_u32 (Andrew Poelstra)
1a6b8b4c7a units: rename relative::Time to MtpInterval (Andrew Poelstra)
39b057fade units: rename absolute::Height consensus functions (Andrew Poelstra)
5a8f33f380 units: rename absolute::Mtp consensus functions (Andrew Poelstra)
8ffcd2cf30 units: rename absolute::Time to absolute::Mtp (Andrew Poelstra)
Pull request description:
This PR does a whole bunch of renames to the `units` locktime modules. These modules contain the underlying units for locktime types and hopefully aren't used directly too often, which should minimize the disruption from these renames.
This fixes a number of issues:
* Confusion between blocktimes and MTPs (which in fairness, *are* blocktimes, but they're not the one that users are likely to reach for)
* Constructor and conversion names that imply there is a "consensus encoding" for these bare units, which corresponded to the consensus encoding of the corresponding locktimes/sequence numbers
* `from_*` methods without `to_*` methods and vice-versa (overlaps with the above)
* the horribly named `value` method on relative heights and times (but not absolute ones)
This PR does **not** remove the `MtpAndHeight` type, nor does it add constructors for `Mtp` from lists of blocktimes. This is because the PR was too big already and I felt I should split it up.
Alternate to #4427
ACKs for top commit:
tcharding:
ACK 826acb8273
Tree-SHA512: 6e0491e17927625cde85c2cf92ff152a10613e632474122a626ee31b662d21c09fcb9fa3014c44708c97536535a33845cbbcd81e73dcdf98e9ee9fd6143c698f
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
This is disruptive, but makes the type name consistent with
`MtpInterval` and also greatly improves clarity, helping to distinguish
between absolute and relative locktimes and reminding the author (and
reviewer) of locktime code that this needs to be a diff.
This is not a generic UNIX timestamp, but rather a MTP restricted to
have values between 500 million and u32::MAX. Most importantly, it is
*not* a blocktime, which is what is implied by its name and
constructors.
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.
5d5a19793a Run the formatter (Tobin C. Harding)
2b72f1f30b Make Lower/Upper hex print scripts with len prefix (Tobin C. Harding)
c27b95fb0d Make script to/from hex use consensus encoding (Tobin C. Harding)
f4bc58dc48 bitcoin: Add a script encoding example (Tobin C. Harding)
Pull request description:
Done while investigating #3910 - this PR is quite invasive.
- Patch 1 adds an example that shows the current behaviour of various API calls for converting scripts to/from hex.
- Patch 2 adds a pair of functions that do not use the length prefix and makes script to/from_hex use the prefix.
- Patch 3 makes `LowerHex` and `UpperHex` use the len prefix also
- Patch 4 runs the formatter
Explicitly keeps serde untouched - i.e., without the len prefix
ACKs for top commit:
apoelstra:
ACK 5d5a19793aae73ff09d7064455a1a995eb796c28; successfully ran local tests
Tree-SHA512: dbbec372ea7b01818fce68ffb807a4531f068e10147e8bedf37b77f66a068a628e380549c379c061b868973e97806ada59729248b03bbd1cf6809f6098170cb6
Add the length prefix when formatting hex strings by way of `LowerHex`
and `UpperHex`.
This looses formatting options because I can't remember right now how
not to - again.
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`.
Our encoding/decoding support for scripts is currently a bit convoluted.
Add an example that shows how all the APIs go together.
Note this highlights a potential problem. `to_hex_string` is in
`bitcoin` and therefore has access to consensus encoding but does not
include the length prefix. This means that the output string from
`to_hex_string` cannot be parsed with
`consensus::encode::deserialize_hex` - which is surprising and has lead
to difficulty by at least one user trying to debug.
8eeceed450 test: extend `valid_v1_witness_programs` test to include P2A (Luis Schwab)
647526dd1d chore: fix docs for `impl WitnessProgram` and P2A (Luis Schwab)
Pull request description:
Closes#4124.
This PR fixes documentation on `impl WitnessProgram` by replacing instances of `address` to `[WitnessProgram]`, adds punctuation and capitalization where it was lacking and extends the `valid_v1_witness_programs` test to include the P2A output.
ACKs for top commit:
Kixunil:
ACK 8eeceed450
apoelstra:
ACK 8eeceed450f7414c8a286a9e47b6f04b652b18ef; successfully ran local tests
Tree-SHA512: 6e62a8de7135da04d6330d2b5596a2cd19da8a849f8c8c892f53578a8690152b23facf58149d4139ae088f1ab297d3526094617c3549e688819e9b1f3688de8b
afd4ec8c5e test: push minimality check for zero(empty) (ChrisCho-H)
Pull request description:
Following https://github.com/rust-bitcoin/rust-bitcoin/pull/4368.
I omitted to test OP_0(empty bytes) and can be covered by this PR.
ACKs for top commit:
apoelstra:
ACK afd4ec8c5e345a1df5abc46076c843e96a226b77; successfully ran local tests
Tree-SHA512: a3643227f9dfde71d5c5707bf11804e0e26eff43346c0443abdd805f0ffad284c3090e22a0bda34e54e1185a980adc7511724db401c04b55a8be79d67a3fce6d
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
merge-script
Tobin C. Harding
Erick Cestari
Jamil Lambert, PhD
Daniel Roberts
Bruno Garcia
yancy
GarmashAlex
Fmt Bot
Shing Him Ng
Fallengirl
Alex Pikme
Luis Schwab