This adds tests for the previously untested reject message
(de)serialization. The two reject messages were received from an
older Bitcoin Core peer that still sends reject messages.
46c34b3fb7 Fix code comments referring to sighash (Tobin Harding)
8f36c3979c Use sighash not sig_hash in identifiers (Tobin Harding)
c3a167b96b Rename SigHash -> Sighash (Tobin Harding)
52b711c084 Rename InvalidSigHashType -> InvalidSighashType (Tobin Harding)
b84f25584e Rename SigHashCache -> SighashCache (Tobin Harding)
e37652578b Rename PsbtSigHashType -> PsbtSighashType (Tobin Harding)
c19ec339ef Rename NonStandardSigHashType -> NonStandardSighashType (Tobin Harding)
130e27349e Rename SigHashTypeParseError -> SighashTypeParseError (Tobin Harding)
6caba2ed24 Rename SchnorrSigHashType -> SchnorrSighashType (Tobin Harding)
5522454583 Rename EcdsaSigHashType -> EcdsaSighashType (Tobin Harding)
Pull request description:
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash' is a well known word in the Bitcoin ecosystem it should appear in identifiers as `Sighash`.
Change various types, variants, and code comments to use sighash as a single word.
- Patches 1-8 are code changes `s/SigHash/Sighash/g`
- Patch 9 is code changes `s/sig_hash/sighash/g`
- Patch 11 is docs fixes
Fixes: #911
## Note to reviewers
I've been particularly pedantic with the patch separation because we are so close to release.
Done as separate patches to make review easier if review is to be done by reading the diffs. Perhaps at least one person could verify this PR programmatically by doing
- Reset the last 2 patches (those are easy to do manually)
- Check out master
- Do `s/SigHash/Sighash/g` on all source files (bash function below)
- Use `git diff branchA..branchB` to verify
The difference between the two branches should only include comment lines (last three patches) and these seven instances of `SigHash:
```
CHANGELOG.md:82:- [Add FromStr/Display implementation for SigHashType](a4a7035a94)
CHANGELOG.md:93:- [Introduce `SigHashCache` structure](https://github.com/rust-bitcoin/rust-bitcoin/pull/390) to replace `SighashComponents` and support all sighash modes
CHANGELOG.md:121: - `SigHash`
src/blockdata/transaction.rs:1190: "SigHash_None",
src/blockdata/transaction.rs:1191: "SigHash_NONE",
src/util/sighash.rs:1175: "SigHash_None",
src/util/sighash.rs:1176: "SigHash_NONE",
```
In case its useful, the shell function I used to do these changes is:
```bash
function search-and-replace() {
if (($# != 2))
then
echo "Usage: $0 <this> <that>"
return
fi
local this="$1"
local that="$2"
# For all files containing $this, replace $this with $that.
for file in $(git grep -l "$this")
do
perl -pi -e "s/$this/$that/g" "$file"
done
}
```
ACKs for top commit:
dr-orlovsky:
ACK 46c34b3fb7
apoelstra:
ACK 46c34b3fb7
Tree-SHA512: fe7e25e9cfb5155e4921de5ac185dbf9f4ca0770846d7892f6968b44fc5431f3f1a183380107449e90f7ea662094c60b118dc0468230384e8f9a8ef98d5ee0a0
f27c4a541d Added push_x_only_key(..) and its test. (mpls)
Pull request description:
**Issue**
I can not use [`XOnlyPublicKey`](ae985dd191/src/key.rs (L973)) in my Scripts which prevents me from working with Taproot.
**Cause**
The current version of [`script::Builder`](0a2d45de09/src/blockdata/script.rs (L121)) does not accept `XOnlyPublicKey`s.
**Solution**
So, I created a function `push_xkey(self, key: &XOnlyPublicKey)` based on the existing [`push_key`](0a2d45de09/src/blockdata/script.rs (L914)) function. I also augmented an [existing test](0a2d45de09/src/blockdata/script.rs (L1108)) in an attempt to reach testing parity with existing code.
After toying around with `push_xkey`, it seems to work on my end.
ACKs for top commit:
dr-orlovsky:
ACK f27c4a541d
sanket1729:
utACK f27c4a541d. Thanks a lot for keeping up the iterations with prompt responses
Tree-SHA512: 064958d49edc1d3636a21e428d62c2e9bcd9b13bd226c5821db9e04ce78663a11fcf601c7667b564f88e845207219a052e1c7413f50e5d27c79003e8129825ed
da731c4825 Add further description to the NodeInfo struct (Tobin Harding)
492ccebd99 Use links for error types (Tobin Harding)
3e05887579 Use 'the' to improve sentence (Tobin Harding)
Pull request description:
See to nits from review of https://github.com/rust-bitcoin/rust-bitcoin/pull/912
Three minor patches to the `taproot` module docs.
CC @dr-orlovsky
ACKs for top commit:
dr-orlovsky:
ACK da731c4825
sanket1729:
ACK da731c4825
Tree-SHA512: 17a27a19c88f9baa8127023b2ee30fc2259cb0058a92dc9d8ae595e9e02ccb047fefcba7548ff7900fffa7bc6853447183e80660b8756d90d055ab8aa96ae938
Recently we added a bunch of additional sighash types, some of the code
comments became stale. Use the non-specific term 'sighash type' instead
of a particular sighash identifier in comments to make the comments more
applicable.
Recently we update all types and docs to use `Sighash` instead of
`SigHash` because 'sighash' is a single word. We should apply the same
logic to functions and variable names.
Do not use an underscore in the identifier 'sighash'.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename the `SigHash` type to `Sighash`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename the `InvalidSigHashType` variant to `InvalidSighashType`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `SigHashCache` to `SighashCache`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `PsbtSigHashType` to `PsbtSighashType`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename the `NonStandardSigHashType` type and error variant to
`NonStandardSighashType`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `SigHashTypeParseError` to `SighashTypeParseError`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `SchnorrSigHashType` to `SchnorrSighashType`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `EcdsaSigHashType` to `EcdsaSighashType`.
c25eddd187 Remove unnecessary documentation (Tobin Harding)
8631474f08 Improve docs in taproot module (Tobin Harding)
Pull request description:
I should have done this PR a month ago, my bad. This one is kind of important IMO because we are going to have so many people looking at this part of the code soon as we release.
As has been done in other places in the codebase; improve the docs in the `taproot` module by doing:
- Use full sentences (capital letters + full stops)
- Use back ticks and links for types where appropriate
- Fix grammar
- Fix stale docs
- Use third person for describing functions
- Use 100 character line width
- Use markdown sections (`# Examples`, `# Returns`) where appropriate
- Separate brief heading from extended description when appropriate
- Use `///` for all functions/types (both private and public)
I also did:
- Build the docs and check all the links
- Read all the built docs, check for sanity and pretty-ness
Its all in one patch, I couldn't really tease it apart. I can try a bit harder if it proves too annoying to review.
ACKs for top commit:
sanket1729:
ACK c25eddd187
dr-orlovsky:
ACK c25eddd187
apoelstra:
ACK c25eddd187
Tree-SHA512: 72f35bf8779392060388db985df5abc42a89796eaad1eafd08ea50b635d469fbd07a53ff253cdf27ad4d4baed7d37cec6ea1da1aece3672b9447f87181e218f8
We deprecated the `bip143::SigHashCache` in
```
commit 53d0e176d3
Author: <elided>
Date: Fri Jul 16 10:44:18 2021 +0200
Deprecate bip143::SigHashCache in favor of sighash::SigHashCache
...
```
This means these changes are unreleased so the deprecated since version
should be the upcoming 0.28 release.
As has been done in other places in the codebase; improve the docs in
the `taproot` module by doing:
- Use full sentences (capital letters + full stops)
- Use back ticks and links for types where appropriate
- Fix grammar
- Fix stale docs
- Use third person for describing functions
- Use 100 character line width
- Use markdown sections (`# Examples`, `# Returns`) where appropriate
- Separate brief heading from extended description when appropriate
- Use `///` for all functions/types (both private and public)
I also did:
- Build the docs and check all the links
- Read all the built docs, check for sanity and pretty-ness
992857ad0a PsbtSighashType unit tests (Dr Maxim Orlovsky)
5be1cdb8c7 PsbtSigHashType Display and FromStr implementation (Dr Maxim Orlovsky)
7cdcdaad6c Support SIGHASH_RESERVED in SchnorrSigHashType::from_u8 (Dr Maxim Orlovsky)
Pull request description:
The newly introduced `PsbtSigHashType` uses very different serde formatting from previously used `EcdsaSigHashType`; for instance it does not output human-readable sighash. This is especially obvious when printing out PSBT as JSON/YAML object and is a breaking change from the `0.27`. Serde human-readable implementation requires `Display/FromStr`, which were also absent.
ACKs for top commit:
sanket1729:
ACK 992857ad0a. This is much better
apoelstra:
ACK 992857ad0a
Tree-SHA512: 71a46471f34b5481e4c1273a66846f59d61bfd98fcb65e7823ca216ff0dd419d81ca86d99c7aaf674fcfe2b1c010e899c8e74328f60a1e809015c663c453cc89
51fef76129 feat: Add Address.is_related_to_pubkey() (Andrew Ahlers)
Pull request description:
## Motivation
This is addressing the second half of this comment: https://github.com/rust-bitcoin/rust-bitcoin/pull/684#issuecomment-1012136845
> but would accept a PR (or two PRs) that returns Result<bool, UnsupportedAddress> and a method to check if a PublicKey is associated with an address.
(The first half was addressed [here](https://github.com/rust-bitcoin/rust-bitcoin/pull/819))
These changes will help build out and improve message signature verification. We don't necessarily need to add it to this crate but it allows for easy verification with something such as:
1. recovering a pubkey
2. checking if that pubkey relates to the given address
## Possible Improvements
- There is likely a better name than `is_related_to_secp256k1_key()`
- This could drop the `secp256k1` part of the name and take in a Pubkey enum that also supports Schnorr pubkeys and then this could be used for taproot addresses as well. This felt like a much larger change that will likely get turned down. Verifying taproot is simple enough and if absolutely desired, similar functions can be added for schnorr keys (tweaked and untweaked)
ACKs for top commit:
Kixunil:
ACK 51fef76129 for merging after TR
apoelstra:
ACK 51fef76129
Tree-SHA512: c9ab8c0f101fb4c647713e7f500656617025d8741676e8eb8a3132009dde9937d50cf9ac3d8055feb14452324a292397e46639cbaca71cac77af4b06dc42d09d
208eb65f1b Make NodeInfo API public (sanket1729)
Pull request description:
Reported by @shesek. Users might find it convenient to manually construct the tree using `NodeInfo` API
```rust
let leaf1 = NodeInfo::from_leaf_with_ver();
let leaf2 = NodeInfo::from_leaf_with_ver();
let root = NodeInfo::combine(leaf1, leaf2);
let spend_info = TaprootSpendInfo::from_node_info(&secp, internal_key, root);
```
ACKs for top commit:
dr-orlovsky:
ACK 208eb65f1b
apoelstra:
ACK 208eb65f1b
Tree-SHA512: b5a6b26e0d4a637f7ad6e987976b31b00d3567feca85f1a0bf63aa03603aded0ddae6578b1cabc1056870a596b8cb1a83e4ef3f45802e03da80c3d58d9bab1f1
e27f8ff594 TapTree iterator implementation (Dr Maxim Orlovsky)
Pull request description:
Implemented after @sanket1729 suggestion in https://github.com/rust-bitcoin/rust-bitcoin/issues/895#issuecomment-1074366108
Iterates all scripts present in TapTree in DFS order returning `(depth, script)` pairs.
I propose to have it as an RC fix since this functionality is really lacking and may be required for many wallets working with Taproot PSBT even outside of the scope where I originally needed it (OP_RETURN tweaks for TapTree described in #895)
ACKs for top commit:
sanket1729:
utACK e27f8ff594.
apoelstra:
ACK e27f8ff594
Tree-SHA512: b398e468a10534561297f22dba47e340391069734a41999edd85d726890752035053690a22014402879ea40b948160f00310f78771443d382c0bbaf0201dfbe5
It is possible, although not immediately obvious, that it is possible to
create a `PsbtSigHashType` with a non-standard value.
Add a unit test to show this and also catch any regressions if we
accidental change this logic.
Improve the `PsbtSigHashType` conversion methods by doing:
- Re-name `inner` -> `to_u32` as per Rust convention
- Add `from_u32` method
Note, we explicitly do _not_ use suffix 'consensus' because these
conversion methods make no guarantees about the validity of the
underlying `u32`.
The functions `from_u32_standard` and `from_u32_consensus` smell a bit
like hungarian notation. We can look at the method definition to see
that the methods accept `u32` arguments without mentioning that in the
method names.
Remove `_u32_` from the method names. This brings the `from_*` methods
in line with the `to_standard` method also.
Rust naming conventions stipulate that conversion methods from owned ->
owned for `Copy` types use the naming convention `to_`.
This change makes the function name objectively better, however it makes
no claims of being the 'best' name. We have had much discussion on using
`to_standard` vs `to_u32` but are unable to reach consensus.
We have conversion functions that include suffixes `_consensus`
and `_standard` to make it explicit what guarantees are provided by the
returned `u32` value. The `From` implementation reduces the clarity of
the API.
2b942cf506 Add Serialize/Deserialize for TaprootSpendInfo (Jeremy Rubin)
Pull request description:
I think this is missing -- unless there is a reason not to have it?
ACKs for top commit:
apoelstra:
ACK 2b942cf506
dr-orlovsky:
ACK 2b942cf506
Tree-SHA512: d1467d8515c85a5057037b1e5bf53c1930275fbe7e4fcbc726079a47febd75d6bbce8e2d99ed4f9d8afccf6fc3782e43763a2258c4c2a934c2453920fe587e4b
0xb10c
sanket1729
Andrew Poelstra
Dr. Maxim Orlovsky
mpls
Tobin Harding