a6e8f581db PSBT BIP32 keys moved to Secp256k1 from bitcoin ECDSA (Dr Maxim Orlovsky)
Pull request description:
Fourth step in implementation of Schnorr key support after #588. This PR is a follow-up to non-API breaking #589 and API-breaking #590, which must be reviewed and merged first. ~~(The current PR includes all commits from #589 and #590, which should be reviewed there. The only commit specific to this PR is b8105e95dc8651626b783403ca060f7d32d21144)~~
UPDATE: All related PRs are merged now and this PR is ready for the review
PR description:
While PSBT BIP174 does not specify whether uncompressed keys are supported in BIP32-related fields, from BIP32 it follows that it is impossible to use uncompressed keys within the extended keys. This PR fixes this situation and is a companion to BIP174 PR clarifying key serialization: https://github.com/bitcoin/bips/pull/1100
ACKs for top commit:
apoelstra:
ACK a6e8f581db
sanket1729:
ACK a6e8f581db. Not sure which order to merge since there are many ready PRs which that would break each other.
Tree-SHA512: 198ba646bbce1949b255a54a97957d952acdad8b7f9580be123116c0f44d773e6d90e0cac0d5993ec9a6b3328aa43aced0908522817861585877c50008fec835
bb70820fed improve example: take hex-encoded seed instead of WIF in bip32 example (KaFai Choi)
Pull request description:
This is my understanding of what we want to fix the confusing bip32 example. Apologize in advance if I misunderstand it.
Closes#748
ACKs for top commit:
dr-orlovsky:
ACK bb70820fed
RCasatta:
utACK bb70820fed
Tree-SHA512: aaec9f7e3e8ce0e58b2a405e6ada75b1fc9de46ee6efb7fa2543fa626aa5f05704b05585158ab6147c495fc19abc6ade3c25225b3d75b3a3edeb8e00ba8d3976
9835736ef5 wrap u8 and LeafVersion in backticks and square bracket in doc (KaFai Choi)
Pull request description:
Found this minor doc issue while reviewing(learning) previous merged PR.
Close https://github.com/rust-bitcoin/rust-bitcoin/issues/763
ACKs for top commit:
Kixunil:
ACK 9835736ef5
dr-orlovsky:
ACK 9835736ef5
Tree-SHA512: 3cdeb88a5a13c26e345552038a80c34c68ec581de5739ec3545643c4900c46cee4636a5cf84f5d1b4c8652a032dc2e4a66fe60fa9cbebc3adfef4b8886c7730c
Fourth step in implementation of Schnorr key support after #588.
While PSBT BIP174 does not specify whether uncompressed keys are supported in BIP32-related fields, from BIP32 it follows that it is impossible to use uncompressed keys within the extended keys. This PR fixes this situation and is a companion to BIP174 PR clarifying key serialization: https://github.com/bitcoin/bips/pull/1100
7f06e91a93 LowerHex and UpperHex implementations for LeafVersion (Dr Maxim Orlovsky)
6a3f3aabaf Inverse alternative formatting for LeafVersion type (Dr Maxim Orlovsky)
bec6694233 Fix docs on error conditions in LeafVersion::from_consensus (Dr Maxim Orlovsky)
7c28b47451 LowerHex and UpperHex implementations for FutureLeafVersion (Dr Maxim Orlovsky)
Pull request description:
Trivial post-merge fixups from review comments in #718
ACKs for top commit:
Kixunil:
ACK 7f06e91a93
sanket1729:
ACK 7f06e91a93
Tree-SHA512: d94c4bd3d0b466287c8965103f74ecaba185d14c13b6c3f37d9fbe194343b3fc902fd2c7716554ad01fe28ff89cda933df199b7e8388a3fa6097028caf62522b
cf0c48cc86 Improve Debug for PrivateKey (Dr Maxim Orlovsky)
b65a6ae49b Test for extended private key keypair generation f5875a (Dr Maxim Orlovsky)
e6a3d603c9 BIP32 extended key `to_ecdsa()` and `to_schnorr()` methods (Dr Maxim Orlovsky)
b72f56c4ae BIP32 extended keys are using Scep256k1 keys instead of bitcoin ECDSA (Dr Maxim Orlovsky)
Pull request description:
This is third step required to introduce Schnorr key support according to #588. This PR starts API-breaking changes and is follow-up to non-API breaking #589, which is already merged.
PR rationale: BIP32 does not support uncompressed keys and using type with compression flag was a mistake
ACKs for top commit:
apoelstra:
ACK cf0c48cc86
sanket1729:
ACK cf0c48cc86. #757 might need rework after this
Tree-SHA512: 6356a65004e7517256bacbf9aaeb69a22fd8536b341e567c5c4e819288e1105d083fe12ac0641404c407c97acf039bdc525f8e02b1b594a6cdda90106f3b1bdc
ef8a3a839e Introduce FutureLeafVersion (Dr Maxim Orlovsky)
b028385a72 Improve docs in LeafVersion (Dr Maxim Orlovsky)
839c022f29 Make serde for LeafVersion to have byte representation (Dr Maxim Orlovsky)
67b8db05a8 Converting LeafVersion into an enum (Dr Maxim Orlovsky)
2405417432 Use TAPROOT_ANNEX_PREFIX in sighash module (Dr Maxim Orlovsky)
Pull request description:
The original `LeafVersion` implementation was just a newtype around `u8`. I think that having enum explicitly listing consensus script implementation rules may be more beneficial in terms of both code readibility and future use of multiple script types, where `LeafVersion` may operate as a context object provided to `Script` to specify interpretation rules for particular op codes.
ACKs for top commit:
Kixunil:
ACK ef8a3a839e
sanket1729:
crACK ef8a3a839e. Waiting a day to let others complete review before merging.
apoelstra:
ACK ef8a3a839e
Tree-SHA512: 3356d2b9b00cf904edfece26d26ffbc646ba74446cc23ec4b2b4026ed50861285802f077226e30ba8fed466f68f8e8556c729ce48cb38581b1d95a02a6fde9cf
14ace92666 Fix SchnorrSig type references in PSBT serialization macros (Dr Maxim Orlovsky)
2b530000d3 Use EcdsaSig in PSBT partial signatures instead of Vec<u8> (Dr Maxim Orlovsky)
141dbbd1b9 Add serde impl for EcdsaSig (Dr Maxim Orlovsky)
c92057d98f PSBT serialize/deserialize impl for EcdsaSig type (Dr Maxim Orlovsky)
0af1c3f320 Add Display and FromStr for EcdsaSig (Dr Maxim Orlovsky)
daf0eacf3d Improve NonStandardSigHashType (Dr Maxim Orlovsky)
c36a3da6f0 Add EcdsaSig::sighash_all convenience constructor (Dr Maxim Orlovsky)
Pull request description:
Previously signatures were kept in PSBT as raw byte vec, without processing. This adds specific data type, capable of holding & serializing/deserializing partial signature with sighash flag information.
ACKs for top commit:
apoelstra:
ACK 14ace92666
Kixunil:
ACK 14ace92666
Tree-SHA512: f505df9d1990735fe3941092174a61e067a4e3db30d2d1b94b136da4607865b3b78b274e674b2cfb877aaf0ab58e007c1ab1738f3927d60a4a3ecc12cadc5a48
247a14f4c3 Use test big block for bench_stream_reader instead of making one (Riccardo Casatta)
b92dfbb63f exclude test_data when publishing the crate (Riccardo Casatta)
f5a9681a2a include a big block in test_data, use it for ser/de benchmark (Riccardo Casatta)
09dada55d6 Move bip158 test vectors to test_data (Riccardo Casatta)
06d1a820c3 Remove testnet block hex from tests, use test_data with include_bytes! (Riccardo Casatta)
Pull request description:
In the first two commits I moved some data from source files to the newly introduced `test_data` dir, including it with `include_[str|bytes]!` macro.
The second-to-last commit introduces a big block in test_data which is very handy in ser/de benchmark (I used it for #672) because with smaller blocks you may not notice performance improvements.
Since I don't want to pollute the package the last commit excludes the `test_data` dir from the published package. I think it's fine to do it because dependent packages don't run dependencies tests.
ACKs for top commit:
apoelstra:
ACK 247a14f4c3
Kixunil:
tACK 247a14f4c3
Tree-SHA512: a2beb635b0a358737d0b57d3e7205b1ddf87652b9a8c889ce63e2867659a8eaf7e43a5b87a453345d56d953745913f40b58596f449e5fbc87340e0dd2aef0727
91470f56c8 Uncomment sighash test (sanket1729)
2178c7367c Update to secp256k1 0.21.2 (sanket1729)
Pull request description:
ACKs for top commit:
Kixunil:
ACK 91470f56c8
dr-orlovsky:
utACK 91470f56c8 in order to unlock the rest of PRs required for Taproot, which depend on this.
Tree-SHA512: ea04a2ae14aee078f33e49e3283cd5c4dad5b118aef75b8e7c6dd00620c66e89096ff2fbd8b450d75b43a98cd20b350dad05c288687683272fa4878a6d9c83c2
533120899e Put rustdocs above attributes (Tobin Harding)
Pull request description:
(Trivial / Very low priority PR)
Rust idiomatic style is to put the rustdoc _above_ any attributes on types, functions, etc.
Audit the codebase and move comments/attributes to the correct place. Add a trailing full stop at times to neaten things up a little extra.
Done after discussion [here](https://github.com/rust-bitcoin/rust-secp256k1/pull/353#discussion_r778393138)
ACKs for top commit:
Kixunil:
ACK 533120899e
RCasatta:
ACK 533120899e
Tree-SHA512: 7cd00dc46de813cbe3f96417bb4b13980064e10110b421224496c8b64bbe87b61b6c757cc621fde1d05754be6ecdc08acdb51fd8978e3f820d2d93f7104062d1
e860333bf3 Fix typos (Riccardo Casatta)
9189539715 Use BufReader internally in StreamReader to avoid performance regression on existing callers (Riccardo Casatta)
5dfb93df71 Deprecate StreamReader (Riccardo Casatta)
9ca6c75b18 Bench StreamReader (Riccardo Casatta)
Pull request description:
`StreamReader` performance is extremely poor in case the object decoded is "big enough" for example a full Block.
In the common case, the buffer is 64k, so to successfully parse a 1MB block 16 decode attempts are made.
Even if a user increases the buffer size, `read` is not going to necessarily fill the buffer, as stated in the doc https://doc.rust-lang.org/stable/std/io/trait.Read.html#tymethod.read. In my tests, the reads are 64kB even with a 1MB buffer.
I think this is the root issue of the performance issue found in electrs in https://github.com/romanz/electrs/issues/547 and they now have decided to decode the TCP stream with their own code in cd0531b8b7 and 05e0221b8e.
Using directly `consensus_encode` seems to make more sense (taking care of using `BufRead` if necessary) so the `StreamReader` is deprecated
ACKs for top commit:
Kixunil:
ACK e860333bf3
apoelstra:
ACK e860333bf3
Tree-SHA512: a15a14f3f087be36271da5008d8dfb63866c9ddeb5ceb0e328b4a6d870131132a8b05103f7a3fed231f5bca099865efd07856b4766834d56ce2384b1bcdb889b
Rust idiomatic style is to put the rustdoc _above_ any attributes on
types, functions, etc.
Audit the codebase and move comments/attributes to the correct place.
Add a trailing full stop at times to neaten things up a little extra.
92ee5a7e5f Test BIP341 sighash code (sanket1729)
Pull request description:
Based on #695 . Adds the remaining test vectors to test sighash paths. Will rebase after #695
ACKs for top commit:
apoelstra:
ACK 92ee5a7e5f
RCasatta:
ACK 92ee5a7e5f
Tree-SHA512: 4bd66632dd9ffeab5a70f44763d87ddd1fccffe22df66385b79835f9033408a67e0b157e855ea2394797ac2572cc08db1d66a5a71bfb58e2adbcb9f2ff7d1f0b
StreamReader before this commit is trying to repeatedly parse big object like
blocks at every read, causing useless overhead.
consensus_encode deal with partial data by simply blocking.
After this changes it doesn't look what remain of the StreamReader is really giving
value, so it's deprecated
7d982fa9a2 Add all tests from BIP 371 (sanket1729)
d22e0149ad Taproot psbt impl BIP 371 (sanket1729)
108fc3d4db Impl encodable traits for TapLeafhash (sanket1729)
c7478d8fd0 Derive serde for taproot stuctures (sanket1729)
Pull request description:
Built on top of #677 . Will rebase and mark ready for review after #677 is merged.
ACKs for top commit:
apoelstra:
ACK 7d982fa9a2
dr-orlovsky:
re-tACK 7d982fa9a2 basing on `git range-diff`. The original PR before last re-base was tested commit-by-commit.
Tree-SHA512: feb30e4b38d13110a9c0fabf6466d8f0fb7df09a82f4e01d70b8371b34ab0187004a6c63f9796c6585ee30841e8ee765ae9becae139d2e1e3d839553d64c3d1e
106acdc3ac Add fuzzing for Witness struct (Riccardo Casatta)
2fd0125bfa Introduce Witness struct mainly to improve ser/de performance while keeping most usability. (Riccardo Casatta)
Pull request description:
At the moment the Witness struct is `Vec<Vec<u8>>`, the vec inside a vec cause a lot of allocations, specifically:
- empty witness -> 1 allocation, while an empty vec doesn't allocate, the outer vec is not empty
- witness with n elements -> n+1 allocations
The proposed Witness struct contains the serialized format of the witness. This reduces the allocations to:
- empty witness -> 0 allocations
- witness with n elements -> 1 allocation for most common cases (you don't know how many bytes is long the entire witness beforehand, thus you need to estimate a good value, not too big to avoid wasting space and not too low to avoid vector reallocation, I used 128 since it covers about 80% of cases on mainnet)
The inconvenience is having slightly less comfortable access to the witness, but the iterator is efficient (no allocations) and you can always collect the iteration to have a Vec of slices. If you collect the iteration you end up doing allocation anyway, but the rationale is that it is an operation you need to do rarely while ser/de is done much more often.
I had to add a bigger block to better see the improvement (ae860247e191e2136d7c87382f78c96e0908d700), these are the results of the benches on my machine:
```
RCasatta/master_with_block
test blockdata::block::benches::bench_block_deserialize ... bench: 5,496,821 ns/iter (+/- 298,859)
test blockdata::block::benches::bench_block_serialize ... bench: 437,389 ns/iter (+/- 31,576)
test blockdata::block::benches::bench_block_serialize_logic ... bench: 108,759 ns/iter (+/- 5,807)
test blockdata::transaction::benches::bench_transaction_deserialize ... bench: 670 ns/iter (+/- 49)
test blockdata::transaction::benches::bench_transaction_get_size ... bench: 7 ns/iter (+/- 0)
test blockdata::transaction::benches::bench_transaction_serialize ... bench: 51 ns/iter (+/- 5)
test blockdata::transaction::benches::bench_transaction_serialize_logic ... bench: 13 ns/iter (+/- 0)
branch witness_with_block (this one)
test blockdata::block::benches::bench_block_deserialize ... bench: 4,302,788 ns/iter (+/- 424,806)
test blockdata::block::benches::bench_block_serialize ... bench: 366,493 ns/iter (+/- 42,216)
test blockdata::block::benches::bench_block_serialize_logic ... bench: 84,646 ns/iter (+/- 7,366)
test blockdata::transaction::benches::bench_transaction_deserialize ... bench: 648 ns/iter (+/- 77)
test blockdata::transaction::benches::bench_transaction_get_size ... bench: 7 ns/iter (+/- 0)
test blockdata::transaction::benches::bench_transaction_serialize ... bench: 50 ns/iter (+/- 5)
test blockdata::transaction::benches::bench_transaction_serialize_logic ... bench: 14 ns/iter (+/- 0)
```
With an increased performance to deserialize a block of about 21% and to serialize a block of about 16% (seems even higher than expected, need to do more tests to confirm, I'll appreciate tests results from reviewers)
ACKs for top commit:
apoelstra:
ACK 106acdc3ac
sanket1729:
ACK 106acdc3ac
dr-orlovsky:
utACK 106acdc3ac
Tree-SHA512: e4f23bdd55075c7ea788bc55846fd9e30f9cb76d5847cb259bddbf72523857715b0d4dbac505be3dfb9d4b1bcae289384ab39885b4887e188f8f1c06caf4049a
sanket1729
Andrew Poelstra
Dr. Maxim Orlovsky
KaFai Choi
Riccardo Casatta
Tobin Harding