Currently function contains nested `if` clauses that arguably obfuscate
the code. We can make the code easier to read by pulling out the error
paths and returning them higher up in the function.
Refactor only, no logic changes.
Put the calls to `impl_hashencode` in the same order, and with the same
whitespace, as the calls to `hash_newtype`. This makes groking the file
easier because its quick to glance down the types and see which ones
implement hashencode (consensus_encode/decode) and which ones do not.
Changes the API from TweakedPublicKey to XonlyPublicKey. I believe we
introduced TweakedPublicKey to guard against creating address API. This
is confusing because when we want to verify control block we have to
call dangerous_assume_tweak.
This is in true in most cases that the key would be tweaked, but we only
want to guard in while creating a new address. If we want to verify
blocks, we should deal with native X-only-keys regardless of how they
were created
5e2449922d Separate merge logic out of Map trait (Tobin Harding)
Pull request description:
Recently we (*cough* Tobin) made the `Map` trait private and neglected
to add a public API for merging together two PSBTs. Doing so broke the
`psbt` module.
Add a public trait `Merge` and implement it for
`PartiallySignedTransaction` using the code currently in the `merge`
method of the now private `Map` trait.
Motivated by https://github.com/rust-bitcoin/rust-bitcoin/pull/841
ACKs for top commit:
JeremyRubin:
> ACK 5e24499
apoelstra:
ACK 5e2449922d
sanket1729:
ACK 5e2449922d. Also verified that the vectors are same of that of BIP174
Tree-SHA512: 79eefe93e870b61231b388aa28a95ee5c8ac06b68910f4ff324569512a79eafe5b86239fd45f54ca7a868cf59dc6301e45d1f046c039a64b2493a8ffcea659fd
c0d36efb8b Don't allow uncompressed public keys without prefix 0x04 (Noah Lanson)
Pull request description:
Was following #520 and through it was a quick fix that I could do:
#### Changes:
- If an uncompressed public key doesn't have prefix 0x04 in `PublicKey::from_slice()`, an error is returned.
<br>
I was wondering if `PublicKey::from_str()` should also enforce the same rules, however I have not incuded this in the PR.
Please let me know if any changes need to be made.
Thanks
ACKs for top commit:
Kixunil:
ACK c0d36efb8b
apoelstra:
ACK c0d36efb8b
sanket1729:
utACK c0d36efb8b. Not thrilled about the error message expecting len 66, when it can be both 66/130. But can live with it
Tree-SHA512: cfbcd569691c9a7f69ee775ec530605f42e988470a2ff9c28b4c881cec6b259053bb2288818e00b6f6b20316b1fb30fecc0b9a240ebbe7618f202ef6b5efeb9b
Recently we (*cough* Tobin) made the `Map` trait private and neglected
to add a public API for combining together two PSBTs. Doing so broke the
`psbt` module.
Pull the merge logic out of the `Map` trait and put it in methods on
each individual type (`Input`, `Output`, `PartiallySignedTransaction`).
Doing so allows for simplification of return types since combining
inputs/outputs never errors.
Use the term 'combine' instead of 'merge' since that is the term used in
BIP 174.
10fedfb3b4 Change Prevouts::All(&[TxOut]) to Prevouts::All(&[Borrow<T>]) (sanket1729)
Pull request description:
I believe this avoids some allocation of creating a vec of TxOut to
create a slice incase the data is already available in psbt/other
methods.
See #834
ACKs for top commit:
apoelstra:
ACK 10fedfb3b4
Kixunil:
ACK 10fedfb3b4
Tree-SHA512: 20f69c626b38d6b3c03c8cb370cfad097bbf0bfefff9bb2379c8af3bc94e25d8cc45fc5d69488aeefad58a95470e8f30eb7b400349992a9ebd0d3a13870cba43
This avoids some allocation of creating a vec of TxOut to
create a slice incase the data is already available in psbt/other
methods. Facilitates creation of Prevouts from &[TxOut] as well as
&[&TxOut]
This changes the type of secp signature from secp256k1::Signature to
bitcoin::PublicKey. Psbt allows storing signatures for both compressed
as well as uncompressed keys. This bug was introduced in #591 while
trying to change the type of BIP32 keys from bitcoin::PublicKey to
secp256k1::PublicKey.
df7bb03a67 Simplify read_scriptbool (Tobin Harding)
4b6e86658d Refactor is_provably_unspendable (Tobin Harding)
e54a2d653b Put && operator at front of line (Tobin Harding)
f5512c4931 Refactor is_p2pkh (Tobin Harding)
373ea89a9a Simplify read_scriptbool (Tobin Harding)
654b2772b8 Add passing unit tests for read_scriptbool (Tobin Harding)
Pull request description:
In an effort to make the code clearer and more explicit, do various refactorings around logical operators. Each done as a separate patch to ease review and limit scope of discussion.
Based on review of https://github.com/rust-bitcoin/rust-bitcoin/pull/806
ACKs for top commit:
Kixunil:
ACK df7bb03a67
apoelstra:
ACK df7bb03a67
Tree-SHA512: 06460979d492eb38cefc147397338b7fd95320c66ce8e8b4f8e2b454bb35721ce308413690a0618bd19d695df56175646d4d0c619388c0268f7fd35d5a7b6a3d
Simplify `read_scriptbool` by doing:
- Use `split_last` to get at the last element
- Mask the last byte against ^0x80 instead of using two equality
statements
151173821b Use fn name to_ instead of into_ (Tobin Harding)
Pull request description:
Rust convention is to use `to_` for conversion methods that convert from
an owned type to an owned `Copy` type. `into_` is for owned to owned
non-`Copy` types.
Re-name conversion methods that use `into_` for `Copy` types to use
`to_`, no need to deprecate these ones because they are unreleased.
**Note to maintainers**
This is similar in concept to #798 but only touches new code introduced in this release. Has been labelled 'RC fix' for that reason. Please feel free to remove the label if you disagree.
From the docs: https://rust-lang.github.io/api-guidelines/naming.html
<h2><a class="header" href="https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv" id="ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv">Ad-hoc conversions follow <code>as_</code>, <code>to_</code>, <code>into_</code> conventions (C-CONV)</a></h2>
<p>Conversions should be provided as methods, with names prefixed as follows:</p>
Prefix | Cost | Ownership
-- | -- | --
as_ | Free | borrowed -> borrowed
to_ | Expensive | borrowed -> borrowed
| | | borrowed -> owned (non-Copy types)
| | | owned -> owned (Copy types)
into_ | Variable | owned -> owned (non-Copy types)
ACKs for top commit:
Kixunil:
ACK 151173821b
apoelstra:
ACK 151173821b
sanket1729:
ACK 151173821b
Tree-SHA512: 4bb97e4fb78beda0fd1ec9482d24ef0f4ade6d3689f5c1bcf2208fa2df3195962522fa5d5ac700e6d4d5ff2096a20b2a0ad51784909a3c12405762aa08d1ced2
8d780e8172 Use 4 character indentation (Tobin Harding)
Pull request description:
Bizarrely the impl block for `EcdsaSigHashType` uses 5 character indentation.
Use 4 character indentation as is typical.
ACKs for top commit:
Kixunil:
ACK 8d780e8172
apoelstra:
ACK 8d780e8172
Tree-SHA512: 4b0789b4ef6f3e817fcbe002ca0d9ee4fca4a3dc70bbeaebae745a2f0497eb072f3c3876805d97ff58eb8a9464ea4443f23cea0c95bfd052e382caa9f76b515d
b138428df7 Re-export public map types from root level (Tobin Harding)
Pull request description:
We currently have the `map` module private but containing a bunch of types that are needed in the public API (specifically in a `PartiallySignedTransaction`).
To give access to them re-export the `util::psbt` module at the root level.
Found while testing `master` with `rust-miniscript`.
ACKs for top commit:
sanket1729:
utACK b138428df7
Kixunil:
ACK b138428df7
RCasatta:
ACK b138428df7
dr-orlovsky:
ACK b138428df7
Tree-SHA512: 36fc8595164c4975abdadb6c8149ef27686a2d681a1815379f91b1bd36f8a56ceaa7faed5979ba6869823684790721a16a0c41e662c6227a09cd0ba576a0a181
Rust convention is to use `to_` for conversion methods that convert from
an owned type to an owned `Copy` type. `into_` is for owned to owned
non-`Copy` types.
Re-name conversion methods that use `into_` for `Copy` types to use
`to_`, no need to deprecate these ones because they are unreleased.
We currently have the `map` module private but containing a bunch of
types that are needed in the public API (specifically in a
`PartiallySignedTransaction`).
Re-export the publicly required types to the `psbt` module and then
again at the root level of `rust-bitcoin` as we do for other types.
abe52f681b Cleanup/Dedup psbt (De)Serialization code (sanket1729)
fbd86dcf63 Update documentation of EcdsaSig::from_slice (sanket1729)
85009a7b50 Update documentation of from_u32_consensus (sanket1729)
0fed04e2d5 Change EcdsaSig hash type deser (sanket1729)
Pull request description:
Changes the parsing behavior in PSBT on non-standard sighash types to give an explicit error, rather than silently mangling the parsed value
ACKs for top commit:
dr-orlovsky:
ACK abe52f681b
apoelstra:
ACK abe52f681b
Kixunil:
ACK abe52f681b
Tree-SHA512: 1d5dbe3aa5885ca16649cf8ea05a7476e8dd977dd870b79358d97a3ce383bee93754d2b88163e7db3792cdc4b9cb867356409c8eea4e110877577ad196ba0786
dfd8924398 Remove insert_pair from Map trait (Tobin Harding)
ad75d5181f Make Map trait private to psbt module (Tobin Harding)
53225c0a6e Improve docs in map module (Tobin Harding)
92059c2841 Add full stops to rustdocs (Tobin Harding)
11c046b707 Refactor match arms (Tobin Harding)
e6af569490 Move imports to top of file (Tobin Harding)
Pull request description:
The `Map` method `insert_pair` is never called for `PartiallySignedTransaction`. Separate the method into its own trait (`Insert`) and delete dead code. The dead code contains the alleged bug in #576.
- Patch 1: Preparatory cleanup
- Patch 2: Preparatory refactor
- Patch 3 and 4: Improve docs in the module that this PR touches
- Patch 5: Make `Map` trait private to the `psbt` module
- ~Patch 6: Make `concensus_decode_global` method into a function~
- Patch ~7~ 6: Pull `insert_pair` method out of `Map` trait into newly create `Insert` trait
Resolves: https://github.com/rust-bitcoin/rust-bitcoin/issues/576
(Title of PR is `Make Map trait private` because that is the API break.)
ACKs for top commit:
dr-orlovsky:
ACK dfd8924398
apoelstra:
ACK dfd8924398
Tree-SHA512: 1a78294bc8a455552d93caf64db697f886345ba979f574abad55820415958fee1c2dd16945f4eafdbe542fa202cb7e08618aa137ec7ee22b3c9dac5df0328157
8a993e8a58 Properly deprecate util::ecdsa key re-exports (Dr Maxim Orlovsky)
bcb8932ccf Re-org keys and ecdsa mods - pt.3 (Dr Maxim Orlovsky)
d1c2213d3b Re-org keys and ecdsa mods - pt.2 (Dr Maxim Orlovsky)
b9170162d5 Re-org keys and ecdsa mods - pt.1 (Dr Maxim Orlovsky)
2d9de78725 Re-export all key types under `util::key`. Deprecate other exports. (Dr Maxim Orlovsky)
Pull request description:
This PR tries to do a minimally-invazive separation of signature- and key-related types, previously mixed in a single `util::ecdsa` module.
Rationale: bitcoin key types are not specific for signature algorithm. See discussion at #588.
This PR became possible after we moved on new `secp256k1` version exposing `XonlyPublicKey` type, since now all key types may co-exist in a single module under different names
The PR goal is achieved through
- Renaming ecdsa mod into private ec module such that the code is not copied and diff size is small;
- Introducing dummy ecdsa mod back in the next commit and re-exporiting only signature types from internal `ec` mod in it;
- Re-exporting all key types under `key` module, removing previous depreciation message for bitcoin keys.
ACKs for top commit:
apoelstra:
ACK 8a993e8a58
sanket1729:
utACK 8a993e8a58
Tree-SHA512: 9f71edaa2cf4cdab4b239cb1d57576e2ba0fc3c2ec0ea19ae232005967b9400da6ded992b33d10b190ca617a66dca9b99be430bc5058a064f0be1489723c4a3a
The method implementation of `insert_pair` is currently not used for
`PartiallySignedTransaction`. Having an implementation available is
deceiving.
Delete the unused `insert_pair` code from
`PartiallySignedTransaction` (dead code). Make the `insert_pair` methods
from `Input` and `Output` be standalone functions.
The `Map` trait has been deemed confusing and not that useful to users
of the library, we still use it internally within the `psbt` module
though so make it visible only in `psbt` and `psbt::map`.
Improve the function rustdocs in the `psbt::map` module by:
- using third person tense as is idiomatic in the Rust ecosystem
- using rustdoc `///` not code comments `//` for methods
- Use `# Return` section for documenting return values
Done for this module only as part of a PR fixing code within this
module.
40f38b3edc enforce strict SI(treat capital of m, u, n, p as invalid) in parsing amount denomiation. add disallow_unknown_denomination test (KaFai Choi)
e80de8b1ee add nano and pico BTC to Donomination enum (KaFai Choi)
Pull request description:
Close [741](https://github.com/rust-bitcoin/rust-bitcoin/issues/741)
ACKs for top commit:
Kixunil:
ACK 40f38b3edc
apoelstra:
ACK 40f38b3edc
dr-orlovsky:
Changing review to ACK 40f38b3edc since it was my misunderstanding and not a bug
Tree-SHA512: 4cc380b8e7403e37e7993e25848b25d74c610d4e9fe274526c613d4b3e2a9f6677c7df52310fc1cab6f1d629d9529ff9f5a2efa41d9e07eab62d0989780ae3a4
This commit tries to achieve separation of signature- and key-related types, previously mixed in a single ECDSA module.
Rationale: bitcoin key types are not specific for signature algorithm.
This is achieved through
- Remove key mod with its content moved to ecdsa mod
- Re-export keys under key module in util mod - to make git generate diff for the rename of ecdsa mod in the next commit correctly.
7405836411 Fix warning about deprecated method use (Dr Maxim Orlovsky)
f39b1300fa CI: do not fail fast (Dr Maxim Orlovsky)
f77c57195a Making Script method new_* names more consistent (Dr Maxim Orlovsky)
91b68a468d Taproot-related methods for Script type (Dr Maxim Orlovsky)
599c5f9488 Generalizing taproot key tweaking for KeyPairs (Dr Maxim Orlovsky)
Pull request description:
* Adds taproot-related methods to `Script`
* Fixes API for existing taproot methods
* Generalizes `TapTweak` trait to work with both public keys and key pairs
~~UPD: PR is pending https://github.com/rust-bitcoin/rust-secp256k1/pull/342~~
ACKs for top commit:
sanket1729:
ACK 7405836411
apoelstra:
ACK 7405836411
Tree-SHA512: 4a76dfffa1452baadc15e19812831ef9d2e66794c090a8fc123388d7119b2c8a1f0420ce723ad22e01683c8198711fe62e0cdf00c9ad2d2974606383baaf1cb0
9a8ab3f3ff Change type of final script witness to Witness from Vec<Vec<u8>> (sanket1729)
Pull request description:
Doing this would certainly help APIs downstream that operate on &Witness because they would not conversion from &Vec<Vec<u8>> to &Witness.
ACKs for top commit:
Kixunil:
ACK 9a8ab3f3ff
RCasatta:
ACK 9a8ab3f3ff
dr-orlovsky:
ACK 9a8ab3f3ff
apoelstra:
ACK 9a8ab3f3ff
Tree-SHA512: 647e18d254a51d6216a0122407146e8bc1d39504e76c1e0e746f740cec7cda587455b61d4cdadc3c59b1cf03eba87000de35fbde645a30fb166a84847ba101b2
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
I think it is more natural to write Wintess::new() followed by Witness::push()
then Witness::default(). In any case, there is no harm in having additional constructors.
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
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
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.
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
2959e04ebd Allow specifing a raw `TapLeafHash` in sighash computation (Alekos Filini)
Pull request description:
Still need to add some tests but the code should be ready for review. Please let me know if you have better ideas for the enum naming.
---
Instead of always requiring the full raw script and leaf version, allow
just specifying a raw leaf hash to the sighash computation functions.
This is very useful when dealing with PSBTs, because the
`PSBT_IN_TAP_BIP32_DERIVATION` field only maps a public key to a leaf
hash, so a signer could just take it and produce a signature with it
rathern than having to jump through hoops to recover the full raw
script.
ACKs for top commit:
sanket1729:
Tested locally. ACK 2959e04. Reviewed range-diff with 5aa1d02
apoelstra:
ACK 2959e04ebd
Tree-SHA512: 830be0b8382ac59b73e6481f61ec1effdcd32859c04382e6cd5a43ac689d6e528f9a8b27c026ee81f5d5b59d2e3c397f9c271145e001ff2dc4815764fc21a2c6
Witness struct is in place of the Vec<Vec<u8>> we have before this commit.
from_vec() and to_vec() methods are provided to switch between this type and Vec<Vec<u8>>
Moreover, implementation of Default, Iterator and others allows to have similar behaviour but
using a single Vec prevent many allocations during deserialization which in turns results in
better performance, even 20% better perfomance on recent block.
last() and second_to_last() allows to access respective element without going through costly Vec
transformation
826fed53f2 transactions: add a note about `get_vsize` and standardness rules (Antoine Poinsot)
Pull request description:
If they ever hit a discrepancy they must really be doing something dodgy but hey :)
ACKs for top commit:
dr-orlovsky:
ACK 826fed53f2
Tree-SHA512: c618a80b047797625a233939d2c1146e8b4ce44215648841813f78178577afc844f5e561e4e60b4084e315735894ecb354af8d81f4702f5354e5d5cd05b52ac4
Instead of always requiring the full raw script and leaf version, allow
just specifying a raw leaf hash to the sighash computation functions.
This is very useful when dealing with PSBTs, because the
`PSBT_IN_TAP_BIP32_DERIVATION` field only maps a public key to a leaf
hash, so a signer could just take it and produce a signature with it
rathern than having to jump through hoops to recover the full raw
script.
f690b8e362 Be more liberal when parsing Denomination (Tobin Harding)
628168e493 Add missing white space character (Tobin Harding)
Pull request description:
There is no reason to force users to use a particular format or case for `Denomination` strings. Users may wish to write any of the following and all seem reasonable
- 100 sats
- 100 sat
- 100 SAT
The same goes for various other `Denomination`s.
- Patch 1 enables usage of "sats", "sat", "bit", "bits"
- Patch 2 enables usage of various lower/uper case formatting
Fixes: #729
ACKs for top commit:
Kixunil:
ACK f690b8e362
apoelstra:
ACK f690b8e362
Tree-SHA512: a785608e19a7ba6f689dc022cb17a709041ff56abeaa74649d0832a8bd8aac4593c7a79b46a47dd417796c588d669f50fb3c8b8a984be332ca38a1fef2dcd4ce
Using the latest version of rust-bitcoin master on rust-miniscript
errors on bitcoin::SigHashType not found. In the original PR, I only
renamed the export to ECDSASigHashType, but original re-export should
also be there in lib.rs to avoid to breaking changes downstream.
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
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
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
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.
Improve the rustdocs for the various `Address` constructors by putting
the brief description on a separate line with further description in its
own paragraph. This is the layout best practice for function documentation
using rustdocs.
Also, favour 'creates' over 'constructs' because it is more common in
the docs of this struct.
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.
0af5a433b6 Return the correct `LeafVersion` when building a Taproot `ControlBlock` (Alekos Filini)
Pull request description:
ACKs for top commit:
sanket1729:
ACK 0af5a433b6
Tree-SHA512: 6b887e86b32b070a2a42ba1a2309b094c36d5a0b0bbf7d4c49c4fd2d8d2b4a7b1d87da699f1bd5f7116926c590413609a292d900b55c27c6bdbadc408529999f
0d463ec19e tests: improve coverage for P2tr and AddressType (Leonardo Comandini)
Pull request description:
The new AddressType test shows addresses that are valid but have
no type. If in the future some of those get a type or become
invalid (either voluntarily or due to a regression), this will
highlight it.
ACKs for top commit:
dr-orlovsky:
utACK 0d463ec19e
sanket1729:
ACK 0d463ec19e
Tree-SHA512: 9e062a1807173638cb62a61a2e8ea5be8324449a8944c356073e8bd9f53941dea369c65a35dfa0019bd8323eaa5dd26a9907c1823522fef9a524e919728973a6
The new AddressType test shows addresses that are valid but have
no type. If in the future some of those get a type or become
invalid (either voluntarily or due to a regression), this will
highlight it.
55c627715f Moving globals into PSBT struct (Dr Maxim Orlovsky)
Pull request description:
I took the most non-invasive approach to reduce diff size. Many parts of the code can be improved in style or further refactored (like some functions are not necessary and can be just moved to be part of other functions), but I'd prefer to do that as a separate PR once this will be merged.
My approach with this PR:
1. Remove `Global` struct by moving its fields right into `PartiallySignedTransaction` - but keep the `util/psbt/map/global.rs` file with all its logic
2. Keep existing `Map for Global` implementation in the same file, but just change it to `Map for PartiallySignedTransaction`
3. With serialization, convert `Global` deserialization into crate-private function and use it from `PartiallySignedTransaction` deserialization
4. Refactor the tests and imports as required to get the thing compile and pass tests
The refactoring will be followed by PR(s) adding support for Taproot
ACKs for top commit:
apoelstra:
ACK 55c627715f
sanket1729:
ACK 55c627715f . Reviewed range diff with ac0c908 that I previously ACKed
Tree-SHA512: 79b329b6e4e60af905e4e00507d6abc558261d921bcf8f5d4ee34dd685322d7a529b18015423da50a388ba6732b7b662a92bc95ad078228cc809254ad010d467
Ambiguous TweakedPublicKey and UntweakedPublicKey type aliases and methods to convert
Use structs for Untweaked and Tweaked key type
swap dangerous api to work on tweaked keys
remove unecessary allocations and rename methods
Use type alias for UntweakedPublicKey
TweakedPublicKey::new(...) method added
minor naming and doc changes
Docs can always do with a bit of love.
Clean up the module level (`//!`) rustdocs for all public modules.
I claim uniform is better than any specific method/style. I tried to fit
in with what ever was either most sane of most prevalent, therefore
attaining uniformity without unnecessary code churn (one exception being
the changes to headings described below).
Notes:
* Headings - use heading as a regular sentence for all modules e.g.,
```
//! Bitcoin network messages.
```
as opposed to
```
//! # Bitcoin Network Messages
```
It was not clear which style to use so I picked a 'random' mature
project and copied their style.
* Added 'This module' in _most_ places as the start of the module
description, however I was not religious about this one.
* Fixed line length if necessary since most of our code seems to follow
short (80 char) line lengths for comments anyways.
* Added periods and fixed obvious (and sometimes not so obvious)
grammatically errors.
* Added a trailing `//!` to every block since this was almost universal
already. I don't really like this one but I'm guessing it is Andrew's
preferred style since its on the copyright notices as well.
This refactors `Script::bytes_to_asm_fmt`` function to use an iterator
instead of index. Such change makes it easier to reason about overflows
or out-of-bounds accesses. As a result this also fixes three unlikely
overflows and happens to improve formatting to not output space at the
beginning in some weird cases.
To improve robustness even better it also moves `read_uint`
implementation to internal function which returns a more specific error
type which can be exhaustively matched on to guarantee correct error
handling. Probably because of lack of this the code was previously
checking the same condition twice, the second time being unreachable and
attempting to behave differently than the first one.
Finally this uses macro to deduplicate code which differs only in single
number, ensuring the code stays in sync across all branches.
5d71a9dd89 Correct input length check for uin128 fuzzer (Matt Corallo)
9c256cc88e Add a fuzz check for `Uint128::increment` (Matt Corallo)
a15f263c4e Move the `increment` fn into the uint macro to add it to Uint128 (Matt Corallo)
d52b88b525 Fix increment of Uint256 with carry (carolcapps)
Pull request description:
This is #578 with review feedback addressed.
ACKs for top commit:
apoelstra:
ACK 5d71a9dd89
sanket1729:
ACK 5d71a9d
Tree-SHA512: 32e5ea6387943ecad8f190a0de336a545fda72b6ff7388d3479037a5f880434276a7d0607f5cf61710d45e984c01954f4e3199a60c542be48b397717afb3d406
This clearly states lack of support for 16-bit architectures as well as
adds readable `compile_error!()` call. It also fixes a few stylistic
mistakes - headings (top-level should not be repeated) and missing
newlines.
Closes#660
76cf74fa9b Added test for the overflow bug and few others (Martin Habovstiak)
a0e1d2e706 Check for overflow in Script::bytes_to_asm_fmt() (Martin Habovstiak)
Pull request description:
This adds an overflow check in `Script::bytes_to_asm_fmt()` motivated by
`electrs` issue. While it was not tested yet, I'm very confident that
overflow is the cause of panic there and even if not it can cause panic
becuase the public function takes unvalidated byte array and reads
`data_len` from it.
The `electrs` issue: https://github.com/romanz/electrs/issues/490
~~Strangely, this breaks a test case and I can't see why. I'm publishing in case someone wants to help.~~
Edit: One damn character. :D Should be OK now.
ACKs for top commit:
apoelstra:
ACK 76cf74fa9b
Tree-SHA512: 4ffeca442a71b10c132f055f056128ae64e66cbdc1891662c3a4e743b82fa5d27075a44513e844be37888b33068eef3bbf6bcced5def70c17c9c5bd5b9d870cc
This adds a test case for script formatting which caused overflow in the
past and a few others from the same "interesting" transaction. Note that
to trigger the bug one has to run the test on 32 bit architecture.
This adds an overflow check in `Script::bytes_to_asm_fmt()` motivated by
`electrs` issue. While it was not tested yet, I'm very confident that
overflow is the cause of panic there and even if not it can cause panic
becuase the public function takes unvalidated byte array and reads
`data_len` from it.
The `electrs` issue: https://github.com/romanz/electrs/issues/490
c704ee7ffe [docs-only] Use backtick in addition to square parentheses for types references, clarify legacy, non_exhaustive comment, remove std:: (Riccardo Casatta)
f223be618f Rename access_witness to witness_mut and return Option (Riccardo Casatta)
c9bc0b928a [fmt-only] autoformatting with `rustfmt src/util/sighash.rs` (Riccardo Casatta)
07774917c2 Use get_or_insert_with in segwit_cache (Martin Habovstiak)
497dbfb7c3 Use get_or_insert_with in common_cache() (Martin Habovstiak)
ca80a5a030 Use get_or_insert_with in taproot_cache (Martin Habovstiak)
6e06a32ccc Wrap ErrorKind in Io enum variant, fix doc comment for the IO variant (Riccardo Casatta)
1a2b54ff23 introduce constant KEY_VERSION_0 (Riccardo Casatta)
417cfe31e3 Derive common traits for structs and enum, make internal struct not pub (Riccardo Casatta)
55ce3dd6ae Fix validation error if SINGLE with missing corresponding output, remove check_index and check with get().ok_or(), more details in errors (Riccardo Casatta)
2b3b22f559 impl Encodable for Annex to avoid allocation (Riccardo Casatta)
1a7afed068 Add Reserved variant to SigHashType for future use (ie SIGHASH_ANYPREVOUT) (Riccardo Casatta)
53d0e176d3 Deprecate bip143::SigHashCache in favor of sighash::SigHashCache (Riccardo Casatta)
15e3caf62d [test] Test also sighash legacy API with legacy tests (Riccardo Casatta)
24acfe3672 Implement Bip341 signature hash, create unified SigHashCache for taproot, segwit and legacy inputs (Riccardo Casatta)
683b9c14ff add [En|De]codable trait for sha256::Hash (Riccardo Casatta)
Pull request description:
Adds https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki message signature algorithm
The base is taken from `bip143::SigHashCache`, some code results duplicated but I think it's more clear to keep things separated
Would mark some bullet point on https://github.com/rust-bitcoin/rust-bitcoin/issues/503
Test vectors are taken by running d1e4c56309/test/functional/feature_taproot.py with a modified `TaprootSignatureHash` function to print intermediate values that I cannot found in the bip341 [test vector json](https://raw.githubusercontent.com/bitcoin-core/qa-assets/main/unit_test_data/script_assets_test.json)
UPDATE: Latest version includes the suggestion from @sanket1729 to create a unified tool for signature message hash for legacy, segwit, and taproot inputs. In particular, makes sense for mixed segwit v0 and taproot v1 inputs because cached values could be shared
ACKs for top commit:
sanket1729:
ACK c704ee7ffe. Reviewed the diff from a37de1ade475e0c31c932121abaa7aec701b9987 which I previously ACKed
dr-orlovsky:
utACK c704ee7ffe by diffing it to 6e06a32ccc having my ACK before.
apoelstra:
ACK c704ee7ffe
Tree-SHA512: 35530995fe9d078acd0178cfca654ca980109f4502c91d578c1a0d5c6cafacab7db1ffd6216288eac99f6a763776cbc0298cfbdff00b5a83e98ec4b15aa764e8
This documents cargo features in two ways: explictly in text and in code
using `#[doc(cfg(...))]` attribute where possible. Notably, this is
impossible for `serde` derives. The attribute is contitional and only
activated for docs.rs or explicit local builds.
This change also adds `package.metadata.docs.rs` field to `Cargo.toml`
which instructs docs.rs to build with relevant features and with
`docsrs` config activated enabling `#[doc(cfg(...))] attributes.
I also took the opportunity to fix a few missing spaces in nearby code.
This refactors the code to make it possible to use `get_or_insert_with`
instead of unwrapping in `segwit_cache()`. To achieve it `common_cache`
is refactored into two functions: one taking only the required borrows
and the original calling the new one. `segwit_cache` then calls the new
function so that borrows are OK.
Apart from removing unwrap, this avoids calling `common_cache` multiple
times.
There was a question whether this is equally performant. There are
multiple good reasons why it should be:
1. `get_or_insert_with` is marked `#[inline]`
2. Any good optimizer will inline a function that is used exactly once
3. 1 and 2 conclude that the closure will get inlined
4. Computing self.tx can then be moved to the only branch where it is
required.
5. Even if get_or_insert_with didn't get optimized, which is extremely
unlikely, the `tx` field is at the beginning of the struct and it
probably has pointer alignment (`Deref` suggests it's a pointer).
Alignment larger than pointer is not used, so we can expect the
fields to be ordered as-defined. (This is not guaranteed by Rust but
there's not good reason to change the order in this case.) We can
assume that offset to tx is zero in most cases which means no
computation is actually needed so the expression before closure is
no-op short of passing it into the closure as an argument.
At the time of writing `#[inline]` can be seen at
https://doc.rust-lang.org/src/core/option.rs.html#933
Tobin Harding
Andrew Ahlers
sanket1729
Rishabh Singhal
Andrew Poelstra
Noah Lanson
Riccardo Casatta
Casey Rodarmor
wim-web
Dr. Maxim Orlovsky
Roman Zeyde
KaFai Choi
Nils Loewen
Alekos Filini
Martin Habovstiak
z8674558
Marko Bencun
Jeremy Rubin
Noah
Leonardo Comandini
Antoine Poinsot
Clark Moody
Vikas S Shetty
Ben Carman