7685461e62 Document the sha256t_hash_newtype direction (Tobin C. Harding)
30e91cc766 Default to forward for tagged hashes (Tobin C. Harding)
5ecc69cd28 Add forward/backward unit test (Tobin C. Harding)
9aee65d1ba Refactor tagged hash tests (Tobin C. Harding)
216422dffc Remove schemars impl for test type (Tobin C. Harding)
Pull request description:
First three patches are preparation, improvements to the units tests in `sha256t`.
From the final patch:
Displaying backward is an anomaly of Bitcoin Core's early days and the
double SHA256 hash type. We should not let this unfortunate beast leak
out into other places.
Default to displaying forward when creating a new tagged hash and remove
all the explicit attributes from `bitcoin` that just clutter the code.
This is an API break and may quietly break some users downstream - eventually we should stop doing that sort of thing.
ACKs for top commit:
apoelstra:
ACK 7685461e62
Tree-SHA512: cb8a41b207aa68ecf63cb7af7f39f7d7c8a3a27f38595867949b288a81a20bff0c17aa4c17bb099e2ecf85194d83bad23c9c9792f511b6c4cd625ff27c1affaa
As part of the ongoing effort to improve `hashes`; stop using slicing of
hash types and use `as_byte_array()` to get an array reference instead.
This gives us more flexability to modify the `hashes` module.
If one writes signing data using one of the two
`*_encode_signing_data_to` functions then creating the message to sign
is slightly nuanced and different for each of the functions. For Taproot
one must use a specific tagged hash and for ECDSA one must use a sha256d
hash.
Add documentation that explains the hashing requirements for each
function.
Displaying backward is an anomaly of Bitcoin Core's early days and the
double SHA256 hash type. We should not let this unfortunate beast leak
out into other places.
Default to displaying forward when creating a new tagged hash and remove
all the explicit attributes from `bitcoin` that just clutter the code.
We are currently using the `base58::Error` type to create errors in
`bitcoin`, these are bitcoin errors not `base58` errors.
Note that we add what looks like duplicate
`InvalidBase58PayloadLengthError` types but they are different because
of the expected length. This could have been a field but I elected not
to do so for two reasons:
1. We will need to do so anyways if we crate smash more
2. The `crypto::key` one can have one of two values 33 or 34.
With this applied we can remove the now unused error variants from
`base58::Error`.
0d64ae6eb4 Added tests for PublicKey::from_str (Sh0g0-1758)
Pull request description:
Fixes: #2550
Added some new tests and refactored some older tests.
ACKs for top commit:
sanket1729:
ACK 0d64ae6eb4
apoelstra:
ACK 0d64ae6eb4 thanks for bearing with me!
tcharding:
ACK 0d64ae6eb4
Tree-SHA512: b6792590c56ccac8e8cf6f182e74cb77c4652c537c0357456ff21a7814ebcc8cf48e0fad4c8d47e6e786a50e2cbb48134cb64406bcc900b4fcad9304d9cf4167
Constructing a pubkey using `PublicKey::from_slice` can fail for reasons
other than just incorrect length - we should not be using `expect` but
rather returning the error.
A purist might argue that we are now returning a nested error type with
an unreachable variant:
`ParsePublicKeyError::Encoding(FromSliceError::InvalidLength)`
Is this acceptable or do we want to further improve this?
Changes -
- in `from_slice` changed the `error` to `FromSliceError`.
- in `verify` changed to `secp256k1::Error` as it can return only one error.
- in `from_str` changed to `FromSliceError`.
- in `CompressedPublicKey` changed `verify` from `Error` to `secp236k1::Error` as it only returns one error.
- introduces CompressedPublicKeyError
- Removes impl from `bip32.rs`
- introduces `ParsePubKeyError` to return errors while generating publickey from string
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
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.
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.
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
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.
The errors `SegwitV0Error` and `LegacyScripthashError` contained only
one variant - out of range. There will not be a new one in the future so
this change flattens it to simplify.
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.
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
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
8783d526bd fix : adds the arrayvec dependency (harshit933)
Pull request description:
This commit adds the arrayvec dependency to the sortKey.
Potential fix#2276
ACKs for top commit:
Kixunil:
ACK 8783d526bd
apoelstra:
ACK 8783d526bd
Tree-SHA512: 35f28ade02dd526ce5dfa2f42578b36cd5af29a5a9f409da70a775bc12046674737e9bce9fabcc87f1b4669080ad10465c75601342f280c11eab11f791f44c36
We only use the network to serialize and deserialize from WIF.
For this we only really need network kind since WIF only differentiates
between mainnet and non-mainnet.
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.
When we use the `fmt::Write` trait it is just to call its methods, we
can therefore, without any change to the logic, use `as _` when
importing the trait. This prevents naming conflicts.
Done in preparation for importing the `io::Write` trait.
Generic types can be single letters, and a writer is conventionally, in
this codebase at least, called `W`.
Use `W` instead of `Write` with no loss of clarity.
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
3d17031725 Derive Debug for PrivateKey for no-std builds (Tobin C. Harding)
Pull request description:
Currently we derive `impl Debug for PrivateKey` for "std" builds and manually implement an obfuscated version for "no-std" builds. Since we enable the `hashes` feature of `rust-secp` this is unnecessary because secp takes care of obfuscating the secret for us.
ACKs for top commit:
apoelstra:
ACK 3d17031725
Kixunil:
ACK 3d17031725
Tree-SHA512: 0ce394c6517c51e8964290a980cddd20186d19bcc6cbb8c71aa09b7485d6a0df373960798418184971e1c6e5a6b8f725dd44ebfa7184e31b63faf105dea69725
Andrew Poelstra
Tobin C. Harding
Fmt Bot
Sh0g0-1758
Liam Aharon
harshit933
Martin Habovstiak