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
6e56feed57 bip158: Replace usage of HashSet with BTreeSet (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
Kixunil:
ACK 6e56feed57
apoelstra:
ACK 6e56feed57
Tree-SHA512: 6e8d6af7ccf22031a22ea19b731fbe5f6cbfdfb1e510119a7dbdd2e8521eeb3b0d2c7b9e000641deae8960287099dbf58515341720cf7c41d1fcab08308b0b74
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.
53ee42d7c1 Fix ASAN in CI (Tobin C. Harding)
Pull request description:
When we merged `hashes` into `rust-bitcoin` we ran the test script a couple of times using `./hashes/contrib/test.sh` this causes the `cargo` commands in the CI script to be run from the crate root which is not what we want.
This showed up recently because `cargo test` was getting run in `bitcoin` after building with address/memory sanitazation configured into the build.
While we are at it run `cargo clean` after the last sanitizer build just to future proof the CI script in case we later accidentally re-use the same build with plain old `cargo` (without `-Zbuild-std etc.`).
ACKs for top commit:
apoelstra:
ACK 53ee42d7c1
sanket1729:
ACK 53ee42d7c1
Tree-SHA512: 3d0b9aa35be6290808e2d23144e4dce467ab433862b375e1f164dc41261919e8e27e4cab117bbe9bfe18814ec7bf468e1c03674d20e9f36b5319e986fc329e89
When we merged `hashes` into `rust-bitcoin` we ran the test script a
couple of times using `./hashes/contrib/test.sh` this causes the `cargo`
commands in the CI script to be run from the crate root which is not
what we want.
This showed up recently because `cargo test` was getting run in
`bitcoin` after building with address/memory sanitazation configured
into the build.
While we are at it run `cargo clean` after the last sanitizer build just
to future proof the CI script in case we later accidentally re-use the same
build with plain old `cargo` (without `-Zbuild-std etc.`).
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
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
308c727c82 pow: Add more mutation testing (Tobin C. Harding)
948b04927d Remove unnecessary parenthesis (Tobin C. Harding)
c3021d852a Fix typo in code comment (Tobin C. Harding)
Pull request description:
Add more mutation testing to the `pow` module.
The first two patches are preparatory clean up.
All functions/methods that are non-trivial are tested excluding:
- `to_compact_lossy`
- `from_compact`
- `fmt_decimal`
I think its ok to not test `fmt_decimal`. The compact format ones require a bit of work to do but should be done at some stage.
ACKs for top commit:
Kixunil:
ACK 308c727c82
apoelstra:
ACK 308c727c82
Tree-SHA512: 1bd8df00521676c2193f482d6e3b557c727e7807211acc11c311aa715ebe94ae957352897c084474b22dc33c589382ef557c33f95c467929b7c1659128fdced0
f4e7def72f internals: Add CI test script (Tobin C. Harding)
db6b3f1df5 Fix method names in rustdoc (Tobin C. Harding)
7593f1f334 Use set -ex (Tobin C. Harding)
Pull request description:
Quick merge this while noone's looking ... we forgot to test the new `internals` crate in CI.
ACKs for top commit:
Kixunil:
ACK f4e7def72f
apoelstra:
ACK f4e7def72f
Tree-SHA512: c603d2308911210eeed17f128ef684ea161e33f5749d3d5567bb15ac4a01695eb07718873e53824bb21159427b453b04265db6728b9839a13092a6817ac9c4d8
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.
bef7c6e687 Use marker type to enforce validation of `Address`'s network (Jiri Jakes)
Pull request description:
Adds marker type `NetworkValidation` to `Address` to help compiler enforce network validation. Inspired by Martin's suggestion. Closes#1460.
Open questions:
1. Compilation fails with serde, which uses `Address:from_str` via macro `serde_string_impl!(Address, "a Bitcoin address");`. I don't think there is much we can do, so unless somebody has a better idea how to combine serde and network validation, I would just demacroed the macro for `Address` and add `unsafe_mark_network_valid` into it.
2. Would someone prefer wrapping the validation types by `mod validation`? As they are now, they live in `address` namespace so I don't think mod is necessary.
3. Almost all methods that used to be on `Address` are now on `Address<NetworkValid>` except one (`address_type`) that needs to be called on both and a few that are only on `Address<NetworkUnchecked>` (mainly `is_valid_for_network`). Some methods (e. g. `to_qr_uri`, `is_standard` and perhaps others) could be, theoretically, called on both valid and unchecked. I think we should encourage validating the network ASAP, so I would leave them on NetworkValid only, but I can move them if others have different opinion.
4. Should `NetworkValid` and `NetworkUnchecked` enums have some trait impls derived? The `PartialEq` was necessary for tests (I think `assert_eq` required it) but I am not sure whether some other would be good to have. The enums are only used as types so I guess it's not necessary, but also I do not fully understand why the `PartialEq` was needed.
ACKs for top commit:
Kixunil:
ACK bef7c6e687
apoelstra:
ACK bef7c6e687
Tree-SHA512: 8d18b25e62c594468625b39ab174b27ae04b98082cd6011283fe657e868a525f0e4cb40d0f68aff44ad145582e9f3c6387bd2077c5c1f2857d4032674121c31f
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.
f39cd88f5f Use TapNodeHash in NodeInfo (sanket1729)
5ff2635585 Rename TapBranchHash -> TapNodeHash (sanket1729)
Pull request description:
This does not completely fix#1393 but is a simple starter. Unfortunately, we still need hash_new_type for `TapNodeHash` because is exported as the Merkle root in taproot psbt. This requires serialization and display/from_str methods.
This also adds a method 1) `TapNodeHash::from_script` to deal with single leaf case cleanly. As a nice side effect, this removes the ugly uses of `sha256::Hash` that I had used to represent both `TapLeafHash` and `TapBranchHash`
Does not fix
1) Exporting `TapTweakHash`. This requires some changes to macros we use.
2) Safer usage of `TapNodeHash` by not making it `hash_new_type` and having guarded constructors. As mentioned above, this is required for psbts and sharing Merkle roots. Perhaps, we might need a new type while constructing trees that is not `hash_new_type`, and convert it to another type that represents the final Merkle root.
My overall feeling is that this is best addressed with more examples than changing and complicating existing code.
I don't feel strongly about the rename, so can go back if `TapBranchHash` instead of `TapNodeHash`.
ACKs for top commit:
Kixunil:
ACK f39cd88f5f
tcharding:
ACK f39cd88f5f
Tree-SHA512: 55310b89442ac1f73cce8a7202bbed6ac5a0bbefec0a938fe6065e65c65fe1d34c9e2b5e30ce56ae2bbd0cc9e4caa1363dbd03e9feb7929acb47bbac9f2a4191
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
16c49df688 Accept amounts with denominations with and without spaces (Casey Rodarmor)
Pull request description:
I didn't add tests for parsing with no space, but wanted to get a PR up to show the approach.
Fixes#1519.
ACKs for top commit:
apoelstra:
ACK 16c49df688
Kixunil:
ACK 16c49df688
Tree-SHA512: 651f12974a23b711a421005cc5905cb613bfdb092b03f7b0a0e2c02b3f9351f81eb985f848d313a17506e4960df7c01b40f673a100e4230a7045364acc4865de
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
8d58ee2ca3 Change `#[cfg(docsrs)]` to `#[cfg(doc)]` on `use` (Martin Habovstiak)
Pull request description:
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`.
Do we want CI for this kind of thing? It feels a bit too much to me but if someone sends a PR I probably won't reject it.
ACKs for top commit:
apoelstra:
ACK 8d58ee2ca3
Tree-SHA512: 89ee4d1a0a43595576850983a82aed9644e898d1ad9efdc982ca697ae2ad9ec894fe59f4c6d2a46aad86c7160d260da964c0bd3dba20b455d4bdbb3d0b09e695
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`.
411174c391 Add fuzz target for sha512_256 (Calvin Kim)
31fc1f8638 Add support for sha512/256 (Calvin Kim)
15b5af1117 Export sha512::HashEngine fields/function within the crate (Calvin Kim)
Pull request description:
Adds a new file named `sha512_256.rs` that implements the `sha512/256` hash. This was needed as a part of https://github.com/rust-bitcoin/rust-bitcoin/discussions/1318 to drop the `sha2` dependency.
All the actual hashing code is exactly the same as `sha512.rs`, minus the initial constants and the use of `hash_type!` macro. Some unit tests were added from wikipedia (for the "" input) and the rest were from the Go standard library's tests for sha512_256.
Benchmarks on my Ryzen 3600 machine show that it is faster than sha256.
```
test sha256::benches::sha256_10 ... bench: 37 ns/iter (+/- 0) = 270 MB/s
test sha256::benches::sha256_1k ... bench: 3,338 ns/iter (+/- 24) = 306 MB/s
test sha256::benches::sha256_64k ... bench: 213,605 ns/iter (+/- 1,806) = 306 MB/s
test sha512_256::benches::sha512_256_10 ... bench: 27 ns/iter (+/- 1) = 370 MB/s
test sha512_256::benches::sha512_256_1k ... bench: 2,196 ns/iter (+/- 12) = 466 MB/s
test sha512_256::benches::sha512_256_64k ... bench: 140,552 ns/iter (+/- 777) = 466 MB/s
```
One caveat is that I could not get hongfuzz to build locally so I couldn't test the fuzz on my machine. I ended up only testing through the CI for the fuzz tests.
I thought adding a completely separate file was the easiest and the most straightforward way of implementing it. I'm very much open to changing the implementation if you guys don't think this is the right direction.
ACKs for top commit:
sanket1729:
ACK 411174c391. Reviwed range diff from 43feb9ea7b282d9119708a27fa7a1c7412d1386a that I had ACked
apoelstra:
ACK 411174c391
Tree-SHA512: 98298a7c177cbb616bfbc02cec5c5860f10204df8275cc9f1e4ea07333b901095e574fbc3fe0a03375e0d321a1579e2c2023a5c14addd863e10cc927f155710c
3e520f9094 Use hex from internals rather than hashes (Martin Habovstiak)
Pull request description:
`bitcoin-internals` contains a more performant implementation of hex encoding than what `bitcoin_hashes` uses internally. This switches the implementations for formatting trait implementations as a step towards moving over completely.
The public macros are also changed to delegate to inner type which is technically a breaking change but we will break the API anyway and the consuers should only call the macro on the actual hash newtypes where the inner types already have the appropriate implementations.
Apart from removing reliance on internal hex from public API this reduces duplicated code generated and compiled. E.g. if you created 10 hash newtypes of SHA256 the formatting implementation would be instantiated 11 times despite being the same.
To do all this some other changes were required to the hex infrastructure. Mainly modifying `put_bytes` to accept iterator (so that `iter().rev()` can be used) and adding a new `DisplayArray` type. The iterator idea was invented by Tobin C. Harding, this commit just adds a bound check and generalizes over `u8` and `&u8` returning iterators.
While it may seem that `DisplayByteSlice` would suffice it'd create and initialize a large array even for small arrays wasting performance. Knowing the exact length `DisplayArray` fixes this.
Another part of refactoring is changing from returning `impl Display` to return `impl LowerHex + UpperHex`. This makes selecting casing less annoying since the consumer no longer needs to import `Case` without cluttering the API with convenience methods.
ACKs for top commit:
tcharding:
ACK 3e520f9094
apoelstra:
ACK 3e520f9094
Tree-SHA512: 62988cec17550ed35990386e572c0d32dc7107e1c36b7c9099080747e15167e6d66497fb300178afbd22481c0360a6b7a1228fd09402d4ce5d295a8594c02aa6
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
Andrew Poelstra
Tobin C. Harding
Jiri Jakes
bnabi
sanket1729
DanGould
Martin Habovstiak
Casey Rodarmor
