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
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
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.
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.
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
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
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`.
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
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
There was and inconsistent usage of `#`, `##` and `###` in rustdoc
headings. The difference in the rendered rustdocs is a minimal font
size change.
Change all headings to be H1 `#`.
Change all subheadings to be `###` to have a noticeable difference in
font size in the rendered docs.
2b37583ca5 test: add test for push slice minimal (ChrisCho-H)
Pull request description:
Following https://github.com/rust-bitcoin/rust-bitcoin/pull/4322.
Test `push_slice` and `push_slice_nom_minimal` from OP_1...OP_16 and OP_1NEGATE.
ACKs for top commit:
apoelstra:
ACK 2b37583ca55195965ce2f129d75a1fc114d8b5f6; successfully ran local tests
Tree-SHA512: 6d8e96d522d317d7ede2f1bc133050d98ccbc1816c59fe3589003de249828367314f751d3a75d6b58b0c90b1b735f85650c0d7acebaf5da21a38fb651ab4177d
f6105ea417 Use InputWeightPrediction to calculate effective_value (yancy)
Pull request description:
closes: https://github.com/rust-bitcoin/rust-bitcoin/issues/2455
Also, what about moving `effective_value` to `InputWeightPrediction`?
Marking as a draft until we can add api changes again.
ACKs for top commit:
apoelstra:
ACK f6105ea4171a85ce21443d7eb76b7aa9cadab53a; successfully ran local tests; yeah, this API does look nicer
Kixunil:
ACK f6105ea417
Tree-SHA512: 20592e49cb93343b1aefa340c3c870e2e21c747711da68a6aa57342f59ff2981c30e9c91de7eab32bcd11da33f040f9df62008db991d93b549079f91a6908055
354e1e42ad fix: enforce minimal push for push_slice (ChrisCho-H)
Pull request description:
Currently `push_slice` doesn't check the standard minimal push rule, which could result in possible money loss(e.g. if non minimal push is used in output script of p2wsh or p2tr).
Introduce `push_slice_non_minimal` to provide the way to push as now, and change `push_slice` logic to follow standard minimal push rule.
99a4ddf5ab/src/script/script.cpp (L366)
ACKs for top commit:
tcharding:
ACK 354e1e42ad
apoelstra:
ACK 354e1e42ad98c7968827d03be563bc14d764a983; successfully ran local tests
Tree-SHA512: 01f53dbd2a1a3c9a9e4387f0aaa7801f9c4570996054503a38d09ed1646ab65a3249d227adcf4139ac37a111d795e6cf986c4c273d5660c20820d1b36ba46f57
d0e1cd72fe chore: Fix the typos in the comments and variables (dufucun)
Pull request description:
Fix the typos in the comments and variables
ACKs for top commit:
apoelstra:
ACK d0e1cd72fec87276034476f1f28c62124cf63c25; successfully ran local tests
Tree-SHA512: 267d2b6b47e5a4f9466507e9dceb62ce94c848b3edefee65b85c3ce31560d1be880ef1c03ffd7dac54198f1f470695b70e06a505f91174f2990639bc20bf86e4
0498f7b7b7 Remove Option return from `minimal_non_dust` (jrakibi)
Pull request description:
Closes#4221
This removes the `Option` return type from `minimal_non_dust
Overflow is only possible in 2 cases:
- `dust_relay_fee` would need to be excessively high
- script size would have to exceed ~6.15 × 10¹⁵ bytes (≈ 6 petabytes)
we now panic with the same message we had before in cf12ba262a/bitcoin/src/blockdata/script/borrowed.rs (L412)
ACKs for top commit:
tcharding:
ACK 0498f7b7b7
apoelstra:
ACK 0498f7b7b7d43cc015d6788efe826df25d6156a5; successfully ran local tests
Tree-SHA512: 826a5d4ebb9c237cdd261f7d8b25fb2118cfba7d79b031839a619e12c440cbd34bbf830ffe513c104ef34e8ae50320e314c736a55be9ba7a82ae50f6022b9cf0
Fmt Bot
merge-script
Tobin C. Harding
yancy
ChrisCho-H
Daniel Roberts
Erick Cestari
Shing Him Ng
Luis Schwab
Jamil Lambert
dufucun