2169b75bba Use lower case error messages (Jamil Lambert, PhD)
Pull request description:
Error messages should be lower case, except for proper nouns and variable names. These have all been changed.
~~They should also state what went wrong. Some expect error messages were positive, giving the correct behaviour or correct input. These have been changed so that they are now negative, i.e. saying what went wrong.~~
EDIT: After further discussion it was decided not to change the expect messages.
ACKs for top commit:
Kixunil:
ACK 2169b75bba
tcharding:
ACK 2169b75bba
Tree-SHA512: 92442c869e0141532425f6fca5195fd319b65026f68c4230a65ad70253565d98931b2b44ee202975c307280525c505147e272297dc81207312e40c43d007021c
64c31cfb97 Move locktimes and Sequence to primitives (Tobin C. Harding)
Pull request description:
The `absolute` and `relative` locktimes as well as the `Sequence` are all primitive bitcoin types.
Move the `Sequence`, and `locktime` stuff over to `primitives`.
There is nothing surprising here, the consensus encoding stuff stays in `bitcoin` and we re-export everything from `blockdata`.
Note please `Sequence` is no longer publicly available at `bitcoin::transaction::Sequence` but it is available at `bitcoin::Sequence`.
ACKs for top commit:
Kixunil:
ACK 64c31cfb97
apoelstra:
ACK 64c31cfb97
Tree-SHA512: 968aa595bfb53e8fcfa860dae797ec5381ed49c67326a8ef9494086ec65d737502dffe4b24143e159042d07c59c5109d50103029082f87e1c3c26671e975f1b3
The `absolute` and `relative` locktimes as well as the `Sequence` are
all primitive bitcoin types.
Move the `Sequence`, and `locktime` stuff over to `primitives`.
There is nothing surprising here, the consensus encoding stuff stays in
`bitcoin` and we re-export everything from `blockdata`.
164b72e07b Update Key documentation (Shing Him Ng)
Pull request description:
The documentation for `Key` was a bit confusing since `<key> := <keylen> <keytype> <keydata>` was right above the `key` field, when in reality the `key` field represents the key data. Moving the documentation that describes a `Key` as a whole to the struct itself and specifying that `key` represents key data will hopefully clear things up a bit
ACKs for top commit:
Kixunil:
ACK 164b72e07b
tcharding:
ACK 164b72e07b
Tree-SHA512: 33c2b24d3006a0ed85a5d96351e0a01c1365c8ea01472233b024de54941319cdbefa0126f8b9538385e8f33ba7e2e3895f0dc5b6a36a1c501c8b97ebbede6502
Done in preparation for moving the script types to `primitives`.
The script types have a bunch of functionality to support scriptPubkeys,
and scriptPubkeys are an address thing.
Create a module under `address` and in it create a bunch of extension
traits to hold all scriptPubkey functionality.
Includes adding an ugly-as-hell macro to create the traits.
Wildcards have been replaced with what is actually used.
In a couple of cases an additional use statement was added to the test
module to import `DisplayHex` which is only used in test, but
previously imported with the wildcard at the top.
8ee1744b9b Make 'use core::fmt' calls consistent (Shing Him Ng)
Pull request description:
I started taking a look at #2869 and looked for everything that was implementing the `Display` trait:
```rust
impl fmt::Display for _
```
but found some places where the imports weren't consistent:
```rust
impl Display for _
```
There were only a few instances of the latter, so I went ahead and cleaned those up before starting #2869
I started pulling this thread when I saw the same thing was happening for `fmt::Debug` and `fmt::Formatter` so I updated the rest of the `use core::fmt::*` statements with a few exceptions:
- No updates to `use core::fmt::*` if it was being called from within a function since I felt like the function scope was small enough to not cause confusion
- No updates to `use core::fmt::{self, Write as _};`
ACKs for top commit:
Kixunil:
ACK 8ee1744b9b
tcharding:
ACK 8ee1744b9b
Tree-SHA512: 33eb6ea0c4e808ef78bc87de6547144b756bde206c50d80488f740e97cd8d11f1abcb8936c487d7bfd29be5e21c7f40ff88f82acdaaec9aacb4b6362ffc4c680
2bde5d002e api: Run just check-api (Jose Storopoli)
d1f84329e4 psbt: implement const for PsbtSighashType::ALL (Jose Storopoli)
Pull request description:
Closes#2751.
I only did the `ALL` which is by far the most common case.
ACKs for top commit:
tcharding:
ACK 2bde5d002e
apoelstra:
ACK 2bde5d002e
Tree-SHA512: 693575de758657a3e172d86ba5114ec0bf3b12b82df598e38c6a7916c99c20cfb5c4e74442108b51ae4e7bb1f1e940fd4a7269145e3f9838f727675c7711c890
In the next commits we are going to stop exposing the ability to hash
arbitrary data into wrapped hash types like Txid etc. In preparation for
this, stop using these methods internally.
This makes our internal code a little bit uglier and less DRY. An
alternative approach would be to implement the from_engine and engine
methods, but privately (and maybe having a macro to provide this). But I
think this approach is more straightforward.
The one exception is for the Taproot hashes, which are tagged hashes and
currently do not have their own engine type. I will address these in a
later PR because this one is already too big.
4652ce20ed API changes for "delete `all_zeros`" (Andrew Poelstra)
8869f35a69 hashes: drop the `all_zeros` method on arbitrary hashes (Andrew Poelstra)
9f8797f486 API changes for constification of hash constructors (Andrew Poelstra)
154e91af8c hashes: constify a bunch of constructors (Andrew Poelstra)
c155cbf8b2 hashes: use workaround to get constfns on tagged hashes with MSRV (Andrew Poelstra)
Pull request description:
I think these changes are both uncontroversial but they have fairly large diffs so I am PRing them together before making more invasive changes.
ACKs for top commit:
tcharding:
ACK 4652ce20ed
Tree-SHA512: 4560fa397deab50448598894b9364f9d8f8b48169901a84db6a44168cdba795ab69b48ad2cac61caebcee5e227a03271335b405cf5514265290a4d1f2fdf52a2
433fd6bf7e api: Run just check-api (Tobin C. Harding)
8fd583b069 Pass hash types by value (Tobin C. Harding)
Pull request description:
We should pass `Copy` types by value not by reference. Pass the hash types by value.
Second step in the pass-copy-types-by-value work, pulled out of #2404.
ACKs for top commit:
apoelstra:
ACK 433fd6bf7e
Kixunil:
ACK 433fd6bf7e
Tree-SHA512: 999d12f60550cacc4ae19b4cbf505b25c1eed803820f22b1a706e9f95da1b7e7b422f393f4115d579927c0c476cd504036a39b3cdc06a1d6befbcff5513f7433
39df0a9fbe update api (Divyansh Gupta)
3a5f2932a4 create constants for ChildNumber enum (Divyansh Gupta)
Pull request description:
this aims to fix#2750
ACKs for top commit:
tcharding:
ACK 39df0a9fbe
Tree-SHA512: e1c38568facd2b9aa55b1b1ec0d5d5f68ff38ca3fe68962bc316c060a062299935aa51bcfc1c255a7f5c9ad97435cab22e2c160d3fd3f52a46f6b5cbb7d5743f
Manually implement it for Wtxid, Txid and BlockHash, where the all-zero
"hash" has a consensus meaning. But in general we should not be
implementing this method unless we have a good reason to do so. It can
be emulated or implemeted in terms of from_byte_array.
The use of Wtxid::all_zeros is obscure and specific enough that I am
tempted to drop it. But for txid and blockhash, the 0 hash appears in
actual blockdata and we should keep it.
All other uses of all_zeros were either in test code or in places where
the specific hash was not important and [u8; 32] was a more appropriate
type.
the `blockdata` directory is code organisation thing, all the
types/modules are re-exported from other places. In preparation for, and
to make easier, the `primitives` crate smashing work - remove all
explicit usage of `blockdata`.
Note that the few instances remain as they seem required e.g.,
`pub(in crate::blockdata::script)`
Refactor only, no logic changes.
There are two limits that the Bitcoin network enforces in regard to
hashing scripts
- For P2SH the redeem script must be less than 520 bytes
- For P2WSH the witness script must be less than 10,000 bytes
Currently we are only enforcing the p2sh limit when creating an address
with `Address::p2sh`.
There are various ways to create addresses from script hashes and if
users manually hash a script then use the `ScriptHash` (or
`WScritpHash`) our APIs assume the script that was hashed is valid. This
means there is the potential for users to get burned by creating
addresses that cannot be spent, something we would like to avoid.
- Add fallible constructors to `ScriptHash` and `WScriptHash`
- Add `TryFrom` impls as well to both types
- Remove the `From` impls
9f01871c11 api: Run just check-api (Tobin C. Harding)
7929b51640 Pass keys by value (Tobin C. Harding)
Pull request description:
We should pass `Copy` types by value not by reference. Pass the key types by value.
This is patch 1 from #2404
ACKs for top commit:
apoelstra:
ACK 9f01871c11 this will annoy some people but I think we should do it
Tree-SHA512: 18afab537edf4ade4dc1c1e5992e50060b8935531f1e3cbe1d3b94b2fcb87aafa39947f342e0e762835bda3b4091dd35b3b74ea79f4dbb3b21660ffd21d1f82e
Currently we have a trait `Hash` that is required for `Hmac`, `Hkdf`,
and other use cases. However, it is unegonomic for users who just want
to do a simple hash to have to import the trait.
Add inherent functions to all hash types including those created with
the new wrapper type macros.
This patch introduces some duplicate code but we are trying to make
progress in the hashes API re-write. We can come back and de-dublicate
later.
Includes making `to_byte_array`,`from_byte_array`, `as_byte_array`, and
`all_zeros` const where easily possible.
9e4b092fce psbt: Use macro instead of function (Tobin C. Harding)
Pull request description:
We have a private function that makes use of the `Hash` trait to generically hash map entries. This usage makes patching the `hashes` module difficult. We can achieve the same thing by using a macro and passing in the concrete type.
This is an internal change, no effect on logic or public API.
ACKs for top commit:
apoelstra:
ACK 9e4b092fce
Tree-SHA512: 8b788fa91d21bbae556c746c2e55e6e9395e022bedf13193555ef7482109b6ef5032b233c5f37543a31ebda49d9b4761c161ca0db501472047eb661a48e944b7
6ba7758b30 Improve array macros (Tobin C. Harding)
Pull request description:
Currently we have two macros used when creating array wrapper types, one is in `internals` and the other in `bitcoin::internal_macros`. It is not immediately obvious what is what and why there are two.
Improve the macros by:
- Move the inherent functions to `impl_array_newtype`
- Use `*_byte_array` for the names instead of `*_bytes`
- Re-name the other macro to match what it now does
ACKs for top commit:
apoelstra:
ACK 6ba7758b30
Tree-SHA512: 36ed0fae0d28f24d29287062eb05bbc1e9e8b565f4ff41fd893503a25404ed8e185a34d75e398a8a660923ffda3b832b6157011598d5a75a5c4aafdffc74af2a
We have a private function that makes use of the `Hash` trait to
generically hash map entries. This usage makes patching the `hashes`
module difficult. We can achieve the same thing by using a macro and
passing in the concrete type.
This is an internal change, no effect on logic or public API.
802af8e417 Removed //! spare line at end of headers (jamil.lambert)
Pull request description:
Some of the headers had a //! at the end but most didn't. They have all been removed in bitcoin/src/ to make the files consistent
ACKs for top commit:
apoelstra:
ACK 802af8e417
Tree-SHA512: a1eb0dda76af68cb96352f6b31231fa5391d49e11df924065e76871f82231ec0d5751190663f142240e5d757975937387243d1fdac3684d9bdbd7e2362dbd0a7
Currently we have two macros used when creating array wrapper types,
one is in `internals` and the other in `bitcoin::internal_macros`. It
is not immediately obvious what is what and why there are two.
Improve the macros by:
- Move the inherent functions to `impl_array_newtype`
- Use `*_byte_array` for the names instead of `*_bytes` for functions
that return arrays
- Add `as_bytes` to return a slice
- Add `to_bytes` to return a vector
- Re-name the other macro to match what it now does
d353be4546 bip32: derive_xpriv should not return a Result (Jose Storopoli)
Pull request description:
We discussed in #2752 that `derive_priv` never fails.
This PR addresses that issue.
ACKs for top commit:
apoelstra:
ACK d353be4546
tcharding:
ACK d353be4546
sanket1729:
ACK d353be4546
Tree-SHA512: 3a3d09027c6079581636b51a506fac2b325e592d182167c0a3d8676b41fb2bef59a85a404ad12b2e14d73e58fd4b6d8f5923a3dc76a4b9724033097fc08b36ac
Currently we require indexing trait bounds as well as `Borrow` on the
`Hash` trait. We also already implement `AsRef`.
It was observed that `Borrow<[u8]>` does not best describe what we want
from the `Hash` trait implementor but rather `AsRef<[u8]>` does.
Remove all the inexing trait bounds. Remove the `borrow::Borrow<[u8]>`
trait bound. Add a `convert::AsRef<[u8]>` trait bound.
This leaves the `Borrow<[u8]>` implementation for hashes created with
`hash_newtype`, I'm not sure if this should be removed or not.
When signing a Taproot input (in a PSBT) using a key path spend we
currently return the pubkey associated with key that signs. However it
is common to think of the internal key as being the one that signs even
though this is not technically true. We also have the internal key in
the PSBT so matching against it is less surprising.
When using the `Psbt` type to sign a Taproot input using a key path
spend return the internal key.
There is no need to panic if input index is out of bounds because we
have a function to check the validity of the `input_index` argument and
use it in other places already.
41e8fb0863 Support signing taproot in psbt (yu)
Pull request description:
Hi team, I'm from Keystone Wallet team. currently rust-bitcoin does not support signing taproot transactions in psbt.
We think this founction should be included in the psbt module, we submit this PR. Some context and discussion about this PR can be found here: #2418.
For this PR, mostly two new functions are introduced:
- `bip32_sign_schnorr`: sign a taproot input.
- `sighash_taproot`: calculate the sighash message to sign a taproot input along with the sighash type.
Looking forward to your feedback.
ACKs for top commit:
tcharding:
ACK 41e8fb0863
sanket1729:
ACK 41e8fb0863.
Tree-SHA512: 2eb14a3204e6ed848515483778dd7986662aacb332783d187da72d29e207b78a2d427939f2b958135a32de5459221385e6f1f5bae89f491b58d8bc79f202b724
merge-script
Jamil Lambert, PhD
Shing Him Ng
Tobin C. Harding
Fmt Bot
Jose Storopoli
Divyansh Gupta
Liam Aharon