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".
f2a6827982 Fix BinaryHeap direction for Taproot Huffman Encoder (Jeremy Rubin)
cccd75d004 Fix Weighting Addition to never error on overflow + prevent overflows from ever happening with wider integers (Jeremy Rubin)
Pull request description:
I noticed one cleanup & one bugfix while looking into the huffman algorithm:
1) the cleanup: we can use a u128 to guarantee no overflows, and saturating_add to guarantee reasonable behavior in any case
2) the bug: the binary heap is a max heap so the behavior ends up merging the nodes of the most likely entries repeatedly. a huffman encoder requires merging the least likely elements, so it should be reversed.
ACKs for top commit:
sanket1729:
ACK f2a6827982
dr-orlovsky:
utACK f2a6827982
Tree-SHA512: 07cadb8dd5cc2b7e6ae3ebc2c1639de054e41bcd7f3b7d338a93e77fd200c9591a89915aaae5d9f5313eff3d94032fdfe06d89fda1e2398881b711d149e9afe9
822c99222d Improve constructor rustdocs for Address (Tobin Harding)
804a38cb67 Improve documentation of `WitnessVersion` (Tobin Harding)
eb8278fd2e util/address: Improve docs (Tobin Harding)
Pull request description:
Improve documentation of the `address` module by doing:
- Add full stops to all sentences
- Use code ticks even inside links e.g., [`WitnessVersion`]
- Use 100 character line length
- Do grammar fixes
- Use comment sections (e.g. `# Returns`)
- Use 3rd person for function comments e.g. 'Converts foo to bar' instead of 'Convert foo to bar'
- Use ticks for scriptPubkey
This patch does a single file because a bunch of these changes pick an
arbitrary stlye, if we can bikeshed on this PR then future PRs should be
able to progress more quickly. I'll take lack of comment on any of the
above as approval and I'll attempt to be uniform when doing the rest of
the codebase. I plan on just chipping away at this, I can only do so
much docs work in a day without getting bored of it :)
Notes:
- I didn't touch 'segwit' vs 'SegWit', seems both are widely used.
- Using ticks inside links may be an overkill but seems more correct?
- I'm not totally sure where the line is in the Rust ecosystem between
readability in an editor and rendering as HTML, open to input on this.
ACKs for top commit:
Kixunil:
ACK 822c99222d
dr-orlovsky:
ACK 822c99222d
Tree-SHA512: bfbaeec74803dd0704ed3e39b9a4966db34dbb3d7ea850ed6230abf220b877687ac1479f4940b7bf39d7e8172cd62c36b232bfaa8186a92cc58b3d7e642674f6
e4774e74eb fixups to taptweaking code (sanket1729)
Pull request description:
This was my bad for not clearly stating the expected spec #687 . Changed values to references so that we only take ownership where it is required.
This should simplify the #697
ACKs for top commit:
Kixunil:
ACK e4774e74eb
dr-orlovsky:
utACK e4774e74eb
Tree-SHA512: adacbfa8a77f46b2c85720f3760ed12a437f40d8422731d0207662d7947c95dda79d576923f6056c77f57977a3dcd25afd270f0ee11e9c3be9d067ccdc63371a
e04795093f Add unit test for bitcoin_merkle_root functions (Tobin Harding)
Pull request description:
We test `bitcoin_merkle_root` over in the `blockdata::block` module.
Although the `bitcoin_merkle_root` and `bitcoin_merkle_root_inline`
functions are almost identical there is enough index manipulation done
that it is not immediately obvious that the code is error free.
Add a unit test that verifies that the two functions return the same
resulting merkle root.
This was done in response to #394.
ACKs for top commit:
RCasatta:
ACK e04795093f
Kixunil:
ACK e04795093f
Tree-SHA512: 68a1cdb3612a9beb6359e3ced1b00aeee48354d9882f13ed56afe5e01062f81deeaad7b1ffb7c1110f5352170f5587704a98fb5304a9bad309ee283223bad22b
We test `bitcoin_merkle_root` over in the `blockdata::block` module.
Although the `bitcoin_merkle_root` and `bitcoin_merkle_root_inline`
functions are almost identical there is enough index manipulation done
that it is not immediately obvious that the code is error free.
Add a unit test that verifies that the two functions return the same
resulting merkle root.
Martin Habovstiak
Riccardo Casatta
sanket1729
z8674558
Dr. Maxim Orlovsky
Marko Bencun
Tobin Harding
Jeremy Rubin
Noah