Skip to content

fuzz: version handshake #20370

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 11, 2021
Merged

fuzz: version handshake #20370

merged 1 commit into from
Feb 11, 2021

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Nov 11, 2020 •
edited
Loading

Not fuzzing the version handshake will limit fuzz coverage

@fanquake fanquake added the Tests label Nov 11, 2020
@practicalswift
Copy link
Contributor

Concept ACK

@practicalswift
Copy link
Contributor

Tested ACK dd8654b

Crypt-iQ
Crypt-iQ reviewed Nov 15, 2020
View reviewed changes
Copy link
Contributor

@Crypt-iQ Crypt-iQ left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK dd8654b

Running now to compare coverage before and after this patch. I like that the fuzzers now have SendMessages coverage.

@maflcko maflcko changed the title fuzz: Permission flags in net_processing fuzzers DRAFT fuzz: Permission flags in net_processing fuzzers Nov 15, 2020
@Crypt-iQ
Copy link
Contributor

Crypt-iQ commented Nov 26, 2020 •
edited
Loading

Coverage from dd8654b:
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.html

Coverage goes up because of SendMessages, but ProcessMessage coverage is worse. I think it's probably due to the seed change.

@DrahtBot DrahtBot mentioned this pull request Dec 14, 2020
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 14, 2020 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot mentioned this pull request Dec 20, 2020
Closed
@maflcko maflcko marked this pull request as draft December 28, 2020 19:55
@DrahtBot DrahtBot mentioned this pull request Dec 29, 2020
Merged
@maflcko maflcko changed the title DRAFT fuzz: Permission flags in net_processing fuzzers fuzz: version handshake Jan 15, 2021
@maflcko maflcko marked this pull request as ready for review January 29, 2021 06:49
@maflcko
Copy link
Member Author

maflcko commented Feb 11, 2021

Rebased

@practicalswift
Copy link
Contributor

cr ACK fabce45: patch looks very much correct

@maflcko maflcko merged commit 0ad4656 into bitcoin:master Feb 11, 2021
@maflcko maflcko deleted the 2011-fuzzNetFlags branch February 11, 2021 16:27
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 11, 2021
Merge bitcoin#20370: fuzz: version handshake
4c4327e 
fabce45 fuzz: version handshake (MarcoFalke)

Pull request description:

  Not fuzzing the version handshake will limit fuzz coverage

ACKs for top commit:
  practicalswift:
    cr ACK fabce45: patch looks very much correct

Tree-SHA512: 4091d27d39edee781d033e471b352084bb54df250d0890e4821a325926a44dff9b26a2614d67dd0529f73bd366b075d7a0a1a570c2837de286a1b93a59a8fb91
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@Crypt-iQ Crypt-iQ Crypt-iQ left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maflcko @practicalswift @Crypt-iQ @DrahtBot @fanquake