Skip to content

fuzz: Make CAddrMan fuzzing harness deterministic#20425

Merged
maflcko merged 1 commit intobitcoin:masterfrom
practicalswift:fuzzers-make-addrman-harness-deterministic
Dec 1, 2020
Merged

fuzz: Make CAddrMan fuzzing harness deterministic#20425
maflcko merged 1 commit intobitcoin:masterfrom
practicalswift:fuzzers-make-addrman-harness-deterministic

Conversation

@practicalswift
Copy link
Copy Markdown
Contributor

@practicalswift practicalswift commented Nov 19, 2020

Make CAddrMan fuzzing harness deterministic.

See doc/fuzzing.md for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.

Happy fuzzing :)

@practicalswift practicalswift changed the title fuzz: Make addrman fuzzing harness deterministic fuzz: Make CAddrMan fuzzing harness deterministic Nov 19, 2020
@maflcko
Copy link
Copy Markdown
Member

maflcko commented Nov 19, 2020

review ACK 760029278c1dce2796605be0f0c368bd56ab8426

will test before merge

@practicalswift practicalswift force-pushed the fuzzers-make-addrman-harness-deterministic branch from 7600292 to bd32dcd Compare November 19, 2020 17:19
@Crypt-iQ
Copy link
Copy Markdown
Contributor

Crypt-iQ commented Nov 26, 2020

utACK 17a5f17

Comment thread src/test/fuzz/addrman.cpp
@maflcko maflcko merged commit dfd0b70 into bitcoin:master Dec 1, 2020
@michaelfolkson
Copy link
Copy Markdown

Just for my own personal understanding - why should the CAddrMan fuzzing harness be deterministic?

@maflcko
Copy link
Copy Markdown
Member

maflcko commented Dec 1, 2020

all testing should be deterministic, otherwise it is impossible to reliably reproduce bugs

@michaelfolkson
Copy link
Copy Markdown

Isn't that one of the reasons for fuzzing? That testers will initially fuzz different inputs and then highlight which inputs create an issue? Maybe I am misunderstanding what you mean by deterministic here...

Everyone runs the same unit and functional tests. But fuzz testers at least in theory try different inputs?

@maflcko
Copy link
Copy Markdown
Member

maflcko commented Dec 1, 2020

Generation of the seeds itself by the fuzz engine doesn't have to be deterministic. And for most fuzz engines it isn't (unless you ask them to).

So yes, fuzz testers will run different inputs during generation. However, if they discover a crash with one input, the crash should be reproducible when the same input is run again (even on a different machine)

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 1, 2020
17a5f17 fuzz: Make addrman fuzzing harness deterministic (practicalswift)

Pull request description:

  Make `CAddrMan` fuzzing harness deterministic.

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

ACKs for top commit:
  Crypt-iQ:
    utACK 17a5f17

Tree-SHA512: 725f983745233e9b616782247fa18847e483c074ca4336a5beea8a9009128c3a74b4d50a12662d8ca2177c2e1fc5fc121834df6b459ac0af43c931d77ef7c4d8
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Dec 15, 2020
…ing harnesses by using mocked GetTime()

8c09c0c fuzz: Avoid time-based "non-determinism" in fuzzing harnesses by using mocked GetTime() (practicalswift)

Pull request description:

  Avoid time-based "non-determinism" in fuzzing harnesses by using mocked `GetTime()`.

  Prior to this commit the fuzzing harnesses `banman`, `connman`, `net` and `rbf` had time-based "non-determinism". `addrman` is fixed in bitcoin#20425. `process_message` and `process_messages` are left to fix: simply using mock time is not enough for them due to interaction with `IsInitialBlockDownload()`.

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

ACKs for top commit:
  MarcoFalke:
    review ACK 8c09c0c
  practicalswift:
    > review ACK [8c09c0c](bitcoin@8c09c0c)

Tree-SHA512: 32dfbead3dfd18cf4ff56dc2ea341aa977441b4e19a54879cf54fa5820c7e2b14b92c7e238d32fd785654f3b28cc82826ae66c03e94c292633c63c41196ba9a8
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 15, 2020
…ing harnesses by using mocked GetTime()

8c09c0c fuzz: Avoid time-based "non-determinism" in fuzzing harnesses by using mocked GetTime() (practicalswift)

Pull request description:

  Avoid time-based "non-determinism" in fuzzing harnesses by using mocked `GetTime()`.

  Prior to this commit the fuzzing harnesses `banman`, `connman`, `net` and `rbf` had time-based "non-determinism". `addrman` is fixed in bitcoin#20425. `process_message` and `process_messages` are left to fix: simply using mock time is not enough for them due to interaction with `IsInitialBlockDownload()`.

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

ACKs for top commit:
  MarcoFalke:
    review ACK 8c09c0c
  practicalswift:
    > review ACK [8c09c0c](bitcoin@8c09c0c)

Tree-SHA512: 32dfbead3dfd18cf4ff56dc2ea341aa977441b4e19a54879cf54fa5820c7e2b14b92c7e238d32fd785654f3b28cc82826ae66c03e94c292633c63c41196ba9a8
@practicalswift practicalswift deleted the fuzzers-make-addrman-harness-deterministic branch April 10, 2021 19:43
@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.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants