Introduce arbitrary crate - #1662
Conversation
Coverage Report for CI Build 33098452929Coverage decreased (-0.08%) to 86.64%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
484d96c to
bfae954
Compare
benalleng
left a comment
There was a problem hiding this comment.
CACK Looks like a great start. interested to see what kinds of fuzzing we can start using on the state machine with this.
| bench = false | ||
|
|
||
| [[bin]] | ||
| name = "arbitrary_url" |
There was a problem hiding this comment.
needs to have the full pathname here url_arbitrary_url
|
@benalleng addressed your feedback on the target name. Anything else that needs updating before it's good to go? |
|
No, sorry I have just been focused on release things lately. I want to make a quick personal fork using the unreleased |
|
gotcha. I had started down that road a little bit, but a lot of stuff changed in the unreleased version 😆 |
@xstoicunicornx I am hoping that this dep addition will allow us to better fuzz the session states. |
Ok, I also tried this and ended up spending way too much time trying to patch every dep that relies on old bitcoin versions only to still got stuck. That is a big change... I will scope my test down a bit. |
caarloshenriq
left a comment
There was a problem hiding this comment.
tACK
Ran url_arbitrary_url for 30s locally; coverage grew from 430 to 738 without saturating, which is a meaningful improvement over the existing decode_url target.
One thing worth considering in arbitrary_url.rs: append_pair is immediately followed by clear_query, so the URL always exits that branch with an empty query. If the intent is to exercise methods that read a populated query, removing clear_query (or moving it to a separate branch) would cover that path. If the intent is specifically to test clear_query after append_pair, a comment explaining that would help.
This crate is used for fuzzing and is the same crate used in `rust-bitcoin`, which means that we will be able to use the arbitrary types built in that crate in this one
I moved some of the logic from the existing `decode_url` target into this one to utilize the fuzzer to test different inputs
|
I added the bitcoin arbitrary feature as well here to expand our ability to add arbitrary tests and impls across a wider range of the crate with support upstream in rust-bitcoin. The single largest limitation for our needs still I would say seems to be support for |
benalleng
left a comment
There was a problem hiding this comment.
ACK 439849b
@spacebear21 I will need an additional ACK here as I made the latest push
I've been trying to implement everything for PSBTs. I don't think it all made it over to the new git forge 'rust-psbt' repo yet since it's just v0.32.x stuff right now and the impls I wrote were for the v0.33.beta one. I've been waiting patiently for the bump to 0.33 😀 |
I've written a bunch of
arbitraryimpls for rust-bitcoin, so I was curious if there's any interest to do it here as well. Those changes haven't been released yet but should be released in rust-bitcoin v0.33 (or whatever the next version is named), and at that point, we will be able to use them to test this crate as well. I've looked through rust-bitcoin to implement all of the types that are used here (Amount,FeeRate,Psbt, etc), so when the new release gets cut, we should be able to add a lot of extensive fuzzing here if we usearbitraryas well.Note: There's a
derivefeature, but rust-bitcoin doesn't have it enabled to cut down on dependencies, so I didn't do it here eitherPull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.