Rational: the expected weight is fixed so this both ensures we don't
accidentally change it somehow, and makes it easier to re-use these test
cases in other codebases (eg python-bitcoinlib).
Seems we no longer need an explicit error handler, remove it.
I did not grok the reason (long thread link below) but just removed it
and checked that the embedded crates still ran correctly.
https://github.com/rust-lang/rust/issues/51540)
a54e1ceab1 Apply rustfmt (The rustfmt Tyranny)
38d11ce3da ci: Make release CI search for NEXT.RELEASE instead (Steven Roose)
dad3abd20f transaction: Rename is_coin_base to is_coinbase (Steven Roose)
Pull request description:
Alternative to https://github.com/rust-bitcoin/rust-bitcoin/pull/1795.
Keep the old method as deprecated and add doc alias. Also change internal usage of the method.
ACKs for top commit:
tcharding:
ACK a54e1ceab1
apoelstra:
ACK a54e1ceab1
Tree-SHA512: 52d9729bf83da164556d960f8867cb836ff57a0f619da3dd3620efffb28a974aac23b8085863ab0e072a4bdb2f13ac576efa43ad2eec9a271ad044227f4d00a4
6c61e1019e Fix pinning (schemars and MSRV) (Tobin C. Harding)
c8e38d6a5a hashes: Implement JsonSchema for sha256t::Hash<T> (Tobin C. Harding)
Pull request description:
This has grown due to now including pinning work also done in https://github.com/rust-bitcoin/rust-bitcoin/pull/1736, I decided to do this because the PRs conflict and doing it all here saves accidentally getting out of sync. And https://github.com/rust-bitcoin/rust-bitcoin/pull/1764 requires this PR.
- Patch 1 is unchanged
- Patch 2 now fixes pinning in bitcoin and hashes CI scripts and in the docs of both as well as the manifest stuff relating to `schemars` - phew.
Fix: #1687
ACKs for top commit:
Kixunil:
ACK 6c61e1019e
apoelstra:
ACK 6c61e1019e
Tree-SHA512: eae4aa9700817bab6ad444e07709e8b1a4ffb1625e08be6ba399abde38bf6f8e5ea216a0836e2e26dfaddc76c392802cd016738ea6e753a1bca2584d9d2a9796
Done as is single patch to make sure all the docs and CI are in sync and
correct.
We currently pin the `schemars` dependency using `<=0.8.3` as well as a
the `dyn-clone` transient dependency in the manifest (`hashes` and the
extended test crate). This is incorrect because it makes usage of the
crate klunky (or possibly impossible) if downstream users wish to use a
later version of `schemars`.
Observe also that we do not have to pin `schemars`, we do however have to pin
the `serde` crate if either `serde` or `schemars` features are enabled.
Do so in CI and document in the readme file within hashes.
Currently we have a pin remaining from the old MSRV (`syn` due to use
of `matches!`).
Fix pinning by:
- Remove pin in manifest for `schemars`
- Fix pinning for MSRV in CI and docs (this includes documenting pinning
requirements for `schemars` feature because it is related to the other
pin of `serde`) in both `hashes` readme and main repo readme.
8f6317fbab Add predict_weight test for witness address types (yancy)
Pull request description:
Add a predict_weight test for address types with witness data
ACKs for top commit:
Kixunil:
ACK 8f6317fbab
apoelstra:
ACK 8f6317fbab
Tree-SHA512: 954908f068d6b0e8f7cc6bc6bffd110d490d7165cf2544ff0f936264761cb1f8e4da1e37fd5a6a34fd8b05f8d72817a3b340bbf968b29c9f538f10853347fdf0
fabcde036f Use package in manifest and shorten import (Tobin C. Harding)
Pull request description:
We can use `package` to rename `bitcoin_hashes` to `hashes` and `bitcoin_internals` to `internals`. This makes imports more terse with no loss of meaning.
ACKs for top commit:
apoelstra:
ACK fabcde036f
Kixunil:
ACK fabcde036f
Tree-SHA512: bc5bff6f7f6bf3b68ba1e0644a83da014081d8c6c9d578c21cb54fdd56a018f68733dd1135d05b590ba193ed9efd12fa9019182c1fed347e604d8548f6ef9103
29cb34eed7 Refactor Address struct and its methods (Harshil Jani)
Pull request description:
Closes#1755
In this PR the `as_unchecked` is added to the Address struct, which returns a reference to the same address but with the type Address<NetworkUnchecked>. Similarly, the `assume_checked_ref` is added to Address<NetworkUnchecked>, which returns a reference to the same address but with the type Address.
ACKs for top commit:
tcharding:
ACK 29cb34eed7
apoelstra:
ACK 29cb34eed7
Tree-SHA512: 75ba40883d9fb31026b0e94e8d3fdcf808ff38a4d10f61f99a1e14ddc48358da86bad44cd78563dc67aa5d39382a5493e73c03891317f361f590e39b6257cb84
91f45a214f Replace hardcoded values with compile-time hashing (Martin Habovstiak)
095b7958dd Make `sha256t_hash_newtype!` evocative of the output. (Martin Habovstiak)
Pull request description:
The Rust API guidelines state that macros should be evocative of the
output, which is a sensible recommendation. We already had this for
`hash_newtype!` macro but didn't for sha256t version.
This changes the macro to have this syntax:
```rust
sha256t_hash_newtype! {
// Order of these structs is fixed.
/// Optional documentation details here. Summary is auto-generated.
/*pub*/ struct Tag = raw(MIDSTATE_BYTES, LEN);
/// Documentation here
#[hash_newtype(forward)] // optional, default is backward
/*pub*/ struct HashType(/* attributes allowed here */ _);
}
```
Closes https://github.com/rust-bitcoin/rust-bitcoin/issues/1427
Depends on #1769
How do you like the syntax? Is weird `struct Foo = bar(..);` acceptable?
ACKs for top commit:
tcharding:
ACK 91f45a214f
apoelstra:
ACK 91f45a214f
Tree-SHA512: f6b555b20311a4c1cb097bc296c92459f6fbe16ba102d8c977eb2383dbcae3cc8ffce32e3cb771e7e22197fda26379971aa4feaadc5e2e70d37fa690ae8b8859
873501a85f Use slice patterns (Martin Habovstiak)
Pull request description:
Some code looks better with slice patterns. This changes `bip32` to use them.
ACKs for top commit:
apoelstra:
ACK 873501a85f
tcharding:
ACK 873501a85f
Tree-SHA512: 2e91b21d0f943c04f592112f241444ba3b1b1dad47ea6bea162f62cfbcaf98e383257cad144072b3807aaa0c122dabab638ac10b1cc1f5179727a5f05aac0659
This commit refactors the Address struct and its methods to improve
its functionality and usability.The AddressInner struct now holds
the payload and network, and the PhantomData<V> type is used to track
the network validation state.
Also as_unchecked and assume_checked_red methods are added to allow
conversion between checked and unchecked network validation state.
Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>
Previous changes enabled passing the string used as a tag into
`sha256t_hash_newtype!` macro rather than hard-coding midstate. This
commit takes advantage of it and replaces the hard-coded values with
compile-time executed (`const`) hashing.
The Rust API guidelines state that macros should be evocative of the
output, which is a sensible recommendation. We already had this for
`hash_newtype!` macro but didn't for sha256t version.
This changes the macro to have this syntax:
```rust
sha256t_hash_newtype! {
// Order of these structs is fixed.
/// Optional documentation details here. Summary is auto-generated.
/*pub*/ struct Tag = raw(MIDSTATE_BYTES, LEN);
/// Documentation here
#[hash_newtype(forward)] // optional, default is backward
/*pub*/ struct HashType(/* attributes allowed here */ _);
}
```
Closes#1427
We've upgraded MSRV but didn't update clippy config, so some things that
could be improved aren't caught by clippy. This updates the config and
fixes the new issues.
I also `rg '1\.41\.1'`ed for interesting changes and found one
additional improvement.
a189942c64 Use doc_auto_cfg (Tobin C. Harding)
Pull request description:
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of `#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.
Sweeeeeet.
Props to pezcore for the lesson :)
ACKs for top commit:
apoelstra:
ACK a189942c64
Kixunil:
ACK a189942c64
Tree-SHA512: 1ced1e09f5d1733b362b83ca650d3f52c89eb57e78e8437f74c496d89776548f8c50feab6750352342e2abe680434681de2c126ce36a81dda21397b9695d4d4e
Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>
implementation of PartialEq<Address> for Address<NetworkUnchecked>
Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of
`#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually
mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.
Sweeeeeet.
ed80df5ebc Add `ChainHash::from_genesis_block_hash` (Martin Habovstiak)
Pull request description:
This improves readability of converting `BlockHash` into `ChainHash`. It's useful in e.g. Electrum protocol which sends `BlockHash` (serialized backward).
Closes#1751
ACKs for top commit:
apoelstra:
ACK ed80df5ebc
tcharding:
ACK ed80df5ebc
Tree-SHA512: b03eb9ca0a1eb38196cc8e43751c2def0d00b463b774003c5319bf56599db598ddd4bb0e2ad0bd3803f21a925135992d421848e7b1ba5e7bab3a405f60fc973c
We can use `package` to rename `bitcoin_hashes` to `hashes` and
`bitcoin_internals` to `internals`. This makes imports more terse with
no loss of meaning.
82b6332b91 create a set of recognized denomination forms (yancy)
Pull request description:
I took a stab at restricting the acceptable forms here. There was some consensus that "BtC" was confusing that was discussed in a previous pr https://github.com/rust-bitcoin/rust-bitcoin/pull/1715.
Also, personally I felt that the `PossiblyConfusingDenomination` enum variant was itself confusing. I think it's probably cleaner to just maintain a list of acceptable forms and treat everything else as unknown. For now I just created a const of possibly confusing forms.
ACKs for top commit:
Kixunil:
ACK 82b6332b91
apoelstra:
ACK 82b6332b91
Tree-SHA512: f3c212046e4d8a34ce2d50f34065b1778ae190ca80dd9ad73ef41eb5925705c88c0a764ec149d731eb1735d670c5542415953633a7f3f672d0c1e46b6e004d0b
This improves readability of converting `BlockHash` into `ChainHash`.
It's useful in e.g. Electrum protocol which sends `BlockHash`
(serialized backward).
Closes#1751
122188f7dd Use shorter import statements (Tobin C. Harding)
Pull request description:
Just patch 2, patch 1 is #1728
From the commit log of patch 2
Use shorter import statements
As per discussion [0] use the shorter form for importing crates that we
re-export (`hashes` and `secp256k1`).
[0] https://github.com/rust-bitcoin/rust-bitcoin/discussions/1661
ACKs for top commit:
apoelstra:
ACK 122188f7dd
sanket1729:
utACK 122188f7dd
Tree-SHA512: 3f540464d38c72ba9d68f8ceda8600540bd0c3eef0ba67531c87fa1e0e4f757af7035cf80a1a5f17aa05604a17fdd9ef59bb6bece6b4145d540dac1e5362fc01
Add the `mutate` attribute to mutate `mul_u64`. Add non-doc comments
listing the two false positives. These are identical but we list them
twice so when devs grep for `mutagen false pos` the same number of lines
for each function is displayed as is displayed by the `mutagen` run.
This coding false positives thing is also introduced in PR #1655.
1dc04fe10f Remove rust_v_1_46 (Tobin C. Harding)
71fa9e81e7 Bump MSRV to 1.48.1 (Tobin C. Harding)
Pull request description:
Just patch 2, patch 1 is #1728
From the commit log of patch 2
Bump MSRV to 1.48.1
As per discussion [0] bump our MSRV for all crates in `rust-bitcoin`
repo to 1.48.1 [1].
[0] https://github.com/rust-bitcoin/rust-bitcoin/discussions/1329
[1] https://blog.rust-lang.org/2020/11/19/Rust-1.48.html
ACKs for top commit:
apoelstra:
ACK 1dc04fe10f
sanket1729:
ACK 1dc04fe10f
Tree-SHA512: 533470c55f7aeede88382db8245033dac9317d75b38ced2ad8256d38319632a524335f893044e96300a1e60048f9aaca2d1634735eb324eb8ed9ad3c9ab2f872
dbd2ea07b5 Add kilo weight unit conversion (yancy)
Pull request description:
The FeeRate module defaults to sats per `kwu` so when doing fee calculations, it would be convenient to easily convert weight to the same units.
ACKs for top commit:
Kixunil:
ACK dbd2ea07b5
tcharding:
ACK dbd2ea07b5
apoelstra:
ACK dbd2ea07b5
Tree-SHA512: fe26b631cf474f1dca627a4d21e9052c80f8ada9a0eb635c46b7f8f42095671b9b161fb5012b723699008372faf7668507d5e92bde7d1808d389f85dba33529b
We just merged a patch to enable formatting in CI but commit: `05fdead2
Feature: Add difficulty_float method for block::Header.` must have
slipped in.
Run the formatter.
913575ac91 hashes: Run the formatter (Tobin C. Harding)
52c4579057 Enable formatting for hashes (Tobin C. Harding)
3f16b6bf9f util: Run the formatter (Tobin C. Harding)
d210d2ac83 Enable formatting for util (Tobin C. Harding)
5973dce9db blockdata: Run the formatter (Tobin C. Harding)
0dcbed3c7b Enable formatting for blockdata (Tobin C. Harding)
a52746d01c psbt: Run the formatter (Tobin C. Harding)
ef306db5e2 Enable formatting for psbt (Tobin C. Harding)
296f2ed82c Make test panic instead of using code comment (Tobin C. Harding)
3ec8a12428 crypto: Run the formatter (Tobin C. Harding)
c8a3c58786 Enable formatting for crypto (Tobin C. Harding)
314e6786b4 crypto: Add rustfmt::skip attributes (Tobin C. Harding)
450a84f6e8 consensus: Run the formatter (Tobin C. Harding)
89143205f9 Enable formatting for consensus (Tobin C. Harding)
ce773af20f tests: Remove useless use of super imports (Tobin C. Harding)
ef01f4d0f6 consensus: Introduce local variables (Tobin C. Harding)
Pull request description:
One final push crew, 16 patches, only a few are big.
All non-trivial formatting is done in separate patches so the changes can be verified mechanically.
With this applied the whole `rust-bitcoin` crate will be formatted.
Big thanks to everyone for putting up with the ongoing formatting PRs, no-one likes doing these but hopefully this an improvement to the project - especially in helping us get more contributors to the project.
ACKs for top commit:
tcharding:
> ACK [913575a](913575ac91). Went through the workflow locally.
sanket1729:
ACK 913575ac91. Went through the workflow locally.
apoelstra:
ACK 913575ac91
Tree-SHA512: b30eaa2893563155de05f8fa97be4a24a7dd8bf43bb426314c5104598477ca2173af279da796da8b18cc53a0ed525908b3d4edd0504836a443465efa0773632d
67618d679d Mark `Denomination` as `non_exhaustive` (Martin Habovstiak)
Pull request description:
It is possible that we will add new variants to `Denomination` in the future so making it `non_exhaustive` is better for forward compatibility.
ACKs for top commit:
tcharding:
ACK 67618d679d
apoelstra:
ACK 67618d679d
sanket1729:
ACK 67618d679d
Tree-SHA512: a28b7c6577f098b3d64c505e948e3b9fd0cc43a911a2b7c35610ada6a2f75514a1cb05ab8c99212340cf4e34426ac8b5f0ecc65d5afa22d67296e82d878b9308
2d23e11569 Remove extern crate hashbrown (Tobin C. Harding)
Pull request description:
(Merge candidate only after release of 0.30.0)
We no longer have a "hashbrown" feature, the feature gated `pub extern crate hashbrown` should have been removed when we removed the feature.
ACKs for top commit:
apoelstra:
ACK 2d23e11569
Kixunil:
ACK 2d23e11569
Tree-SHA512: 36d4c539f3f972d42bbc9fda9e96063a78d35afc6aebbf534b878dcef0440b72c2a990bcbdbb2ad3bf99cab7048cdbd4002899c2b314da21e4a7bacaf8c71f0f
Add `rustfmt::skip` attribute in a couple of places and then remove the
exclude for the `blockdata` module. Do not run the formatter, that will
be done as a separate patch to aid review.
Currently we have a code comment that is supposed to assist devs in
maintaining the `network::constants::Network` type by failing to build
if a new variant is added. This plays havoc with the formatter because
the comment is hanging at the bottom of a match block and the formatting
thinks its for the proceeding line of code.
Instead of using a code comment add a panic so the unit test fails if a
new variant is added to `network::constants::Network`.
In preparation for running the formatter introduce a couple of local
variables to reduce the line length and inhibit function call from being
split over multiple lines.
Refactor only, no logic changes.
bef7992ce5 Update readme to mention pin for 1.47 (Tobin C. Harding)
58033cf14e pin serde dep on 1.47 (Tobin C. Harding)
f5f4a33fa9 pin serde dep on 1.41 (Andrew Poelstra)
ee9b297e98 ci: update dupe check to whitelist syn (Andrew Poelstra)
6aa640ff8d update rust-secp to 0.27.0 (Andrew Poelstra)
Pull request description:
Also remove the spurious dev-dependency copy of rust-secp, which should've been updated to remove the "recovery" feature in https://www.github.com/rust-bitcoin/rust-bitcoin/pull/545 and then been removed entirely in https://www.github.com/rust-bitcoin/rust-bitcoin/pull/1387
ACKs for top commit:
Kixunil:
ACK bef7992ce5
tcharding:
ACK bef7992ce5
Tree-SHA512: 7d1bf062dc6920bcafa85311b4b5ed348e31fb20aa21156e545bf7a4cc4c194e2b6dcfd0d3c6db3df337e4e2cd0f7b74f41ea421549521f07080feb88fbd9382
00b46d6d9d Indent functions (Martin Habovstiak)
d56d202aeb Support weight prediction in `const` context (Martin Habovstiak)
Pull request description:
**Notes for reviewers:**
This is something that I want to use in my code and hopefully reasonably easy to review, so if this can get into 0.30 that'd be really nice. No hard feelings if it doesn't.
I tried to put extra effort into making review easier by:
* intentionally "mis-formatting" the first commit so diff is smaller and easy to understand - see individual commits.
* copying patterns from non-const fn to const fn so it's obviously correct (includes same variable names)
* not bothering with the array trick in `VarInt::len` and simply accepting the limitation of Rust 1.46+ (I use 1.48 BTW).
**Description**
Some smart contracts or simplified wallets statically know the sizes of
transactions or inputs. The possible approaches to handling them so far
were re-computing the values (and hoping the optimizer will const fold
them) or using a simple constant which may be harder to understand and
get right. It's much nicer to just use a `const` but our code didn't
support it until now.
This change adds methods that can compute the prediction in `const`
context for Rust versions >= 1.46.0 which allow use of loops (and
conditions but those could be workaround anyway).
As a side effect of this, the change also adds `const` to `VarInt::len`
in Rust 1.46+. While this one could be made unconditional using array
trick it's probably not worth it because of the planned MSRV bump.
ACKs for top commit:
apoelstra:
ACK 00b46d6d9d
tcharding:
ACK 00b46d6d9d
Tree-SHA512: 5509886a68b4de5227db0e28d92a40be8de64592e0b189c519213db21bcfe98ca03d9a1936b1024729b97db69e8ec0b55fac870a7ce9bab0d0c9a47b2087990f
Some smart contracts or simplified wallets statically know the sizes of
transactions or inputs. The possible approaches to handling them so far
were re-computing the values (and hoping the optimizer will const fold
them) or using a simple constant which may be harder to understand and
get right. It's much nicer to just use a `const` but our code didn't
support it until now.
This change adds methods that can compute the prediction in `const`
context for Rust versions >= 1.46.0 which allow use of loops (and
conditions but those could be workaround anyway).
As a side effect of this, the change also adds `const` to `VarInt::len`
in Rust 1.46+. While this one could be made unconditional using array
trick it's probably not worth it because of the planned MSRV bump.
Note: this commit is intentionally unformatted to make diff easier to
understand. Formatting will be done in future commit.
It wasn't obvious that displaying address with alternate formatting
upper cases bech32 addresses.
This change adds information about this and also a note about the
compatibility of various wallets.
2158f88f1d Add a method to `pow::Target` for returning difficulty as an f64. (junderw)
Pull request description:
Closes#1703
This adds a conversion function to U256 to get an f64. We use the method shown in the following blog post.
https://blog.m-ou.se/floats/
Target::MAX was converted to a f64 and set as a const that is verified in a unit test.
The code is rather confusing, so I took a crack at explaining it in my comments as well. Please let me know if you want it cleaned up some more.
ACKs for top commit:
apoelstra:
ACK 2158f88f1d
tcharding:
ACK 2158f88f1d
Tree-SHA512: 7c0e82bd1756950c1c6dfb9da91fd71b276e2e7dc8a33e69112f87b87e358240f0f7c4894d24ab228e149d862938833a2e65e345ce2712a78dc86dec197da34f
This adds a conversion function to U256 to get an f64. We use the method shown in the following blog post. https://blog.m-ou.se/floats/
Target::MAX was converted to a f64 and set as a const that is verified in a unit test.
bfd401c96e bitcoin_hashes: add CHANgELOG (Andrew Poelstra)
d1b7b54e3a bump bitcoin-hashes version to 0.12 (Andrew Poelstra)
Pull request description:
It was a little tricky to bump the version number because of https://github.com/rust-bitcoin/rust-bitcoin/issues/1553. There are a couple other things I considered trying, which maybe we'll do for future releases, but I believe this works for now.
Maybe should wait for #1111.
ACKs for top commit:
tcharding:
ACK bfd401c96e
sanket1729:
utACK bfd401c96e. ChangeLog looks good to me, did not review whether all noteworthy changes were included.
Tree-SHA512: d2104fc93e364415ae955e8123e6087c1eaa4c955aeaf4647ead051a223563326f66c0e278d68f64335e22c9d0af9b296dc3b744cd9d82d206844461fe7bf9c9
e3f95ee22b Add tests for the FeeRate type (yancy)
Pull request description:
Adds some tests for the `FeeRate` type.
ACKs for top commit:
apoelstra:
ACK e3f95ee22b
tcharding:
ACK e3f95ee22b
Kixunil:
ACK e3f95ee22b
Tree-SHA512: 74d6597c747d5aa62a6510bf9fa8de971f89ad56f571aadd496f6487e80cc88bb2b5a1c6bcfed825d09d18ca2310b2bfd6fdbe330f2760369d167a653d26bef8
3eb648df01 Add constants to `InputWeightPrediction` (Martin Habovstiak)
Pull request description:
There are several common spends in Bitcoin that have known input weight predictions. It can be useful to have these as constants, so this change adds them. However, this only adds native segwit ones as the others are slowly fading away and might clutter the API.
If anyone wants other constants, please write them for me, their value is not that great to me so I'm not motivated to figure out the correct numbers. :)
This would be nice to add to 0.30 since it's small and easy but not critical.
ACKs for top commit:
apoelstra:
ACK 3eb648df01
tcharding:
ACK 3eb648df01
sanket1729:
ACK 3eb648df01
Tree-SHA512: 51d2cd2ecef7e6b79f9d4b52319e34b908fe8b5a337551dc2088994feeafc9c3ca87884c3db8369f8bd002947d6d14b373f08ef1419db282713206ed6f1b309a
There are several common spends in Bitcoin that have known input weight
predictions. It can be useful to have these as constants, so this change
adds them. However, this only adds native segwit ones as the others are
slowly fading away and might clutter the API.
Enable formatting in CI by doing:
- Add a section to the `test.sh` scripts to run the formatter (guarded by
the env variable `DO_FMT`) for all crates (bitcoin, hashes, internals).
- Add `DO_FMT` to the nightly `Tests` CI job.
Various formatting issues have crept into the codebase because we do not
run the formatter in CI.
In preparation for enabling formatting checks in CI run `cargo +nightly
fmt` to fix current formatting issues. No changes other than those
create by the formatter.
Because we have rust-secp in the loop, we need to update rust-secp, push
a new tag, and use that here, to ensure that the direct dependency on
bitcoin_hashes, and the rust-secp version, are compatible.
74022baa44 Rename ScriptLeaf to LeafNode (sanket1729)
289dc1e7f5 Remove serde for taprootspendinfo (sanket1729)
a397ab0c19 Remove serde for ScriptLeaf (sanket1729)
9affda3012 Introduce Hidden leaves in ScriptLeaves (sanket1729)
22bc39a143 Fix serde for TaprootMerkleBranch (sanket1729)
38ed9bdf49 MOVE ONLY: Move TapTree to taproot module (sanket1729)
Pull request description:
This PR changes/removes the serde implementation for the following types
- TaprootSpendInfo: Removed. This data structure contains derived information for taproot spending that cannot be validated easily. To elaborate, `TaprootSpendInfo` is constructed from a tree, but loses information about the tree structure and maintains handy information like `script_control_block_map`, cached tweaked key, Merkle root etc.
- TaprootBuillder: Removed. Hard to implement and not very useful.
- TapTree: Modified to check invariants.
- NodeInfo: Now implements serde with support for Hidden nodes
- ScriptLeaf: Removed serde. Users should not directly construct this. This is just an output iterator item of `TapTree::script_leaves()`
Data structure changes:
- Introduced `LeafNode`: Supports Hidden leaves. Has serde implemented
- Cleanly separate `TapTree` and `NodeInfo`: `TapTree` is a full BIP370 compatible tree with no hidden nodes. `NodeInfo` is a tree that can potentially contain hidden leaves.
- Added `NodeInfo::leaf_nodes`: Iterator that iterates over known and hidden leaves of `NodeInfo`.
- Updated `TapTree::script_leaves`: Iterator that is guaranteed to output known leaves.
ACKs for top commit:
tcharding:
ACK 74022baa44
apoelstra:
ACK 74022baa44
Tree-SHA512: 919ea5bf60dc0cd8431301c1b744b046d1d781b3bed302b83a600cfa644216b6c682752795d463646b2723ac8661879284f557862a67e4572fd965fcf3dc194d
b0b0cdb46c Improve the public API for Feerate and Weight (yancy)
Pull request description:
Small nit for https://github.com/rust-bitcoin/rust-bitcoin/pull/1627/ to re-export `Weight` and `FeeRate` to shorten the use path.
```
use bitcoin::Weight;
use bitcoin::FeeRate;
````
ACKs for top commit:
tcharding:
ACK b0b0cdb46c
Kixunil:
ACK b0b0cdb46c
apoelstra:
ACK b0b0cdb46c
Tree-SHA512: 81e508c980c4f37e3791d26616459608dd60e5a6255ef28b3a049c1d27281b2853b92474d42f10031254c8c46878adf666eb709826aa4ffde1b4b3542451e080
73e876ffd4 Include address in Error::NetworkValidation (Subhradeep Chakraborty)
Pull request description:
Fixes: #1677
## Change
In `bitcoin/src/address.rs`, a new field `address` is added to the enum variant `Error::NetworkValidation`. Also, the implementation of `Display` trait for `Error` is updated to print the `address` field.
However, to print the `address` through `Display`, either the reference is needed or `Address` and `Payload` both need to derive the `Copy` trait. Since I am little new to both the rust-bitcoin codebase and rust itself, I am confused about choosing between the two and have moved with the first one. Would appreciate any feedback on this.
ACKs for top commit:
Kixunil:
ACK 73e876ffd4
tcharding:
ACK 73e876ffd4
apoelstra:
ACK 73e876ffd4
Tree-SHA512: dd53b8648bccc8372c829e56817402fb02a9d51d1dffc854f24e68814bbefe7ea777f67aefb0d170762dbf6cdd50bd3ec55af325a1ffc21b1241d1df5531cd24
Implementing this for spendinfo is really complicated because it
contains some cached data without retaining the components that are used
to compute them.
Users should serde the 1) NodeInfo and 2) internal key and reconstruct
TaprootSpendInfo from it.
This was incorrect and not needed. Users should not be able to create
only tree leaves directly without going through the tree construction in
rust-bitcoin
Cleanly separate `TapTree` and `NodeInfo`. Fix serde not respecting
invariants for several data structures
Repurpose some tests from removed taproot builder for taptree
42b07586ac Improve the public API (Tobin C. Harding)
Pull request description:
We created the `crypto` crate as a container for cryptography modules with the idea that it may be split out into a separate crate. There is no reason for users of the lib to know about this module. Also, we have two `taproot` modules, one in `crypto` and one at the crate root, this makes for un-ergonomic usage of the lib.
Improve the public API by doing:
- Make the `crypto` module private (`pub(crate)`).
- Re-export `crypto::taproot::Signature` (and `Error`) from `crate::taproot`
Fix: #1668
ACKs for top commit:
apoelstra:
ACK 42b07586ac
Kixunil:
ACK 42b07586ac
Tree-SHA512: 5713b98b2a48d2776cdd6ca2c0e798d6e2df604d780e5182bd770fb2df807cf1f65bc24663ee6f7734fc1e0c80494c5a87dc60e44c83acefcffec7f059203a47
We created the `crypto` crate as a container for cryptography modules
with the idea that it may be split out into a separate crate. There is
no reason for users of the lib to know about this module. Also, we have
two `taproot` modules, one in `crypto` and one at the crate root, this
makes for un-ergonomic usage of the lib.
Improve the public API by doing:
- Make the `crypto` module private (`pub(crate)`).
- Re-export `crypto::taproot::Signature` (and `Error`) from
`crate::taproot`
7d1645aea0 Add constant for coinbase maturity (benthecarman)
Pull request description:
Not sure if this is the best place to put this but it is nice to have a constant for this instead of having other libraries make their own (ie https://github.com/lightningdevkit/rust-lightning/pull/1924#pullrequestreview-1222807626)
ACKs for top commit:
tcharding:
ACK 7d1645aea0
apoelstra:
ACK 7d1645aea0
Tree-SHA512: 5ac2a3359cadd303158c66ba45db8f4bf8cc80b6c19604262999ff361fd0bd98e2a4851c57da1962cb5c74f5789a85c8b3861f1742706a60ce1fbc57c3c200cc
56569b32ef Add utils to convert ChainHash to a Network (benthecarman)
Pull request description:
ACKs for top commit:
Kixunil:
ACK 56569b32ef
tcharding:
ACK 56569b32ef
Tree-SHA512: a489fcb1c1208db4271076d88288658988a63209e56e7433bde82d7d5719450433348fcc3cb6aae59ffa6ed8aff510d6b031c6899d5cf64c503a53b2d4c692b8
161273b209 Re-name hash inner/byte methods (Tobin C. Harding)
324b6f264b Use `into` for hash argument (Tobin C. Harding)
Pull request description:
Currently we have an associated type on hash types `Inner` with accompanying methods `into_inner`, `from_inner`, `as_inner`. Also, we provide a way to create new wrapped hash types. The use of 'inner' becomes ambiguous with the addition of wrapped types because the inner could be the inner hash type or the `Inner` byte array of the inner wrapped hash type.
In an effort to make the API more clear and uniform do the following:
- Rename `Inner` -> `Bytes`
- Rename `*_inner` -> `*_byte_array`
- Rename the inner hash to/from methods to `*_raw_hash`
Correct method prefix `into_` -> `to_` because theses methods convert owned `Copy` types.
Add the trait Bound `Copy` to the `Bytes` type because we rely on this trait bound for the conversion methods to be correctly named according to convention.
Because of the dependency hole created by `secp256k1` this patch changes the secp dependency to a git tag dependency that includes changes to the hashes calls required so that we can get green lights on CI in this repo.
Fix: #1554
ACKs for top commit:
Kixunil:
ACK 161273b209
apoelstra:
ACK 161273b209
Tree-SHA512: b51b851a1855e6a26a7ef8ccb9f554723d4cc39b368812703587a50e81e7ab49714a81696af0be743b947f09e1fca227a5331b6735912c5b0d5cd0178905f006
a4b5fb4002 Fix docs for UnknownMagic to be accurate (benthecarman)
Pull request description:
I assume the old docs are a copy-paste error, strings are not involved when this error is encountered.
ACKs for top commit:
Kixunil:
ACK a4b5fb4002
apoelstra:
ACK a4b5fb4002
Tree-SHA512: b2b71f81be8a0d979b15471e7262e01284443e05626b26a19236fd25581700d9e37409576a4b73d5bb537c49ae83a4b7d40f0888dff078b07bd7550026cd778a
76c4c647cf Reexport `Magic` (Martin Habovstiak)
Pull request description:
Writing `network::Magic` is more natural and less annoying than `network::constants::Magic`, so this change reexports it.
Closes#1667
ACKs for top commit:
apoelstra:
ACK 76c4c647cf
tcharding:
ACK 76c4c647cf
Tree-SHA512: 1d85372ecd9723c0871faa4552ba5f93a3c95d0b213fa8aace398949f97157cdfde164bbcb2c35bae4c61ae0185e3fbdb854714cde0849c1afaf90e4e8ec223f
Currently we have an associated type on hash types `Inner` with
accompanying methods `into_inner`, `from_inner`, `as_inner`. Also, we
provide a way to create new wrapped hash types. The use of 'inner'
becomes ambiguous with the addition of wrapped types because the inner
could be the inner hash type or the `Inner` byte array of the inner
wrapped hash type.
In an effort to make the API more clear and uniform do the following:
- Rename `Inner` -> `Bytes`
- Rename `*_inner` -> `*_byte_array`
- Rename the inner hash to/from methods to `*_raw_hash`
Correct method prefix `into_` -> `to_` because theses methods convert
owned `Copy` types.
Add the trait Bound `Copy` to the `Bytes` type because we rely on this
trait bound for the conversion methods to be correctly named according
to convention.
Because of the dependency hole created by `secp256k1` this patch changes
the secp dependency to a git tag dependency that includes changes to the
hashes calls required so that we can get green lights on CI in this
repo.
Hash types can be converted into a `Message` because `Message`
implements `From` for any type that implements `ThirtyTwoByteHash`,
which hash types do.
Use `into` to convert the hash argument to a message to sign.
090dad770f Improve string parsing (Tobin C. Harding)
Pull request description:
Currently we implement string parsing for height/time from the `absolute` module but not the `relative` module.
Improve the macros used to implement string parsing and use the new versions to implement string parsing for the height and time types in `relative`.
Done while reviewing data structures in relation to `serde`.
ACKs for top commit:
apoelstra:
ACK 090dad770f
Kixunil:
ACK 090dad770f
Tree-SHA512: bfa88efbaf5dc35755eb46df373a08e223f112860e8a65f58db9fdd77e2c01dc9377da735b33ef58940004fe5fe11690ac09be19591fded2c9fd04cd7d2bdf73
d71c31c235 Create Address::matches_script_pubkey method (hashmap)
Pull request description:
to check if an address creates a particular script without allocating.
fixesrust-bitcoin/rust-bitcoin#1604
ACKs for top commit:
Kixunil:
ACK d71c31c235
apoelstra:
ACK d71c31c235
Tree-SHA512: cb60a53ae2be7c47dcd27415c883a73c81d57cbbf0bc92eaf76243d79d9c8e2c2efe91bef65a7e67ed26fec376f11325709ff27025d054813b1907ea2bf4961c
06f1f027ab Make `hash_newtype` evocative of the output (Martin Habovstiak)
b018f3e90b Remove the `$len` argument from `hash_newtype` (Martin Habovstiak)
752817e20d Stop using `$len` in `hash_newtype` (Martin Habovstiak)
Pull request description:
The API guidelines say macro input should be evocative of the output.
`hash_newtype` didn't have this property.
This change makes it look exactly like the resulting struct, `$len`
parameter was removed since it's not needed, reversing is controlled
using an attribute. The macro is also better documented and ready to be
extended in the future.
The tagged SHA256 newtype is not yet modified because it has a more
complicated input parameters.
Closes#1648
ACKs for top commit:
apoelstra:
ACK 06f1f027ab
Tree-SHA512: 9762db1eca9cd749980e5d68ca286f6c926620295a602f62365f199c7b333334b976db25ba25c64e56403cd1ba046b21b99e1c73cc528ad95612ef8901f216e5
438ee45691 Show cache construction in rustdoc (Tobin C. Harding)
Pull request description:
To make it more clear what the cache is show the cache construction line in rustdoc.
ACKs for top commit:
apoelstra:
ACK 438ee45691
Kixunil:
ACK 438ee45691
Tree-SHA512: d6da6bad57fddf9e2f4bcfb7c9b87df38bf4b2bb914e92e52d5ae8afa3405a9793536d7164223021ab6d183ddde732cf6889370834e36f37bae470127b0271fa
2620f3b69d Put optional = true at the end (Tobin C. Harding)
d86ef3b01b Put non-optional dependency with other non-optional (Tobin C. Harding)
Pull request description:
Late stage, super anal, manifest cleanup.
- Patch 1: put non-optional dependencies together
- Patch 2: put `optional = true` at the end of the line
ACKs for top commit:
apoelstra:
ACK 2620f3b69d
Kixunil:
ACK 2620f3b69d
Tree-SHA512: bca2bd2eb9a50aea04cab7c6eab7c264e91937ff20473ccf4496ba380897d1d50696719aa3379bd3f29b8a3cbe43d5485ac068f0e2c126462f361fcfe9941548
The API guidelines say macro input should be evocative of the output.
`hash_newtype` didn't have this property.
This change makes it look exactly like the resulting struct, `$len`
parameter was removed since it's not needed, reversing is controlled
using an attribute. The macro is also better documented and ready to be
extended in the future.
The tagged SHA256 newtype is not yet modified because it has a more
complicated input parameters.
Closes#1648
It may not be obvious why the condition in `push_bytes` module checks
for negation of 16 and 32 bit architectures rather than 64 bit. This
adds a comment about it being conservative.
6fb2d12373 Get rid of BadFormat error (hashmap)
Pull request description:
add additional variants instead.
as discussed in https://github.com/rust-bitcoin/rust-bitcoin/pull/1365
ACKs for top commit:
Kixunil:
ACK 6fb2d12373
apoelstra:
ACK 6fb2d12373
Tree-SHA512: 2cf9146670c372a3a482448f84a30943cd2ff2fa4e724075d67a52dba5ac0ad38f99ca2af3dd3494e13584653f2b23e913e6421328d40be52e868a107fffe03b
"schnorr" is a dirty word; the current `schnorr` module defines a
`Signature` that includes a sighash type, this sighash type is a bitcoin
specific construct related to taproot. Therefore the `Signature` is
better named `taproot::Signature`. Note also that the usage of `schnorr`
in `secp256k1` is probably justified because the
`secp256::schnorr::Signature` is just doing the crypto.
While we are at it, update docs and error messages to use "taproot"
instead of "schnorr". Also change function names and identifiers that
use "schnorr".
Currently we have `TapSighash` that is used for taproot sighashes but
for non-taproot sighashes we use `hash_types::Sighash`. We can improve
the API by creating a `LegacySighash`, and `SegwitV0Sighash`.
Copy the original `Sighash` macro calls to create the two new types in
the `sighash` module.
While we are at it, put the `TapSighash` and `TapSighashTag` into the
`sighash` module also.
There is never any use for the `sighash` module unless one is signing,
which requires the `crypto` module. The `sighash` module should
therefore live in the `crypto` module. This is not an API breaking
change because we reexport it at the crate root.
`Signature` only supported serialization into `Vec` which required a
heap allocation as well as prevented statically proving maximum length.
Adding a specialized type that holds a byte array and size solves this.
The solution is very similar to `secp256k1::ecdsa::SerializedSignature`.
The difference is that serialized signature in this crate contains
sighash bytes flag while in `secp256k1` it doesn't.
Script parsing is composed of several functions which implicitly rely on
various properties. Adding a type that restricts the valid values makes
local review easier.
So far we deserialized hex into `Vec<u8>` at run time. This was mainly
in tests where it had negligible performance cost. However moving the
computation to compile time has a few benefits: it allows proving the
length of the decoded bytes and identifies potential typos before the
code goes through LLVM and other compilation machinery which makes
feedback faster.
This change uses the `hex_lit` crate to move computation to compile
time. It is implemented as `const` declarative macro which doesn't blow
up compilation time.
a121e19e94 hashes: Implement AsRef for fixed size arrays (Tobin C. Harding)
Pull request description:
Implement `AsRef<[u8; X]>` for hash types including wrapped hash types. Doing so means at times the compiler can no longer infer the type because we have `AsRef<[u8]` implemented also but we can use `into_inner` and `as_inner` to get the inner array if needed.
Fix: #1462
## Note
This touches code that will likely be changed by #1577 and when we do #1491 but I believe its a step forward.
ACKs for top commit:
arturomf94:
ACK [`a121e19`](a121e19e94)
apoelstra:
ACK a121e19e94
Kixunil:
ACK a121e19e94
Tree-SHA512: 257c44826c7649db25bb3a6f023f68b2f17b70c546a056afad044bc8a16bf61f654c3846222505aaf5e6f9a0ad1d2113272d61317b407d0ac83702e41060a1ee
c3cc9e52ab Fix absolute lock time examples and tests (Tobin C. Harding)
Pull request description:
An absolute lock time of 100 is nonsensical because we are well past block 100. This value was used because it makes sense for _relative_ locktimes but for absolute lock times it makes the examples and tests slightly confusing.
ACKs for top commit:
apoelstra:
ACK c3cc9e52ab
Kixunil:
ACK c3cc9e52ab
Tree-SHA512: f490ef111bce0989c4ce8300c507c21b454448af4a91b9ef7a2fc05407411ca8721c9caa3dd1f0e8c0c133c4892c5c512f2d881af2cc67ae843d87eacae76ef1
4a03e2e721 psbt: Remove unused error variant (Tobin C. Harding)
Pull request description:
Remove an unused error variant for PSBT code (API breaking because the error type is public).
Woops, somehow I managed to get what was patch 1 of this series merged yesterday, I thought I left it out. Anyways, this is just the remove unused error variant now. No changes to that patch from previous versions of the PR.
ACKs for top commit:
apoelstra:
ACK 4a03e2e721
Kixunil:
ACK 4a03e2e721
Tree-SHA512: 228c661b97c6656db5a2bcc9ceb494ea485363b7f7262a97c677ee1639b5209c92ec3715ff48fdb108c95c828bfc83b6c475aa66f0ce8c5b0f286bfa7cc19554
An absolute lock time of 100 is nonsensical because we are well past
block 100. This value was used because it makes sense for _relative_
locktimes but for absolute lock times it makes the examples and tests
slightly confusing.
dd316e4d14 pow: Remove Mul/Div by arbitrary integer types (Tobin C. Harding)
Pull request description:
When we added `Target` and `Work` types we implemented multiplication and division by anything `Into<u64>`, this is not typically done in the Rust stdlib and also is semantically incorrect for the types.
Remove `Mul` and `Div` impls from `Target` and `Work`. Also remove `Mul<T>` for `T: Into<u64>` from the private `U256` type.
Fix#1632
ACKs for top commit:
apoelstra:
ACK dd316e4d14
Kixunil:
ACK dd316e4d14
Tree-SHA512: ede53555844adab321ff344535b7b8bab3c5c73855823dfc3ad728b077ae199451b7e22a1d203ef73a076073b7f0cbf9637cefa5fe82fc78ab454d02fa0b62b9
When we added `Target` and `Work` types we implemented multiplication
and division by anything `Into<u64>`, this is not typically done in the
Rust stdlib and also is semantically incorrect for the types.
Remove `Mul` and `Div` impls from `Target` and `Work`. Also remove
`Mul<T>` for `T: Into<u64>` from the private `U256` type.
272cdbcf7c Flatten the types directory (Tobin C. Harding)
Pull request description:
We recently created a `types` subdirectory under `script` to keep all the `Script` and `ScriptBuf` impls together. Turns out this additional level of subdirectory is a bit annoying and we can achieve the same grouping by just using `script/mod.rs`.
Move code from `types/mod.rs` to `script/mod.rs`, move the two submodules up a level, remove the `types` directory.
Fix: #1640
ACKs for top commit:
Kixunil:
ACK 272cdbcf7c
apoelstra:
ACK 272cdbcf7c
Tree-SHA512: 91fd78084829fa24f3b6420602d7d5094670647fff43e6e193d6de3126f1657132873ea133540d87db7d0d4dfc4cb9666489e39c861377085ce0254da81fd564
ae2aaaa436 Add `script_pubkey_lens` method (Martin Habovstiak)
cf068d16b0 Implement transaction weight prediction (Martin Habovstiak)
Pull request description:
When creating a transaction one must know the the fee beforehand to set
appropriate amounts for outputs and to know the fee, weight is required.
So far we only had a method on an already-constructed transaction. This
method clearly wasn't helpful when constructing the transaction except
for hacks like temporarily adding an all-zeroes signature.
This change adds a function that can compute the transaction weight
without knowing individual bytes of the scripts, witnesses and other
elements. It only needs to know their sizes.
To make the API less error-prone a special, trivial, type is also added
for computing the lengths of witnesses.
Based on #1627
ACKs for top commit:
apoelstra:
ACK ae2aaaa436
tcharding:
ACK ae2aaaa436
Tree-SHA512: 55376601c2c2826bb0909cc25ff5b65816f0b1a2d57fb2cd8831f3db5382de0f4a364d518b312f0528bb5f44c30f3f74f8d254145eed2bfd65e2332b7c4d7c8b
6be89bf94f Add `minimal_non_dust` to `TxOut` (Martin Habovstiak)
Pull request description:
In some scenarios it's useful to create outputs with minimal relayable value. E.g. outputs designated for fee bumping using CPFP. A method for this is useful.
This implements a constructor of `TxOut` that computes the minimal non-dust value from the passed script.
Closes#1459
This one is quite easy, so if we could get it in 0.30, that'd be great.
ACKs for top commit:
apoelstra:
ACK 6be89bf94f
tcharding:
ACK 6be89bf94f
Tree-SHA512: f31ae5f649fbba95ccaabf465cb814df193e7ef89c6e0de7b316a2a484e172beada0da8851da96b195a69a4da1b0991741d4c119f9b0c94fff34150e4f033bd5
We recently created a `types` subdirectory under `script` to keep all
the `Script` and `ScriptBuf` impls together. Turns out this additional
level of subdirectory is a bit annoying and we can achieve the same
grouping by just using `script/mod.rs`.
Move code from `types/mod.rs` to `script/mod.rs`, move the two
submodules up a level, remove the `types` directory.
In some cases people construct the transaction with a dummy fee output
value before calculating the weight. A method to create the iterator
over `script_pubkey` lengths is useful in such cases.
097e4e9c7f Fix license on bip158 module (Tobin C. Harding)
Pull request description:
When we introduced the SPDX license blurb in [0] we incorrectly gave attribution to Andrew when the original file author had the attribution as "the rust-bitcoin developers". The original author [1] was Tamas Blummer and he copied this code from code he wrote and explicitly re-licenses it. In order to make the re-licensing comment a little clearer and fix the mis-attribution use Tamas' name in the attribution.
[0] commit: `91ff2f628ce7db732d234a812e29fa8508f501a1 Introduce SPDX license identifiers`
[1] commit: `c93a70487f81a93c7d479ae046c75590d9fb7733 Add client side block filter (BIP158) (#281)`
ACKs for top commit:
apoelstra:
ACK 097e4e9c7f
Kixunil:
ACK 097e4e9c7f
Tree-SHA512: cb80d32c739ad562b2d657a34355bb28b1dd5c477b03018fbfbb14de40e03b806663aee89b578bcd8c681b067aa8d02611d4cde36e6fb9a8fa84ad4baf2e290e
6d99d3c061 Use ignore to stop rustdoc code from being built (Tobin C. Harding)
Pull request description:
Currently we have an attempted tag ```compile_fail that seems to be aiming at allowing code that does not build to exist in rustdoc. This is causing an error when running tests.
No clue how this made it through CI.
Use ```ignore to prevent rustdoc code from being built.
ACKs for top commit:
apoelstra:
ACK 6d99d3c061
Kixunil:
ACK 6d99d3c061
Tree-SHA512: 6c4b076000ba29377ac8cf942df66e849ff6421da6f9214664d487550cf45889e163b4de652079010bae327019163b63a1962ff8e6a04d918db63ffb0285ccd1
5f86b3091c Add From<Address> for ScriptBuf (Tobin C. Harding)
Pull request description:
Add an implementation of `From<Address> for ScriptBuf` that calls through to `address.script_pubkey` (which calls
`address.payload.script_pubkey()`).
Fix: #1457
ACKs for top commit:
apoelstra:
ACK 5f86b3091c
Kixunil:
ACK 5f86b3091c
Tree-SHA512: 8a45f292578765b345863946b276607d561b9bc75f6b9bb97f48b32d503143e234aedb658997db802c87289576361ec9ee6cb31fe3bbccfc06cc2fdabc7c41bb
In some scenarios it's useful to create outputs with minimal relayable
value. E.g. outputs designated for fee bumping using CPFP. A method for
this is useful.
This implements a constructor of `TxOut` that computes the minimal
non-dust value from the passed script.
Closes#1459
When creating a transaction one must know the the fee beforehand to set
appropriate amounts for outputs and to know the fee, weight is required.
So far we only had a method on an already-constructed transaction. This
method clearly wasn't helpful when constructing the transaction except
for hacks like temporarily adding an all-zeroes signature.
This change adds a function that can compute the transaction weight
without knowing individual bytes of the scripts, witnesses and other
elements. It only needs to know their sizes.
To make the API less error-prone a special, trivial, type is also added
for computing the lengths of witnesses.
41f2dcf6ae Improve test coverage for docs build (Tobin C. Harding)
b4c14a4b7c hashes: Use automatic link (Tobin C. Harding)
96e8a080d1 ci: Remove redundant || exit (Tobin C. Harding)
Pull request description:
Currently the docs build commands in `hashes` and `bitcoin` differ, they should be the same.
Add a command `cargo doc` to improve coverage e.g., recently we botched the feature guarding but since CI only runs `cargo rustdoc` with custom compiler conditional set we didn't catch it.
Done after seeing: https://github.com/rust-bitcoin/rust-bitcoin/pull/1504 and CI should fail on this PR until 1504 is in.
ACKs for top commit:
apoelstra:
ACK 41f2dcf6ae
Kixunil:
ACK 41f2dcf6ae
Tree-SHA512: 7cde68292cfc6f32b75d066e188e7c418ee251f9a5abc57fbd642ba33e9cd5bd8ef7c5ba7cffd206acae6ddec2f8c3db38c8c911a4319e979158666b8225953d
a7117bf8f1 Document source of logic fro read_scriptint (Tobin C. Harding)
2eb2420b40 Add comment on rountripping read/write scripint (Tobin C. Harding)
657dd51e8b Use OP_0 to better mimic bitcoin core code (Tobin C. Harding)
31d254a6a8 Fix push operators URL (Tobin C. Harding)
84cd4ca964 Deprecate script::read_uint (Tobin C. Harding)
Pull request description:
Patch one does the deprecation, the rest of the PR is made up of tiny improvements to the code around reading/writing 'scriptint's (conceptually `CScriptNum`s). I did all this while trying to decipher the discussion on #1547.
### Note Please
There are many more changes in the pipeline for all this read/write "script int" stuff. This PR was done ages ago and I believe it stall adds value.
I re-did the whole PR manually because of the recent `script` module changes. I hope no one else has to do that - if you do please feel free to holla and I'll "rebase" your PR for you.
ACKs for top commit:
Kixunil:
ACK a7117bf8f1
apoelstra:
ACK a7117bf8f1
Tree-SHA512: 5e8ee7fa8d1393a1a50e4241dd947b837cc0ddd15ff1239a49e4839489459fb95d184d6773f73633d55c436310bfab0c73f806d492ed4a4215f924c6c0993936
1e0e712bb0 Add push_* methods for lock times (Tobin C. Harding)
Pull request description:
Lock times are `u32` and can require encoding using 5 bytes.
Add methods `push_lock_time` and `push_sequence` for pushing absolute lock times and sequence numbers. We do not push relative locktimes because they are only 16 bits from the original sequence number.
ACKs for top commit:
Kixunil:
ACK 1e0e712bb0
apoelstra:
ACK 1e0e712bb0
Tree-SHA512: 4b511679270e7ef73937259ccf7d1b9b4b7512b2464f302310519a6e02d55c9cc24e3559302aeb671156e68130478258c1c565f474880e8be708b0ee234e67ff
8596e402f2 Fix docs.rs to use all features (Tobin C. Harding)
89086d094d hashes: Use angle braces to make hyperlinks (Tobin C. Harding)
9b5c2ad7af hashes: Clean up optional dependencies (Tobin C. Harding)
5b4f19c01f hashes: Improve std/alloc features (Tobin C. Harding)
132d2f90b6 bitcoin: Enable alloc feature in features list (Tobin C. Harding)
aa62ca224a hashes: Do not enable core2/alloc feature (Tobin C. Harding)
c15f8dee29 Improve manifest package section (Tobin C. Harding)
12f5e37ed9 Add excludes to manifests (Tobin C. Harding)
Pull request description:
Do a complete overhaul of the manifest of the top level crates (i.e., not `embedded`, `fuzzing` ect.).
Many of the problems being fixed here were introduced over the last year by my poor understanding of exactly what was going on with _every_ line of code in the manifests, after this PR I hope that is no longer a problem.
I'm closing #1571 because it is now done more fully at the end of this PR.
During review please be liberal with any questions so we can ensure everything is spot on now - as we add more crates there are going to be a proliferation of manifest files, best get it right now.
ACKs for top commit:
apoelstra:
ACK 8596e402f2
Kixunil:
ACK 8596e402f2
Tree-SHA512: 9e4ae21221cd5b185c04b6d1af73983d324e6d4e09abf94763f6eabcff420d9a3b2ec62a88cd20844fda518756862b0e64a85be54187166f3b1b2b206b82cb08
861fdd6ab1 Put the `MerkleBlock` struct at the top of the file (Tobin C. Harding)
f0d968197a Put error at the bottom of the file (Tobin C. Harding)
19e094788f Use self for Error variant imports (Tobin C. Harding)
83c2a552db Put helper function below where its called (Tobin C. Harding)
5076579fb9 Fix indentation in pmt_tests macro (Tobin C. Harding)
a7edbfb52e Move hex data to tests/data (Tobin C. Harding)
Pull request description:
PR 2 in the `merkle_tree::block` series, used to be on top of the now merged #1374.
Do a bunch of refactorings in preparation for more invasive changes. This is a separate PR because, other than the first patch which moves hex strings to `tests/data/` the other patches are refactoring only patches, no logic changes. However the last patch is big and will be annoying to review - sorry about that. If you really oppose this basically stylistic patch putting important things first, the opposite of C code, please say and I'll try to stop doing it.
ACKs for top commit:
Kixunil:
ACK 861fdd6ab1
apoelstra:
ACK 861fdd6ab1
Tree-SHA512: 3da0a600898f490b602ab05a396061587d86ffef55697877885a8c611eff96e7382a2d816fe9594c100d378dc56fe7fdc88009a0343bc602b7f4c180836adbd3
Currently we implement string parsing for height/time from the
`absolute` module but not the `relative` module.
Improve the macros used to implement string parsing and use the new
versions to implement string parsing for the height and time types in
`relative`.
Lock times are u32 and can necessitate encoding using 5 bytes. As such
they are "special".
Add methods `push_lock_time` and `push_sequence` for pushing absolute
lock times and sequence numbers. We do not push relative locktimes
because they are only 16 bits from the original sequence number.
Our `script::read_scriptint` function is based on the constructor
code (incl. call to `set_vch`) code from Bitcoin Core. Add rustdoc
comment saying so, emit a link because there are already multiple links
to `script.h` in this file (one just right below the added comment).
We only support reads of upto 4 bytes where as Bitcoin Core allows
reading a `CScriptNum` with more bytes than that. Add a rustdoc
comment (incl. link to Bitcoin Core) mentioning that.
Our `Builder::push_int` method is the same as Bitcoin Core `CScript`
`push_int64` method. We currently use `OP_FALSE` (equivalent to `OP_0`)
but recently we added `OP_0`, lets use it to make our code better mimic
Core (also saves devs checking that `OP_FALSE` is the same as `OP_0`).
The `MerkleBlock` struct is the main type in this file, put it at the
top of the file. This leaves the next most important type,
`PartialMerkleTree` below that.
Refactor only, no logic changes.
70cf4515db Add `Weight` and `FeeRate` newtypes (Martin Habovstiak)
Pull request description:
Use of general-purpose integers is often error-prone and annoying. We're working towards improving it by introducing newtypes.
This adds newtypes for weight and fee rate to make fee computation easier and more readable. Note however that this dosn't change the type for individual parts of the transaction since computing the total weight is not as simple as summing them up and we want to avoid such confusion.
Part of #630
Replaces #1607 (I want to get this in quickly and don't want to be blocked on DanGould's availability.)
ACKs for top commit:
apoelstra:
ACK 70cf4515db
tcharding:
ACK 70cf4515db
Tree-SHA512: ab9cc9f554a52ab0109ff23565b3e2cb2d3f609b557457b4afd8763e3e1b418aecbb3d22733e33304e858ecf900904a1af6e6fdc16dc21483b9ef84f56f103b2
a9108d3939 Refactor script module (Tobin C. Harding)
Pull request description:
The `script` module is large and unwieldy.
Refactor the `script` module, splitting it up into a tree of modules. Here are a few of the changes and their stated benefits
- Split the two script types out into separate files: Readers of the methods can then tell immediately from the file name which type they are reading.
- Put all the impls for the two script types together: Makes parsing the API easier because one can more quickly see which traits are implemented on what i.e., all the `AsRef` imlps are grouped together.
- Put the impls for the two script types in order, first `Script` then `ScriptBuf`: Makes it easier for us to see if we missed something.
- Put the `Builder` and `Instruction` (and associated) types in their own modules: Some devs find long files hard to navigate, so far there hasn't been too much push back against short files.
- Put tests in a separate file: This idea was recently discussed.
This is only moving code and fixing import statements etc. No other changes to the code.
## Note to reviewers
This PR is impossible to review from the diff because it moves so much code. Perhaps better to look at the resulting `src/blockdata/script/` directory and see if you like it.
#### Motivation
While adding script tagging I was having difficulty navigating the script module.
ACKs for top commit:
apoelstra:
ACK a9108d3939
Kixunil:
ACK a9108d3939
Tree-SHA512: 19123c8cfbdce6c42b322fa75a74073a0114b0ed21bd06ca5727981b3573b74cf05075723b774b92ae2b497e20644fca6e2fac14e30cc44f2802dde5aa567f66
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.
This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.
Part of #630
In `bitcoin` when we use the `core2` dependency we always need the
"alloc" feature. Enabling "alloc" when enabling "core2" in the "no-std"
feature is confusing because it makes it seem that we don't always need
it.
Set usage of the "alloc" feature of `core2` in the `features` list.
`core2` is for Read/Write, nothing to do with allocation and we do not
use the "alloc" feature of `core2` in `hashes`.
Fix core2 dependency/features by doing:
- Explicitly enable "bitcoin_hashes/core2" in `bitcoin`.
- Do not enable "core2/alloc" in `hashes`
Improve all manifest package sections by doing:
- Order the list of options uniformly
- Remove unnecessary homepage option (currently same as repo)
- Add categories section
We can check which files are included in the packaged release with
`cargo package --list `.
Add an `exclude` section to each manifest that excludes `tests/` and
`contrib/`. Not all crates have a `tests/` directory yet but they should
so add the exclude anyway to future proof the crates.
32d2d62e0f Rename from_slice methods to decode (Tobin C. Harding)
Pull request description:
The `TaprootMerkleBranch` and `ControlBlock` both have methods on them called `from_slice` but these methods do more that just basic copy from a slice. `decode` is a more descriptive name.
Deprecate the `from_slice` methods and implement `decode`, on other changes to the logic.
cc sanket1729
ACKs for top commit:
apoelstra:
ACK 32d2d62e0f
Kixunil:
ACK 32d2d62e0f
Tree-SHA512: e8c089545411a214ef9393f65d3990be46983000bd045182cc27dd70b62273bf48ac97adaf89d1e7fc807c72964a01eef176c7685684e8f87a01c219746d6d3d
55d17f2c18 Instruct devs to use nightly for embedded (Tobin C. Harding)
Pull request description:
The embedded test crate requires usage of the nightly toolchain, fix the docs to show this.
ACKs for top commit:
apoelstra:
ACK 55d17f2c18
Kixunil:
ACK 55d17f2c18
Tree-SHA512: 5d3d611ff4331d8475f77d260ebaa3da3ae71960b709eb4603056d7034885306b8bcc39be488f3391697d47a1074a695979fb50b6cb31af414f873da8e82bdbd
86f372774b Add '_ back into the BitStreamWriter (Tobin C. Harding)
Pull request description:
Recently we merged `commit 53d4fe66b57c255086def2b5e47afaddee776b75` to fix CI even though a better approach is to use `'_` because it assists reading the code (shows that the bit stream writer is not writing from a reference since its writing a `Copy` type `n`).
Add back in the `'_` (I forget what its called).
ACKs for top commit:
apoelstra:
ACK 86f372774b
Kixunil:
ACK 86f372774b
Tree-SHA512: 2a9989164562dbe7bf133e3aeb090fbff7831bfeefb0ac8431e75b17d57184c4d60ac206578c6ebbcff903a3832502a162027ed9f37e5ed87e42a6bf61efa594
The `script` module is large and unwieldy.
Refactor the `script` module, splitting it up into a tree of modules.
Here are a few of the changes and their stated benefits
- Split the two script types out into separate files: Readers of the
methods can then tell immediately from the file name which type they are
reading.
- Put all the impls for the two script types together: Makes parsing the
API easier because one can more quickly see which traits are implemented
on what i.e., all the `AsRef` imlps are grouped together.
- Put the impls for the two script types in order, first `Script` then
`ScriptBuf`: Makes it easier for us to see if we missed something.
- Put the `Builder` and `Instruction` (and associated) types in their
own modules: Some devs find long files hard to navigate, so far there
hasn't been too much push back against short files.
- Put tests in a separate file: This idea was recently discussed.
This is only moving code and fixing import statements etc. No other
changes to the code.
The `TaprootMerkleBranch` and `ControlBlock` both have methods on them
called `from_slice` but these methods do more that just basic copy from
a slice. `decode` is a more descriptive name.
Deprecate the `from_slice` methods and implement `decode`, on other
changes to the logic.
75b266a129 Improve `sighash` module documentation (Martin Habovstiak)
Pull request description:
"Sighash" is a technical term that newbies in Bitcoin may not know and could get lost when trying to find how to sign a transaction. This change attempts to make it more obvious that this module is needed for signing.
Closes#1463
ACKs for top commit:
tcharding:
ACK 75b266a129
apoelstra:
ACK 75b266a129
Tree-SHA512: 7157566c1639c63ce0fba2832e8e5e846e689d89e24077ed7769b721c5db4613cd7fd8d91464992eb78de74b42912ca877e7182a9c3c9c8848bf94d89767b8cc
b3188bbac3 Add `Transaction` accessors to `SighashCache` (Martin Habovstiak)
7c6854fe02 Use `Borrow` instead of `Deref` in `SighashCache` (Martin Habovstiak)
Pull request description:
This changes the bound from `Deref<Target = Transaction>` to `Borrow<Transaction>` (with respective `mut` changes) and adds accessors.
Closes#1423 (PSBT stuff will be separate issue).
ACKs for top commit:
tcharding:
ACK b3188bbac3
apoelstra:
ACK b3188bbac3
Tree-SHA512: 9db2c5890b26e9eefd483d697b42e84b1d7d3b8676fc39b4f39075c149e12697aa538828a757f9187578a958d72a592bb913f8f5788c93feb273db5370979d99
118a593c89 Implement from arrays for TaprootMerkleBranch (Tobin C. Harding)
Pull request description:
The `TaprootMerkleBranch` contains a vector of `TapNodeHash`s, as such it can trivially be constructed from an array of the same type.
Implement `From` for all array sizes 1 - 128 inclusive.
Fix: #1469
ACKs for top commit:
Kixunil:
ACK 118a593c89
apoelstra:
ACK 118a593c89
Tree-SHA512: dd497abd9143ea8b43485133beaccac9049fb915a95a3422d41c1f99961b59ec95df93efe759aa02f62ba1cf3e1afc4597671f1202ff0fa78eeee8b305d21305
The `TaprootMerkleBranch` contains a vector of `TapNodeHash`s, as such
it can trivially be constructed from an array of the same type.
Implement `From` for all array sizes 1 - 128 inclusive.
"Sighash" is a technical term that newbies in Bitcoin may not know and
could get lost when trying to find how to sign a transaction. This
change attempts to make it more obvious that this module is needed for
signing.
Closes#1463
277e8e96bd Add KeyPair import to rustdoc example (Tobin C. Harding)
Pull request description:
Recently, and bizarrely, a PR merged that broke `cargo test --doc`.
Add an import for `KeyPair` to the `schnorr` rustdoc example.
ACKs for top commit:
apoelstra:
ACK 277e8e96bd
Kixunil:
ACK 277e8e96bd
Tree-SHA512: ad214b668827b35848cc7b260cbd2104a916a82a5a6d242bdc498c62edc9a0e864f4bdb4abcade42924dbaf951223ae80feacbe68d8a4ccb4562d8ead50b23a9
bb612fdafa Set rustv_1_53 in build script (Tobin C. Harding)
Pull request description:
The rust version is supposed to be set by the build script so that users automagically get features matching the toolchain in use. Currently we have a feature in the manifest for `rustv_1_53` instead setting a compiler conditional configuration option in the build script. This causes `cargo +1.41.1 --all-features check` to fail.
## Note
I don't see `rustv_1_46` used anywhere, do we need that still?
ACKs for top commit:
apoelstra:
ACK bb612fdafa
Kixunil:
ACK bb612fdafa
Tree-SHA512: f74195d4ee5a5bc5f209e99d30789df3552cef10aee5ea8b61a5a701b753999c34d04be9fe0321ccee7a8ec14fa5a05e0b454b9dc5f8deddd7b5b8d4f3d7e744
It may be useful to access the transaction stored in `SighashCache`
during signing or afterwards, especially when the transaction is stored
without indirection (to enable long-lived storage).
This change adds the appropriate accessors.
The requirement for a type dereferencing to `Transaction` prevented
storing the cache in long-lived data without resorting to silly
wrappers. Since `Borrow` is implemented both for `T` and for smart
pointers it's a more flexible bound which this change implements.
While this is technically breaking, all usual non-generic code will
continue to work beause smart pointers generally have `Borrow`
implemented.
Currently we use a wildcard to export all the hash types in
`hash_types`. We are moving to a world were we only export
normal/standard types from the crate root.
Remove the reexport of the following hash types:
- `FilterHash`
- `FilterHeader`
- `TxMerkleNode`
- `WitnessCommitment`
- `WitnessMerkleNode`
- `XpubIdentifier`
- `Sighash`
Fix: #1541
f0e4e38844 Add newline in rustdoc (Tobin C. Harding)
Pull request description:
Docs created with the `sha256t_hash_newtype` macro are missing a newline between the doc heading and doc main section.
Note that the strings used span multiple lines and therefor the subsequent lines must be aligned with the start of the line (not indented).
Fix: #1540
ACKs for top commit:
Kixunil:
ACK f0e4e38844
apoelstra:
ACK f0e4e38844
Tree-SHA512: 240c68864da63688c400498903d5cc345bee224dcd3235df0127dcf391c66ee08c487d31fe59f890009c674574810b689d9a53628d07d8cdd46b79bc0ac3eb2b
Docs created with the `sha256t_hash_newtype` macro are missing a newline
between the doc heading and doc main section.
Note that the strings used span multiple lines and therefor the
subsequent lines must be aligned with the start of the line (not
indented).
Fix: #1540
The rust version is supposed to be set by the build script so that users
automagically get features matching the toolchain in use. Currently we
have a feature in the manifest for `rustv_1_53` instead setting a
compiler conditional configuration option in the build script. This
causes `cargo +1.41.1 --all-features check` to fail.
32ca6cc320 Remove hex_from_slice and display Sighash forwards (Tobin C. Harding)
a308e1e2ea Remove FromHex for all types except Vec and array (Tobin C. Harding)
3e70c01826 Manually format a bunch of vecs (Tobin C. Harding)
83e1c40c4d Remove script:: prefix from unambiguous types (Tobin C. Harding)
5ab5c264d2 Use fully qualified path in macro (Tobin C. Harding)
7e85452cd9 hashes: Implement std::error::Error (Tobin C. Harding)
5e3abc5e11 Fix feature gating on unit tests (Tobin C. Harding)
3344cf6be2 Favour $reverse instead of $reversed (Tobin C. Harding)
Pull request description:
This work started out, as the branch name suggests, as an effort to use the `hex_lit` crate. But once I got to this stage it seems that the `hex!` macro we have provides different, useful, functionality than the `hex_lit::hex!` macro (it allows usage with non-consts). So I'm unsure if we want to remove it now.
- Patches 1 - 6 are preparatory clean ups
- Patch 7 reduces usage of `FromHex`, please see git log for full description
- Patch 8 removes `hex_from_slice` and fixes a bug in how we display `Sighash`
ACKs for top commit:
apoelstra:
ACK 32ca6cc320
Kixunil:
ACK 32ca6cc320
Tree-SHA512: 11b45b39ec2fc0f837d7395b5fb86de7cc44641fd51cf7e93394a635e6a8fb1c7ac441a6070d5516dae60e084c04cc6e8b605a5167093f964679e445ef60c271
facaefc49c Add conversions for TweakedKeyPair -> TweakedPublicKey (Tobin C. Harding)
2407f241e4 Remove sep256k1 path from Parity (Tobin C. Harding)
Pull request description:
It is trivially possible to get `TweakedPublicKey` from a `TweakedKeyPair`, add conversion methods for doing so.
Patch 1 is preparatory cleanup. Please note `From` is not implemented because the conversion returns the `Parity` also.
Fix: #1452
ACKs for top commit:
apoelstra:
ACK facaefc49c
Kixunil:
ACK facaefc49c
Tree-SHA512: 597026c481fe2622a625cbeb381cac345af6f49f4a115418b69817345fc3c2140bbdbc5208eae1149d7d171f94c776365d302ffe1f9c01d944e738807db28a89
`Sighash` should be displayed forwards according to BIP143. Currently we
are displaying it backwards (as we do for double SHA256). This is
working because parse using `Vec::from_hex`.
We have the means to parse hex strings directly for hashes, we no longer
need `hex_from_slice`.
BIP143 test vectors display double SHA256 forwards but we display
backwards, this is acceptable because there is no fixed display in the
ecosystem for double SHA256 hashes. In order to overcome this we parse
test vector hex strings with into `Vec` when needed.
Remove `FromHex` from hash and script types
- Remove the `FromHex` implementation from hash types and `ScriptBuf`
- Remove the `FromStr` implementation from `ScriptBuf` because it does not
roundtrip with `Display`.
- Implement a method `from_hex` on `ScriptBuf`.
- Implement `FromStr` on hash types using a fixed size array.
This leaves `FromHex` implementations only on `Vec` and fixed size arrays.
In preparation for modifying some unit test data structures, manually
format the code so it is uniform.
Move elements added to a vec with `vec!` onto a new line so they all
line up and one can better see what fields go where.
Refactor only, no logic changes.
ed6f6d11dd Implement fmt traits for ScriptBuf (Tobin C. Harding)
Pull request description:
We can improve ergonomics of the `script` module by implementing the `fmt` traits on `ScriptBuf`, trivial because we can call through to the `Script` implementations.
Fix: #1585
ACKs for top commit:
Kixunil:
ACK ed6f6d11dd
apoelstra:
ACK ed6f6d11dd
Tree-SHA512: 878a1522af4ed1e10d1d8d60d150e6571008c008b5e5c662c67462f9e09075b4f1fe4e399ed50e98cd7253b6815937c6732cd1ce02b74a5be017d5b8fcdbbd2f
We can improve ergonomics of the `script` module by implementing the
`fmt` traits on `ScriptBuf`, trivial because we can call through to the
`Script` implementations.
The `max_value` and `min_value` functions only exist to be
compatible/uniform with Rust 1.41.1 they will never change and they just
return a constant value. They can therefore be made const functions.
Recently we merged `commit 53d4fe66b57c255086def2b5e47afaddee776b75` to
fix CI even though a better approach is to use `'_` because it assists
reading the code (shows that the bit stream writer is not writing from a
reference since its writing a `Copy` type `n`).
Add back in the `'_` (I forget what its called).
877f9af364 Add new hex parse error variant (Tobin C. Harding)
Pull request description:
Recently we used an error type that holds only one expected hex string length when parsing but for `PublicKey`s we have two (66 and 130). Add a new error variant to express the error. Requires adding a variant to `bip32` for the same thing.
Fix: #1281
ACKs for top commit:
Kixunil:
ACK 877f9af364
apoelstra:
ACK 877f9af364
Tree-SHA512: c1ca493ee30418bd82bc326b35c18731260e4217c371f37301a73c64f9a6631163801acc217c6c2c7b14f632a2ad5043174266c1b4fdce127698e68ab8494f20
3c0598b399 Add standard constants to lock times (Tobin C. Harding)
Pull request description:
Some of the lock time structs (`Height`, `Time` ect.) are missing standard constants for min, max ect.
Add standard constants taking into consideration the various locktime corner cases.
Add `max_value` and `min_value` to be consistent with Rust 1.41.1 (incl. `Sequence`).
Fix: #1451
This PR is not complex in itself but **locktimes are notoriously complex, please wait for 3 acks before merging** - and ack'ing makes no guarantee that reviewer got all corner cases :)
There is no rush on this one, apoelstra, Kixunil, sanket1729 please just review when your brain is fresh.
ACKs for top commit:
apoelstra:
ACK 3c0598b399
Kixunil:
ACK 3c0598b399
Tree-SHA512: aa3d112db83b4785edb0a7a517cc335ded59f5967eb39b8979a6d68f9bba4644a27e5ca400fcabf368a1f8c0eecdef0b87b1011933ac7fd96b467b8501533203
Recently we used an error type that holds only one expected hex string
length when parsing but for `PublicKey`s we have two (66 and 130). Add a
new error variant to express the error. Requires adding a variant to
`bip32` for the same thing.
Fix: #1281
Implement `AsRef<[u8; X]>` for hash types including wrapped hash types.
Doing so means at times the compiler can no longer infer the type because we have
`AsRef<[u8]` implemented also but we can use `into_inner` and `as_inner`
to get the inner array if needed.
Add an implementation of `From<Address> for ScriptBuf` that calls
through to `address.script_pubkey` (which calls
`address.payload.script_pubkey()`).
Fix: #1457
Some of the lock time structs (`Height`, `Time` ect.) are missing
standard constants for min, max ect.
Add standard constants taking into consideration the various locktime
corner cases.
Add `max_value` and `min_value` to be consistent with Rust 1.41.1 (incl.
`Sequence`).
Fix: #1451
a762a89b48 Add documentation to Sequence::is_final (Tobin C. Harding)
b1490a26ea Move enables_absolute_lock_time method (Tobin C. Harding)
Pull request description:
The term "final" is an archaic Bitcoin term however it is well used, it exists in Bitcoin Core code as well as in various bips. To help folks new to Bitcoin add documentation to the `is_final` method including historical notes.
Note, this does _not_ deprecate `is_final` - while writing the notes I found the term "final" in enough official places that I think its fair game to keep the term, some things people just have to learn, we can definitely help with that learning though.
Fix: #1198
ACKs for top commit:
Kixunil:
ACK a762a89b48
apoelstra:
ACK a762a89b48
Tree-SHA512: 895fbdce90223d90c0a68fb1e3d6b7aada4a3606d1294ea4df1f4194681a79d970b0434e7bb078f6d5cbf413b3550e72560d6d5cf811a5a959adf53f7f778ab2
8c0e5213d3 Delegate debug for ScriptBuf to Script (Tobin C. Harding)
Pull request description:
Currently the derived implementation of `Debug` for `ScriptBuf` prints the inner vector of u8s as integers, this is ugly and hard to read. The `Script` implementation of `Debug` prints the script opcodes and data as hex, we can just delegate to it.
With this applied we get debug output of form:
Script(OP_DUP OP_HASH160 OP_PUSHBYTES_20 3bde42dbee7e4dbe6a21b2d50ce2f0167faa8159 OP_EQUALVERIFY OP_CHECKSIG)
Fix: #1516
ACKs for top commit:
Kixunil:
ACK 8c0e5213d3
apoelstra:
ACK 8c0e5213d3
Tree-SHA512: ca07d9fb191f4e0379cbd96b2944e6881094a8334d39b97209b6bf452a3c15d4aede53b9c88176b9b7667b7a539d47897940bc561dc9f8cd83ce1990a08047e1
1d3d5a9c5b Take Into<secp256k1::PublicKey> in PublicKey constructors (Tobin C. Harding)
b13a76407b keys: Clean up test imports (Tobin C. Harding)
Pull request description:
We can make the API more ergonomic by taking a generic argument that implements `Into<secp256k1::PublicKey>` in the `bitcoin::PublicKey` constructors.
The only thing than this is useful for is passing in `KeyPair` and the `From` implementation already exists. Add a unit test to verify.
Fix: #1453
## Note
As per the discussion in #1453 I checked secp and bitcoin for all keys that can be converted using `From` and it turns out its only `KeyPair` which already has `From` impls - good rust-bitcoin devs :)
ACKs for top commit:
Kixunil:
ACK 1d3d5a9c5b
apoelstra:
ACK 1d3d5a9c5b
Tree-SHA512: b5e5272561de15cdcfb15913aa5d42ddc96bf2fd5835068a5a9aa0274074ffa698ec9e81707f102b7d1b244f1abd0fdbd0eb4b6b505c84c3d5719dcb01d46efb
49e8b8da32 Use write_all for sighash encoding (Tobin C. Harding)
Pull request description:
From BIP143:
> If sighash type is SINGLE and the input index is smaller than the number of outputs, hashOutputs is the double SHA256 of the output amount with scriptPubKey of the same index as the input;
Currently we are using a `Sighash` which wraps double sha256 so while technically correct this means we are relying on `Sighash` to implement `Encodable`. We can remove this requirement by directly using the `sha256d::Hash` type to hash the outputs data.
Fix: #1549
ACKs for top commit:
Kixunil:
ACK 49e8b8da32
apoelstra:
ACK 49e8b8da32
Tree-SHA512: 8dd0037245a7cf180ba8a6eceeadad912d4adc14fc3f49df9008856de262624666d7d575195eea4868b2a5252dc565590e6be78471053b5e6367f3d2363310e8
e7bbfd3913 Improve Psbt error handling (DanGould)
Pull request description:
## Separate `encode::Error` and `psbt::Error` recursive dependency
This initial work attempts to fix#837's first 2 points
> - The current psbt::serialize::Deserialize has an error type of consensus::encode::Error. I think we should cleanly separate consensus encoding errors from application-level encoding errors like psbt.
> - There is a recursive dependence between encode::Error and psbt::Error which would need to be cleanly dissected and separated so that there is no dependence or only one-way dependence.
## Better `ParseError(String)` types
arturomf94 how compatible do your #1310 changes look to address #837's third point with this design?
> - There are a lot ParseError(String) messages that could use a better type to downflow the information.
I think your prior art would completely address this issue now.
## On handling `io::Error` with an associated error
`encode::Error` has an `Io` variant. now that `Psbt::deserialize` returns `psbt::Error` and produces an `io::Error`, we need an `Io` variant on `psbt::Error`. Except that doing so breaks `#[derive(Eq)]` and lots of tests for `psbt::Error`.
Kixunil, I'm trying to understand your feedback regarding a solution to this problem.
> I believe that the best error untangling would be to make decodable error associated.
> I meant having associated `Error` type at `Decodable` trait. Encoding should only fail if the writer fails so we should have `io::Error` there (at least until we have something like `genio`).
>
> > [it] is a problem to instantiate consensus::encode::Error in [the psbt] module for `io::Error`?
>
> It certainly does look strange. Maybe we should have this shared type:
>
> ```rust
> /// Error used when reading or decoding fails.
> pub enum ReadError<Io, Decode> {
> /// Reading failed
> Io(Io),
> /// Decoding failed
> Decode(Decode), // consensus and PSBT error here
> }
> ```
>
> However this one will be annoying to use with `?` :( We could have `ResultExt` to provide `decode()` and `io()` methods to make it easier.
>
> If that's not acceptable then I think deduplicated IO error is better.
Kixunil didn't we just get rid of Psbt as `Decodable`? Would this make more sense to have as an error associated with `Deserialize`? Or did we do the opposite of what we should have by making Psbt only `Serialize`/`Deserialize` because of #934, where only consensus objects are allowed to be `Decodable`? I wonder if we prioritized that strict categorization and are stuck with worth machinery because of it. My goal with #988 was to get to a point where we could address #837 and ultimately implement PSBTv2.
ACKs for top commit:
tcharding:
ACK e7bbfd3913
apoelstra:
ACK e7bbfd3913
Tree-SHA512: 32975594fde42727ea9030f46570a1403ae1a108570ab115519ebeddc28938f141e2134b04d6b29ce94817ed776c13815dea5647c463e4a13b47ba55f4e7858a
44d3ec487d Rename Payload::as_bytes to inner_prog_as_bytes (sanket1729)
a446df583c Make Payload non-exhaustive (sanket1729)
6ebc9de252 Introduce WitnessProgram struct and cleanup Address validity invariants (sanket1729)
41652caf05 Introduce is_spend_standard method (sanket1729)
Pull request description:
Fixes#1561.
Highlights:
- Segwitv0 programs with lengths apart from 20 or 32 are invalid `Address` struct. Such Addresses are useless and we should not parse/create them.
- Renamed `is_standard` to `is_spend_standard`.
ACKs for top commit:
apoelstra:
ACK 44d3ec487d
tcharding:
ACK 44d3ec487d
Tree-SHA512: 1ee36f7ea25c65619ddf7d643d025690096876843dbe6fbdf877ce23e88049d79b0bbd78cee6cf4b415bca028b3634bb70c9f52d1098bd90558e6ba7f8731332
Addresses with Segwitv0 not having len 20/32 are invalid and cannot be
constructed. Also cleans up a API bug in
ScriptBuf::new_witness_prog(ver, prog) allowing prog of invalid lenghts.
ebfbe74243 Implement `Debug` for generic `Address<V: NetworkValidation>` (Jiri Jakes)
Pull request description:
Previously `Debug` was implemented for both `Address<NetworkChecked>` and `Address<NetworkUnchecked>`, but not for cases when the `NetworkValidation` parameter was generic. This change adds this ability. Based on Kixunil's tip.
With previous implementation, the `test_address_debug()` resulted in error:
The added `Debug` on `NetworkChecked` and `NetworkUnchecked` are required by compiler.
---
While dealing with derives and impls, I also attempted to turn all the derives on `Address` into manual impls (see Kixunil's suggestion in https://github.com/rust-bitcoin/rust-bitcoin/pull/1489#discussion_r1052448057). The motivation behind this was the possibility to remove derives on `NetworkChecked` and `NetworkUnchecked`, too. However, even with manual impls, all the traits on `NetworkChecked` and `NetworkUnchecked` were still required by compiler in this sort of situations (see also the rest of the same discussion linked above). I do not fully understand why, perhaps limitation of this way of sealing traits?
It can be demonstrated by removing `Debug` derivation on `NetworkUnchecked` and `NetworkChecked` in this PR and running `test_address_debug()`.
Therefore, if we want to allow users of the library to define types generic in `NetworkValidation` and at the same time derive impls, it seems to me that `NetworkChecked` and `NetworkUnchecked` will have to have the same set of impls as `Address` itself.
ACKs for top commit:
Kixunil:
ACK ebfbe74243
tcharding:
ACK ebfbe74243
apoelstra:
ACK ebfbe74243
Tree-SHA512: 87f3fa4539602f31bf4513a29543b04e943c3899d8ece36d0d905c3b5a2d76e29eb86242694b5c494faa5e54bb8f69f5048849916c6438ddd35030368f710353
We can make the API more ergonomic by taking a generic argument that
implements `Into<secp256k1::PublicKey>` in the `bitcoin::PublicKey`
constructors.
The only thing than this is useful for is passing in `KeyPair` and the
`From` implementation already exists. Add a unit test to verify.
Fix: #1453
Currently we have an attempted tag ```compile_fail that seems to be
aiming at allowing code that does not build to exist in rustdoc. This is
causing an error when running tests.
No clue how this made it through CI.
Use ```ignore to prevent rustdoc code from being built.
Currently the derived implementation of `Debug` for `ScriptBuf` prints
the inner vector of u8s as integers, this is ugly and hard to read. The
`Script` implementation of `Debug` prints the script opcodes and data as
hex, we can just delegate to it.
With this applied we get debug output of form:
Script(OP_DUP OP_HASH160 OP_PUSHBYTES_20 \
3bde42dbee7e4dbe6a21b2d50ce2f0167faa8159 OP_EQUALVERIFY OP_CHECKSIG)
Fix: #1516
When we introduced the SPDX license blurb in [0] we incorrectly gave
attribution to Andrew when the original file author had the attribution
as "the rust-bitcoin developers". The original author [1] was Tamas
Blummer and he copied this code from code he wrote and explicitly
re-licenses it. In order to make the re-licensing comment a little
clearer and fix the mis-attribution use Tamas' name in the attribution.
[0] commit: `91ff2f628ce7db732d234a812e29fa8508f501a1 Introduce SPDX license identifiers`
[1] commit: `c93a70487f81a93c7d479ae046c75590d9fb7733 Add client side block filter (BIP158) (#281)`
The term "final" is an archaic Bitcoin term however it is well used, it
exists in Bitcoin Core code as well as in various bips. To help folks
new to Bitcoin add documentation to the `is_final` method including
historical notes.
The `bip158` module uses a `HashSet` and in order to do so requires the
`hashbrown` dependency for "no-std" builds.
We can replace the usage of `HashSet` with a `BTreeSet` in `bip158` and
remove the `hashbrown` dependency entirely.
This patch makes no claims about performance cost or benefit of this
change. The patch also makes no claims about the validity of the current
`HashSet` usage.
The `hashbrown` dependency and `HashSet` usage can be trivially added
back in if someone comes up with perf data to back it up.
Previously `Debug` was implemented for both `Address<NetworkChecked>`
and `Address<NetworkUnchecked>`, but not for cases when the
`NetworkValidation` parameter was generic. This change adds this
ability.
In preparation for deprecating the `is_final` method; move the
`enables_absolute_lock_time` method to be directly above the `is_final`
method.
Refactor only, no logic changes.
`Sighash` does not need to implement `Encodable` because it is
claimed (I don't know exactly myself) that `Sighash` is never consensus
encode in Bitcoin.
We are currently relying on `Sighash` to implement `Encodable` when
encoding creating the segwit v0 sighash for a single input.
For reference, from BIP143:
If sighash type is SINGLE and the input index is smaller than the
number of outputs, hashOutputs is the double SHA256 of the output
amount with scriptPubKey of the same index as the input;
We can use `write_all` directly to write the hashed bytes and remove the
implementation of `Encodable` from the `Sighash` type.
While we are at it, use `write_all` to write the zero hash also to make
the code more uniform and understandable.
Fix: #1549
70fe07f1ce Export the DisplayHex trait from within prelude (Tobin C. Harding)
Pull request description:
We use `internals::hex::display::DisplayHex` in many places, we can improve ergonomics of the `internals` crate by re-exporting it from the `prelude` module.
ACKs for top commit:
Kixunil:
ACK 70fe07f1ce
apoelstra:
ACK 70fe07f1ce
Tree-SHA512: 96a89135cb0b829b7b5926a3b344f78e178b5b48e772a69da5133fab6d2e14e7b7bbaa56b7a417a5c1a64337546a1c7bac32307d3a1f27aa199ed61f590902bf
Currently the docs build commands in `hashes` and `bitcoin` differ, they
should be the same.
Add a command `cargo doc` to improve coverage e.g., recently we botched
the feature guarding but since CI only runs `cargo rustdoc` with custom
compiler conditional set we didn't catch it.
Run docs in CI using nightly and stable toolchains as required.
a7dd4b5ab0 Add a rustdoc test to Denomination (Tobin C. Harding)
Pull request description:
Add a rustdoc test to the `Denomination` type to show basic usage.
ACKs for top commit:
Kixunil:
ACK a7dd4b5ab0
apoelstra:
ACK a7dd4b5ab0
Tree-SHA512: 08f15c4641e70f043276b873e60fcf0e195fe50b6c5c18a245d2d609f4a4a4badc291aae1be532fc4890a91b2057cd308c86d0d3b85770b600a07499303a7bc4
Recently we introduced some mutation testing to the `pow` module but
testing is never done - add more `mutate` attributes and add unit tests
to ensure all mutants are killed.
Of note, the `from_compact` and `to_compact_lossy` functions are not
done, doing so results in a bunch of surviving mutants.
Parsing addresses from strings required a subsequent validation of
network of the parsed address. However, this validation was not
enforced by compiler, one had to remember to perform it.
This change adds a marker type to `Address` that will assist the
compiler in enforcing this validation.
We use `internals::hex::display::DisplayHex` in many places, we can
improve ergonomics of the `internals` crate by re-exporting it from the
`prelude` module.
1b0988833a Remove `ToHex` (Martin Habovstiak)
Pull request description:
The `ToHex` trait was replaced by either simple `Display`/`LowerHex` where appropriate or `DisplayHex` from `bitcoin_internals` which is faster.
This change replaces the usages and removes the trait.
ACKs for top commit:
tcharding:
ACK 1b0988833a
apoelstra:
ACK 1b0988833a
Tree-SHA512: a1b508e24ac247a0692c01b7cb1e7fa8f23fbfa3d6c3d5dfe669eec01f940a96c3f88508cb0b2e3529eebd9cb51e7d41435dbd5f4cbaf3bc14b9c7e7d790308b
a400757676 Add failing tests from serde-json (sanket1729)
b3246bf73f Fix LeafVersion serde (sanket1729)
Pull request description:
The default implementation maps to visit_u64. The current implementation
does not roundtrip with many deserializers, including serde_json. See
the failing test in the second commit
ACKs for top commit:
Kixunil:
tACK a400757676
tcharding:
ACK a400757676
Tree-SHA512: a6307b799b0bb4af398addc9ddbff0d811b632d1cc6ab4bdd77aaf3f151e48dd1cd10e3f90a71ef8fc3feb1cd988f7a719f92ec745b5fdf2ae0f0ad97ab2fa10
The `ToHex` trait was replaced by either simple `Display`/`LowerHex`
where appropriate or `DisplayHex` from `bitcoin_internals` which is
faster.
This change replaces the usages and removes the trait.
94b678e73f Rename `push_scriptint` and make it private (Martin Habovstiak)
Pull request description:
`push_scriptint` is a significant footgun with an unclear name. This renames it and unpublishes to avoid mistakes by downstream crates.
Closes#1517
ACKs for top commit:
apoelstra:
ACK 94b678e73f
Tree-SHA512: 9e1772c6fb326d8b0c78d702ad9926a79a91589feb8650aed7c5e75bfbdbf0164357b4d5b190877c40b8469e0e3be3d3453fe407741b5dae0c5758176f756417
c4363e5ba1 Deserialize Psbt fields, don't consensus_encode (DanGould)
c1dd6ad8a2 Serialize Psbt fields, don't consensus_encode them (DanGould)
1b7b08aa5d De/serialize Psbt without consensus traits (DanGould)
Pull request description:
fix https://github.com/rust-bitcoin/rust-bitcoin/issues/934
Instead of using consensus {de,en}code, serialize high-level structures (PartiallySignedTransaciton, Output, Input) borrow the signature from `Serialize`, `Deserialize` traits and implement them on Psbt:
```rs
impl Psbt {
/// Serialize a value as raw data.
fn serialize(&self) -> Vec<u8>;
/// Deserialize a value from raw data.
fn deserialize(bytes: &[u8]) -> Result<Self, encode::Error>;
}
```
ACKs for top commit:
apoelstra:
ACK c4363e5ba1
tcharding:
ACK c4363e5ba1
sanket1729:
ACK c4363e5ba1. One small comment, but can be addressed in follow up if there is nothing else from other reviewers.
Kixunil:
ACK c4363e5ba1
Tree-SHA512: d8dd5ef1189b36fde08969b1ec36006a6fc55ae990f62ea744e6669e03a7f9e90e1d5907be7eac48ee1af23bc20a62aa7e60ff1ff78080d0e923bb9ccedcd432
The default implementation maps to visit_u64. The current implementation
does not roundtrip with many deserializers, including serde_json. See
failing test in the subsequent commit
089a1e452d Replace `Vec::from_hex` with `hex!` (Martin Habovstiak)
Pull request description:
This makes the code less noisy and is a preparation for changing it to `const`-based literal. Because of the preparation, places that used variables to store the hex string were changed to constants.
There are still some instances of `Vec::from_hex` left - where they won't be changeable to `const` and where `hex!` is unavailable (integration tests). These may be dealt with later.
See also #1189
Note that while the change appears big it's nearly entirely mechanical, so should be pretty easy to review. (But I don't feel it's `trivial`.)
ACKs for top commit:
apoelstra:
ACK 089a1e452d
tcharding:
ACK 089a1e452d
Tree-SHA512: c55357b8cffc86f8e107c0f8390490ede75948260018f63dc926455700cbcaf422f6ae72444f93943065e6f4ffa4335bee9160a64184ea4f8e91721f30a46ace
This makes the code less noisy and is a preparation for changing it to
`const`-based literal. Because of the preparation, places that used
variables to store the hex string were changed to constants.
There are still some instances of `Vec::from_hex` left - where they
won't be changeable to `const` and where `hex!` is unavailable
(integration tests). These may be dealt with later.
See also #1189
The additional `use` items were added to improve the style of
documentation. Because they were only used for doc they were `cfg`ed.
But because this is independent from being built by `docs.rs` the `cfg`
should've been `doc` not `docsrs`.
IOW `docsrs` means roughly `all(doc, nightly)` and the added items are
unrelated to `nightly`.
941083ec4e Remove rand-std dev-dependency from secp256k1 (Tobin C. Harding)
f71335f971 Add rand-std feature (Tobin C. Harding)
Pull request description:
This PR uncovered incorrect feature gating in `secp256k1`, fixed in https://github.com/rust-bitcoin/rust-secp256k1/pull/519
Currently we enable "secp256k1/rand-std" in the "rand" feature, this is incorrect because it means "rand" implies "std" which it does not.
Add a "rand-std" feature that turns on "seck256k1/rand-std" and make the "rand" feature turn on "seck256k1/rand".
Fix: #1384
ACKs for top commit:
sanket1729:
ACK 941083ec4e
apoelstra:
ACK 941083ec4e
Tree-SHA512: e1d12196766c2b3082b488fe7d0c03a865ebbfc70ffa6f128c57074df14da71e56c31ea92982991d376ac62fa86c6639049ce17aac93613b523ddcc99677c269
64f7d2549e Fix wrong newtype APIs (Martin Habovstiak)
Pull request description:
This fixes several API bugs:
* Use `TryFrom` instead of `From` for fallible conversions
* Move byte conversion methods from `impl_array_newtype` to `impl_bytes_newtype`
* Add missing trait impls like `AsRef`, `Borrow`, their mutable versions and infallible conversions from arrays
Closes#1336
Notably this change is much less bad than even I expected and some places actually get cleaner than before.
ACKs for top commit:
tcharding:
ACK 64f7d2549e
sanket1729:
code review ACK 64f7d2549e
apoelstra:
ACK 64f7d2549e
Tree-SHA512: 0c5ab19aa1a01ccf34ad1c2f3f66bd003a3142a840cb7cf57e1449e721f6eed1523555c62f0e3391012708767ff86315ff7edce9afd85017a67c61cfe57dddbc
ca471557a5 locktime: Add mutation testing (Tobin C. Harding)
26c0da41b4 locktime: Add inline to public functions (Tobin C. Harding)
Pull request description:
Add mutation testing to the `locktime` module and add unit tests to cover all mutants and ensure they are killed.
ACKs for top commit:
sanket1729:
ACK ca471557a5. How stable are the mutation tests?
apoelstra:
ACK ca471557a5
Tree-SHA512: 46a59c90fc25b0c803e96f7c5b98bd39055f7835e45ba137b2a01ad4221a676c54bc228b9ef7663b7300bb4260a6c2c80a0820c4f1bf0987650e1e2bd699f62d
cf9733d678 Verify and fix mul_u64 (Tobin C. Harding)
Pull request description:
Add kani verification for `U256::mul_u64`, doing so uncovered a bug in the current implementation due to overflow. Re-write the `mul_u64` method.
Fix: #1497
## Note
This PR now _only_ tests `mul_u64`, I will add testing div as a separate PR.
ACKs for top commit:
Kixunil:
ACK cf9733d678
apoelstra:
ACK cf9733d678
Tree-SHA512: c044fb385073bb068412195c079f26057ba560b2a7eca22693c360d324a7c0119cf10da2e70e544c69e05ca8269a81156418d307f4e11d8a4c1c1a7cafa32d0c
5a2a37d4be Allow dead_code/unused_imports when fuzzing (Tobin C. Harding)
Pull request description:
Littering the codebase with `#[cfg(not(fuzzing))]` is a bit messy just to quieten the linter during fuzzing. Instead just globally allow.
Done while debugging #1409
ACKs for top commit:
sanket1729:
ACK 5a2a37d4be
apoelstra:
ACK 5a2a37d4be
Tree-SHA512: fb84215a2b00ad6d3321b2781ba285af513ff8fd413c0997045a41c4f23028d2ef0fdf083839289d0c5108c990aa66bdae4430ad3ef32881eac5324b2e881b3b
e0bc50953a Make `Witness::tapscript()` return `Script` instead of raw bytes (Jiri Jakes)
Pull request description:
Since there is unsized `Script` now, this method can return it.
ACKs for top commit:
sanket1729:
utACK e0bc50953a
tcharding:
ACK e0bc50953a
Tree-SHA512: 32d4ca14f1b0fc1029f7376b1a43db90332b869a806609c82f660cb2690a4f0e1b91e1799fdac0d43c8a630aed0331f251d4159a662e86e5942c6fb698c42cd2
Add `#[inline]` to all public functions/methods excluding error types
and `Display` impls. Error paths do not need to be fast and presumably
`Display` is called on code paths that do IO so this also does not need
to be fast.
Add kani verification for `U256::mul_u64`, doing so uncovered a bug in
the current implementation due to overflow.
Re-write the `mul_u64` method.
Props to Elichai for the algorithm.
Co-authored-by: Elichai Turkel <elichai.turkel@gmail.com>
920599da94 Add test for previous commit (Martin Habovstiak)
a7f3458c27 Fix bug in `ScriptBuf::extend` for short iterators (Martin Habovstiak)
Pull request description:
`ScriptBuf::extend` contained an optimization for short scripts that was
supposed to preallocate the buffer and then fill it. By mistake it
attempted to fill it from already-exhausted iterator instead of the
temporary array it drained the items into. This obviously produced
garbage (empty) values.
This was not caught by tests because the optimization is only active for
scripts with known maximum length and the test used `Instructions` which
doesn't know the maximum length.
ACKs for top commit:
sanket1729:
ACK 920599da94 . Tested that the bug is correctly fixed and tested in the new test
tcharding:
ACK 920599da94
Tree-SHA512: a80f5f262a840d8e77efd42d63c511224380ee3efa6c31855233e81c90332ac15db228e8d552d039d729d7b642e03c3939c8b6a92d3279001377515acb83abea
8ce928b8e7 Add testing section to readme (Tobin C. Harding)
2e79a0bdc4 Introduce mutation testing (Tobin C. Harding)
Pull request description:
Introduce mutation testing by way of mutagen [0] (see #1484 for context).
- Conditionally add the dev-dependency `mutagen` (using `do_mutate` flag)
This flag is not very well named but `mutagen` and `mutate` are already taken?
- Mutate all methods of the `U256` struct that do not require additional unit tests.
Uses `cfg(all(test, do_mutate), mutate)` - I cannot workout why we need to check on `test` as well i.e., I don't understand why we cannot use `cfg(do_mutate, mutate)`?
With this applied test can be run as usual with a stable toolchain. To run mutagen we use `RUSTFLAGS='--cfg=do_mutate' cargo +nightly mutagen` (doing so runs 29 mutants).
[0] https://github.com/llogiq/mutagen
ACKs for top commit:
Kixunil:
ACK 8ce928b8e7
apoelstra:
ACK 8ce928b8e7
Tree-SHA512: 024ba5d2dc983f7cd0444e09ba13280771157204999d2a44502e07a1d6436f571b75003c7cb543c943f17949b848d4070eda4e194bda774a3e41443ff79af0af
This fixes several API bugs:
* Use `TryFrom` instead of `From` for fallible conversions
* Move byte conversion methods from `impl_array_newtype` to
`impl_bytes_newtype`
* Add missing trait impls like `AsRef`, `Borrow`, their mutable versions
and infallible conversions from arrays
Closes#1336
`ScriptBuf::extend` contained an optimization for short scripts that was
supposed to preallocate the buffer and then fill it. By mistake it
attempted to fill it from already-exhausted iterator instead of the
temporary array it drained the items into. This obviously produced
garbage (empty) values.
This was not caught by tests because the optimization is only active for
scripts with known maximum length and the test used `Instructions` which
doesn't know the maximum lenght.
In order to get better test coverage we should not enable the secp26k1
feature "rand-std" in dev-dependencies but instead feature gate tests
that depend on this feature.
Currently we enable "secp256k1/rand-std" in the "rand" feature, this is
incorrect because it means "rand" implies "std" which it does not.
Add a "rand-std" feature that turns on "seck256k1/rand-std" and make the
"rand" feature turn on "seck256k1/rand".
Introduce mutation testing by way of mutagen [0]
- Conditionally add the dev-dependency `mutagen` (using `do_mutate`
flag)
This flag is not very well named but `mutagen` and `mutate` are already
taken?
- Mutate all methods of the `U256` struct that do not require additional
unit tests.
Uses `cfg(all(test, do_mutate), mutate)` - I cannot workout why we need
to check on `test` as well i.e., I don't understand why we cannot use
`cfg(do_mutate, mutate)`?
With this applied test can be run as usual with a stable toolchain. To
run mutagen we use `RUSTFLAGS='--cfg=do_mutate' cargo +nightly mutagen`.
[0] https://github.com/llogiq/mutagen
This is useful when one already has bytes allocated in a vec that can be
reused.
The change also documents that the mirror method `into_bytes()` doesn't
allocate.
bae264d0c2 Add `tapscript_leaf_hash()` to `Script` (Jiri Jakes)
Pull request description:
Adds convenience method to `Script` for computing leaf hash of tapscript. Closes#1482.
The little test case is taken from `bip341_tests.json`.
ACKs for top commit:
Kixunil:
ACK bae264d0c2
apoelstra:
ACK bae264d0c2
Tree-SHA512: fb7a3a552017208decd56ca7d27eab1987a3a92aae5b8620896b3a02986c2fc13043c200ccfbadf9cfdd2d74af38b0bc25936338f55b7d318c1296acc88bf22a
6acf9ac8b8 Patch hashes and update the code (Martin Habovstiak)
Pull request description:
This patches `bitcoin_hashes` to use the version in the repository and fixes the code after removal of `Deref`.
ACKs for top commit:
tcharding:
ACK 6acf9ac8b8
apoelstra:
ACK 6acf9ac8b8
Tree-SHA512: b779fa79309f1d648020146b58e96346b67e9f76e29551cbd50251ea6bb7bcfc4c5d42f49cc7ad5660dcd0a5f6306efe96dfcd9530e4b32c62edf4af7076d830
This patches `bitcoin_hashes` to use the version in the repository and
fixes the code after removal of `Deref`.
This also turns off `AS_DEPENDENCY` check with the intention to refactor
it later.
02c1cd6291 add some documentation clarifying the locktime ordering shenanigans in #1330 (Andrew Poelstra)
Pull request description:
Updates the CHANGELOG and also the doccomment on `Transaction`.
ACKs for top commit:
tcharding:
ACK 02c1cd6291
Kixunil:
ACK 02c1cd6291
sanket1729:
ACK 02c1cd6291
Tree-SHA512: e2d23a90fb1e53758449fe49a3db7ae1497a260ce7efcade4b50265fa70840db273609019590d9d0a69e1272607a6bcf37924b805b4f09909487eb0c3b91a3cd
This renames `Script` to `ScriptBuf` and adds unsized `Script` modeled
after `PathBuf`/`Path`. The change cleans up the API a bit, especially
all functions that previously accepted `&Script` now accept truly
borrowed version. Some functions that perviously accepted `&[u8]` can
now accept `&Script` because constructing it is no loger costly.
1b15a13e5a run cargo clippy and fmt (Andrew Poelstra)
f2a5596899 examples: clean up taproot PSBT example locktime handling (Andrew Poelstra)
821842e1a1 drop Ord on absolute::LockTime; add Ord to Transaction (Andrew Poelstra)
5b7d801ee6 remove PackedLockTime type (Andrew Poelstra)
4dee116b8a delete PackedLockTime by aliasing it to LockTime (Andrew Poelstra)
fa81568fb6 locktime: add `FromHexStr` impl for `LockTime` (Andrew Poelstra)
74ff4946e4 locktime: unify serde impls (Andrew Poelstra)
Pull request description:
This is potentially a controversial PR, but hear me out. My argument is that right now `absolute::LockTime` and `PackedLockTime` are basically identical types; they can be converted between each other with `From` and they have exactly the same semantics except that `PackedLockTime` has `Ord` on it. This makes it very confusing to tell which one should be used, for example in PSBT2 where there are now extra locktime-related fields.
The motivation for having `PackedLockTime` independent of `LockTime` are:
* `PackedLockTime` is theoretically more efficient because you don't need to unpack the field, don't need to store a enum discriminate, etc. I don't buy this. If you are trying to save individual bytes in your transaction-parsing there are lots of places you'd look before messing around with locktimes, so we shouldn't privilege this specific thing.
* `PackedLockTIme` has an `Ord` impl, and removing that will have a cascading effect on transactions, blocks, etc., preventing them from being held in `BTreeMaps` etc. **My proposal**, implemented here, is to just manually impl `Ord` on `Transaction` and don't impl it on `LockTime`.
I recall some argument that we need to be able to sort miniscripts, and miniscripts might have locktimes in them, but I think this is wrong -- miniscripts always have explicitly either a `Height` or a `Time`, and there is no problem ordering these.
Closes#1455
ACKs for top commit:
sanket1729:
code review ACK 1b15a13e5a
tcharding:
ACK 1b15a13e5a
Tree-SHA512: d9776f14560c3822980e8fff8978bf8ca753035152f957a84af25d063c66e4e9df3d5cf98af38d6cb59cc52ba407282f722925b1ca670ae623ac91add3149d2f
b7a84d0c68 hashes: Do not implement Deref (Tobin C. Harding)
Pull request description:
Currently we implement `Deref` for hashes. From the docs [0]
> Deref should only be implemented for smart pointers to avoid confusion
Furthermore because we implement `Deref` as well as implement `internals::hex::display::DisplayHex` for slices hashes get coerced into slices and `to_lower_hex_string` can be called on them, this is incorrect because `DisplayHex` does not account for hashes that display backwards so we end up with the wrong string.
This is an API breaking change, and I have not built any other crates in our stack to check if anything breaks.
[0] https://doc.rust-lang.org/std/ops/trait.Deref.html
ACKs for top commit:
apoelstra:
ACK b7a84d0c68
sanket1729:
ACK b7a84d0c68
Tree-SHA512: 573169095dd9f9032e3f685e3b0af3b569f1cb5368e1b2f37940504fd887592f46488abcfbe84107baf9c5453c7db47bc342a6bc273ff98cb0570ffacb549c21
This still has the line
let lock_time = absolute::LockTime::from_height(psbt.unsigned_tx.lock_time.to_consensus_u32() + lock_time_delta).unwrap();
I'm unsure whether this "adding height to a locktime" concept is a
meaningful thing or just the sort of thing that shows up in example
code. Maybe we should have first-class support for it.
Note that the line, as written, depends on the fact that the original
locktime was a small blockheight. A proper function for this would
handle the exceptional case gracefully.
Currently we implement `Deref` for hashes. From the docs [0]
> Deref should only be implemented for smart pointers to avoid confusion
Furthermore because we implement `Deref` as well as implement
`internals::hex::display::DisplayHex` for slices hashes get coerced into
slices and `to_lower_hex_string` can be called on them, this is
incorrect because `DisplayHex` does not account for hashes that display
backwards so we end up with the wrong string.
[0] https://doc.rust-lang.org/std/ops/trait.Deref.html
This can be replicated by deleting the `type PackedLockTime = LockTime'
line, and then running
find . -type f | xargs sed -i 's/PackedLockTime/LockTime/g
at the root of the repo.
e00dfa9806 impl FromHexStr for structs with single u32 member (connormullett)
Pull request description:
Closes: #1112
- Adds new trait `FromStrHex` with 2 methods: `from_hex_str` and `from_hex_str_no_prefix`
- Impl new trait on each tuple struct with single u32 member. eg `Time(u32)`
As stated in the issue, grep through codebase with `\(u32\)` and `\(pub u32\)` to see all implementations and verify none were missed.
NonStandardSighashType is an error type and should never be constructed from a hex string. Therefore, it has been omitted from this change.
Tests are somewhat redundant, but cover 4 cases each. 2 happy paths, 1 for each function. 1 case for malformed/invalid hex input, and 1 for calling no_prefix without a prefix
ACKs for top commit:
Kixunil:
ACK e00dfa9806
apoelstra:
ACK e00dfa9806
Tree-SHA512: 221faef7fc1fa8fdb4cba79cfae317a0b63984937c345c6ca2287123a078f38911cdc07db7589a88b7bc6fbecf389e9bcff47952728410510ffcfc1857e0f91f
Adds new module `string` to be later converted to its own
crate. The module currently contains the FromHexStr trait and an error
type to be used for implementing hex parsing on types. This change
also adds implementations of FromHexStr for types with a single u32
member such as `Sequence(pub u32)`. All structs that match the
following regex have been given this implementation
`\(u32\)` and `\(pub u32\)`. All implementations have associated
unit tests matching all possible cases. NonStandardSighashType has
been ommitted from this change as it is an error and should not be
constructed using the methods added in this change.
Adds parse::hex_u32 for future use to be made generic to allow
different sizes of integers to be parsed from hex strings.
The error type FromHexError implements required traits such as
Display and std::error::Error
d7006ef80d Adds roundtrip tests for Network::from_core_arg (Sergi Delgado Segura)
bd1eb29f61 Adds Network::to_core_arg (Sergi Delgado Segura)
Pull request description:
Comming from https://github.com/rust-bitcoin/rust-bitcoincore-rpc/pull/247
`Network::from_str` only considers `rust-bitcoin` string as possible inputs to create a `Network` instance. This PR adds a new method to `Network`, `from_core_arg`, which is complementary to the existing `Network::to_core_arg`. This method allows the conversion between `bitcoind -network` string and `Network` variants.
This also links `Network::from_str` to `Network::from_core_arg` so the default case on the former calls the latter, and an error is only returned if none of the cases match.
ACKs for top commit:
Kixunil:
ACK d7006ef80d
apoelstra:
ACK d7006ef80d
Tree-SHA512: 97a66f858a7d4a3642bdef9016457833cfc1181e276f7ead7c6b87f6fcdcb7c5d1cfdb4b621225b806bc5949c3c5cc6a32b7df934157542d7c79aa00a9e20f41
c822fcf435 Remove helper variables (Martin Habovstiak)
70d1a0348e Use serde derive rather than manual parsing (Martin Habovstiak)
Pull request description:
Manual parsing Json is tedious and error-prone. It contained a bunch of
`unwrap`s and was hard to read.
This replaces manual Json parsing with serde_derive implementation.
Closes https://github.com/rust-bitcoin/rust-bitcoin/issues/1231
ACKs for top commit:
apoelstra:
ACK c822fcf435
tcharding:
ACK c822fcf435
Tree-SHA512: 0e1df6a62dc8438d67979a00658f8a2820135beebdd01d7275149b06feacd0d18accb86cecbf8c85f9a02ddc724575eaffff079b45cfe0e7765c8559c5eb03f7
Recently we (tcharding) do some mechanical improvements to the rustdocs
in the `blockdata` module without considering the content. On review a
bunch of improvements were suggested.
Improve the content of various rustdoc comments in the `blockdata`
module.
Suggested content came from reviewers, all mistakes are my own :)
Reduce the number of lines of code by using a longer column width, 100
as is more-or-less standard in this repo.
This patch only changes column width (line length), no other changes.
Andrew Poelstra
Peter Todd
Tobin C. Harding
The rustfmt Tyranny
yancy
Steven Roose
Sergi Delgado Segura
Harshil Jani
Martin Habovstiak
junderw
Salvatore Ingala
Subhradeep Chakraborty
sanket1729
benthecarman
hashmap
Lorenzo Maturano
DanGould
Arturo Marquez
Jiri Jakes
bnabi
Casey Rodarmor
Byron Hambly
Daniela Brozzoni
connormullett
