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
`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.
Introduce mutation testing by way of mutagen [0]
- Conditionally add the dev-dependency `mutagen` (using `do_mutate`
flag)
This flag is not very well named but `mutagen` and `mutate` are already
taken?
- Mutate all methods of the `U256` struct that do not require additional
unit tests.
Uses `cfg(all(test, do_mutate), mutate)` - I cannot workout why we need
to check on `test` as well i.e., I don't understand why we cannot use
`cfg(do_mutate, mutate)`?
With this applied test can be run as usual with a stable toolchain. To
run mutagen we use `RUSTFLAGS='--cfg=do_mutate' cargo +nightly mutagen`.
[0] https://github.com/llogiq/mutagen
This is useful when one already has bytes allocated in a vec that can be
reused.
The change also documents that the mirror method `into_bytes()` doesn't
allocate.
bae264d0c2 Add `tapscript_leaf_hash()` to `Script` (Jiri Jakes)
Pull request description:
Adds convenience method to `Script` for computing leaf hash of tapscript. Closes#1482.
The little test case is taken from `bip341_tests.json`.
ACKs for top commit:
Kixunil:
ACK bae264d0c2
apoelstra:
ACK bae264d0c2
Tree-SHA512: fb7a3a552017208decd56ca7d27eab1987a3a92aae5b8620896b3a02986c2fc13043c200ccfbadf9cfdd2d74af38b0bc25936338f55b7d318c1296acc88bf22a
6acf9ac8b8 Patch hashes and update the code (Martin Habovstiak)
Pull request description:
This patches `bitcoin_hashes` to use the version in the repository and fixes the code after removal of `Deref`.
ACKs for top commit:
tcharding:
ACK 6acf9ac8b8
apoelstra:
ACK 6acf9ac8b8
Tree-SHA512: b779fa79309f1d648020146b58e96346b67e9f76e29551cbd50251ea6bb7bcfc4c5d42f49cc7ad5660dcd0a5f6306efe96dfcd9530e4b32c62edf4af7076d830
This patches `bitcoin_hashes` to use the version in the repository and
fixes the code after removal of `Deref`.
This also turns off `AS_DEPENDENCY` check with the intention to refactor
it later.
02c1cd6291 add some documentation clarifying the locktime ordering shenanigans in #1330 (Andrew Poelstra)
Pull request description:
Updates the CHANGELOG and also the doccomment on `Transaction`.
ACKs for top commit:
tcharding:
ACK 02c1cd6291
Kixunil:
ACK 02c1cd6291
sanket1729:
ACK 02c1cd6291
Tree-SHA512: e2d23a90fb1e53758449fe49a3db7ae1497a260ce7efcade4b50265fa70840db273609019590d9d0a69e1272607a6bcf37924b805b4f09909487eb0c3b91a3cd
This renames `Script` to `ScriptBuf` and adds unsized `Script` modeled
after `PathBuf`/`Path`. The change cleans up the API a bit, especially
all functions that previously accepted `&Script` now accept truly
borrowed version. Some functions that perviously accepted `&[u8]` can
now accept `&Script` because constructing it is no loger costly.
1b15a13e5a run cargo clippy and fmt (Andrew Poelstra)
f2a5596899 examples: clean up taproot PSBT example locktime handling (Andrew Poelstra)
821842e1a1 drop Ord on absolute::LockTime; add Ord to Transaction (Andrew Poelstra)
5b7d801ee6 remove PackedLockTime type (Andrew Poelstra)
4dee116b8a delete PackedLockTime by aliasing it to LockTime (Andrew Poelstra)
fa81568fb6 locktime: add `FromHexStr` impl for `LockTime` (Andrew Poelstra)
74ff4946e4 locktime: unify serde impls (Andrew Poelstra)
Pull request description:
This is potentially a controversial PR, but hear me out. My argument is that right now `absolute::LockTime` and `PackedLockTime` are basically identical types; they can be converted between each other with `From` and they have exactly the same semantics except that `PackedLockTime` has `Ord` on it. This makes it very confusing to tell which one should be used, for example in PSBT2 where there are now extra locktime-related fields.
The motivation for having `PackedLockTime` independent of `LockTime` are:
* `PackedLockTime` is theoretically more efficient because you don't need to unpack the field, don't need to store a enum discriminate, etc. I don't buy this. If you are trying to save individual bytes in your transaction-parsing there are lots of places you'd look before messing around with locktimes, so we shouldn't privilege this specific thing.
* `PackedLockTIme` has an `Ord` impl, and removing that will have a cascading effect on transactions, blocks, etc., preventing them from being held in `BTreeMaps` etc. **My proposal**, implemented here, is to just manually impl `Ord` on `Transaction` and don't impl it on `LockTime`.
I recall some argument that we need to be able to sort miniscripts, and miniscripts might have locktimes in them, but I think this is wrong -- miniscripts always have explicitly either a `Height` or a `Time`, and there is no problem ordering these.
Closes#1455
ACKs for top commit:
sanket1729:
code review ACK 1b15a13e5a
tcharding:
ACK 1b15a13e5a
Tree-SHA512: d9776f14560c3822980e8fff8978bf8ca753035152f957a84af25d063c66e4e9df3d5cf98af38d6cb59cc52ba407282f722925b1ca670ae623ac91add3149d2f
b7a84d0c68 hashes: Do not implement Deref (Tobin C. Harding)
Pull request description:
Currently we implement `Deref` for hashes. From the docs [0]
> Deref should only be implemented for smart pointers to avoid confusion
Furthermore because we implement `Deref` as well as implement `internals::hex::display::DisplayHex` for slices hashes get coerced into slices and `to_lower_hex_string` can be called on them, this is incorrect because `DisplayHex` does not account for hashes that display backwards so we end up with the wrong string.
This is an API breaking change, and I have not built any other crates in our stack to check if anything breaks.
[0] https://doc.rust-lang.org/std/ops/trait.Deref.html
ACKs for top commit:
apoelstra:
ACK b7a84d0c68
sanket1729:
ACK b7a84d0c68
Tree-SHA512: 573169095dd9f9032e3f685e3b0af3b569f1cb5368e1b2f37940504fd887592f46488abcfbe84107baf9c5453c7db47bc342a6bc273ff98cb0570ffacb549c21
This still has the line
let lock_time = absolute::LockTime::from_height(psbt.unsigned_tx.lock_time.to_consensus_u32() + lock_time_delta).unwrap();
I'm unsure whether this "adding height to a locktime" concept is a
meaningful thing or just the sort of thing that shows up in example
code. Maybe we should have first-class support for it.
Note that the line, as written, depends on the fact that the original
locktime was a small blockheight. A proper function for this would
handle the exceptional case gracefully.
Currently we implement `Deref` for hashes. From the docs [0]
> Deref should only be implemented for smart pointers to avoid confusion
Furthermore because we implement `Deref` as well as implement
`internals::hex::display::DisplayHex` for slices hashes get coerced into
slices and `to_lower_hex_string` can be called on them, this is
incorrect because `DisplayHex` does not account for hashes that display
backwards so we end up with the wrong string.
[0] https://doc.rust-lang.org/std/ops/trait.Deref.html
This can be replicated by deleting the `type PackedLockTime = LockTime'
line, and then running
find . -type f | xargs sed -i 's/PackedLockTime/LockTime/g
at the root of the repo.
e00dfa9806 impl FromHexStr for structs with single u32 member (connormullett)
Pull request description:
Closes: #1112
- Adds new trait `FromStrHex` with 2 methods: `from_hex_str` and `from_hex_str_no_prefix`
- Impl new trait on each tuple struct with single u32 member. eg `Time(u32)`
As stated in the issue, grep through codebase with `\(u32\)` and `\(pub u32\)` to see all implementations and verify none were missed.
NonStandardSighashType is an error type and should never be constructed from a hex string. Therefore, it has been omitted from this change.
Tests are somewhat redundant, but cover 4 cases each. 2 happy paths, 1 for each function. 1 case for malformed/invalid hex input, and 1 for calling no_prefix without a prefix
ACKs for top commit:
Kixunil:
ACK e00dfa9806
apoelstra:
ACK e00dfa9806
Tree-SHA512: 221faef7fc1fa8fdb4cba79cfae317a0b63984937c345c6ca2287123a078f38911cdc07db7589a88b7bc6fbecf389e9bcff47952728410510ffcfc1857e0f91f
Adds new module `string` to be later converted to its own
crate. The module currently contains the FromHexStr trait and an error
type to be used for implementing hex parsing on types. This change
also adds implementations of FromHexStr for types with a single u32
member such as `Sequence(pub u32)`. All structs that match the
following regex have been given this implementation
`\(u32\)` and `\(pub u32\)`. All implementations have associated
unit tests matching all possible cases. NonStandardSighashType has
been ommitted from this change as it is an error and should not be
constructed using the methods added in this change.
Adds parse::hex_u32 for future use to be made generic to allow
different sizes of integers to be parsed from hex strings.
The error type FromHexError implements required traits such as
Display and std::error::Error
d7006ef80d Adds roundtrip tests for Network::from_core_arg (Sergi Delgado Segura)
bd1eb29f61 Adds Network::to_core_arg (Sergi Delgado Segura)
Pull request description:
Comming from https://github.com/rust-bitcoin/rust-bitcoincore-rpc/pull/247
`Network::from_str` only considers `rust-bitcoin` string as possible inputs to create a `Network` instance. This PR adds a new method to `Network`, `from_core_arg`, which is complementary to the existing `Network::to_core_arg`. This method allows the conversion between `bitcoind -network` string and `Network` variants.
This also links `Network::from_str` to `Network::from_core_arg` so the default case on the former calls the latter, and an error is only returned if none of the cases match.
ACKs for top commit:
Kixunil:
ACK d7006ef80d
apoelstra:
ACK d7006ef80d
Tree-SHA512: 97a66f858a7d4a3642bdef9016457833cfc1181e276f7ead7c6b87f6fcdcb7c5d1cfdb4b621225b806bc5949c3c5cc6a32b7df934157542d7c79aa00a9e20f41
c822fcf435 Remove helper variables (Martin Habovstiak)
70d1a0348e Use serde derive rather than manual parsing (Martin Habovstiak)
Pull request description:
Manual parsing Json is tedious and error-prone. It contained a bunch of
`unwrap`s and was hard to read.
This replaces manual Json parsing with serde_derive implementation.
Closes https://github.com/rust-bitcoin/rust-bitcoin/issues/1231
ACKs for top commit:
apoelstra:
ACK c822fcf435
tcharding:
ACK c822fcf435
Tree-SHA512: 0e1df6a62dc8438d67979a00658f8a2820135beebdd01d7275149b06feacd0d18accb86cecbf8c85f9a02ddc724575eaffff079b45cfe0e7765c8559c5eb03f7
Recently we (tcharding) do some mechanical improvements to the rustdocs
in the `blockdata` module without considering the content. On review a
bunch of improvements were suggested.
Improve the content of various rustdoc comments in the `blockdata`
module.
Suggested content came from reviewers, all mistakes are my own :)
Reduce the number of lines of code by using a longer column width, 100
as is more-or-less standard in this repo.
This patch only changes column width (line length), no other changes.
Andrew Poelstra
sanket1729
Tobin C. Harding
Jiri Jakes
Martin Habovstiak
Byron Hambly
Daniela Brozzoni
Casey Rodarmor
connormullett
Sergi Delgado Segura