3c62f74684 Add public functions p2wpkh_script_code (Tobin C. Harding)
a246dc98a4 Run sighash example in CI (Tobin C. Harding)
Pull request description:
This was done to fix#1920, it may be of questionable value though.
- Patch 1 is definitely useful, its a CI fix.
- Patch 2 adds two new API functions.
Fix: #1920
ACKs for top commit:
Kixunil:
ACK 3c62f74684
apoelstra:
ACK 3c62f74684
Tree-SHA512: 58743612c48e392f9ac0a94477588aee959c5fe9191dd04405bbb71aed7b0730b5927ad98f9da34dc93caaaac939617348c3f71318cc7e65c2c154b0f3897b89
c084afa8b2 Print hex in Debug for Sequence (Tobin C. Harding)
Pull request description:
Printing the `Sequence` as a decimal is not super useful when debugging, print it in hex instead.
Using code:
let seq = Sequence::from_consensus(0xFFFFFFFF);
println!("sequence: {:?}", seq);
Before applying this patch we get:
sequence: Sequence(4294967295)
And after applying we get:
sequence: Sequence(0xffffffff)
ACKs for top commit:
Kixunil:
ACK c084afa8b2
apoelstra:
ACK c084afa8b2
Tree-SHA512: d60cd8896ca56a30fc8bd030cf3dd1bc1fd3a1609e99bfc2f26b9bd665b11c34c9df93b3f3ad731506d916513ca4a192dde476e16d99f2d4c4b2697f70a7bc98
Add two public API functions on the two public keys, both called
`p2wpkh_script_code` to do exactly as the name suggests.
Of note, I was not able to find anywhere to use these in example code,
this is because of we always use the new `p2wpkh_signature_hash`
function. The new functions may be useful for a user calling
`segwit_v0_encode_signing_data_to`. The may help document the library as
well.
Printing the `Sequence` as a decimal is not super useful when debugging,
print it in hex instead.
Using code:
let seq = Sequence::from_consensus(0xFFFFFFFF);
println!("sequence: {:?}", seq);
Before applying this patch we get:
sequence: Sequence(4294967295)
And after applying we get:
sequence: Sequence(0xffffffff)
Using `non_exhaustive` as well as a public inner field is incorrect, it
prohibits users from creating or matching on the error and does not
achieve forward comparability.
This was never right, we shouldn't have done it.
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.
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
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
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
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.
fe8d559d69 test: add invalid segwit transaction test (startup-dreamer)
Pull request description:
Tries to close#2183
Added the test for invalid segwit transaction (witness flag is set but no witness is present) using [This suggested hex](https://github.com/rust-bitcoin/rust-bitcoin/issues/2183#issuecomment-1901207149) by Kixunil
ACKs for top commit:
Kixunil:
ACK fe8d559d69
apoelstra:
ACK fe8d559d69
Tree-SHA512: 723027e0ad9944a4763fba1e12398d7bcacdef691a40168f0be7cecdb170936f7e0c3690c4de911d086b8c5d42f5a25784f53fe096404f5cf69d6fc75c645d6e
a338a61cc3 Remove quadratic algorithm (Tobin C. Harding)
Pull request description:
Currently we loop over transaction inputs within a loop over transaction inputs - ouch.
Cache the `use_segwit_serialization` outside the iteration loop.
Fix: #2357
ACKs for top commit:
Kixunil:
ACK a338a61cc3
apoelstra:
ACK a338a61cc3
Tree-SHA512: 91d0b46b235db57d9c28fc8da5d43a52c76a29916797a4ec44273b91eb120a928050a79cdbd704b922635dd2130db7b6e7863fd10e878eee52882c661af54c11
e356ff6611 Remove the now unused sighash::Error type (Tobin C. Harding)
c17324c574 Introduce segwit sighash error types (Tobin C. Harding)
f0b567313b Introduce sighash::LegacyError (Tobin C. Harding)
a1b21e2f1d Introduce sighash::TaprootError (Tobin C. Harding)
b0f20903a5 Introduce AnnexError (Tobin C. Harding)
a1a2056829 Add tx_in/tx_out accessor methods on Transaction (Tobin C. Harding)
f08aa16e91 Use Self:: in error return type (Tobin C. Harding)
Pull request description:
Improve the error handling in the `sighash` module by adding small specific error types.
Close: #2150
ACKs for top commit:
Kixunil:
ACK e356ff6611
apoelstra:
ACK e356ff6611
Tree-SHA512: e2e98a4caccae4e4acdc0e577e369fc90ee39a2206a8a1451739695fbe33ec2c3a52482b70cec8f9ee6bdb3ad7a2f4f639e8c87031878cd5d816fae24d913c42
61bf462806 Use full path in all macro usage of Result (josibake)
Pull request description:
Follow-up to https://github.com/rust-bitcoin/rust-bitcoin/pull/2355
Couldn't think of a clever way to do this , so just grepped for all instances of `macro_rules` and added the full path for the imports. Wasn't sure if it was necessary for `fmt::Result`, but went ahead and added the full path for consistency.
Tested locally and confirmed this fixes the issue I was seeing.
ACKs for top commit:
apoelstra:
ACK 61bf462806
Kixunil:
ACK 61bf462806
tcharding:
ACK 61bf462806
Tree-SHA512: 8af105b3e6a36723804b290f8254f52e65cd42a61c323f1190e3bcbcb9e4427ff9b026a4530bafcd14aab644ccd9401fed351494457194c3a68a55f11b2a3d04
In a few places in the codebase we want to grab an reference to an input
by index. To reduce code duplication add two methods on `Transaction`,
each to get a reference to an input or output respectively.
These are public methods, do not use them yet internally.
Currently we loop over transaction inputs within a loop over transaction
inputs - ouch.
Cache the `use_segwit_serialization` outside the iteration loop.
Fix: #2357
Our decoding code reads bytes in very small chunks. Which is not
efficient when dealing with the OS where the cost of a context switch is
significant. People could already buffer the data but it's easy to
forget it by accident.
This change requires the new `io::BufRead` trait instead of `io::Read`
in all bounds.
Code such as `Transaction::consensus_decode(&mut File::open(foo))` will
break after this is applied, uncovering the inefficiency.
This was originally Kix's work, done before we had the `io` crate.
Changes to `bitcoin` were originally his, any new mistakes are my own.
Changes to `io` are mine.
Co-developed-by: Martin Habovstiak <martin.habovstiak@gmail.com>
The effective_value method is useful for coin selection algorithms. By
providing this effective value method, the effective value of each
output can be known during the coin selection process.
518f0970c9 Implement ArbitaryOrd for absolute::LockTime (Tobin C. Harding)
Pull request description:
At times we would like to provide types that do not implement `PartialOrd` and `Ord` because it does not make sense. I.e we do not want users writing `a < b`. This could range from kind-of-iffy to down-right-buggy (like comparing absolute locktimes).
However this decision effects downstream users who may not care about what the ordering means they just need to use it for some other reason e.g., to use as part of a key for a `BTreeMap` (as we do in `miniscript` requiring the `AbsLockTime` type).
A solution to this problem is to provide a wrapper data type that adds `PartialOrd` and `Ord` implementations. I wrote the `ordered` crate is for this very purpose.
ACKs for top commit:
apoelstra:
ACK 518f0970c9
Kixunil:
ACK 518f0970c9
Tree-SHA512: 05c753e650b6e2f181caf7dc363c4f8ec89237b42883bd695a64da0661436c9a7e715347f8fcf4fb19ce069cbf75a93032052e946f05fd8029f61860cf9c6225
Applies to both `ecdsa::Signature` and `taproot::Signature`.
Re-name the `Signature` fields with more descriptive names. The
names used were decided upon in the issue discussion.
Impove rustdocs while we are at it.
Note, the change to `sign-tx-segwit-v0` is refactor only, the diff does
not show it but we have a local variable already called `sighash_type`
that is equal to `EcdsaSighashType::All`.
Includes a function argument rename as well, just to be uniform.
Fix: #2139
At times we would like to provide types that do not implement
`PartialOrd` and `Ord` because it does not make sense. I.e., we do not
want users writing `a < b`. This could range from kind-of-iffy to
down-right-buggy (like comparing absolute locktimes).
However this decision effects downstream users who may not care about
what the ordering means they just need to use it for some other reason
e.g., to use as part of a key for a `BTreeMap` (as we do in `miniscript`
requiring the `AbsLockTime` type).
A solution to this problem is to provide a wrapper data type that adds
`PartialOrd` and `Ord` implementations. I wrote the `ordered` crate is
for this very purpose.
Feature gate a new dependency on `ordered` and implement `ArbitraryOrd`
for `absolute::LockTime`.
b02c7d1d33 Derive Copy for WitnessProgram (Tobin C. Harding)
Pull request description:
Recently we started using our custom `ArrayVec` for the `program` field of `WitnessProgram`, this means we can now derive `Copy`.
Fix: #2313
ACKs for top commit:
apoelstra:
ACK b02c7d1d33
sanket1729:
ACK b02c7d1d33
Tree-SHA512: 5741081d578f7b056c156d046dc3b0817b4b13cf69dcc1dfb8c7f4dbe8a4f9ed6c8802aaaf2b0084dbf3984d3fde807a02dbaa8c3bd29c220b3b32d3cb7c9f38
a8d50a5541 Remove Push enum (Tobin C. Harding)
Pull request description:
The `Push` enum is only ever used to get access to one of its variants. Since it is a private type we can remove it entirely and just return `PushBytes` from the `last_pushdata` function.
Needs careful review but I believe the function name is still correctly descriptive.
This was discovered by of a new nightly clippy warning.
ACKs for top commit:
apoelstra:
ACK a8d50a5541 Looks good to me. The latest compiler complains about the currently-unused variants.
sanket1729:
ACK a8d50a5541.
Tree-SHA512: 7f96057b0f6f5673252578253ad4f1789793dbf6e917d3974274dedf942da27e6247946262a0669eb500d47987788fcca0e020ed16c0d672188e95ee31163242
089ce8f0fb Deprecate `Script::is_provably_unspendable` (Martin Habovstiak)
Pull request description:
This method is not really that useful because it checked an arbitrary condition. There already exists `OP_RETURN` semantics and the method didn't cover all possible ways the script may be invalid.
This deprecates the method and documents why.
Closes#2191
ACKs for top commit:
apoelstra:
ACK 089ce8f0fb
tcharding:
ACK 089ce8f0fb
sanket1729:
ACK 089ce8f0fb
Tree-SHA512: 044f1c06fb8cbea4f84817be41bf10315f690b2a42748a07c1dd1eb0ba10932456780956fc628fec4bf57fe0722129537874a77be482d6660f9e02de5fc5a8a0
The `Push` enum is only ever used to get access to one of its variants.
Since it is a private type we can remove it entirely and just return
`PushBytes` from the `last_pushdata` function.
Needs careful review but I believe the function name is still correctly
descriptive.
geekvest
Tobin C. Harding
yancy
Andrew Poelstra
Martin Habovstiak
startup-dreamer
josibake
Fmt Bot
GoodDaisy