Currently we do not build the code in the kani tests when PRs are
pushed, instead we run the verifier once a day. We should at least check
the code builds on each PR. One way to do this is to build the proofs
without running them, `kani --only-codegen` does that.
Recently (in #2379) we patched the `ParseAmountError` but we don't check
kani code on every pull request so we broke it.
Fix kani test to use the new `OutOfRangeError`.
Close: #2424
93dba898c2 Improve lock time errors (Martin Habovstiak)
Pull request description:
The errors returned from various lock time functions had several issues. Among the obvious - `Error` being returned from all operations even when some of its variants were unreachable, there were subtle issues around error messages:
* `ParseIntError` didn't contain information whether the parsed object is `Height` or `Time`.
* Logically overflow and out-of-bounds should be the same thing but produced different error messages.
* Mentioning integers is too technical for a user, talking about upper and lower bound is easier to understand.
* When minus sign is present `std` reports it as invalid digit which is less helpful than saying negative numbers are not allowed.
It is also possible that `ParseIntError` will need to be removed from public API during crate smashing or stabilization, so avoiding it may be better.
This commit significantly refactors the errors. It adds separate types for parsing `Height` and `Time`. Notice that we don't compose them from `ParseIntError` and `ConversionError` - that's not helpful because they carry information that wouldn't be used when displaying which is wasteful. Keeping errors small can be important.
It's also worth noting that exposing the inner representation could cause confusion since the same thing: out of bounds can be represented as an overflow or as a conversion error. So for now we conservatively hide the details and even pretend there's no `source` in case of overflow. This can be expanded in the future if needed.
The returned errors are now minimal. `LockTime` parsing errors are currentlly unchanged.
I can add `LockTime` changes in the same commit or separate within this PR if you want. Just wanted to push something for review before I go to sleep.
ACKs for top commit:
apoelstra:
ACK 93dba898c2
tcharding:
ACK 93dba898c2
Tree-SHA512: 68b60b413b1a1a0fc3648970d37f43e8b1b79f197ded053d83cfc1cf4fab4bed77d77841c2ae4d066b6436ee7187723c5d8cf934193c04c03520e797b7f7e82d
The errors returned from various lock time functions had several issues.
Among the obvious - `Error` being returned from all operations even when
some of its variants were unreachable, there were subtle issues around
error messages:
* `ParseIntError` didn't contain information whether the parsed object
is `Height` or `Time`.
* Logically overflow and out-of-bounds should be the same thing but
produced different error messages.
* Mentioning integers is too technical for a user, talking about upper
and lower bound is easier to understand.
* When minus sign is present `std` reports it as invalid digit which is
less helpful than saying negative numbers are not allowed.
It is also possible that `ParseIntError` will need to be removed from
public API during crate smashing or stabilization, so avoiding it may be
better.
This commit significantly refactors the errors. It adds separate types
for parsing `Height` and `Time`. Notice that we don't compose them from
`ParseIntError` and `ConversionError` - that's not helpful because they
carry information that wouldn't be used when displaying which is
wasteful. Keeping errors small can be important.
It's also worth noting that exposing the inner representation could
cause confusion since the same thing: out of bounds can be represented
as an overflow or as a conversion error. So for now we conservatively
hide the details and even pretend there's no `source` in case of
overflow. This can be expanded in the future if needed.
The returned errors are now minimal. `LockTime` parsing errors are
currentlly unchanged.
Make the trait level attributes uniform across all released crates in
the repo. Excludes things that are obviously not needed, eg, bench stuff
if there is not bench code.
- Remove `uninhabited_references` - this is allow by default now.
- Remove `unconditional_recursion` and mark the single false positive we
have with an `allow`.
Note, this does not add `missing_docs` to the `io` crate. There is an
open PR at the moment to add that along with the required docs.
bd4f14ee51 just: Reduce docs commands (Tobin C. Harding)
Pull request description:
`cargo --doc` works from the workspace root, no need to run the docs builds individually.
## More context
`cargo test` does not run docs tests if run from the workspace root as it does when run in a crate directory.
ACKs for top commit:
sanket1729:
utACK bd4f14ee51
Kixunil:
ACK bd4f14ee51
Tree-SHA512: f4003edcaf9bbc22cfcdcd142665fbb924b6a74af9145ee38d9f14275660e849d53b2f8d10ee45bf5b6c8e3b5123ead762da3fbc5f84714a55d2fe5273950270
13c8a709f1 Use nightly toolchain in pre-commit hook (Tobin C. Harding)
Pull request description:
We use the nightly toolchain to run `clippy` now, update the git pre-commit hook to mirror this.
ACKs for top commit:
apoelstra:
ACK 13c8a709f1
Tree-SHA512: 46700641b81a1e5a2e39ff67ca0d31afaedfb58e008bf453c13f0312b2c6936ae38c58548934cae40abc9de0dc1ec4ce4bba4b8142b204d774612bd9721679c9
dda83707a2 Use `unsigned_abs` instead of manual code (Martin Habovstiak)
Pull request description:
The code originally used `if` and incorrectly casted the value into `usize` rather than `u64`. This change replaces the whole thing with `unsigned_abs`.
Closes#1247
ACKs for top commit:
apoelstra:
ACK dda83707a2
tcharding:
ACK dda83707a2
Tree-SHA512: c57bf440705c69917206aab36bbbe5b048ed52d7a96ebd055416b8a249fc3f7ba32ebff3d2251b971e7ef999ff8169ac3db8e599ab38cf0776ecc030447a9a4a
ac26171c32 Clean up `no_std` usage (Martin Habovstiak)
fce03cec85 Provide `Amount` & co in no-alloc (Martin Habovstiak)
Pull request description:
Using the crate without allocation was previously disabled making the
crate empty without the feature. This chage makes it more fine-grained:
it only disables string and float conversions which use allocator. We
could later provide float conversions by using a sufficiently-long
`ArrayString`.
Note that this is API-breaking because we disallow calling the methods of the sealed `SerdeAmount` trait. However I think it should've been obvious that the thing is internal and calling them is not a great idea. (BTW I only learned this trick recently).
Closes#2389
ACKs for top commit:
apoelstra:
ACK ac26171c32 maaybe `private` should be renamed to `internal_api` or something
tcharding:
ACK ac26171c32
Tree-SHA512: 2ca2ff11c3c362f868c3993b5698ace32e6ce2cf2e8028bc43fe65797eb2239b4841c1c722a0184a7c8f4afea3b475b1a049dd77fa30358cb742d60463018e9f
The code originally used `if` and incorrectly casted the value into
`usize` rather than `u64`. This change replaces the whole thing with
`unsigned_abs`.
Closes#1247
61f8bab65f just: Add quick and dirty CI command (Tobin C. Harding)
e185fe46df just: Lint with nightly toolchain (Tobin C. Harding)
Pull request description:
Improve the `justfile` by doing:
- Update linter toolchain
- Add `just sane` to run a minimal set of checks/tests that can be used pre-push but is not as slow as using `DO_FEATURE_MATRIX=true contrib/test.sh`.
ACKs for top commit:
apoelstra:
ACK 61f8bab65f
Kixunil:
ACK 61f8bab65f
Tree-SHA512: 730874f0381db9ae30052ad6511805f76ae5c13c4c5cc5ad272430cf7e67cfbe7b288aa6dc47035ff5e8e42e03d3e4a3bf9d3e9a072c9aa922f9df62c43850b3
Previously the crate used negative reasoning to enable `std` which was
hard to understand, required the `prelude` module and wasn't really
needed because it's only needed when a crate wants to add `alloc`
feature-backwards compatibly and this crate always had the feature.
This cleans up usage to unconditionally use `#[no_std]` and then just
add `extern crate` on top as needed by activated features.
Using the crate without allocation was previously disabled making the
crate empty without the feature. This chage makes it more fine-grained:
it only disables string and float conversions which use allocator. We
could later provide float conversions by using a sufficiently-long
`ArrayString`.
7bf478373a Model `TooBig` and `Negative` as `OutOfRange` (Martin Habovstiak)
54cbbf804f Express `i64::MAX + 1` as `i64::MIN.unsigned_abs()` (Martin Habovstiak)
b562a18914 Move denomination error out of `ParseAmountError` (Martin Habovstiak)
5e6c65bc1a Clean up `unsigned_abs` (Martin Habovstiak)
Pull request description:
Closes#2265Closes#2266
Disclaimer: I did this in December and don't remember why I haven't pushed it. Maybe because it's somehow broken but I don't see how so please review a bit more carefully just in case.
ACKs for top commit:
tcharding:
ACK 7bf478373a
apoelstra:
ACK 7bf478373a
Tree-SHA512: 1f6e9adae9168bd045c9b09f06d9a69efd47ccc7709ac9ecaf48cb86e265b448b9b52a199ac5e6838d5029f5bc7514c5d7deb15a4d7c8a4606a353f390745570
6ddb5cce37 Use Magic::BITCOIN in unit tests (Tobin C. Harding)
Pull request description:
We are currently calling `From` to create the magic bytes, this is unnecessary since `Magic` provides consts.
Refactor only, no logic changes.
ACKs for top commit:
Kixunil:
ACK 6ddb5cce37
apoelstra:
ACK 6ddb5cce37
Tree-SHA512: 20e2e017683f123309e3c0876bba42d86a9411bb225f07c486716184fc79837e04a832338ec8b18874ac76791260f6a4620b932ede92c8b222dac08d468cef8a
5eb2de1660 Remove TODO about rand trait (Tobin C. Harding)
66cc007c2b p2p: Remove TODO comments (Tobin C. Harding)
0b5fb45ea0 consensus: Remove HEX_BUF_SIZE todo (Tobin C. Harding)
579668892a consensus: Remove TODO (Tobin C. Harding)
53beb9db30 Remove ancient todos in test code (Tobin C. Harding)
abe2241828 units: Remove "alloc" TODO (Tobin C. Harding)
5386ef0fd2 psbt: Delete TODO comments (Tobin C. Harding)
14c8a2232b examples: Remove TODO (Tobin C. Harding)
Pull request description:
Done while working on #2368. There are 5 left. Do we want to leave the MSRV ones in there?
```bash
bitcoin/src/blockdata/weight.rs:66: // TODO replace with panic!() when MSRV = 1.57+
bitcoin/src/consensus/serde.rs:101: // TODO: statically prove impossible cases
bitcoin/src/pow.rs:445: // TODO: Use `carrying_mul` when stabilized: https://github.com/rust-lang/rust/issues/85532
units/src/amount.rs:595: // TODO replace whith unwrap() when available in const context.
units/src/amount.rs:599: // TODO replace with panic!() when MSRV = 1.57+
```
ACKs for top commit:
Kixunil:
ACK 5eb2de1660
apoelstra:
ACK 5eb2de1660
Tree-SHA512: 285b1711a6e6fba126e2c4159b25454c7f894122b76fde1d3d29e57b2ec0a6e90230e46ac79d70aa133da177c75d267fc5a13489b69881862649de771027ec8e
6715e93e89 Add Witness::p2tr_key_spend function (Tobin C. Harding)
Pull request description:
Add a function for creating the witness when doing a key path spend for a P2TR output.
This mirrors what we did for P2WPKH when adding `Witness::p2wpkh`.
Includes update to the taproot signing example to use the new constructor.
ACKs for top commit:
Kixunil:
ACK 6715e93e89
apoelstra:
ACK 6715e93e89
Tree-SHA512: aab51329e8fda471442bb9cebd6327636548dd157bb9842fe66993fcdd211bb04b2b829aa9d5962dd619f5c0b73d19644a44529c1a5958df1a6bc892147b44f5
Development for `psbt` has move to another repo, these TODO comments are
over there alread, lets just remove them from `rust-bitcoin` as part of
an effort to remove TODOs from the codebase.
fb81bff61f Add a from impl for ParseIntError (Tobin C. Harding)
2130150df6 absolute: Use Self in error type (Tobin C. Harding)
Pull request description:
While reviewing #2335 I noticed a few places that error code needed some love.
ACKs for top commit:
Kixunil:
ACK fb81bff61f
Harshit933:
ACK [`fb81bff`](fb81bff61f)
apoelstra:
ACK fb81bff61f
Tree-SHA512: 17f4e448862be47534d4c1c2962d5db8f90a471e53226022d7c2e153fc501705cd65b070eb17f3239fb8ad242223340f78fe828ea7df3d43707d3e42fcbef557
3cfd746bbc Add functionality to serialize signatures to a writer (Tobin C. Harding)
Pull request description:
Serializing the ecdsa and taproot `Signature` straight to a writer is a useful thing to be able to do.
Add `to_writer` to both `SerializedSignature`s and also to the `Signature`s (calling through to `SerializedSignature`).
Remove TODO comments from code.
ACKs for top commit:
Kixunil:
ACK 3cfd746bbc
Tree-SHA512: 82eb6d42c7b327cdfe5e89348890e45ea39c664420f7ea17d7826a5c388c7aaae917b1334e3f3df645fc4a81a11b59d97c7d6958e99077fbd67193e2a588f2eb
faa45cf10f Remove stale comment (Tobin C. Harding)
c82f26e960 Use hex-conservative to display pubkey (Tobin C. Harding)
Pull request description:
We introduced `hex-conservative` ages ago, use it to display the `PublicKey`.
ACKs for top commit:
Kixunil:
ACK faa45cf10f
apoelstra:
ACK faa45cf10f
Tree-SHA512: 8ad14c7697314f8393ecb9a287215c505924d0655f7bf3536d4be83af983b142e06a96f802beb4548e2de051f1783549d8d1d1a8ebfb678f372a54010717752e
1c4614ea30 hashes: Remove serde-std feature (Tobin C. Harding)
Pull request description:
As we do for "rand-std" in the `bitcoin` crate we can enable "std" when the "serde-std" feature is enabled. This makes the features more explicit and arguably more ergonomic.
Found while working on #2353, no additional testing added hear (in CI).
ACKs for top commit:
Kixunil:
ACK 1c4614ea30
apoelstra:
ACK 1c4614ea30
Tree-SHA512: b82b72eacdd84cca076747a0eb0c7a625ec6f918701699ca7e7159e0606b089c80182e9d54e1d48b2957815883a36dfa337e353573a6ffe4fa19458615111080
3c4f6850f4 Flatten trivial errors. (Martin Habovstiak)
a4d01d0b6c Factor out `io::Error` from sighash errors (Martin Habovstiak)
Pull request description:
The hadnling of `io::Error` in sighash had a few problems:
* It used `io::ErrorKind` instead of `io::Error` losing inforation
* Changing `io::ErrorKind` to `io::Error` would disable `PartialEq`&co
* The `Io` error wariants were duplicated
It turns out all of these can be solved by moving the `Io` variant into a separate error.
ACKs for top commit:
apoelstra:
ACK 3c4f6850f4
tcharding:
ACK 3c4f6850f4
Tree-SHA512: b7ad6b692062d636ce29e4ebb448a8ac8ea3090feee1d349472e13f905f1f3785decc86e037d2d9658c1331a271e730076139a8d8f6c9b7dadda8b3221f6d434
The error returned when parsing amount had a `Negative` variant which
was weird/unreachable when parsing `SignedAmount`. Also weirdly, parsing
would return `TooBig` when the amount was negative - too low.
To resolve this we merge them into one `OutOfRange` variant that nuges
the consumers to make principled decisions and print error messages as
amounts being more than or less than a specific value which is easier to
understand for the users. Notably, the API still allows getting
information about which type was parsed and which bound was crossed but
in a less obvious way. This is OK since users who have a very good
reason can use this information but most won't.
Closes#2266
The `from_str_in` methods on amounts returned `ParseAmountError` which
contained `InvalidDenomination` among its variants. This one was never
returned because the method doesn't parse denomination.
This change separates the error out.
Closes#2265
Previousle we copied `unsigned_abs` method from `core` because it was
unstable in older MSRV. Our current MSRV allows using the method
directly so this removes our old one and uses the one from standard
library instead.
Add a function for creating the witness when doing a key path spend for
a P2TR output.
This mirrors what we did for P2WPKH when adding `Witness::p2wpkh`.
Includes update to the taproot signing example to use the new constructor.
As is customary add a `From` impl for the `ParseIntError` and use `?`.
While this does not make much difference it saves devs wondering why
there is a `From` impl for one of the variants and not the other.
Serializing the ecdsa and taproot `Signature` straight to a writer is a
useful thing to be able to do.
To both ECDSA and Taproot types:
- Add `SerializedSignature::to_writer`
- Add `Signature::serialize_to_writer`
Remove TODO comments from code.
Tobin C. Harding
Andrew Poelstra
Martin Habovstiak