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
c3d30d51a7 Remove deprecated method use for sighash conversion (Dr. Maxim Orlovsky)
Pull request description:
Post-merge #796 follow-up. Feel free to add other changes/nits which hadn't get into #796.
ACKs for top commit:
tcharding:
ACK c3d30d51a7
sanket1729:
ACK c3d30d51a7
apoelstra:
ACK c3d30d51a7
Tree-SHA512: 7418f90bad9ce43b5504a3e45f06fecd636f62f2f7bd75bfc27e29faa181202595ed9b5175866e0fce01a301ea34c2b07afb16e658757215823965e7e1440c2e
8e2422f92b Add unit test for deserialize non-standard sighash (Tobin Harding)
e05776f176 Improve PsbtSigHashType conversion methods (Tobin Harding)
ac462897b1 Remove hungarian-ish notation (Tobin Harding)
564682627c Remove deprecated conversion method (Tobin Harding)
d1753d7ff1 Rename as_u32 -> to_u32 (Tobin Harding)
2bd71c3748 Remove From<EcdsaSigHashType> for u32 (Tobin Harding)
Pull request description:
This PR has evolved into a full blown clean up of the conversion methods for all three sighash types based on the discussion below.
Everything is split up into very small patches to aid review (and bikeshedding, this PR is almost totally just naming things).
EDIT: I'm not convinced this should be an RC-fix, the changes are too wide spread now. It started as just a single method rename. Leaving label as is for others to consider.
ACKs for top commit:
sanket1729:
utACK 8e2422f92b.
dr-orlovsky:
ACK 8e2422f92b
Tree-SHA512: 8269bdf6d00a98b472a720b891f25728d5dedf3b2648aa60a461efdbdf37d883a1824a15f1838a792329ec9ac50a9c05618ab2a34cf201fcf63e4ad145955571
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.
3d3993a3ba Add Rust-Bitcoin logo and project header. (Hunter Trujillo)
Pull request description:
Adds a logo and a project header with some shields based on BDK's. Preview in dark mode:
ACKs for top commit:
dr-orlovsky:
ACK 3d3993a3ba
sanket1729:
ACK 3d3993a3ba
Tree-SHA512: e1e44109584e915f6f4981ec91f04b046721e0076e6d27d7e995eb2a52f190a4de83fe9d4e8210e0f0d1c7c7ae603a1e6cc7eb91a096962df9166b922b5ce19b
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
a77907d59c Remove unnecessary explicit type annotation (Tobin Harding)
71cf00a314 Use less vertical lines (Tobin Harding)
a5c06e0a96 Refactor vector initialisation (Tobin Harding)
aabf2d1681 Use brace not parenth fo macro arm (Tobin Harding)
b021415a88 Use block stlye function call (Tobin Harding)
d6462bae7b Refactor usage of + (Tobin Harding)
702e8bf82d Refactor consensus_encode (Tobin Harding)
a8ed95ea07 Refactor where statements (Tobin Harding)
6d84998168 Improve braces usage (Tobin Harding)
39ec59620d Fix unusual indentation (Tobin Harding)
b9b6e7e1c6 Remove unneeded braces (Tobin Harding)
5d68ad85ed Remove unneeded return statement (Tobin Harding)
bf4f5638e0 Refactor whitespace (Tobin Harding)
1c502399f1 Remove trailing whitespace (Tobin Harding)
Pull request description:
`rustfmt` is still under discussion, while researching the topic I came across a maintainer of another project that does not use `rustfmt` who mentioned that he manually implemented the `rusfmt` suggestions that he liked ever month or so. This seemed like a good idea so I did it. This was extremely painful but I believe I have put together a PR that is non-controversial with well separated patches.
Totally non urgent.
ACKs for top commit:
apoelstra:
ACK a77907d59c
sanket1729:
ACK a77907d59c.
Tree-SHA512: 27aa10d1c6d02d0e5bc335a5cda9cf2664b968c298d2ea6c653b8074abf18764a9d0f19c36222852fc23b887ab64144901dae059088e61478e9a90a042221e61
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
This significatnly refactors the amount formatting code to make
formatting more configurable. The main addition is the
`amount::Display` type which is a builder that can configure
denomination or other things (possibly more in the future).
Further, this makes all representations of numbers minimal by default,
so should be documented as a possibly-breaking change.
Because of the effort to support all other `fmt::Formatter` options this
required practically complete rewrite of `fmt_satoshi_in`. As a
byproduct I took the opportunity of removing one allocation from there.
Closes#709
sanket1729
Andrew Poelstra
Dr Maxim Orlovsky
Tobin Harding
Jeremy Rubin
Steven Roose
Hunter Trujillo
Martin Habovstiak
