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
sanket1729
Riccardo Casatta
Tobin Harding
Dr. Maxim Orlovsky
Alekos Filini
Martin Habovstiak
z8674558
Marko Bencun
Jeremy Rubin
Noah
Leonardo Comandini
Noah Lanson