779d4110c6 Fixed a bunch of clippy lints, added clippy.toml (Martin Habovstiak)
Pull request description:
This is the initial step towards using and maybe enforcing clippy.
It does not fix all lints as some are not applicable. They may be
explicitly ignored later.
Some discussion about clippy was in #685
ACKs for top commit:
apoelstra:
ACK 779d4110c6
RCasatta:
ACK 779d4110c6
Tree-SHA512: fb9192c77565a0b1b2118877c6413945d65900e4e95b3741107bf6cddef1fa65ff09fc5b7814de421382292321cca6bd860bf17b73a227d193a0a13758ee25eb
7aacc3782a Add tests from BIP341 (sanket1729)
61629cc733 Make taproot hashes forward display (sanket1729)
Pull request description:
Add tests for taproot.
- ~Also fixes one bug in #677, namely, I was returning `LeafVersion::default()` instead of given version~
- ~ Fixes a bug in #691 about taking secp context as a reference instead of consuming it. This should have not passed my review, but this is easy to miss. ~
- Makes the display on taproot hashes forward instead of the reverse (because the BIP prints in a forward way, I think we should too and it is more natural. )
ACKs for top commit:
RCasatta:
ACK 7aacc3782a
apoelstra:
ACK 7aacc3782a
Tree-SHA512: 2e0442131fc036ffa10f88c91c8fc02d9b67ff6c16c592aa6f4e6a220c26a00fc6ca95a288f14aa40667a289fb0446219fd6c76c0196ead766252356592b9941
There is no reason to force users to use one particular form when
providing a denomination string. We can be liberal in what we accept
with no loss of clarity.
Allow `Denomination` strings to use a variety of forms, in particular
lower case and uppercase.
Note, we explicitly disallow various forms of `Msat` because it is
ambiguous whether this means milli or mega sats.
Co-developed-by: Martin Habovštiak <martin.habovstiak@gmail.com>
This is the initial step towards using and maybe enforcing clippy.
It does not fix all lints as some are not applicable. They may be
explicitly ignored later.
b454cf8e15 Return None from merkle_root functions (Tobin Harding)
7a8b017ea3 Use correct spelling of merkle (Tobin Harding)
Pull request description:
~Do two minor refactorings to the `bitcoin_merkle_root[_inline] functions.~
This PR has grown, is no longer a refactoring because the two functions have been changed to return an `Option`.
First patch is cleanup. Here is the commit message for the second patch
```
The merkle_root of an empty tree is undefined, this is the only error
case we have for the two `bitcoin_merkle_root*` functions. We can fully
describe this error case by returning an `Option` if args are found to
be empty.
While we are at it, refactor out a recursive helper function to make
reading the code between the two functions easier.
```
ACKs for top commit:
Kixunil:
ACK b454cf8e15
dr-orlovsky:
ACK b454cf8e15
Tree-SHA512: 961714a8b0eb0dad493a1548317d875d64ca22d2d584c905c502369b5f6e5a9f8be1edd7345136b44964dc0bde7a4c43bfaff4287d1dbf7fd736da79818074e3
8361129518 Add SchnorrSig type (sanket1729)
94cfe79170 Rename existing SigHashType to EcdsaSigHashType (sanket1729)
648b3975a5 Add SchnorrSigHashType::from_u8 (sanket1729)
410e8bf46c Rename sighash::SigHashType::SigHashType to SchnorrSigHashType (sanket1729)
fa112a793a Add EcdsaSig (sanket1729)
Pull request description:
Fixes#670 . Separates `SchnorrSigHashType` and `LegacySigHashType`. Also adds the following new structs:
```rust
pub struct SchnorrSig {
/// The underlying schnorr signature
pub sig: secp256k1::schnorrsig::Signature,
/// The corresponding hash type
pub hash_ty: SchnorrSigHashType,
}
pub struct EcdsaSig {
/// The underlying DER serialized Signature
pub sig: secp256k1::Signature,
/// The corresponding hash type
pub hash_ty: LegacySigHashType,
}
```
This code is currently minimal to aid reviews. We can at a later point implement (Encodeable, psbt::Serialize, FromHex, ToHex) etc in follow-up PRs.
ACKs for top commit:
Kixunil:
ACK 8361129518
RCasatta:
ACK 8361129518
Tree-SHA512: 800ddcb3677a4f19e9d1c2a7eb7e95b0a677e9135e1e99f9e42956fc6a3fc94f639403076b4925b3adba6fdd95f56a99c2e47d0310675ad51ce5e7453c7355b6
506e03fa4d util/address: use hash functions of PublicKey/Script (Marko Bencun)
f826316c25 util/address: avoid .expect/panic (Marko Bencun)
ad83f6ae00 util/address: make address encoding more modular (Marko Bencun)
Pull request description:
This allow library clients to plug their own encoding parameters in a
backwards compatible manner.
Top commit has no ACKs.
Tree-SHA512: ae2ececbdfe4984fd62c975f4956686d79f6f5a6e65c34b55daa76fe785b8483ed7f35208d36b8bee545c7edd39ac878277a0fb8ea8c64a1943081e15c818bff
d0a87bea72 Add slice 'serialize' method for TweakedPublicKey (Dr. Maxim Orlovsky)
37352d1df5 Add Display and LowerHex to TweakedPublicKey (Dr. Maxim Orlovsky)
Pull request description:
Extraction of a portion from #696 which can be done without changes in `rust-secp256k1`
ACKs for top commit:
Kixunil:
ACK d0a87bea72
sanket1729:
ACK d0a87bea72
Tree-SHA512: d439ea1a4c4235bea9867e5d87514f928ad481f7a32403922654c33e101cfaba444eec8b61899f2aaaf1dcf5236bb618b9e14674736d3798effd56ca7097dc78
b5bf6d7319 Improve rustdocs on schnorr module (Tobin Harding)
a6d3514f2b Return parity when doing tap_tweak (Tobin Harding)
7af0999745 Re-name TweakedPublicKey constructor (Tobin Harding)
3c3cf0396b Remove use of unreachable in error branch (Tobin Harding)
d8e42d153e Remove 'what' comments (Tobin Harding)
b60db79a3b Use un/tweaked public key types (Tobin Harding)
402bd993b2 Add standard derives to TweakedPublickKey (Tobin Harding)
9c015d9ce3 Add newline to end of file (Tobin Harding)
Pull request description:
We have two types for tweaked/untweaked schnorr public keys to help users of the taproot API not mix these two keys up. Currently the `taproot` module uses 'raw' `schnoor::PublicKey`s.
Use the `schnoor` module's tweak/untweaked public key types for the `taproot` API.
Fixes: #725
Please note, I saw this was labeled 'good-first-issue' but I ignored that and greedily implemented a solution because of two reasons
1. We want to get taproot stuff done post haste.
2. I'm struggling to follow what is going on with all the taproot work so this seemed like a way to get my hands dirty.
ACKs for top commit:
dr-orlovsky:
utACK b5bf6d7319
sanket1729:
ACK b5bf6d7319
Tree-SHA512: e3e0480e0d193877c33ac11d0e3a288b0393d9475b26056914e439cb3f19583c1936e70d048df8d2120a36a63b6b592d12e21ca3ab7e058dce6f8f873c3b598b
1518517374 Decrease Huffman weight type to 32 bits (Jeremy Rubin)
Pull request description:
This builds on https://github.com/rust-bitcoin/rust-bitcoin/pull/699 but is the more bikesheddable part since it changes the API.
> u32 of weight should be enough for any branch.
-- Bill Gates
ACKs for top commit:
dr-orlovsky:
utACK 1518517374
Kixunil:
ACK 1518517374
Tree-SHA512: 9c507ae6129dda8dc069b0a142181a78cf89cb3ebf9d2169c46662822cb4ea9ed075bf484528f5399fe0ed383a425174a702e2d685f31c246f5a86c46ed17c3a
Currently we calculate the parity during `tap_tweak` but do not return
it, this means others must re-do work done inside `tap_tweak` in order
to calculate the parity. We can just return the parity along with the
tweaked key.
Keeping inline with the method on `UntweakedPublicKey` that outputs a
`TweakedPublicKey` we can use the same name, for the same reasons.
Use `dangerous_assume_tweaked` as the constructor name to highlight the
fact that this constructor should probably not be being used.
We currently run `tweak_add_check` and use the result as a conditional
branch, the error path of which uses `unreachable`. This usage of
`unreachable` is non-typical. An 'unreachable' statement is by
definition supposed to be unreachable, it is not clear why we would need
to have a conditional branch to check an unreachable statement.
Use `debug_assert!` so programmer errors get caught in un-optimised
builds but in optimised builds the call to `tweak_add_check` is not even
done.
We have two types for tweaked/untweaked schnorr public keys to help
users of the taproot API not mix these two keys up. Currently the
`taproot` module uses 'raw' `schnoor::PublicKey`s.
Use the `schnoor` module's tweak/untweaked public key types for the
`taproot` API.
The merkle_root of an empty tree is undefined, this is the only error
case we have for the two `bitcoin_merkle_root*` functions. We can fully
describe this error case by returning an `Option` if args are found to
be empty. We can do the same for the wrapper functions in `block`
module.
While we are at it, refactor out a recursive helper function to make
reading the code between the two functions easier.
04a8f89f05 Implement `FusedIterator` for `Instructions` (Martin Habovstiak)
Pull request description:
`Instructions` guarantee to return `None` from empty iterator so we
should signal this in type system so that the code can be optimized
better. This also adds a test to make sure this property holds.
ACKs for top commit:
sanket1729:
utACK 04a8f89f05. Any special reasons for doing this?
RCasatta:
ACK 04a8f89f05
Tree-SHA512: 3c6284e97e3bdd28ac5e948e3e9946eb8aa285cba753a6a0bdcbf971ebceab6d93c206d284128c232531b3de5996ece91187e4369d88bdfe6c531b4b7f787dd8
4eedd46d58 Hotfix for secp256k1 alloc feature (Dr Maxim Orlovsky)
Pull request description:
Latest merges has broken builds since they have introduced `alloc` feature in `secp256k1` which appeared only in `0.20.3` version, while `Cargo.toml` here still required `0.20.2`
This was not caught by CI since it always runs with `cargo update`
ACKs for top commit:
tcharding:
ACK 4eedd46d58
sanket1729:
ACK 4eedd46d58
Kixunil:
ACK 4eedd46d58
Tree-SHA512: 52be0356390a0bde1fe29ae130be56fef6692360e58772f62cb8536c5c38b2a63eb755f664f84c6ff5987c0ae614afc63bf496b1765a0fdbfb586798e0c2327f
5b21a9cb1f Use TapTweakHash method for computing tweak (Noah)
Pull request description:
Quick follow up PR to #691 using a method from #677.
### Changes
- Updated `UntweakedPublicKey::tap_tweak(...)` to use `TapTweakHash::from_key_and_tweak(...)`
ACKs for top commit:
Kixunil:
ACK 5b21a9cb1f
dr-orlovsky:
utACK 5b21a9cb1f
Tree-SHA512: d00455bba51981e9ec942a6cf69672666e227850d073b1fdcd92d2eb6ad553659fb2967aec2ce12d3ed109cee5fa125cdda649cddb25404f08adae2bfd3e19bb
`Instructions` guarantee to return `None` from empty iterator so we
should signal this in type system so that the code can be optimized
better. This also adds a test to make sure this property holds.
e7b84e20d3 Use expect for concensus_encode on Vec (Tobin Harding)
4031fbf4ba Use expect for concensus_encode on sinks (Tobin Harding)
fa513bb5b5 Use expect for concensus_encode on engines (Tobin Harding)
a2efafcf9a Use error instead of err (Tobin Harding)
Pull request description:
Calls to `unwrap` outside of tests are generally unfavourable. We currently call `unwrap` in a bunch of places on calls to `consensus_encode` when passing writers that do not fail.
Remove `unwrap` calls on all calls to `consensus_encode` that pass a writer argument for which write functions do not fail. Use `expect` with a descriptive string instead.
Fixes: #714
ACKs for top commit:
Kixunil:
ACK e7b84e20d3
RCasatta:
ACK e7b84e20d3
Tree-SHA512: 3f84598a14ecf3dcde4f418ad1a1dc5278b3ef8b2604f4e9fc4cf4e9aed8390a4a1cf0df47edb5956cc5b667d6c8864e34621c0dae974ea75d6daf1b133165dd
Calls to `unwrap` outside of tests are typically unfavourable.
In memory writers (`Vec`) do not error. We can use `expect` with a
descriptive message string to indicate this.
Calls to `unwrap` outside of tests are typically unfavourable.
Sink writers do not error. We can use `expect` with a descriptive
message string to indicate this.
Calls to `unwrap` outside of tests are typically unfavourable.
Hash engines do not error when calling `consensus_encode`. Instead of
the current usage of `unwrap` we can use `expect` with a descriptive
string as is done in other parts of the codebase.
In the name of uniformity use the same error message as argument to
`expect` througout the codebase.
Use "engines don't error" instead of "engines don't err".
Riccardo Casatta
Tobin Harding
Martin Habovstiak
sanket1729
z8674558
Dr. Maxim Orlovsky
Marko Bencun
Jeremy Rubin