fuzz-tests: Improved fuzz coverage for fuzz-addr #8232
Conversation
There was a problem hiding this comment.
Thanks for the contribution. The fuzz-addr corpus hasn't been updated for 2+ years, and there's been some changes to the underlying logic since then, so it's good to add the new inputs.
At the same time, I don't think the change to fuzz-addr.c is actually helpful. Could you remove that part from this PR?
Also, I think it may be worth minimizing the fuzz-addr corpus again in a second commit, since the underlying logic has changed and maybe we can prune some inputs that way:
cd tests/fuzz
mkdir -p tmp_corpora/fuzz-addr
mv corpora/fuzz-addr/* tmp_corpora/fuzz-addr/
./run.py corpora --merge_dir tmp_corpora
tests/fuzz/fuzz-addr.c
Outdated
| @@ -13,9 +13,11 @@ void init(int *argc, char ***argv) | |||
|
|
|||
| void run(const uint8_t *data, size_t size) | |||
| { | |||
| uint8_t *script_pubkey = tal_dup_arr(tmpctx, uint8_t, data, size, 0); | |||
| if(size == 22 || size == 23 || size == 25 || size == 34) { | |||
There was a problem hiding this comment.
I'm skeptical that this materially improves the fuzzer's ability to find new coverage, since all the size checks are also present deeper in the encode_scriptpubkey_to_addr logic. I think the increased coverage you saw is likely due to the fact that this change introduces at least 5 new branches for the fuzzer to find.
This change also doesn't seem very future-proof -- if support for a new script type is added, the fuzzer won't be able to exercise that code.
There was a problem hiding this comment.
Hi! Thanks a lot for the feedback — really appreciate it.
I've removed the changes to fuzz-addr.c as suggested, and kept only the corpus update. Also, I ran the corpus minimization steps using ./run.py corpora --merge_dir, and added the minimized corpus.
I'm still getting the hang of fuzzing, so happy to learn and iterate!
…into fuzz_improvement
…into fuzz_improvement
|
Could you rework the commit structure as follows?
That would make this PR much easier to review, and is probably the commit structure we'd want for merging anyway. |
The current target
fuzz-addrhas the following coverage, features and the corpus when run for around 5 mins.By adding a simple size constraint on the data which aligns with the actual size of script pub key for P2PKH, P2SH, P2WSH, P2WPKH, P2TR. Coverage with the new fuzz target.
Added the newly discovered inputs to the seed corpus which increases the code-coverage.
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked: