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.
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.
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.
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)]`.
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.
No idea why this re-export is here, the `Prevouts` type is not even used
in the `psbt` module.
Remove the re-export of `crate::sighash::Prevouts` from `pstb`.
We currently use the functions `min_value` and `max_value` because the
consts were not available in Rust 1.41.1, however we recently bumped the
MSRV so we can use the consts now.
fabcde036f Use package in manifest and shorten import (Tobin C. Harding)
Pull request description:
We can use `package` to rename `bitcoin_hashes` to `hashes` and `bitcoin_internals` to `internals`. This makes imports more terse with no loss of meaning.
ACKs for top commit:
apoelstra:
ACK fabcde036f
Kixunil:
ACK fabcde036f
Tree-SHA512: bc5bff6f7f6bf3b68ba1e0644a83da014081d8c6c9d578c21cb54fdd56a018f68733dd1135d05b590ba193ed9efd12fa9019182c1fed347e604d8548f6ef9103
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of
`#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually
mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.
Sweeeeeet.
We can use `package` to rename `bitcoin_hashes` to `hashes` and
`bitcoin_internals` to `internals`. This makes imports more terse with
no loss of meaning.
"schnorr" is a dirty word; the current `schnorr` module defines a
`Signature` that includes a sighash type, this sighash type is a bitcoin
specific construct related to taproot. Therefore the `Signature` is
better named `taproot::Signature`. Note also that the usage of `schnorr`
in `secp256k1` is probably justified because the
`secp256::schnorr::Signature` is just doing the crypto.
While we are at it, update docs and error messages to use "taproot"
instead of "schnorr". Also change function names and identifiers that
use "schnorr".
Currently we have `TapSighash` that is used for taproot sighashes but
for non-taproot sighashes we use `hash_types::Sighash`. We can improve
the API by creating a `LegacySighash`, and `SegwitV0Sighash`.
Copy the original `Sighash` macro calls to create the two new types in
the `sighash` module.
While we are at it, put the `TapSighash` and `TapSighashTag` into the
`sighash` module also.
4a03e2e721 psbt: Remove unused error variant (Tobin C. Harding)
Pull request description:
Remove an unused error variant for PSBT code (API breaking because the error type is public).
Woops, somehow I managed to get what was patch 1 of this series merged yesterday, I thought I left it out. Anyways, this is just the remove unused error variant now. No changes to that patch from previous versions of the PR.
ACKs for top commit:
apoelstra:
ACK 4a03e2e721
Kixunil:
ACK 4a03e2e721
Tree-SHA512: 228c661b97c6656db5a2bcc9ceb494ea485363b7f7262a97c677ee1639b5209c92ec3715ff48fdb108c95c828bfc83b6c475aa66f0ce8c5b0f286bfa7cc19554
The requirement for a type dereferencing to `Transaction` prevented
storing the cache in long-lived data without resorting to silly
wrappers. Since `Borrow` is implemented both for `T` and for smart
pointers it's a more flexible bound which this change implements.
While this is technically breaking, all usual non-generic code will
continue to work beause smart pointers generally have `Borrow`
implemented.
`Sighash` should be displayed forwards according to BIP143. Currently we
are displaying it backwards (as we do for double SHA256). This is
working because parse using `Vec::from_hex`.
We have the means to parse hex strings directly for hashes, we no longer
need `hex_from_slice`.
BIP143 test vectors display double SHA256 forwards but we display
backwards, this is acceptable because there is no fixed display in the
ecosystem for double SHA256 hashes. In order to overcome this we parse
test vector hex strings with into `Vec` when needed.
Remove `FromHex` from hash and script types
- Remove the `FromHex` implementation from hash types and `ScriptBuf`
- Remove the `FromStr` implementation from `ScriptBuf` because it does not
roundtrip with `Display`.
- Implement a method `from_hex` on `ScriptBuf`.
- Implement `FromStr` on hash types using a fixed size array.
This leaves `FromHex` implementations only on `Vec` and fixed size arrays.
In preparation for modifying some unit test data structures, manually
format the code so it is uniform.
Move elements added to a vec with `vec!` onto a new line so they all
line up and one can better see what fields go where.
Refactor only, no logic changes.
e7bbfd3913 Improve Psbt error handling (DanGould)
Pull request description:
## Separate `encode::Error` and `psbt::Error` recursive dependency
This initial work attempts to fix#837's first 2 points
> - The current psbt::serialize::Deserialize has an error type of consensus::encode::Error. I think we should cleanly separate consensus encoding errors from application-level encoding errors like psbt.
> - There is a recursive dependence between encode::Error and psbt::Error which would need to be cleanly dissected and separated so that there is no dependence or only one-way dependence.
## Better `ParseError(String)` types
arturomf94 how compatible do your #1310 changes look to address #837's third point with this design?
> - There are a lot ParseError(String) messages that could use a better type to downflow the information.
I think your prior art would completely address this issue now.
## On handling `io::Error` with an associated error
`encode::Error` has an `Io` variant. now that `Psbt::deserialize` returns `psbt::Error` and produces an `io::Error`, we need an `Io` variant on `psbt::Error`. Except that doing so breaks `#[derive(Eq)]` and lots of tests for `psbt::Error`.
Kixunil, I'm trying to understand your feedback regarding a solution to this problem.
> I believe that the best error untangling would be to make decodable error associated.
> I meant having associated `Error` type at `Decodable` trait. Encoding should only fail if the writer fails so we should have `io::Error` there (at least until we have something like `genio`).
>
> > [it] is a problem to instantiate consensus::encode::Error in [the psbt] module for `io::Error`?
>
> It certainly does look strange. Maybe we should have this shared type:
>
> ```rust
> /// Error used when reading or decoding fails.
> pub enum ReadError<Io, Decode> {
> /// Reading failed
> Io(Io),
> /// Decoding failed
> Decode(Decode), // consensus and PSBT error here
> }
> ```
>
> However this one will be annoying to use with `?` :( We could have `ResultExt` to provide `decode()` and `io()` methods to make it easier.
>
> If that's not acceptable then I think deduplicated IO error is better.
Kixunil didn't we just get rid of Psbt as `Decodable`? Would this make more sense to have as an error associated with `Deserialize`? Or did we do the opposite of what we should have by making Psbt only `Serialize`/`Deserialize` because of #934, where only consensus objects are allowed to be `Decodable`? I wonder if we prioritized that strict categorization and are stuck with worth machinery because of it. My goal with #988 was to get to a point where we could address #837 and ultimately implement PSBTv2.
ACKs for top commit:
tcharding:
ACK e7bbfd3913
apoelstra:
ACK e7bbfd3913
Tree-SHA512: 32975594fde42727ea9030f46570a1403ae1a108570ab115519ebeddc28938f141e2134b04d6b29ce94817ed776c13815dea5647c463e4a13b47ba55f4e7858a
Addresses with Segwitv0 not having len 20/32 are invalid and cannot be
constructed. Also cleans up a API bug in
ScriptBuf::new_witness_prog(ver, prog) allowing prog of invalid lenghts.
c4363e5ba1 Deserialize Psbt fields, don't consensus_encode (DanGould)
c1dd6ad8a2 Serialize Psbt fields, don't consensus_encode them (DanGould)
1b7b08aa5d De/serialize Psbt without consensus traits (DanGould)
Pull request description:
fix https://github.com/rust-bitcoin/rust-bitcoin/issues/934
Instead of using consensus {de,en}code, serialize high-level structures (PartiallySignedTransaciton, Output, Input) borrow the signature from `Serialize`, `Deserialize` traits and implement them on Psbt:
```rs
impl Psbt {
/// Serialize a value as raw data.
fn serialize(&self) -> Vec<u8>;
/// Deserialize a value from raw data.
fn deserialize(bytes: &[u8]) -> Result<Self, encode::Error>;
}
```
ACKs for top commit:
apoelstra:
ACK c4363e5ba1
tcharding:
ACK c4363e5ba1
sanket1729:
ACK c4363e5ba1. One small comment, but can be addressed in follow up if there is nothing else from other reviewers.
Kixunil:
ACK c4363e5ba1
Tree-SHA512: d8dd5ef1189b36fde08969b1ec36006a6fc55ae990f62ea744e6669e03a7f9e90e1d5907be7eac48ee1af23bc20a62aa7e60ff1ff78080d0e923bb9ccedcd432
This makes the code less noisy and is a preparation for changing it to
`const`-based literal. Because of the preparation, places that used
variables to store the hex string were changed to constants.
There are still some instances of `Vec::from_hex` left - where they
won't be changeable to `const` and where `hex!` is unavailable
(integration tests). These may be dealt with later.
See also #1189
Currently we enable "secp256k1/rand-std" in the "rand" feature, this is
incorrect because it means "rand" implies "std" which it does not.
Add a "rand-std" feature that turns on "seck256k1/rand-std" and make the
"rand" feature turn on "seck256k1/rand".
This patches `bitcoin_hashes` to use the version in the repository and
fixes the code after removal of `Deref`.
This also turns off `AS_DEPENDENCY` check with the intention to refactor
it later.
This renames `Script` to `ScriptBuf` and adds unsized `Script` modeled
after `PathBuf`/`Path`. The change cleans up the API a bit, especially
all functions that previously accepted `&Script` now accept truly
borrowed version. Some functions that perviously accepted `&[u8]` can
now accept `&Script` because constructing it is no loger costly.
junderw
Andrew Poelstra
Tobin C. Harding
The rustfmt Tyranny
Steven Roose
yancy
sanket1729
Martin Habovstiak
DanGould
Arturo Marquez