hex_psbt was added as a macro so that a panic would reveal the line
number of the failing test by expanding the macro at the test location.
However, a stack trace can be used to reveal the test that caused the
failure using RUST_BACKTRACE=1. Furthermore, the track_caller macro is
added to the helper methods which will reveal the line number of the
calling function (the offending test). More detailed information for
debugging has been added to hex_psbt() so that the offending string
will be included in the panic message.
The macro psbt_with_values is used by the psbt test module. Since
there is no pre-processing required, there is no reason to use
metaprogramming here, so this commit moves the logic from a
macro to common function in the test module.
We have a new API function available with recent version of `secp256k1`
to create a `Message` directly from a sighash byte array.
Use `Message::from_digest(sighash.to_byte_array())` to construct
messages ready to sign.
Upgrade the `secp256k1` dependency to the newly released `v0.28.0`.
FTR this includes two simple changes:
- Use `Message::from_digest_slice` instead of `Message::from_slice`.
- Use `secp256k1::Keypair` instead of `secp256k1::KeyPair`.
6b5d06f23e ci: fix the byteorder to 1.4.3 for edition 2018 (Vincenzo Palazzo)
98513ef151 clippy: more worning fixes (Vincenzo Palazzo)
05d3dc5d72 Remove redundant guard (Tobin C. Harding)
4537634e7e ci: bump rustc to 1.60 for fuzz test (Vincenzo Palazzo)
Pull request description:
Ci looks like broken, so this should fix
it
ACKs for top commit:
apoelstra:
ACK 6b5d06f23e
Tree-SHA512: bfa0eaf8cbc02a671237d99221db8c21264ce9df91301818c95c41dcc5ad4935e91254b0b3fa8f36738a9d71b6541fb8784ac8280d67057960a3d20e385a9f17
8eff4d0385 Remove private hex test macro (Tobin C. Harding)
Pull request description:
We have this macro in `hex-conservative` now, remove the version here.
This patch does not change the public API and only touches test code.
ACKs for top commit:
apoelstra:
ACK 8eff4d0385
clarkmoody:
ACK 8eff4d0385
Tree-SHA512: 93a08fff778930071cd1a28c19202e4a94ca8881b2e873538de2e942b71c2cd6184ed6364c572538a8a699295a71761c6f836accaf251a15683138b71f148fab
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.
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
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
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
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.
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.
66d5800ac0 psbt: Add IndexOutOfBounds error (Tobin C. Harding)
Pull request description:
We currently have a bunch of functions that are infallible if the `index` argument is within-bounds however we return a `SignError`, this obfuscates the code.
Add an `IndexOutOfBoundsError`. While we are at it make it an enum so users can differentiate between which vector the out of bounds access was attempted against.
ACKs for top commit:
sanket1729:
utACK 66d5800ac0. This is a clean improvement over existing code.
apoelstra:
ACK 66d5800ac0
Tree-SHA512: fa8a24990d1dcdab0c9b019fb2387b5a518b02d0a65715f0ab62519894b19c0c74750d3dcdc928626fa68b146038b907d79de3ba9712c9287db8fa64693ebc11
be05f9d852 Rename xpub and xpriv types (Tobin C. Harding)
Pull request description:
The BIP-32 extended public key and extended private key exist in the Bitcoin vernacular as xpub and xpriv. We can use these terms with no loss of clarity.
Rename our current BIP-32 types
- `ExtendedPubKey` to `Xpub`
- `ExtendedPrivKey` to `Xpriv`
This patch is a mechanical search-and-replace, followed by running the formatter, no other manual changes.
ACKs for top commit:
apoelstra:
ACK be05f9d852
sanket1729:
ACK be05f9d852
Tree-SHA512: 49925688783c3f37a9b92a9767a0df095323a3fa51f3d672a0b5dd1d8bca86f7facbcc33921274bc147b369de09042c4850b08c31e63f71110903435daa6c00c
We currently have a bunch of functions that are infallible if the
`index` argument is within-bounds however we return a `SignError`, this
obfuscates the code.
Add an `IndexOutOfBoundsErorr`. While we are at it make it an enum so
users can differentiate between which vector the out of bounds access
was attempted against.
Throughout the codebase we cast values to `u64` when constructing a
`VarInt`. We can make the code marginally cleaner by adding `From<T>`
impls for all unsigned integer types less than or equal to 64 bits.
Also allows us to (possibly unnecessarily) comment the cast in a single
place.
The `ThirtyTwoByteHash` trait is defined in `secp256k1` and used in
`hashes` as well as `bitcoin`. This means that we must use the same
version of `hashes` in both `bitcoin` and `secp256k1`. This makes doing
release difficult.
Remove usage of `ThirtyTwoByteHash` and use `Message::from_slice`.
Include TODO above each usage because as soon as we release the new
version of secp we can use the new `Message::from_digest`.
This is step backwards as far as type safety goes and it makes the code
more ugly as well because it uses `expect` but thems the breaks.
The BIP-32 extended public key and extended private key exist in the
Bitcoin vernacular as xpub and xpriv. We can use these terms with no
loss of clarity.
Rename our current BIP-32 types
- `ExtendedPubKey` to `Xpub`
- `ExtendedPrivKey` to `Xpriv`
This patch is a mechanical search-and-replace, followed by running the
formatter, no other manual changes.
The word "segwit" refers to segwit v0 and taproot but currently we have
`segwit_signature_hash` that is version specific (segwit v0).
- Rename `segwit_encode_signing_data_to` to
`segwit_v0_encode_signing_data_to`
- Add `p2wpkh_signature_hash` and `p2wsh_signature_hash` functions
We keep the single encode function because the error handling is better
that way.
While we are at it test the bip-143 test vectors against all the
sighash types of wrapped p2wsh.
154552e334 docs: Do not link to std::option::Option (Tobin C. Harding)
24843468c3 Remove rustdocs links to serde (Tobin C. Harding)
Pull request description:
Two minor patches to fix up docs links. These were originally done as part of #1880 but are unrelated so pushing them up separately.
ACKs for top commit:
apoelstra:
ACK 154552e334
RCasatta:
utACK 154552e334
Tree-SHA512: e45e1538c66b59d63a66898896927bb6c1336fb4c8515bb9e2204c8035870ef8e4a6fd32dfc83db2938afda67feb27c48989e382410f9e7ea7a967132941c720
3c0bb63423 Do trivial rustdoc improvements (Tobin C. Harding)
3225aa9556 Use defensive documentation (Tobin C. Harding)
80d5d6665a crypto: key: Move error code to the bottom of the file (Tobin C. Harding)
fe3b1e1140 Move From for Error impl (Tobin C. Harding)
5f8e0ad67e Fix docs on error type (Tobin C. Harding)
f23155aa16 Do not capitalize error messages (Tobin C. Harding)
ae07786c27 Add InvalidSighashTypeError (Tobin C. Harding)
baba0fde57 Put NonStandardSighashTypeError inside ecdsa::Error variant (Tobin C. Harding)
6c9d9d9c36 Improve error display imlps (Tobin C. Harding)
22c7aa8808 Rename non standard sighash error type (Tobin C. Harding)
Pull request description:
EDIT: The commit hashes below are stale but the text is valid still.
In an effort to "perfect" our error handling, overhaul the error handling in the `crypto` module.
The aim is to do a small chunk so we can bikeshed on it then I can apply the learnings to the rest of the codebase.
Its all pretty trivial except:
- commit `4c180277 Put NonStandardSighashTypeError inside ecdsa::Error variant`
- comimt `5a196535 Add InvalidSighashTypeError`
- commit `05772ade Use defensive documentation`
Particularly the last one might be incorrect/controversial.
Also, please take the time to check the overall state of error code in the `crypto` module on this branch in case there is anything else we want to do.
Thanks
ACKs for top commit:
apoelstra:
ACK 3c0bb63423
Tree-SHA512: 7e5f8590aec5826098d4d8d33351a41b10c42b6379ff86e5b889e73271b71921fc3ca9525baa5da53e07fa2e961e710393694e04658a8243799950b4604caf43
The `network` module deals with data types and logic related to
internetworking bitcoind nodes, this is commonly referred to as the p2p
layer.
Rename the `network` module to `p2p` and fix all the paths.
As we do for `NonStandardSighashErrorType` add an error struct for
invalid sighash type, used by the `taproot` module instead of returning
a generic error enum with loads of unused variants.
Error types conventionally include `Error` as a suffix.
Rename `NonStandardSighashType` to `NonStandardSighashTypeError`.
While we are at it make the inner type private to the crate, there is no
need to leak the inner values type.
07041d584d Apply rustfmt (The rustfmt Tyranny)
dada6d65b7 script: Move some inspector methods from ScriptBuf to Script (Steven Roose)
Pull request description:
Noticed that these methods belong in Script.
ACKs for top commit:
tcharding:
ACK 07041d584d
sanket1729:
ACK 07041d584d.
apoelstra:
ACK 07041d584d
Tree-SHA512: cdcbdf22f0457123205621ec2834164c4598be1e5b221cf859d60e88110b19f8c1e484e86f60653af237e9c2acbcdbe5d2b4c98ccf239924386639c4ba6222f7
As part of an ongoing effort to make our error types stable and useful
add a stand set of derives to all error types in the library.
`#[derive(Debug, Clone, PartialEq, Eq)]`
Add `Copy` if possible and the error type does not include
`#[non_exhaustive]`.
If an error type includes `io::Error` it only gets `#[derive(Debug)]`.
bb8bd16302 internals: Remove hex module (Tobin C. Harding)
2268b44911 Depend on hex-conservative (Tobin C. Harding)
db50509cd3 Add usage docs to the "core2" feature (Tobin C. Harding)
Pull request description:
Use the newly released `hex-conservative` crate, by doing the following:
- Depend on `hex-conservative` in `bitcoin` and `hashes`
- Re-export `hex-conservative` as `hex` from both crate roots.
- Remove all the old hex code from `hashes`
- Remove all the old hex code from `internals`
- Remove the now unused `internals::prelude`
- Fix all the import statements (makes up the bulk of the lines changes in this patch)
ACKs for top commit:
apoelstra:
ACK bb8bd16302
sanket1729:
utACK bb8bd16302
Tree-SHA512: ec83b3941cae6f32272471779f28461bb04959a3f6a126a68bbf2c748d83ff9518ff8932d9e937a6f389c10028bf3eb58c6b6d71ea066924dd7a34faaec7a087
We have just released the `hex-conservative` crate, we can now use it.
Do the following:
- Depend on `hex-conservative` in `bitcoin` and `hashes`
- Re-export `hex-conservative` as `hex` from both crate roots.
- Remove all the old hex code from `hashes`
- Fix all the import statements (makes up the bulk of the lines changed
in this patch)
92749d29e4 Rename PartiallySignedTransaction to Psbt (Tobin C. Harding)
Pull request description:
Last release we added a type alias for `Psbt`, now lets just rename the type and be done with it.
Includes re-export at the crate root because `bitcoin::Psbt` is clear and obvious.
ACKs for top commit:
sanket1729:
ACK 92749d29e4.
apoelstra:
ACK 92749d29e4
Tree-SHA512: 2ded728409829709a46acd2a83ce9a91839bce222264b2fca122b346ec4f45a52c3f970eb05001794e2f355ce9391df1a184b57baf24589e8a5c3f77f72f6ec7
Last release we added a type alias for `Psbt`, now lets just rename the
type and be done with it.
Includes re-export at the crate root because `bitcoin::Psbt` is clear
and obvious.
From BIP 141:
> A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that
> consists of a 1-byte push opcode (for 0 to 16) followed by a data push
> between 2 and 40 bytes gets a new special meaning. The value of the
> first push is called the "version byte". The following byte vector
> pushed is called the "witness program".
`WitnessVersion` and `WitnessProgram` are scriptPubkey concerns and
scriptPubkey is basically synonymous with address so in one way it makes
sense that these types are in `address` however we are in the process of
overhauling the `Address` (and `AddressInner`) types so lets move the
witness stuff to `script` and put it in individual sub-modules.
This move helps simplify the address error type also.
Note please, there are a bunch of formatting changes in here in the
error type that I cannot explain and could not remove.
This error type is only used in the `from_slice` function. Use prefix
`Sig` because `taproot::FromSliceError` does not fully express how the
error came about.
Use specific identifier for the error, this aids usage but also prevents
us later adding "random" other variants into this error and using it in
other functions.
yancy
Tobin C. Harding
Andrew Poelstra
Vincenzo Palazzo
junderw
Clark Moody
Riccardo Casatta
The rustfmt Tyranny
Steven Roose