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
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 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
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
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.
Tobin C. Harding
Andrew Poelstra
Arturo Marquez
sanket1729
Jiri Jakes
bnabi
DanGould
Martin Habovstiak
Casey Rodarmor
Byron Hambly
Daniela Brozzoni
