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
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.
429a3ecec4 Add the implementation of `Display` for `transaction::Version` (harshit933)
Pull request description:
Adds the implementation of `Display` trait for `transaction::Version`
fixes#2308
This is unrelated to the issue but can anyone suggest some good issues that needs to be fixed. I am also taking a look but I am confused as to which I would be able to solve. I am here to learn more.
Thank you.
ACKs for top commit:
apoelstra:
ACK 429a3ecec4 Merry Christmas
tcharding:
ACK 429a3ecec4
Tree-SHA512: 9e59a8fe494b01caa8f211441744709f26df03891be171242bea4f7ccd7c3cc58b548cad241cab5270ad66fc9bb33ea7d6f98cc60d496c47647fb3396db9410f
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.
a92d49fe33 Implement `CompressedPublicKey` (Martin Habovstiak)
Pull request description:
P2WPKH requires keys to be compressed which introduces error handling even in cases when it's statically known that a key is compressed. To avoid it, this change introduces `CompressedPublicKey` which is similar to `PublicKey` except it's statically known to be compressed.
This also changes relevant code to use `CompressedPublicKey` instead of `PublicKey`.
ACKs for top commit:
tcharding:
ACK a92d49fe33
apoelstra:
ACK a92d49fe33
Tree-SHA512: ff5ff8f0cf81035f042dd8fdd52a0801f0488aea56f3cdd840663abaf7ac1d25a0339cd8d1b00f1f92878c5bd55881bc1740424683cde0c28539b546f171ed4b
P2WPKH requires keys to be compressed which introduces error handling
even in cases when it's statically known that a key is compressed. To
avoid it, this change introduces `CompressedPublicKey` which is similar
to `PublicKey` except it's statically known to be compressed.
This also changes relevant code to use `CompressedPublicKey` instead of
`PublicKey`.
There is no advantage in having `io::Read` as opposed to `Read` and
importing the trait. It is surprising that we do so.
Remove `io::` path from `io::Read` and `io::Write`. Some docs keep the
path, leave them as is. Add import `use io::{Read, Write}`.
Refactor only, no logic changes.
1ee989a3af Remove private fmt_internal function (Tobin C. Harding)
923ce7402d Remove Network from AddressInner (Tobin C. Harding)
3490433618 Return error from wpubkey_hash (Tobin C. Harding)
f7ab253ce4 Remove stale comment (Tobin C. Harding)
Pull request description:
An `AddressInner` struct (contains `Network` field) is created when parsing address strings however address strings do not map 1:1 to `Network` because signet and testnet use the same bech32 prefix "tb".
We can fix this by inlining the `Payload` variants into `AddressInner` and adding prefix enums for legacy addresses and an `Hrp` for bech32 addresses.
Fix: #1819
ACKs for top commit:
Kixunil:
ACK 1ee989a3af
apoelstra:
ACK 1ee989a3af
Tree-SHA512: 1c2749dc929a1e9ad9b9feb01bec5c96b5aec07c6d646d88652deca7abe485907403116e9e29a0ab7dc06223254c4b49a384043284ec0a68fd76f9ab551e9e8a
1b23220d10 Fix: TxOut::minimal_non_dust and Script::dust_value (Jonathan Underwood)
Pull request description:
Fixes#2192
TxOut::minimal_non_dust has 3 problems.
1. There is an invisible dependency on Bitcoin Core's default minrelaytxfee value. It has been made explicit.
2. There is an off by one error. The dust limit comparison uses < and therefore `+ 1` was not needed. It has been fixed.
3. It was not returning 0 amount for OP_RETURN outputs.
Script::dust_value has 2 problems.
1. The dust amount depends on minrelaytxfee which is configurable in Bitcoin Core. This method was not configurable.
2. The division operation was done before multiplying the byte amount, which can cause small differences when using uncommon scripts and minrelaytxfee values.
ACKs for top commit:
Kixunil:
ACK 1b23220d10
apoelstra:
ACK 1b23220d10
Tree-SHA512: eafd5112fbf773d86e094e3a69c519dd32f5074f5c9c63a8d69b1c9796579a8f2c2d11ad0995d8252c25b7fed5cd7c968ab88a70588986981a0a63649d43e197
TxOut::minimal_non_dust has 3 problems.
1. There is an invisible dependency on Bitcoin Core's default minrelaytxfee value. It has been made explicit.
2. There is an off by one error. The dust limit comparison uses < and therefore `+ 1` was not needed. It has been fixed.
3. It was not returning 0 amount for OP_RETURN outputs.
Script::dust_value has 2 problems.
1. The dust amount depends on minrelaytxfee which is configurable in Bitcoin Core. This method was not configurable.
2. The division operation was done before multiplying the byte amount, which can cause small differences when using uncommon scripts and minrelaytxfee values.
We are emptying the `hash_types` module. `impl_asref_push_bytes!` is an
internal macro, as such it can live in the `internal_macros` module.
While we are at it import the macro and call it without any qualifying
path, this is typical for our usage of other internals/internal_macros
usage.
We would like all the various hash types to be defined where they
rightly live instead of in the `hash_types` module.
Move transaction hash types to the `transaction` module.
Andrew Poelstra
Tobin C. Harding
yancy
startup-dreamer
josibake
Fmt Bot
GoodDaisy
shuoer86
conduition
harshit933
Martin Habovstiak
Jonathan Underwood