3355400d67 docs: document IWP function return limit and panic case (yancy)
8559a49e03 Do not bound Arbitrary parameters passed to InputWeightPrediction (yancy)
e4c3d1e7a6 Use saturating add in IWP constructors (yancy)
8552534b61 Use u32 for struct and member variables in IWP, saturating to u32::MAX (yancy)
Pull request description:
Use u32 for struct and member variables in InputWeightPrediction (saturating to u32::MAX). To avoid panics during construction and while using auxiliary methods such as `total_size()`, support saturating operations.
closes: https://github.com/rust-bitcoin/rust-bitcoin/issues/4547
ACKs for top commit:
apoelstra:
ACK 3355400d6706ce8fee3daa258e9dbbd648a87dca; successfully ran local tests; looks great!
tcharding:
ACK 3355400d67
Tree-SHA512: 8e4af86914152b4c159749ba71f1a2a45682b7c16ba7b35a0c4fd4e8c7c162d3999f4280dffd71b231c7e24f0b4a18907465bd99d8ef958fb7bc81f519059f63
To prevent panics during addition if `usize` is `u64`, use `u32` member
variables internally. TK use `u32::saturating_add` instead of basic
addition. However, to use `u32::saturating_add()`, the variables need
to be of type `u32`. Therefore, this commit transforms the internal
types to `u32`.
In so doing, add a `const` function `saturate_to_u32()` which saturates
a usize to `u32`. Also, replace `compact_size::encoded_size()` with a new
function `encoded_size()` which returns a `u32`, avoiding needless casts.
Lint error from new nightly "lifetime flowing from input to output with
different syntax can be confusing".
Add the anonymous lifetime to make it explicit.
0ff8d82193 Make FeeRate from sat constructors infallible (Tobin C. Harding)
3b0286bd56 Return NumOpResult for FeeRate and Weight (Tobin C. Harding)
15065b78c7 Return NumOpResult when calculating fee on Amount (Tobin C. Harding)
75106e6d82 Remove checked_ prefix from fee functions (Tobin C. Harding)
Pull request description:
This was 14dc950b54 from #4610.
Remove the `checked_` prefix from `fee` functions then make them all return `NumOpResult`.
ACKs for top commit:
apoelstra:
ACK 0ff8d82193ef562c1aedabe98a6283d09f803c0c; successfully ran local tests; nice!
Tree-SHA512: 62f2af6701f2a0e17b9f0a0ee132ca4a9289fe00e22c94f746602ed3c1f3758bb22323b224362cb659432ff5297391f3106ee460e9a8f65a39f904d72bc98aeb
We now have constructors that take an arbitrary size fee
rate (`Amount`). The `from_sat_per_foo` constructors can be made
infallible by taking a `u32` instead of `u64`. This makes the API more
ergonomic but limits the fee rate to just under 42 BTC which is plenty.
Note we just delete the `from_sat_per_vb_u32` function because it is
unreleased, in the past we had `from_sat_per_vb_unchecked` so we could
put that back in if we wanted to be a bit more kind to downstream. Can
be done later, we likely want to go over the public API before release
and add a few things back in that we forgot to deprecate or could not
for some reason during dev.
Fuzz with a new function that consumes a `u32`.
New clippy lint in rustc nightly "lifetime flowing from input to output
with different syntax can be confusing".
Apply the suggested fix and use the anonymous lifetime for paths.
56516757ad Add code comment to amount calculation (Tobin C. Harding)
8cf1dc39b5 Fix incorrect code comment (Tobin C. Harding)
7c186e6081 Refactor fee functions (Tobin C. Harding)
bf0776e3dd Remove panic using checked arithmetic (Tobin C. Harding)
b65860067f Make fee functions const (Tobin C. Harding)
9b2fc021b2 Improve rustdocs on FeeRate (Tobin C. Harding)
1bd1e89458 Re-introduce FeeRate encapsulate module (Tobin C. Harding)
b27d8e5819 Change the internal representation of FeeRate (Tobin C. Harding)
2e0b88ba76 bitcoin: Fix dust 'fee' identifiers (Tobin C. Harding)
399bca531c Reduce the FeeRate::MAX value (Tobin C. Harding)
d174c06a4a Saturate to_fee to Amount::MAX (Tobin C. Harding)
64ac33754f Add missing argument docs (Tobin C. Harding)
fe0a448e78 Temporarily remove const from fee calc function (Tobin C. Harding)
b929022d56 Add floor/ceil versions of to_sat_per_kwu (Tobin C. Harding)
64098e4578 Remove encapsulate module from fee rate (Tobin C. Harding)
Pull request description:
The `FeeRate` is a bit entangled with amount and weight. Also we have an off-by-one bug caused by rounding errors and the fact that we use kwu internally.
We can get more precision in the fee rate by internally using per million virtual bytes.
- Fix: #4516
- Fix: #4497
- Fix: #3806
ACKs for top commit:
apoelstra:
ACK 56516757ad8874d8121dba468946546bb8fd7d4e; successfully ran local tests
Tree-SHA512: 0c6e7e2c9420886d5332e32519838d6ea415d269abda916e51d5847aa2475c87fa4abfc29b5f75e8b10c44df67ae29d823aa93f7cfabbc89eb27f2173b103242
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.
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
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.
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
Andrew Poelstra
yancy
Jamil Lambert, PhD
Tobin C. Harding
Fmt Bot
ChrisCho-H
Daniel Roberts