Recently we added a bunch of additional sighash types, some of the code
comments became stale. Use the non-specific term 'sighash type' instead
of a particular sighash identifier in comments to make the comments more
applicable.
Recently we update all types and docs to use `Sighash` instead of
`SigHash` because 'sighash' is a single word. We should apply the same
logic to functions and variable names.
Do not use an underscore in the identifier 'sighash'.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename the `SigHash` type to `Sighash`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename the `InvalidSigHashType` variant to `InvalidSighashType`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `SigHashCache` to `SighashCache`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `PsbtSigHashType` to `PsbtSighashType`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename the `NonStandardSigHashType` type and error variant to
`NonStandardSighashType`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `SigHashTypeParseError` to `SighashTypeParseError`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `SchnorrSigHashType` to `SchnorrSighashType`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `EcdsaSigHashType` to `EcdsaSighashType`.
c25eddd187 Remove unnecessary documentation (Tobin Harding)
8631474f08 Improve docs in taproot module (Tobin Harding)
Pull request description:
I should have done this PR a month ago, my bad. This one is kind of important IMO because we are going to have so many people looking at this part of the code soon as we release.
As has been done in other places in the codebase; improve the docs in the `taproot` module by doing:
- Use full sentences (capital letters + full stops)
- Use back ticks and links for types where appropriate
- Fix grammar
- Fix stale docs
- Use third person for describing functions
- Use 100 character line width
- Use markdown sections (`# Examples`, `# Returns`) where appropriate
- Separate brief heading from extended description when appropriate
- Use `///` for all functions/types (both private and public)
I also did:
- Build the docs and check all the links
- Read all the built docs, check for sanity and pretty-ness
Its all in one patch, I couldn't really tease it apart. I can try a bit harder if it proves too annoying to review.
ACKs for top commit:
sanket1729:
ACK c25eddd187
dr-orlovsky:
ACK c25eddd187
apoelstra:
ACK c25eddd187
Tree-SHA512: 72f35bf8779392060388db985df5abc42a89796eaad1eafd08ea50b635d469fbd07a53ff253cdf27ad4d4baed7d37cec6ea1da1aece3672b9447f87181e218f8
We deprecated the `bip143::SigHashCache` in
```
commit 53d0e176d3
Author: <elided>
Date: Fri Jul 16 10:44:18 2021 +0200
Deprecate bip143::SigHashCache in favor of sighash::SigHashCache
...
```
This means these changes are unreleased so the deprecated since version
should be the upcoming 0.28 release.
As has been done in other places in the codebase; improve the docs in
the `taproot` module by doing:
- Use full sentences (capital letters + full stops)
- Use back ticks and links for types where appropriate
- Fix grammar
- Fix stale docs
- Use third person for describing functions
- Use 100 character line width
- Use markdown sections (`# Examples`, `# Returns`) where appropriate
- Separate brief heading from extended description when appropriate
- Use `///` for all functions/types (both private and public)
I also did:
- Build the docs and check all the links
- Read all the built docs, check for sanity and pretty-ness
992857ad0a PsbtSighashType unit tests (Dr Maxim Orlovsky)
5be1cdb8c7 PsbtSigHashType Display and FromStr implementation (Dr Maxim Orlovsky)
7cdcdaad6c Support SIGHASH_RESERVED in SchnorrSigHashType::from_u8 (Dr Maxim Orlovsky)
Pull request description:
The newly introduced `PsbtSigHashType` uses very different serde formatting from previously used `EcdsaSigHashType`; for instance it does not output human-readable sighash. This is especially obvious when printing out PSBT as JSON/YAML object and is a breaking change from the `0.27`. Serde human-readable implementation requires `Display/FromStr`, which were also absent.
ACKs for top commit:
sanket1729:
ACK 992857ad0a. This is much better
apoelstra:
ACK 992857ad0a
Tree-SHA512: 71a46471f34b5481e4c1273a66846f59d61bfd98fcb65e7823ca216ff0dd419d81ca86d99c7aaf674fcfe2b1c010e899c8e74328f60a1e809015c663c453cc89
51fef76129 feat: Add Address.is_related_to_pubkey() (Andrew Ahlers)
Pull request description:
## Motivation
This is addressing the second half of this comment: https://github.com/rust-bitcoin/rust-bitcoin/pull/684#issuecomment-1012136845
> but would accept a PR (or two PRs) that returns Result<bool, UnsupportedAddress> and a method to check if a PublicKey is associated with an address.
(The first half was addressed [here](https://github.com/rust-bitcoin/rust-bitcoin/pull/819))
These changes will help build out and improve message signature verification. We don't necessarily need to add it to this crate but it allows for easy verification with something such as:
1. recovering a pubkey
2. checking if that pubkey relates to the given address
## Possible Improvements
- There is likely a better name than `is_related_to_secp256k1_key()`
- This could drop the `secp256k1` part of the name and take in a Pubkey enum that also supports Schnorr pubkeys and then this could be used for taproot addresses as well. This felt like a much larger change that will likely get turned down. Verifying taproot is simple enough and if absolutely desired, similar functions can be added for schnorr keys (tweaked and untweaked)
ACKs for top commit:
Kixunil:
ACK 51fef76129 for merging after TR
apoelstra:
ACK 51fef76129
Tree-SHA512: c9ab8c0f101fb4c647713e7f500656617025d8741676e8eb8a3132009dde9937d50cf9ac3d8055feb14452324a292397e46639cbaca71cac77af4b06dc42d09d
208eb65f1b Make NodeInfo API public (sanket1729)
Pull request description:
Reported by @shesek. Users might find it convenient to manually construct the tree using `NodeInfo` API
```rust
let leaf1 = NodeInfo::from_leaf_with_ver();
let leaf2 = NodeInfo::from_leaf_with_ver();
let root = NodeInfo::combine(leaf1, leaf2);
let spend_info = TaprootSpendInfo::from_node_info(&secp, internal_key, root);
```
ACKs for top commit:
dr-orlovsky:
ACK 208eb65f1b
apoelstra:
ACK 208eb65f1b
Tree-SHA512: b5a6b26e0d4a637f7ad6e987976b31b00d3567feca85f1a0bf63aa03603aded0ddae6578b1cabc1056870a596b8cb1a83e4ef3f45802e03da80c3d58d9bab1f1
e27f8ff594 TapTree iterator implementation (Dr Maxim Orlovsky)
Pull request description:
Implemented after @sanket1729 suggestion in https://github.com/rust-bitcoin/rust-bitcoin/issues/895#issuecomment-1074366108
Iterates all scripts present in TapTree in DFS order returning `(depth, script)` pairs.
I propose to have it as an RC fix since this functionality is really lacking and may be required for many wallets working with Taproot PSBT even outside of the scope where I originally needed it (OP_RETURN tweaks for TapTree described in #895)
ACKs for top commit:
sanket1729:
utACK e27f8ff594.
apoelstra:
ACK e27f8ff594
Tree-SHA512: b398e468a10534561297f22dba47e340391069734a41999edd85d726890752035053690a22014402879ea40b948160f00310f78771443d382c0bbaf0201dfbe5
It is possible, although not immediately obvious, that it is possible to
create a `PsbtSigHashType` with a non-standard value.
Add a unit test to show this and also catch any regressions if we
accidental change this logic.
Improve the `PsbtSigHashType` conversion methods by doing:
- Re-name `inner` -> `to_u32` as per Rust convention
- Add `from_u32` method
Note, we explicitly do _not_ use suffix 'consensus' because these
conversion methods make no guarantees about the validity of the
underlying `u32`.
The functions `from_u32_standard` and `from_u32_consensus` smell a bit
like hungarian notation. We can look at the method definition to see
that the methods accept `u32` arguments without mentioning that in the
method names.
Remove `_u32_` from the method names. This brings the `from_*` methods
in line with the `to_standard` method also.
Rust naming conventions stipulate that conversion methods from owned ->
owned for `Copy` types use the naming convention `to_`.
This change makes the function name objectively better, however it makes
no claims of being the 'best' name. We have had much discussion on using
`to_standard` vs `to_u32` but are unable to reach consensus.
35b682d495 Implement Display/FromStr for SchnorrSigHashType (Tobin Harding)
46c4164d67 Improve SigHashTypeParseError field (Tobin Harding)
c009210d4c Use full path for String in macro (Tobin Harding)
Pull request description:
Implement Display/FromStr for SchnorrSigHashType
We currently implement `Display` and `FromStr` on `EcdsaSigHashType` and use them in the `serde_string_impl` macro to implement ser/de.
Mirror this logic in `SchnorrSigHashType`.
Patch 1 and 2 are preparatory patches for patch 3.
## Notes to reviewers
This PR has some conflicts with https://github.com/rust-bitcoin/rust-bitcoin/pull/898 but is pushing in the same direction, I'm happy to let 898 go in first and rebase on top.
ACKs for top commit:
sanket1729:
ACK 35b682d495. Thanks, much easier to review now that the diff is small
dr-orlovsky:
ACK 35b682d495
Tree-SHA512: 481f192a3064ff39acf8904737dfb25b54ef128a37e0ca765ebb39138edac772d4f01ed10aa98ff185a8ed5668d64fa5d5957206b920ffe87950cafcf5a3b516
63e36fe6b4 Remove impl_index_newtype macro (Tobin Harding)
Pull request description:
This macro is no longer needed since we bumped MSRV to 1.29.
~We can implement `SliceIndex` to get the `Index` implementations.~
We can implement `core::ops::Index` directly since all the inner types implement `Index` already.
Original ~Idea shamelessly stolen from @elichai [in this comment](https://github.com/rust-bitcoin/rust-bitcoin/issues/352#issuecomment-560331856).~
New idea proposed by @Kixunil during review below. Thanks.
ACKs for top commit:
apoelstra:
ACK 63e36fe6b4
dr-orlovsky:
utACK 63e36fe6b4
sanket1729:
ACK 63e36fe6b4
Tree-SHA512: f7b4555c7fd9a2d458dcd53ec8caece0d12f3af77a10e850f35201bd7a580ba8fd7cb1d47a7f78ba6582e777dffa13416916ecacac6e0e874bdbb1c866132dc2
We currently implement `Display` and `FromStr` on `EcdsaSigHashType` and
use them in the `serde_string_impl` macro to implement ser/de.
Mirror this logic in `SchnorrSigHashType`.
The exact code formatting we use is not as important as uniformity.
Since we do not use tooling to control the formatting we have to be
vigilant ourselves. Recently I (Tobin) changed the way default type
parameters were formatted (arbitrarily but uniformly). Turns out I
picked the wrong way, there is already a convention as shown in the rust
documentation online (e.g. [1]).
Use 'conventional' spacing for default type parameters. Make the change
across the whole repository, found using
git grep '\<.* = .*\>'
[1] - https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
This macro is no longer needed since we bumped MSRV to 1.29.
We can implement `core::ops::Index` directly since all the inner types
implement `Index` already.
This significatnly refactors the amount formatting code to make
formatting more configurable. The main addition is the
`amount::Display` type which is a builder that can configure
denomination or other things (possibly more in the future).
Further, this makes all representations of numbers minimal by default,
so should be documented as a possibly-breaking change.
Because of the effort to support all other `fmt::Formatter` options this
required practically complete rewrite of `fmt_satoshi_in`. As a
byproduct I took the opportunity of removing one allocation from there.
Closes#709
In this library we specifically do not use rustfmt and tend to favour
terse statements that do not use extra lines unnecessarily. In order to
help new devs understand the style modify code that seems to use an
unnecessary number of lines.
None of these changes should reduce the readability of the code.
This function uses neither "Block" nor "Visual" style (as defined by
`rustfmt`). This is unusual, code that is regular is less jarring to
read. We tent to use "Block" style for functions so elect to do that
here.
Our usage of `where` statements is not uniform, nor is it inline with
the typical layout suggested by `rustfmt`.
Make an effort to be more uniform with usage of `where` statements.
However, explicitly do _not_ do every usage since sometimes our usage
favours terseness (all on a single line).
We have a few instances of strange indentation:
- Incorrect number of characters
- Usage of neither "Block" style or "View" style (elect to use "Block")
Do various whitespace refactorings, of note:
- Use space around equals e.g., 'since = "blah"'
- Put return/break/continue on separate line
Whitespace only, no logic changes.
7f33fe6a9b Delete contract hash module (Tobin Harding)
Pull request description:
This module has been deprecated in commit 1ffdce9 in August 2020, it is safe to delete it now.
Fixes: #322
ACKs for top commit:
apoelstra:
ACK 7f33fe6a9b
Kixunil:
ACK 7f33fe6a9b
dr-orlovsky:
ACK 7f33fe6a9b
Tree-SHA512: f218c8b0c09b14cd885cd7cf03c0a4623e5ead785decbc62a2f9610d438d5ea3efd2e2b47172a7608e33714996efa121707583d4257fa683dbfc9717988ceda6
e391ce9939 test: Add a test for incorrect message signature (Andrew Ahlers)
Pull request description:
In response to this comment: https://github.com/rust-bitcoin/rust-bitcoin/pull/819#discussion_r801477961
This should be straightforward. Let me know if there are any style issues. I tried to keep things similar to the existing test while cutting out any extra cruft to keep things small.
ACKs for top commit:
apoelstra:
ACK e391ce9939
Kixunil:
ACK e391ce9939
dr-orlovsky:
ACK e391ce9939
Tree-SHA512: 47296a7e0b2f45d5e50f507727ae4360686730a386f37dedfd1360b8cdf4b9dd3ce3bb5d05ea630177379ce4109059b6924fa362396b984ebab0ed1754318627
Update our `rust-secp256k1` dependency to the latest version.
Requires doing:
- Add a new variant to `Error` for the case where parity of the internal
key is an invalid value (not 0 or 1).
- Use non-deprecated const
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.
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
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
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
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.
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
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
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
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.
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
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.
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.
Tobin Harding
Dr. Maxim Orlovsky
Andrew Poelstra
sanket1729
Jeremy Rubin
Steven Roose
Martin Habovstiak
Andrew Ahlers
Rishabh Singhal
Noah Lanson
Riccardo Casatta
wim-web
KaFai Choi
Alekos Filini
z8674558
Marko Bencun
Noah