4ccecf5dec Fix stale Height type link (Tobin C. Harding)
caebb1bf73 units: relative: Do minor rustdocs fixes (Tobin C. Harding)
40bb177bc2 Put is_satisfied_by functions together (Tobin C. Harding)
480a2cd62a Favour new function `from_mtp` over deprecated (Tobin C. Harding)
f9d6453d5b Shorten locktime type term (Tobin C. Harding)
727047bd39 Fix off-by-one-bug in absolute locktime (Tobin C. Harding)
3ffdc54ca5 Fix off-by-one bug in relative locktime (Tobin C. Harding)
a2ff8ddbbb Improve relative::LockTime is_satisfied_by_{height, time} (Tobin C. Harding)
Pull request description:
Make the APIs uniform in relative and absolute locktimes in relation to the `is_satisfied_by` functions. In doing so improve the API and fix an off-by-one bug when checking satisfaction of locks by height.
Done in three patches but maybe should be squashed? Probably easiest to review by looking at all the `is_satisfied_by*` functions and convincing yourself we got it right.
EDIT: Now has 5 cleanup patches also (mostly docs cleanups).
ACKs for top commit:
apoelstra:
ACK 4ccecf5decfead9818b74fbdee73115c349e2f3e; successfully ran local tests
Tree-SHA512: 9206cb464a06647510a35a7d564062823117e75df60251969be458616f4f5d04acf0aada53dbf7d493a2a2a72d26b3a300417a6499e45413d5f2a011538b7826
To get more precision use sats per million virtual bytes.
To make review easier keep most calls in tests using
`FeeRate::from_sats_per_kwu` and just unwrap. These can likely be
cleaned up later on if we want to.
For `serde` just change the module to `_floor` and leave it at that. The
serde stuff likely needs re-visiting before release anyways.
Currently we get the fee_rate per kwu then multiply it by 4. Instead
lets add a per_kvb function to `FeeRate`. We are about to change the
internal representation of `FeeRate` to use MvB so for now just panic on
ovelflow.
Also these are fee _rates_ - we already have suboptimal names from Core
in the consts in `policy`, no need to let them infect other identifiers.
In preparation for changing the internal representation of `FeeRate` to
use MvB reduce the max value by 4_000.
Done separately to make the change explicit.
The `FeeRate::checked_mul_by_weight` function currently returns a
`NumOpResult` but all other `checked_` functions return an `Option`.
This is surprising and adds no additional information.
Change `checked_mul_by_weight` to return `None` on overflow. But in
`to_fee` saturate to `Amount::MAX` because doing so makes a few APIs
better without any risk since a fee must be checked anyway so
`Amount::MAX` as a fee is equally descriptive in the error case.
This leads to removing the `NumOpResult` from `effective_value` also.
Note that sadly we remove the very nice docs on `NumOpResult::map`
because they no longer work.
Fix: #4497
In preparation for changing the inner representation of `FeeRate` add
floor and ceil versions of the getter function `to_sat_per_kwu`.
For now both functions return the same thing but still call the
correct one so that when we change the representation we do not need
to re-visit them.
Motivated by moving the `p2p` module to its own crate. `TryFrom` and
`From` are already implement for converting to and from
`Network`/`Magic`. The methods related to `Magic` are removed from
`Network`, as well as any reference to `p2p` in the documentation, as
`bitcoin` would no longer depend on `p2p`.
The deser roundtrip test are relocated to `p2p/mod.rs`
6335c623f6 fix(p2p): remove `AddrV2` <> `SocketAddr` conversions (Luis Schwab)
Pull request description:
This PR reverts #4521 due to loss/creation of port information during the conversion. Users should use the `AddrV2` <> `IpAddr` conversion instead.
Closes#4566
ACKs for top commit:
Kixunil:
ACK 6335c623f6
tcharding:
ACK 6335c623f6
Tree-SHA512: f183756467ca17bb7e7078023a769ec33d3deb4146ab7ec63ef961107ab93ec975c9adffd9eeddc79250abfa1cee9eccbbcaef7466cf4c2c21205b69d8e9eb4f
9d956e8643 test: remove redundant `ServiceFlags` test (rustaceanrob)
Pull request description:
The `ServiceFlags` type is already tested within p2p/mod.rs with a nearly identical test. This type also has nothing to do with `network`
ref: https://github.com/rust-bitcoin/rust-bitcoin/blob/master/bitcoin/src/p2p/mod.rs#L400
ACKs for top commit:
tcharding:
ACK 9d956e8643
apoelstra:
ACK 9d956e8643214b3c1ad0e42cc630e2f35f7b7994; successfully ran local tests
Tree-SHA512: d113609070e3df6d44b1ed57c40c1bce251d59fa22eb1abbf6c576fd84e21464553bee7fa97eba000ad9c1b8ef98e2ba04aec7077486273de9b7e54341ba894b
9dac4d69e0 Implement CheckedSum for Weight iterator (yancy)
0dbcd09bbc Move CheckedSum trait to crate root (yancy)
Pull request description:
* Move CheckedSum to the crate root so that other types can be added
* Add Weight to CheckedSum
ACKs for top commit:
tcharding:
ACK 9dac4d69e0
apoelstra:
ACK 9dac4d69e0f142e9a2dc4b61ea49365a8cae3f4b; successfully ran local tests
Tree-SHA512: 3e4b7f79074e23493ccd17a026542081f0d7a811f4f35edb7994ada95bf414a166531f142ad4986d27fcec448209b2ffa56813b495b5df6b6e8fcd589392e0c1
All of the `Encodable` implementations are defined within `p2p` with the
exception of `p2p` types that are a `Vec`. To fully decouple `p2p` from
`encode` we can define these in `p2p` like the others. This is the final
step in removing anything `p2p` related from the rest of `bitcoin`.
In preparation to move `p2p` to its own crate, any `Vec<T>` that satisfies
`T: Encodable` will not trivially implement `Encodable` because of the
orphan rule. A type defined within p2p may implement `Encodable`,
however, and the simplest possible type is one that simply wraps the
vector. Three types that will implement `Encodable` within `p2p` are
added here.
Often we want to have the global-context feature in secp256k1 without
having to add manually the secp256k1 dependency and enabling the
global-context feature.
Having the ability to do that directly from bitcoin without having to
add secp256k1 and do the whole tango of tightly coupling the two
dependecy versions together, e.g. bitcoin 0.32.x and secp256k1 0.29.x
would be really nice and would also simplify a lot code maintainability
for anyone who depends on bitcoin.
This needs to be backported to 0.32.x, which I'll gladly do as well.
In order to add other types to CheckedSum, remove from the Amount
module. In so doing, other types added to CheeckSum do not need to be
imported into Amount.
2481695b45 Add tests for BIP-373 PSBT_{IN,OUT}_MUSIG2_PARTICIPANT_PUBKEYS serialization and deserialization (Daniel Roberts)
3e8e6d9aa1 Add BIP-373 PSBT_{IN,OUT}_MUSIG2_PARTICIPANT_PUBKEYS serialization and deserialization (Daniel Roberts)
Pull request description:
This change adds support for serializing and deserializing two PSBT keys from BIP-373: `PSBT_IN_MUSIG2_PARTICIPANT_PUBKEYS` and `PSBT_OUT_MUSIG2_PARTICIPANT_PUBKEYS`
This is a part of #4207 that can be implemented independently of the rest (which depends on https://github.com/rust-bitcoin/rust-secp256k1/pull/716). I believe this satisfactorily avoids changing things multiple times on end users, *however* it's not *completely* transparent to end users, since any code that currently accesses these fields through `unknown` will need to be updated. Later, when `PSBT_IN_MUSIG2_PUB_NONCE` and `PSBT_IN_MUSIG2_PARTIAL_SIG` are supported, code will need to be updated a second time to retrieve them from the correct place instead of `unknown`. I'm of the opinion that this imposes a very minor maintenance burden, only consisting of *removing* deserialization code.
### Notes/Requests for feedback
- For the most part I used my judgement rather than `cargo fmt` since `cargo fmt` already had a lot of other complaints, but of course I'll update if I need to.
- To satisfy the requirement that every commit pass tests, the commit updating the psbt serde regression test should probably be squashed into the first commit, but I just wanted to confirm that before I did it. I suppose similarly, the test commit could be squashed as well?
- I waffled between `musig2_participants` and `musig2_participant_pubkeys`, but I've decided to go with `musig2_participant_pubkeys` because that is consistent with Bitcoin Core
ACKs for top commit:
tcharding:
ACK 2481695b45
apoelstra:
ACK 2481695b456bcccbb25c247c1fd39bbda24dbb30; successfully ran local tests
Tree-SHA512: af884923593c9cbb24ff3f1f08219458538592fabde85d5d65bc2d9bc7bf0b1a73dac38d2c56303b4f3162088db129ea7e879c3d4b324e965933c121ef939a07
This type creates sane Arbitrary InputWeightPrediction types that do
not panic. To this end, when a custom type is created by calling new()
or from_slice() constructor, only use values that would no greater than
block size 4 Mwu.
07c4f76052 Add comparison traits to InputWeightPrediction (yancy)
Pull request description:
* Partial Eq is added to Enable symmetric and transitive comparison.
* Eq is added to enable reflexive comparison.
ACKs for top commit:
apoelstra:
ACK 07c4f760523b7a196bf160f585c2b437dea5b532; successfully ran local tests
tcharding:
ACK 07c4f76052
Tree-SHA512: baeb957f000ac0f3be89166243b9cc7126daad06ad6688b811037ca5f5713cad1184c7135b2f4f32235457c0f53eb41304846bdd8a84e57b10a6eff0905224e8
9aa235c24d BREAKING: Change Psbt serde implementations (Daniel Roberts)
d7e9a84339 Fix Psbt preimage keys in serde test (Daniel Roberts)
62026c1e2d Don't use PSBT_GLOBAL_XPUB as an unknown key in Psbt serde test (Daniel Roberts)
Pull request description:
Implements the conclusion of #3454 by serializing Psbts using the BIP-174 binary encoding, optionally using base64 where appropriate.
The core of the problem is the old derived serde implementation by its nature can't be backwards compatible when serialized in binary formats like bincode. Fields will be added to the Psbt (my motivating case in #4440 ) and this will always break formats like bincode.
ACKs for top commit:
apoelstra:
ACK 9aa235c24d65d23de2afc21fcbd019892bf4ad2a; successfully ran local tests
tcharding:
ACK 9aa235c24d
Tree-SHA512: 4dc9dbf1a71f06769d74fada7e3d5557a3df3ee78769c66c2d8480c434baa0abd2efba555137563af58da2cc1d545813eb43b6c696b363a5777a9836bc1b7382
a1ce2d1ac8 Use _u32 in FeeRate constructor instead of _unchecked (Tobin C. Harding)
Pull request description:
When constructing an `Amount` it was observed recently that a `u32` is often big enough. For the same reason we can use a `u32` to construct a fee rate instead of overflowing.
Use `_u32`in the constructor and remove the panic.
ACKs for top commit:
apoelstra:
ACK a1ce2d1ac8d646b63532ed9ac459ffea39ec8c20; successfully ran local tests
Tree-SHA512: 339974c954ad613b0be684f7b2fa1402f59fe401969f3785a702ffbb14ac913ecf4c8228240d068ba4b482e38263590154167a071d823ccc9b4d0691036ca484
Replace derived Psbt serde implementation with one that conforms to
BIP-174. In human readable serde contexts, serialize to the base64
encoded format, and in binary serde contexts, serialize to the raw
binary format.
The previous derived serde implementation cannot be used in a backward or
forward compatible way in binary formats like bincode, which means that
every field added to the Psbt struct would break serde de/serialization
into binary formats. Instead, this one-time breaking change will fix the
issue going forward.
Downstream users with persisted data in the old serde format should continue
using 0.32.x to create migrations to the new format.
The preimage values in the serde Psbt don't actually correspond to the
hash keys they should in the serde test. This doesn't cause an error
currently because the derived serde implementation doesn't enforce their
validity during deserialization, but it will when the serde
implementation is modified to use the BIP-174 format.
The previous test used global key id 1 which is not unknown, it's PSBT_GLOBAL_XPUB.
That's an inconsistent state for a Psbt to be in, and will cause a
deserialization error when the Psbt serde implementation is modified to
use the BIP-174 format.
When constructing an `Amount` it was observed recently that a `u32` is
often big enough. For the same reason we can use a `u32` to construct a
fee rate instead of overflowing.
Use `_u32`in the constructor and remove the panic.
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
29b12bec6b Remove Display/FromStr for FeeRate (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
apoelstra:
ACK 29b12bec6b34148d6df9a4e6207132a667c53b4c; successfully ran local tests
Tree-SHA512: ffb49ab5d4f98be603eb1ca2f2e9d28ff7eaae66607eb9d2d5fef1f98ba2ac284a0007a86c3cae88f06e5b44f1e3e3ecb2014323b82ad4007e8ec59d6d04759b
a1d4bc31e5 test(p2p): add tests for `AddrV2` <> `SocketAddr` conversions (Luis Schwab)
64387f566e feat(p2p): add `AddrV2` <> `SocketAddr` conversions (Luis Schwab)
Pull request description:
Closes#4436.
Note: I made the `AddrV2::Cjdns` to `SocketAddr` conversion throw the `CjdnsNotRecommended` error. Do we want this behavior or just assume the user knows what he is doing? cc Kixunil
### Changelog
- Implement `From<SocketAddr> for AddrV2`.
- Implement `TryFrom<AddrV2> for SocketAddr`.
- Implement `AddrV2ConversionError` enum and it's `fmt::Display`.
- Tests for these conversions.
ACKs for top commit:
apoelstra:
ACK a1d4bc31e5c7cfe0227db64aec8671efcc0c6677; successfully ran local tests
tcharding:
ACK a1d4bc31e5
Tree-SHA512: c11f3053428d2c8ca971bbc6bc4ad4619260fe95cba055586f4889d7397733f7d286dcafa111234a6be4a739fd56cdd7e64dbf71b106a71d2483794ca7018105
d74eede260 fix(taproot): raname `from_key_and_tweak` to `from_key_and_merkle_root` (Luis Schwab)
Pull request description:
Closes#4236.
### Changelog
- Rename `TapTweakHash::from_key_and_tweak` to `TapTweakHash::from_key_and_merkle_root`. The naming was just wrong, since a TapTweak takes in a public key and a Merkle root to produce a tweak.
ACKs for top commit:
apoelstra:
ACK d74eede26064a40d70c7aeb3335b9a4a28eb6bd9; successfully ran local tests
tcharding:
ACK d74eede260
Tree-SHA512: 03fdb73758f965290c083165b23a0345325e475f159aa76ff141a9aa3251960041366ddf195b74cada7b289d4493190cf9b17130736002d48b6fac68941012fb
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.
2c0f388108 Fix up script to/from hex (Tobin C. Harding)
Pull request description:
We (I) have recently done to PRs patching the way we handle converting scripts to and from hex.
In doing these I made a mess of the deprecation because after both PRs were in I had managed to change the return type and the behaviour of the deprecated function.
On top of that the docs were either outright wrong or not that clear.
Props to Kix for doing post merge review and finding my mistakes.
Close#4498
ACKs for top commit:
apoelstra:
ACK 2c0f3881085ba540d517de121d4ba005f9e73a8c; successfully ran local tests
Tree-SHA512: 8bd8590c07efdbfcf113bfcb4b96dc01992c4f215a11e4a1b1f907c8ae9fa47d83c29298bd9b2afc2628b12eb51afd023a48f241a456a0e02a37854b41f6654b
We (I) have recently done to PRs patching the way we handle converting
scripts to and from hex.
In doing these I made a mess of the deprecation because after both PRs
were in I had managed to change the return type and the behaviour of the
deprecated function.
On top of that the docs were either outright wrong or not that clear.
Props to Kix for doing post merge review and finding my mistakes.
d003d48592 Add `Builder::with_capacity()` (Daniel Roberts)
e492f94289 Update `ScriptBuf::with_capacity` docs (Daniel Roberts)
Pull request description:
Enable the creation of `bitcoin::script::Builder` with a preallocated capacity.
This is pretty minor, but it provides a small speedup if used correctly. I've observed a 0.4% speedup and a 0.7% speedup in two code bases that create lots of outputs (on the order of tens to hundreds of thousands). It provided a much larger speedup (5% or so) in the latter code base before some other optimizations dwarfed it.
I have a branch that also uses it for `ScriptBufExt::new_*()` but while it provides a small performance improvement for all script types except one, `ScriptBufExt::new_p2tr()` actually causes a small performance regression. Since the only use case I have for creating lots and lots of scripts is in taproot with CHECKTEMPLATEVERIFY, I'm holding back that change if/until I figure out what's going on exactly (and hopefully resolve it).
ACKs for top commit:
tcharding:
ACK d003d48592
apoelstra:
ACK d003d4859251abb01929ccab6009a02e0c96b0cd; successfully ran local tests
Tree-SHA512: 51137574ca3d9dc5c319df124f470abd0f82413c093a5636af0439eb0fc2ad01dcf83df366a02279fc7d28feed24aa8c7db33b3c474d8bde7a9b6636343f8e9a
3fbd6fb6b3 test: Add constructor test (yancy)
Pull request description:
The constructor currently has no test coverage.
ACKs for top commit:
apoelstra:
ACK 3fbd6fb6b3c6dc1f1d5c14b3f3486140a781d0f0; successfully ran local tests
tcharding:
ACK 3fbd6fb6b3
Tree-SHA512: 9819bd058b84a7b633279d12da48c9af7af765fd8dca5d297787872badc431769c026e57b03bf6d907c89a7b7a5c116cda1be98cb6261dd2a6c331276e627cc3
873880b192 test: push int minimality (ChrisCho-H)
Pull request description:
Integrate the minimality test of `push_int` into that of `push_slice`. This increases test coverage.
ACKs for top commit:
apoelstra:
utACK 873880b192
tcharding:
ACK 873880b192
Tree-SHA512: 8bbd0b7ec4c69faaadb9ab4bae7429bbebd66d1d718b80f19b323a1059a983ea1b41f743a920b6fdacce213e66708ed1028227246021457601bce968b8bf3f22
b038520c4d Change the return type of effective_value (yancy)
Pull request description:
Prefer the more informative return type NumOpResult over Option. Also a returns section was added which describes the different possible returns.
The api of NumOpResult could probably be extended to cleanup the match statement. Also consider api addition for unchecked calculations natively.
ACKs for top commit:
tcharding:
ACK b038520c4d
apoelstra:
ACK b038520c4dc8c2f92cb40a9fd272608ae2e9b799; successfully ran local tests
Tree-SHA512: 2e66a3ca83514f0ad011660178f8e5ea9d9b1de03dc030e7c57f558e08a42261724251364bb7a746b6970b4288a45539a33d3b36b114c855b6246a56fee3c61c
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
Define 'is satisfied by' - this is a classic off-by-one problem, if a
relative lock is satisfied does that mean it can go in this block or the
next? Its most useful if it means 'it can go in the next' and this is
how relative height and MTP are used in Core.
Ramifications:
- When checking a time based lock we check against the chain tip MTP,
then when Core verifies a block with the output in it it uses the
previous block (and this is still the chain tip).
- When checking a height base lock we check against chain tip height + 1
because Core checks against height of the block being verified.
Additionally we currently have a false negative in the satisfaction
functions when the `crate` type (height or MTP) is to big to fit in a
u16 - in this case we should return true not false because a value too
big definitely is > the lock value.
One final API paper cut - currently if the caller puts the args in the
wrong order they get a false negative instead of an error.
Fix all this by making the satisfaction functions return errors, update
the docs to explicitly define 'satisfaction'.
For now remove the examples in rustdocs, we can circle back to these
once the dust settles.
API test of Errors:
Some of the errors are being 'API tested' tested in `primitives` but
they should be being done in `units/tests/api.rs` - put all the new
errors in the correct places.
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
merge-script
Tobin C. Harding
rustaceanrob
Luis Schwab
Jose Storopoli
yancy
Daniel Roberts
Fmt Bot
Erick Cestari
frankomosh
ChrisCho-H
Jamil Lambert, PhD
Bruno Garcia
GarmashAlex
Shing Him Ng