ci: Remove redundant valgrind fuzz task #20368

1. 
   MarcoFalke commented at 8:12 am on November 11, 2020: member

   This task has several issues:

   • It slows down other tasks and times out: It needs a lot of resources (CPU, RAM, time), because it builds more than 100 fuzzers, clones a 2 GB repo with 100k seeds and pipes them all through valgrind
   • It doesn’t add a lot of value: Except for one issue in the boost time library, it hasn’t found any issues that the existing fuzz,asan,ubsan fuzzer has already found
   • It is redundant: It is already run in the bitcoin-core/qa-assets repo on every push of new seeds and once daily

   Fix all issues by removing it here.

2. ci: Remove redundant valgrind fuzz task fa92cf29d9
3. 
   practicalswift commented at 8:50 am on November 11, 2020: contributor

   Please don’t remove this unless you add a corresponding MSan job: we urgently need UUM (use of uninitialized memory) testing of also the fuzzers. We’ve seen way too many recent UUM issues in the last years. See partial list below.

   It doesn’t add a lot of value: Except for one issue in the boost time library, it hasn’t found any issues that the existing fuzz,asan,ubsan fuzzer has already found

   Please note that UUM issues will not be found by ASan or UBSan: only MSan or Valgrind will catch these.

   It is redundant: It is already run in the bitcoin-core/qa-assets repo on every push of new seeds and once daily

   It is great that you run the this testing on your own, but “I run this anyways on my own intrastructure” is really not equivalent to “everyone can easily confirm that we test this continuously in our shared CI infrastructure”.

   ---

   Some recent uninitialized reads (incomplete list):

   • 2020: Use of uninitialized memory in signet validation code
   • 2020: Use of uninitialized memory in Taproot deployment for Signet
   • 2020: Use of uninitialized memory in Erlay networking code - found pre-merge
   • 2020: Use of uninitialized memory in BIP324 encrypted p2p transport de-/serializer code (truth in advertising: I haven’t verified this one by writing a PoC) - found pre-merge
   • 2020: util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return
   • 2019: Use of uninitialized memory in networking code when receiving a transaction we already have - found post-merge
   • 2019: wallet: Uninitialized read in bumpfee(…)
   • 2018: wallet: Fix non-determinism in ParseHDKeypath(…). Avoid using an uninitialized variable in path calculation.
   • 2018: wallet: Fix use of uninitialized value bnb_used in CWallet::CreateTransaction(…)
   • 2017: [net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest&)
   • 2017: [test] Avoid reading a potentially uninitialized variable in tx_invalid-test (transaction_tests.cpp)
4. DrahtBot added the label Tests on Nov 11, 2020
5. 
   MarcoFalke commented at 9:00 am on November 11, 2020: member

   We’ve seen way too many recent UUM issues in the last two years.

   I am only aware of one memory issue that the valgrind fuzz task has found in our time parsing/encoding helper function, excluding issues that have been found by the other fuzz task.

   It is great that you run the this testing on your own, but “I run this anyways on my own intrastructure” is really not equivalent to “everyone can easily confirm that we test this continuously in our shared CI infrastructure”.

   I don’t run this on my own. It runs on the same ci provider completely public: https://cirrus-ci.com/github/bitcoin-core/qa-assets

   Right now the task doesn’t run in this repo because it times out after 2 hours. I don’t see the value of keeping a task that times out and marks everything red.

6. 
   practicalswift commented at 9:19 am on November 11, 2020: contributor

   I am only aware of one memory issue that the valgrind fuzz task has found in our time parsing/encoding helper function, excluding issues that have been found by the other fuzz task.

   Considering that the fuzz tests are known to exercise code paths not covered by the unit tests or functional tests I’m not really comfortable with us removing CI Valgrind UUM fuzz testing before we have a replacement in place.

   I think we should add a MSan fuzzing job before we remove the Valgrind fuzzing job. As discussed in previous PRs MSan is way way faster than Valgrind.

   I don’t run this on my own. It runs on the same ci provider completely public: https://cirrus-ci.com/github/bitcoin-core/qa-assets

   Thanks. I didn’t know that. What happens if one of those cron jobs fail due to detected UUM? Will someone catch that, and/or get notified?

7. 
   MarcoFalke commented at 10:04 am on November 11, 2020: member

   Will someone catch that, and/or get notified?

   Yes, pushing new seeds to the repo will be marked red.

8. 
   MarcoFalke commented at 1:21 pm on November 11, 2020: member

   going to merge this because all pull requests and master builds are red right now
9. MarcoFalke merged this on Nov 11, 2020
10. MarcoFalke closed this on Nov 11, 2020

    ---

11. 
    decryp2kanon commented at 2:19 pm on November 11, 2020: contributor

    ACK fa92cf29d971811ff8dd844ee1a1e2e12b79e389 i really wanted to remove this against time out
12. MarcoFalke deleted the branch on Nov 11, 2020
13. sidhujag referenced this in commit 789d25705b on Nov 11, 2020
14. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke practicalswift decryp2kanon

Labels
Tests