Skip to content

fuzz: Check banman roundtrip #22322

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
maflcko merged 1 commit into from
Jun 25, 2021
Merged

fuzz: Check banman roundtrip #22322

maflcko merged 1 commit into from
Jun 25, 2021

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jun 23, 2021

No description provided.

@maflcko maflcko requested review from practicalswift and vasild June 23, 2021 10:50
@fanquake fanquake added the Tests label Jun 23, 2021
@jonatack
Copy link
Member

jonatack commented Jun 23, 2021

Concept ACK, good idea.

jonatack
jonatack reviewed Jun 23, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

tACK fabadb7702a9cbe477e9e5f91706620456154903

practicalswift
practicalswift approved these changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@practicalswift practicalswift left a comment

Choose a reason for hiding this comment

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

cr ACK fabadb7702a9cbe477e9e5f91706620456154903

Great work!

vasild
vasild reviewed Jun 23, 2021
View reviewed changes
vasild
vasild reviewed Jun 24, 2021
View reviewed changes
@maflcko
Copy link
Member Author

maflcko commented Jun 24, 2021

Addressed feedback

vasild
vasild approved these changes Jun 24, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK fa57878340e6e9e34b01e26091d9d57f1c26f771

@jonatack
Copy link
Member

jonatack commented Jun 24, 2021

Thanks for updating.

Tested ACK fa57878340e6e9e34b01e26091d9d57f1c26f771

vasild
vasild approved these changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK fa485d0

@practicalswift
Copy link
Contributor

practicalswift commented Jun 25, 2021

cr ACK fa485d0

@maflcko maflcko merged commit 246daf1 into bitcoin:master Jun 25, 2021
@maflcko maflcko deleted the 2106-fuzzBanman branch June 25, 2021 08:09
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 25, 2021
Merge bitcoin#22322: fuzz: Check banman roundtrip
68ec9ea 
fa485d0 fuzz: Check banman roundtrip (MarcoFalke)

Pull request description:

ACKs for top commit:
  practicalswift:
    cr ACK fa485d0
  vasild:
    ACK fa485d0

Tree-SHA512: 84e297c0b90ef68d72afd2053bfda2888496c1b180233516a8caaf76d6c03403f1e4ed59f1eb32d799873fc34009634b4ce372244b9d546d04626af41ac4d1d7
hebasto
hebasto reviewed Aug 6, 2021
View reviewed changes
src/test/fuzz/banman.cpp
BanMan ban_man_read{banlist_file, /* client_interface */ nullptr, /* default_ban_time */ 0};
banmap_t banmap_read;
ban_man_read.GetBanned(banmap_read);
assert(banmap == banmap_read);
Copy link
Member

@hebasto hebasto Aug 6, 2021

Choose a reason for hiding this comment

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

Errors in CI job (https://cirrus-ci.com/task/6055643191705600?logs=ci#L2769):

fuzz: test/fuzz/banman.cpp:112: void banman_fuzz_target(FuzzBufferType): Assertion `banmap == banmap_read' failed.

Copy link
Contributor

@vasild vasild Aug 6, 2021

Choose a reason for hiding this comment

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

Is there a way to retrieve ./crash-0671aac15e619e99522e2119487eaa9cc97e5a34 from the CI machine or make the fuzzer print it in base64?

Latest master (d67330d) has been changed to:

bitcoin/src/test/fuzz/banman.cpp

Lines 111 to 113 in d67330d

// Assert temporarily disabled to allow the remainder of the fuzz test to run while a
// fix is being worked on. See https://github.com/bitcoin/bitcoin/pull/22517
(void)(banmap == banmap_read);

@hebasto hebasto mentioned this pull request Aug 6, 2021
Merged
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge 246daf1 into merged_master (Bitcoin PR bitcoin/bitcoin#22322)
9a6ea2c
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto left review comments

+3 more reviewers

@vasild vasild vasild approved these changes

@jonatack jonatack jonatack left review comments

@practicalswift practicalswift practicalswift approved these changes

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.

6 participants

@maflcko @jonatack @practicalswift @vasild @hebasto @fanquake