10374af75c Make error types uniform (Tobin C. Harding)
43d3306822 Use explicit error::Error impl instead of the default (Tobin C. Harding)
2512dbafc2 Remove impl_std_error macro (Tobin C. Harding)
6933ca4fc2 Add suffix to HiddenNodes error type (Tobin C. Harding)
2b40ea24fb Add suffix to IncompleteBuilder error type (Tobin C. Harding)
f41416a0ea Add suffix to UnknownMagic error type (Tobin C. Harding)
5658dac024 Add suffix to UnknownChainHash error type (Tobin C. Harding)
2fb71dd943 Move p2p error types to bottom of file (Tobin C. Harding)
39314ad52f Move error code to match conventional layout (Tobin C. Harding)
Pull request description:
PR aims to achieve two things:
- Make error code brain dead easy to read
- Get error code closer to being ready for v1.0
The first 8 patches are pretty basic, and are broken up into really small changes. The last patch is much bigger, it has a long git log to explain it but reviewing should not take too much brain power.
This PR does not introduce anything new, it just applies what we have been doing recently with errors. Before v1.0.0 others will likely want to re go over all the error types. As such I believe this PR can be merged under the one ack carve-out.
### TODOs (future PRs)
We have a few errors that still need splitting up:
- Split up `merkle_tree::block::MerkleBlockError`
- Split up `psbt::error::Error`
- Split up `IncompleteBuilderError`
Also, all error From's should probably have `#[inline]`, I noticed late in the process and did not have the heart to visit every error again.
ACKs for top commit:
apoelstra:
ACK 10374af75c
clarkmoody:
ACK 10374af75c
Tree-SHA512: 4f4f3533f42dc11af8e7978f3272752bb56d12a68199752ed4af0c02a46a87892b55c695b7007bc3d0bdf389493068d068e2be1780e8c3008815efec3a02eedf
On our way to v1.0.0 we are defining a standard for our error types,
this includes:
- Uses the following derives (unless not possible, usually because of `io::Error`)
`#[derive(Debug, Clone, PartialEq, Eq)]`
- Has `non_exhaustive` unless we really know we can commit to not adding
anything.
Furthermore, we are trying to make the codebase easy to read. Error code
is write-once-read-many (well it should be) so if we make all the error
code super uniform the users can flick to an error and quickly see what
it includes. In an effort to achieve this I have made up a style and
over recent times have change much of the error code to that new style,
this PR audits _all_ error types in the code base and enforces the
style, specifically:
- Is layed out: definition, [impl block], Display impl, error::Error impl, From impls
- `error::Error` impl matches on enum even if it returns `None` for all variants
- Display/Error impls import enum variants locally
- match uses *self and `ref e`
- error::Error variants that return `Some` come first, `None` after
Re: non_exhaustive
To make dev and review easier I have added `non_exhaustive` to _every_
error type. We can then remove it error by error as we see fit. This is
because it takes a bit of thinking to do and review where as this patch
should not take much brain power to review.
In a further effort to make the code brain-dead easy to read; use an
explicit implementation of `std::error::Error` that returns `None`
instead of relying on the default trait implementation.
We would like the codebase to be optimized for readability not ease of
development, as such code that is write-once-read-many should not use
macros.
Currently we use the `impl_std_error` macro to implement
`std::error::Error` for struct error types. This makes the code harder
to read at a glance because one has to think what the macro does.
Remove the `impl_std_error` macro and write the code explicitly.
752adff9d1 Add method calc_height (Tobin C. Harding)
46f5588646 Add unit test for calc_tree_width (Tobin C. Harding)
Pull request description:
Add a private `PartialMerkleTree::calc_tree_width` function and a unit test to test it.
ACKs for top commit:
apoelstra:
ACK 752adff9d1
clarkmoody:
ACK 752adff9d1
Tree-SHA512: 9c4ad9f6ff47d8faad1c7c1e977427f1528af2712ceffd05357d0c9117b5fdb7b2783afc00a75cb19b853bfbd7b3895baa3a3563bdc496593cc9b06ce80dbbf8
3b60ad5567 example: Modify `taproot-psbt.rs` to make the use of prevouts clearer. (S. Santos)
Pull request description:
The `taroot-psbt.rs` example uses only one input, and therefore the current code may not make it clear that the number of prevout items must correspond to the number of transaction inputs, since the prevout slice is built within a loop.
This PR aims to make this clear to any user who wants to reuse the logic from the example code.
ACKs for top commit:
tcharding:
ACK 3b60ad5567
apoelstra:
ACK 3b60ad5567
Tree-SHA512: afad63782b0e8a459de6cf69712d31fdab860c0d4cf9f3a51c3d85544a067bd50f4febc10ec4046e3a37d9ca518bbf2460c2599f1569549701c07f8a267dfd05
dac627cc09 Feature: Psbt fee checks (junderw)
Pull request description:
Closes#2061
These new methods on Psbt will add checks for high fees by default. The threshold for "high fees" is currently set to 25000 sat/vbyte, which is about 20x higher than the highest next block fees seen on the "Mempool" website.
The primary goal of this change is to prevent users of the library from accidentally sending absurd amounts of fees.
(ie. Recently in September 2023 there was a transaction that sent an absurd amount of fees and made news in the Bitcoin world. Luckily the mining pool gave it back, but some might not be so lucky.)
There are variants of the method that allow for users to set their own "absurd" threshold using a `FeeRate` value. And there is a method that performs no checks, and the method name is alarming enough to draw attention in a review, so at least developers will be aware of the concept.
ACKs for top commit:
apoelstra:
ACK dac627cc09
tcharding:
ACK dac627cc09
Tree-SHA512: ae0beafdb50339ba3efc44a48ba19c0aeeb0a2671eb43867c1e02b807677ce99fb6b4c47b74a9ed2999f827b3edc00a8871fa4730dd12a4cb265be99437c13db
c34e3cc7cc Re-write size/weight API (Tobin C. Harding)
73f7fbf520 Add code comments to transaction serialization (Tobin C. Harding)
29f20c1d0b Add segwit serialization constants (Tobin C. Harding)
Pull request description:
Audit and re-write the weight/size API for `Block` and `Transaction`. First two patches are trivial, patch 3 contains justification and explanation for this work, copied here:
```
Recently we introduced a bug in the weight/size code, while
investigating I found that our `Transaction`/`Block` weight/size APIs
were in a total mess because:
- The docs were stale
- The concept of weight (weight units) and size (bytes) were mixed up
I audited all the API functions, read some bips (141, 144) and re-wrote
the API with the following goals:
- Use terminology from the bips
- Use abstractions that mirror the bips where possible
```
Please note, this PR introduces panics if a sciptPubkey overflows the calculation `weight = spk.size() * 4`.
Fix#2049
ACKs for top commit:
apoelstra:
ACK c34e3cc7cc
sanket1729:
ACK c34e3cc7cc.
Tree-SHA512: 4944f652e6e362a282a5731140a9438a82d243a4c646b4627d9046a9f9cf13c476881750d432cfbc6b5fe5de1f0c4c9c44ed4569dac4bc11b55a5db28793803c
5901d35095 Add push_p2wpkh function on Witness (Tobin C. Harding)
8cd409d561 Deprecate push_bitcoin_signature (Tobin C. Harding)
Pull request description:
In order to create the witness to spend p2wpkh output one must create a `Witness` that includes the signature and the pubkey, we should have a function for this.
## Notes
The PR originally added a `push_p2wphk` method, this is now instead a constrcutor `Witness:p2wpkh` (after review discussion below).
- Patch 1 changes `push_bitcoin_signature` to take an `ecdsa::Sigtnture` instead of an `ecdsa::SerializedSignature`
- Patch 2 takes a `secp256k1::PublicKey` removing the need for an error path (discussed below).
ACKs for top commit:
sanket1729:
ACK 5901d35095
apoelstra:
ACK 5901d35095
Tree-SHA512: 646014d97daafbf0909106d8990debaf481ac6f3578f0ddf232d739c3e2d55ae1d0275abe5a4a1db1c5c192c8c5f0b5546fc65aac37b91a3729db881c5ad3dec
24e5e19ed0 Add changelog entries (Tobin C. Harding)
79f2157311 bitcoin: Grab changelog entry from 0.30.1 release (Tobin C. Harding)
Pull request description:
Gearing up for the next release add changelog entries for everything merged upto July 24.
ACKs for top commit:
apoelstra:
ACK 24e5e19ed0
clarkmoody:
ACK 24e5e19ed0
Tree-SHA512: 12761623f3d61fea77b5dfe4c505dcdb5041ec4473ae05524b818eb2d4e3cbc3b039b8f1400022916c9f2d1ee97fcccd7e7b6da0bb5f868a1b77bdcfade3d7ed
Recently we introduced a bug in the weight/size code, while
investigating I found that our `Transaction`/`Block` weight/size APIs
were in a total mess because:
- The docs were stale
- The concept of weight (weight units) and size (bytes) were mixed up
I audited all the API functions, read some bips (141, 144) and re-wrote
the API with the following goals:
- Use terminology from the bips
- Use abstractions that mirror the bips where possible
The `Witness::push_bitcoin_signature` method is old and a bit stale.
Bitcoin has taproot signatures now so the name is stale, also we have
the `crate::ecdsa::Signature` type that holds the secp sig and the hash
type so we can use that instead of having two separate parameters.
Add a new, up to date, `Witness::push_ecdsa_signature` function and
deprecate the `push_bitcoin_signature` one.
There is no logical default for the transaction version number, there is
only pre-bip68 (v1) and post-bip68 (v2). Uses should specify the version
they want not rely on us making the choice.
(I originally added this impl to support testing, this was in hindsight
the wrong thing to do, props to Sanket for noticing.)
One of our stated aims is to make it possible to learn bitcoin by using
our library. To help with this aim add to private consts for the segwit
transaction marker and flag serialization fields.
158ba26a8a Feature: Count sigops for Transaction (junderw)
Pull request description:
I copied over the sigop counting logic from Bitcoin Core, but I made a few adjustments.
1. I removed 2 consensus flags that checked for P2SH and SegWit activation. This code assumes both are activated. If we were to include that, what would be a good way to go about it? (ie. If I run this method on a transaction from the 1000th block and it just so happened to have a P2SH-like input, Bitcoin Core wouldn't accidentally count those sigops because the consensus flag will stop them from running the P2SH logic. Same goes for SegWit)
3. Since there's no guarantee that we have an index from which we can get the prevout scripts, I made it into a generic closure that looks up the prevout script for us. If the caller doesn't provide it, We can only count sigops directly in the scriptSig and scriptPubkey (no P2SH or SegWit).
## TODO
- [x] Write tests for transaction sigop counting
~~Edit: The test changes are just to get the 1.48 tests passing. I'll remove them and replace them with whatever solution that is agreed upon in another PR etc.~~
Edit 2: This is the code I used as a guide:
8105bce5b3/src/consensus/tx_verify.cpp (L147-L166)
Edit 3: I found a subtle bug in the implementation of `count_sigops` (https://github.com/rust-bitcoin/rust-bitcoin/pull/2073#issuecomment-1722403687)
ACKs for top commit:
apoelstra:
ACK 158ba26a8a
tcharding:
ACK 158ba26a8a
Tree-SHA512: 2b8a0c50b9390bfb914da1ba687e8599b957c75c511f764a2f3ed3414580150ce3aa2ac7aed97a4f7587d3fbeece269444c65c7449b88f1bdb02e573e6f6febd
e4c7e01a6f Use the new bech32 iterator API (Tobin C. Harding)
Pull request description:
Depend on the newly released version of `bech32`, BOOM!
ACKs for top commit:
apoelstra:
ACK e4c7e01a6f
clarkmoody:
ACK e4c7e01a6f
Tree-SHA512: 91675a830cf67f8dcabd42e7dc1b70d80b669330be5244bb8102e0ec5d1a206d5ead07f73b328a158b761c328bc78d573185af8d31f14183ccc17318d752c02b
95b7a95fc2 test: correct psbt used for regression test (Subhradeep Chakraborty)
Pull request description:
The `unknown` key used in the `Psbt` (in `serde_regression_psbt` test) is not really unknown. Also the `preimages` in the `Input` don't seem to be valid. Though they don't affect the tests because of the `serde::serialize`, the `Psbt` itself is invalid and hence this PR.
ACKs for top commit:
tcharding:
ACK 95b7a95fc2
apoelstra:
ACK 95b7a95fc2
Tree-SHA512: 275320bb273163364640351e55a3f16c34c1eef06e9dde79b72bf40187d313f037edb5e4544f4dce32fb8a7cc552504aa2e4d22a646c89a3dec022f917539219
b2a7d7023c Rename XpubIdentifier to XKeyIdentifier (Tobin C. Harding)
ffd2466ad1 Move XpubIdentifier to the bip32 module (Tobin C. Harding)
Pull request description:
- Patch 1: Move the hash to the `bip32` module where it is used, as we have done with other hashes recently (and re-export it at crate root).
- Patch 2: Rename the hash to `XKeyIdentifier` as discussed in #2014Fix: #2014
ACKs for top commit:
apoelstra:
ACK b2a7d7023c
Tree-SHA512: 5efa9fc857c71e506263bf6adee3b4294f22838d5b119177c9108c69191d545338c11a4796bc95e956a67f3418010725f3d12c06d2c4c3bb5cf038d59976ae0f
71a5fe2b54 Customize Debug implementation of absolute::LockTime (Subhradeep Chakraborty)
Pull request description:
Fixes https://github.com/rust-bitcoin/rust-bitcoin/issues/2011.
This PR aims to make the "pretty print" of `absolute::LockTime` prettier by printing `X blocks` and `X seconds` for `Blocks` and `Seconds` respectively instead of the default Enum printing.
ACKs for top commit:
apoelstra:
ACK 71a5fe2b54
tcharding:
ACK 71a5fe2b54
Tree-SHA512: 79baad5ee0ba1aa892cb38588dae6a24977b8e42356208d6cc9adb007d1f8371e61dc82e35bb4dac9961216d68c56907f0db376c5c6fbb5a406728b8f7c3f5ad
bc398204bf Remove redundant segwit version from function names (Tobin C. Harding)
Pull request description:
A P2TR output does not need to be clarified with version 1, it is implicit. As with p2wpkh/p2wsh and version 0.
Remove redundant version identifiers from function names, deprecating the originals.
ACKs for top commit:
apoelstra:
ACK bc398204bf
Tree-SHA512: 49806c564badca25ce02161445b2b41497b565f2002aa1edfc0cf0c57b38683480deec0d9b682e18dc7e59c22128e0b641abcccc2cbedd0b5603cbcbf2fd26df
f17bb0d18f Remove unnecessary reference (Tobin C. Harding)
Pull request description:
`T` is a generic that implements`AsRef<PushBytes>`, it should not be a reference. This is inline with other usages of `AsRef<PushBytes>` for example in `Builder::push_slice`.
Found while working on #2003
ACKs for top commit:
apoelstra:
ACK f17bb0d18f
Tree-SHA512: 6f6ae0ba5d5010db53d9c2af107df84bc058277b2b7cc35800f4e6ed93d351838b7f101284b7d80345bee639615d27d76a2e5c4c784782c5b3e5090444defe29
BIP-68 activated a fair while ago (circa 2019) and since then only
transaction versions 1 and 2 have been considered standard.
Currently in our `Transaction` struct we use an `i32`, this means users
can construct a non-standard transaction if they do not first look up
what the value should be. We can help folk out here by abstracting over
the version number.
Since the version number only governs standardness elect to make the
inner `i32` public (ie., not an invariant). The aim of the type is to
make life easy not restrict what versions are used.
Add transaction::Version data type that simply provides two consts `ONE`
and `TWO`.
Add a `Default` impl on `Version` that returns `Version::TWO`.
In tests that used version 0, instead use `Version::default` because the
test obviously does not care.
52f2332383 Remove docs from witness version conversion functions (Tobin C. Harding)
47d6d785cb Remove bip 173/350 test vectors (Tobin C. Harding)
e0eaeaad99 Split ParseError out of Error (Tobin C. Harding)
0f536e86dc Add new UnknownAddressTypeError for parsing address type (Tobin C. Harding)
e2014cba1b Import error variants within dislay impl (Tobin C. Harding)
9d7791fcd6 Remove unnecessary self:: from error import (Tobin C. Harding)
b2e485ed51 Split the address error code out into a separate module (Tobin C. Harding)
f34ca0c52b Move address.rs to address/mod.rs (Tobin C. Harding)
Pull request description:
In preparation for depending on the recently released version of `rust-bech32` do a bunch of preparatory fixes.
1. Improve `address` module error handling as we are doing else where at the moment
2. Remove bip 173 and 350 test vector tests, these are fully covered in bech32
3. Trim down the docs on `WitnessVersion`
This PR is the first 8 patches of https://github.com/rust-bitcoin/rust-bitcoin/pull/1951
ACKs for top commit:
sanket1729:
ACK 52f2332383
apoelstra:
ACK 52f2332383
Tree-SHA512: 67a4ea4020b4e5c9c8396e4195e06dbd1d11335788f9e52f60abbc0b399e37e5dacc9bb7fa4e88221670322fa3c3407ade059d5c709f96e2df97240f4524e08c
ccdcffe69c Re-export Opcode (Tobin C. Harding)
Pull request description:
We recently rename `opcodes::All` to `Opcode` but did not re-export it from the crate root. Since it now has a nice clear name we can do so.
Found while hacking up the `rust-bitcoincore-rpc` dependency upgrade for upcoming release `bitcoin v0.31.0`.
ACKs for top commit:
apoelstra:
ACK ccdcffe69c
Tree-SHA512: 8acac90d01f14245f93edc10f3483d8aa9865aca59b6ef42ab744b875558fca8ad74894ad95a83637f0cec7a2a353d74d4d6f7ee5f1d1276cc0fc3dcb6983362
acbf23aaa5 Add `is_multisig` helper to Script type (Clark Moody)
Pull request description:
A new `is_multisig` helper method to classify bare multisig output scripts.
The form of a valid multisig script is:
- Pushnum `M`
- <N> pubkeys
- Pushnum `N`
- `OP_CHECKMULTISIG`
`N` must equal the number of pushed pubkeys, and `M` must be less than or equal to `N`.
I've tested this against the RPC output of Core at the block level, checking that the total number of multisig outputs matches.
```
Block 350338, 89 multisig
Block 350340, 29 multisig
Block 350341, 4 multisig
Block 350343, 579 multisig
Block 350344, 48 multisig
Block 350346, 11 multisig
Block 350347, 404 multisig
Block 350350, 127 multisig
Block 350351, 1 multisig
Block 350353, 40 multisig
Block 350356, 13 multisig
Block 350357, 2 multisig
Block 350358, 1 multisig
```
ACKs for top commit:
tcharding:
ACK acbf23aaa5
apoelstra:
ACK acbf23aaa5
Tree-SHA512: b8feeaa8725ac63a658897dac3b303fc8b3d56674d796b14569548124928329993bea45482928d9ce85231f1b5837922af8c0a77b2601a92f88b5e2a9394e97f
026a55809e Fix: Script::count_sigops parsing should not return a Result (junderw)
Pull request description:
When implementing some tests for the Transaction PR, I noticed that there were coinbase transactions that would pass Bitcoin Core parsing and fail my code.
It turns out that the Script parsing for sigops calls `break` to exit the loop and returns the current n value whenever there is an EarlyEndOfScript error.
See this comment: https://github.com/rust-bitcoin/rust-bitcoin/pull/2073#issuecomment-1722403687 for some links to the relevant source.
ACKs for top commit:
apoelstra:
ACK 026a55809e
tcharding:
ACK 026a55809e
Tree-SHA512: 57c1b88add5e1c9ef9245fcec0e471db55c2f9b1b0b0f8ebd471f1bede0ca5eeb8492d8c75dea1fd43f1343037df44969c9b9fde26a7de9ac68a26dca899e47f
These docs do not add that much value, we do not typically bother
documenting `From` and `TryFrom` implementations because they are super
well known and its obvious from the function signature what is going on.
The `address::Error` is module level general, we can make the code
easier to maintain and easier to stabalize by splitting the parse error
out of the general error.
Create a `ParseError` that is returned by `FromStr for Address`. Remove
the now unused variants from the general `address::Error`.
Andrew Poelstra
Tobin C. Harding
sanket1729
S. Santos
junderw
Clark Moody
Subhradeep Chakraborty