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