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
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
Andrew Poelstra
sanket1729
Tobin C. Harding
Jiri Jakes
bnabi
DanGould
Martin Habovstiak
Casey Rodarmor
