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
83dda74ecb Check for SIGHASH_SINGLE bug in writer fn (Tobin Harding)
Pull request description:
Recently we moved the logic for checking for the SIGHASH_SINGLE bug to
the `signature_hash()` function. Although this left users of the
`encode_signing_data_to()` function without correct handling of the bug
there is not much else we can do but alert users to this behaviour.
Add documentation to highlight the behaviour of `encdoe_signing_data_to`
in regards to the sighash single bug. Requires updating docs for
`signature_hash` also.
Please note, uses non-conventional markdown header `# Warning`.
Closes: #817
ACKs for top commit:
sanket1729:
ACK 83dda74ecb. This is much cleaner
dr-orlovsky:
ACK 83dda74ecb
apoelstra:
ACK 83dda74ecb
Tree-SHA512: 1263b06ddfbb05a293c80e7dbf6f87eac5922c501e7db1c1d26d41d3ea0172c6b7a44afc0b1843b06e78985d3ecf70a3a3feb2515d535a7413685aed0a338c64
6ad2902814 Remove feature gated enum variants (Tobin Harding)
Pull request description:
This is the updated version of #874 (which I closed, force pushed, and then was unable to re-open - my bad).
Feature gating enum variants makes code that uses the library brittle while we do not have `non_exhaustive`, we should avoid doing so. Instead we can add a dummy type that is available when the feature is not turned on. Doing so enables the compiler to enforce that we do not create the error type that is feature gated when the feature is not enabled.
Remove the feature gating around `bitcoinconsensus` error enum variants.
Closes: #645
ACKs for top commit:
sanket1729:
tACK 6ad2902814. This is an improvment.
dr-orlovsky:
ACK 6ad2902814
Tree-SHA512: 07d8c6b500d2d5b92e367b89e296b86bec046bab4fe9f624eb087d52ea24a900d7f7a41a98065949c67b307a1f374a7f4cf1b77cb93b6cf19e3d779c27fd7f1d
35b682d495 Implement Display/FromStr for SchnorrSigHashType (Tobin Harding)
46c4164d67 Improve SigHashTypeParseError field (Tobin Harding)
c009210d4c Use full path for String in macro (Tobin Harding)
Pull request description:
Implement Display/FromStr for SchnorrSigHashType
We currently implement `Display` and `FromStr` on `EcdsaSigHashType` and use them in the `serde_string_impl` macro to implement ser/de.
Mirror this logic in `SchnorrSigHashType`.
Patch 1 and 2 are preparatory patches for patch 3.
## Notes to reviewers
This PR has some conflicts with https://github.com/rust-bitcoin/rust-bitcoin/pull/898 but is pushing in the same direction, I'm happy to let 898 go in first and rebase on top.
ACKs for top commit:
sanket1729:
ACK 35b682d495. Thanks, much easier to review now that the diff is small
dr-orlovsky:
ACK 35b682d495
Tree-SHA512: 481f192a3064ff39acf8904737dfb25b54ef128a37e0ca765ebb39138edac772d4f01ed10aa98ff185a8ed5668d64fa5d5957206b920ffe87950cafcf5a3b516
63e36fe6b4 Remove impl_index_newtype macro (Tobin Harding)
Pull request description:
This macro is no longer needed since we bumped MSRV to 1.29.
~We can implement `SliceIndex` to get the `Index` implementations.~
We can implement `core::ops::Index` directly since all the inner types implement `Index` already.
Original ~Idea shamelessly stolen from @elichai [in this comment](https://github.com/rust-bitcoin/rust-bitcoin/issues/352#issuecomment-560331856).~
New idea proposed by @Kixunil during review below. Thanks.
ACKs for top commit:
apoelstra:
ACK 63e36fe6b4
dr-orlovsky:
utACK 63e36fe6b4
sanket1729:
ACK 63e36fe6b4
Tree-SHA512: f7b4555c7fd9a2d458dcd53ec8caece0d12f3af77a10e850f35201bd7a580ba8fd7cb1d47a7f78ba6582e777dffa13416916ecacac6e0e874bdbb1c866132dc2
We currently implement `Display` and `FromStr` on `EcdsaSigHashType` and
use them in the `serde_string_impl` macro to implement ser/de.
Mirror this logic in `SchnorrSigHashType`.
In preparation for constructing an error outside of this module improve
the `SigHashTypeParseError` by doing:
- Make the field public
- Rename the field to `unrecognized` to better describe its usage
Recently we moved the logic for checking for the SIGHASH_SINGLE bug to
the `signature_hash()` function. Although this left users of the
`encode_signing_data_to()` function without correct handling of the bug
there is not much else we can do but alert users to this behaviour.
Add documentation to highlight the behaviour of `encdoe_signing_data_to`
in regards to the sighash single bug. Requires updating docs for
`signature_hash` also.
Please note, uses non-conventional markdown header `# Warning`.
As is done in the rest of the `internal_macros` module use the fully
qualified path for the `String` type.
Done in preparation for using `serde_string_impl` in the `sighash`
module.
Feature gating enum variants makes code that uses the library brittle
while we do not have `non_exhaustive`, we should avoid doing so. Instead
we can add a dummy type that is available when the feature is not turned
on. Doing so enables the compiler to enforce that we do not create the
error type that is feature gated when the feature is not enabled.
Remove the feature gating around `bitcoinconsensus` error enum variants.
Closes: #645
3bde1a205c Remove get_ prefix (Tobin Harding)
Pull request description:
This one might be a viewed as code churn or unnecessarily modifying the API, feel free to NACK :)
We have a bunch of methods that use the prefix `get_`, they are not exactly getters because they do more than just access a struct fields so Rust convention relating to getters does not apply, however, the `get_` prefix does not add to the descriptiveness of name hence the shorter form can be used with no loss of clarity.
Improve docs and deprecate any methods changed that are pubic.
ACKs for top commit:
dr-orlovsky:
ACK 3bde1a205c
apoelstra:
ACK 3bde1a205c
sanket1729:
ACK 3bde1a205c
Tree-SHA512: d9e618ba7fec81ad157c2c806d1db273f899d63707c78254c133b619293f9f0c9a4f3a3e091e9aad399479ff80d5d052c424501164374c21bb90fb9783a4824e
1629348c24 Use conventional spacing for default type parameters (Tobin Harding)
Pull request description:
The exact code formatting we use is not as important as uniformity. Since we do not use tooling to control the formatting we have to be vigilant ourselves. Recently I (Tobin) changed the way default type parameters were formatted (arbitrarily but uniformly). Turns out I picked the wrong way, there is already a convention as shown in the rust documentation online (e.g. [1]).
Use 'conventional' spacing for default type parameters. Make the changeacross the whole repository, found using
git grep '\<.* = .*\>'
[1] - https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
**Note**
I also audited our whole stack to make sure I had not botched this anywhere else. Apologies for the code churn.
ACKs for top commit:
dr-orlovsky:
utACK 1629348c24
apoelstra:
ACK 1629348c24
Tree-SHA512: 61c8a22acc557d8b99f7b591cf8f91b94778a954ac1c9d6cf04a2bbd10953c241e8298e71928aad3b065c98117b816b913226d973fdaa9c3a1aea8daf8bcbe72
51a51cd67d Improve ClassifyContext rustdocs (Tobin Harding)
Pull request description:
Improve the rustdocs on the `ClassifyContext` enum by doing:
- Use link for `OP_RESERVED`
- Use term `OP_SUCCESSx` is done in BIP342 (no code link, does not exist in code).
- Use enum::variant form for both variant mentions
- Direct readers to BIP342 for full list of opcode re-names
ACKs for top commit:
sanket1729:
ACK 51a51cd67d
apoelstra:
ACK 51a51cd67d
dr-orlovsky:
ACK 51a51cd67d
Tree-SHA512: 1a9067246ef84eae39b0adef64190b9212dacb55a420909ee38c582ef1960fceb572f82d3eeff518b58fc2cceffe71b3da4e78da54cd4cb6e05a0e48a3a9d03c
We have a bunch of methods that use the prefix `get_`, they are not
exactly getters because they do more than just access a struct fields so
Rust convention relating to getters does not apply, however, the `get_`
prefix does not add to the descriptiveness of name hence the shorter
form can be used with no loss of clarity.
Improve docs and deprecate any methods changed that are pubic.
d1abfd9c30 Add unit test for sighash single bug (Tobin Harding)
82f29b4267 Use 1 signature hash for invalid SIGHASH_SINGLE (Tobin Harding)
3831816a73 Move test helper function (Tobin Harding)
3e21295b88 Remove unnecessary whitespace character (Tobin Harding)
Pull request description:
Fix up the logic that handles correctly returning the special array 1,0,0,...,0 for signature hash when the sighash single bug is exploitable i.e., when signing a transaction with SIGHASH_SINGLE for an input index that does not have a corresponding transaction output of the same index.
- Patch 1 and 2: Clean up
- Patch 3: Implements the fix
- Patch 4: Adds a passing test that fails if moved to before patch 3
Resolves: #817
ACKs for top commit:
apoelstra:
ACK d1abfd9c30
dr-orlovsky:
ACK d1abfd9c30
Tree-SHA512: f2d09e929d2f91348ae0b0758b3d4be6c6ce0cb38c4988e0bebb29f5918ca8491b9e7b31fe745f7c20d9348612fe2166f0a12b782f256aad5f6b6c027c2218b7
The exact code formatting we use is not as important as uniformity.
Since we do not use tooling to control the formatting we have to be
vigilant ourselves. Recently I (Tobin) changed the way default type
parameters were formatted (arbitrarily but uniformly). Turns out I
picked the wrong way, there is already a convention as shown in the rust
documentation online (e.g. [1]).
Use 'conventional' spacing for default type parameters. Make the change
across the whole repository, found using
git grep '\<.* = .*\>'
[1] - https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
This macro is no longer needed since we bumped MSRV to 1.29.
We can implement `core::ops::Index` directly since all the inner types
implement `Index` already.
Improve the rustdocs on the `ClassifyContext` enum by doing:
- Use link for `OP_RESERVED`
- Use term `OP_SUCCESSx` is done in BIP342 (no code link, does not exist
in code).
- Use enum::variant form for both variant mentions
- Direct readers to BIP342 for full list of opcode re-names
In this library we specifically do not use rustfmt and tend to favour
terse statements that do not use extra lines unnecessarily. In order to
help new devs understand the style modify code that seems to use an
unnecessary number of lines.
None of these changes should reduce the readability of the code.
Vector initialisation uses neither "Block" nor "Visual" stlye, this is
irregular for no added benefit.
Elect to use "Block" style (as defined by `rustfmt`).
This function uses neither "Block" nor "Visual" style (as defined by
`rustfmt`). This is unusual, code that is regular is less jarring to
read. We tent to use "Block" style for functions so elect to do that
here.
Our usage of `where` statements is not uniform, nor is it inline with
the typical layout suggested by `rustfmt`.
Make an effort to be more uniform with usage of `where` statements.
However, explicitly do _not_ do every usage since sometimes our usage
favours terseness (all on a single line).
We have a few instances of strange indentation:
- Incorrect number of characters
- Usage of neither "Block" style or "View" style (elect to use "Block")
Do various whitespace refactorings, of note:
- Use space around equals e.g., 'since = "blah"'
- Put return/break/continue on separate line
Whitespace only, no logic changes.
Andrew Poelstra
Dr Maxim Orlovsky
sanket1729
Tobin Harding
Jeremy Rubin