fuzz: version handshake #20370
pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2011-fuzzNetFlags changing 2 files +2 −2-
MarcoFalke commented at 1:19 pm on November 11, 2020: memberNot fuzzing the version handshake will limit fuzz coverage
-
fanquake added the label Tests on Nov 11, 2020
-
practicalswift commented at 1:25 pm on November 11, 2020: contributorConcept ACK
-
practicalswift commented at 4:12 pm on November 12, 2020: contributorTested ACK dd8654bcd53dade1f5114d68bb51ef22e585b9ba
-
MarcoFalke added the label Waiting for author on Nov 13, 2020
-
in src/test/fuzz/process_messages.cpp:51 in dd8654bcd5 outdated
57+ connman.SetPeerPermissionFlags(p2p_node, permission_flags); 58 p2p_node.fSuccessfullyConnected = true; 59- p2p_node.fPauseSend = false; 60 p2p_node.nVersion = PROTOCOL_VERSION; 61 p2p_node.SetCommonVersion(PROTOCOL_VERSION); 62+ p2p_node.fPauseSend = false;
Crypt-iQ commented at 12:12 pm on November 15, 2020:Why was this moved?
MarcoFalke commented at 5:17 pm on November 15, 2020:There are some more changes that I haven’t yet pushed to increase coverage even further. They change the seed format as well, so I am going to mark this as draft for now.
Crypt-iQ commented at 11:31 pm on November 22, 2020:What is the procedure for when a PR needs to modify the fuzzing harness and may invalidate all of the seeds? Should something akin to a database migration be used if possible, applied to the seeds to get at least the same coverage as before?
MarcoFalke commented at 8:05 am on November 23, 2020:The format might be arbitrarily complex (even depending on the consensus rules or other Bitcoin Core code), so I don’t think this can be done automatically in a trivial manner. I mostly do it manually if the fuzz engine can’t figure it out by itself after a bit of CPU time. -
Crypt-iQ commented at 12:16 pm on November 15, 2020: contributor
utACK dd8654bcd53dade1f5114d68bb51ef22e585b9ba
Running now to compare coverage before and after this patch. I like that the fuzzers now have
SendMessages
coverage. -
MarcoFalke renamed this:
fuzz: Permission flags in net_processing fuzzers
DRAFT fuzz: Permission flags in net_processing fuzzers
on Nov 15, 2020 -
Crypt-iQ commented at 7:41 pm on November 26, 2020: contributor
Coverage from dd8654bcd53dade1f5114d68bb51ef22e585b9ba: https://crypt-iq.github.io/20370_review/covrun1/src/net_processing.cpp.gcov.html
Compared to a recent
master
commit (should have recorded this…oh well): https://crypt-iq.github.io/20370_review/covrun_master/src/net_processing.cpp.gcov.htmlCoverage goes up because of
SendMessages
, butProcessMessage
coverage is worse. I think it’s probably due to the seed change. -
DrahtBot commented at 10:25 pm on December 14, 2020: member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
No conflicts as of last run.
-
MarcoFalke marked this as a draft on Dec 28, 2020
-
MarcoFalke renamed this:
DRAFT fuzz: Permission flags in net_processing fuzzers
fuzz: version handshake
on Jan 15, 2021 -
MarcoFalke removed the label Waiting for author on Jan 15, 2021
-
MarcoFalke force-pushed on Jan 29, 2021
-
MarcoFalke marked this as ready for review on Jan 29, 2021
-
fuzz: version handshake fabce459bb
-
MarcoFalke force-pushed on Feb 11, 2021
-
MarcoFalke commented at 12:20 pm on February 11, 2021: memberRebased
-
practicalswift commented at 4:22 pm on February 11, 2021: contributorcr ACK fabce459bb44e90dc7ae9c44eeedab707435af5b: patch looks very much correct
-
MarcoFalke merged this on Feb 11, 2021
-
MarcoFalke closed this on Feb 11, 2021
-
MarcoFalke deleted the branch on Feb 11, 2021
-
sidhujag referenced this in commit 4c4327e623 on Feb 11, 2021
-
DrahtBot locked this on Aug 18, 2022