We already run against valgrind and msan is a subset of valgrind according to: "MSan implements a subset of functionality found in Valgrind (Memcheck tool). It is significantly faster than Memcheck (TODO:benchmark)." (https://github.com/google/sanitizers/wiki/MemorySanitizer)

@MarcoFalke

I think we should use both. While both tools try to find uses of uninitialized memory (UUM) they are implemented in totally different ways: Valgrind's memcheck is using binary translation while MSan is using compiler-based instrumentation. From my experience it is not guaranteed that Valgrind finds all issues reported by MSan and vice versa.

Furthermore, having the combination libFuzzer+MSan readily available is really crucial when fuzzing. The combination libFuzzer+Valgrind is only usable for testing against an existing fuzzing corpus like we do in Travis -- not for doing any actual fuzzing. For the libFuzzer+MSan combination to be available it is important to have the cumbersome MSan setup process documented and also kept up-to-date and working over time. We get that for free by having an MSan build in Travis.

Also MSan is order of magnitudes faster than Valgrind which means that we can run also tests that are too slow for Valgrind in Travis.

And last but not least MSan can report the name of the variable that has not been initialized (thanks to MSan using compiler-based instrumentation). Valgrind only points to the function name.

In summary: I think having MSan in Travis is a very clear win. Let's use all available tools to try to kill the uninitialized memory (UUM) bug class :)

Let me know if you can think of any problems that could arise from testing also under MSan in Travis.

@MarcoFalke

We already run against valgrind […]

Correct if you mean the unit tests.

Incorrect if you mean the functional tests :)

Due to Valgrind's slowness we currently only run a single(!) functional test under Valgrind (TEST_RUNNER_EXTRA="p2p_segwit.py").

Note that this PR runs all available functional tests (and unit tests) under MSan.

I'd love to see a bug that msan catches that valgrind doesn't.

From my experience it is not guaranteed that Valgrind finds all issues reported by MSan and vice versa.

I'd love to see a bug that msan catches that valgrind doesn't.

Sorry about making a vague claim that I cannot back up with an example. Until I can provide an example we can simplify the discussion by assuming that I was flat out wrong about the possibility of Valgrind not catching an issue found by MSan (given enough runtime to compensate for being orders of magnitude slower than MSan).

From a practical perspective the main reason to start using MSan is the speedup that will allow us to go from detecting use of uninitialized memory in code covered by a single(!) functional test (p2p_segwit.py) to detecting use of uninitialized memory in all code covered by our functional tests.

Please note that I'm not suggesting removing the Valgrind job: I think we should use both and ideally bump the timeouts so that Valgrind can test a couple of more functional tests at least :)

(Just to be clear: I love both Valgrind (proof of love: #18166, #18162, #18159, #17633, #17640, #17624, #17455, #15117, #11035, #11024, #10977) and MSan – I use them daily :))

Let me know if you can think of any problems that could arise from testing also under MSan in Travis.

That's fine, if you ever find any-- I'd be interested in seeing them whenever that is. The question was primarily driven out of my own curiosity.

Faster can be pretty useful, though I've found msan in the past to be somewhat lacking compared to valgrind in terms of sensitivity -- though my experience is mostly from when msan was very new. I've found msan most useful in the context of fuzzing, where any speed difference makes for a meaningful throughput difference which ultimately results in a sensitivity difference.

The fact that even paid travis is so cpu starved that this is a big issue for just CI tests is something of is own problem-- labour is expensive, insight is rare. CPU time is the one resource needed by the project which should never be the limiting ingredient, this matters because a surplus of CPU time can be a force multiplier for human labour (though my idea of 'reasonable amounts of computing power' starts at about a thousand core ghz per person, and I've always found it really weird how software people who are at the top of their field often work from just a single underpowered laptop; so maybe I'm just a weirdo). FWIW, for a number of years I tested every release of bitcoin in valgrind, including doing IBDs (overnight).

I don't see any problem with making more use of msan, though it's not a complete replacement for valgrind. But more is more. :)

@MarcoFalke

Would you mind reviewing? :)

I believe that having this merged would have allowed us to automatically detect at least two remotely triggerable uses of uninitialized memory (UUM) in our P2P networking code:

• 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
• 2019: Use of uninitialized memory in networking code when receiving a transaction we already have - found post-merge

FWIW - a subset of non-networking uses of uninitialized memory:

• 2020: util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return
• 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)

Not sure though if we should also run this on travis, given that travis hopefully will run valgrind on everything: #18304

Even if we assume that we get Valgrind to run all the unit and functional tests within the Travis time limit (which I unfortunately doubt is technically possible) I still don't see why we would not like to use both Valgrind and MSan in Travis? :)

As @gmaxwell put it: "more is more" :)

MSan is literally orders of magnitude faster than Valgrind and also gives better debugging info in case of presence of UUM.

Let me know if you can think of any problems that could arise from testing also under MSan in Travis.

Concept ACK. cd120cf looks sane, doesn't hurt my macOS build, but otherwise a bit above my pay-grade. The changes are contained to their own machine, except:

• boost depends tweak that's now moved to depends: Make it possible to build Boost dependency using a clang toolset (./b2 toolset=clang) #18308
• MSAN handling in GetOSRand, which is a more surgical alternative to Suppress false positive warning about uninitialized entropy buffers #17627

For the libFuzzer+MSan combination to be available it is important to have the cumbersome MSan setup process documented and also kept up-to-date and working over time. We get that for free by having an MSan build in Travis.

Keeping this up to date, by having it run on Travis, seems useful. We can always triage Travis resources if funding runs dry.

The build took 1 hours and 15 minutes the first time. You could push a trivial commit to see much of that is shaved off by the cache.

Slow Travis builds can be a burden on review, because Github won't show a build as failed until all Travis machines of the current phase are done. I often find myself reviewing PRs in EU morning with trivial bugs not spotted by US devs before they went to be.

This can be mitigated if you add a new stage grind(?) and move the machine there. (I also agree with @gmaxwell about developers not using enough tooling, guilty as charged)

It could also run as part of a cron job on master ("$TRAVIS_EVENT_TYPE" = "cron"), but I'm not sure who gets notified when those fail. And it's better to catch stuff before merge, so let's not do that unless we run short on resources.

The build took 1 hours and 15 minutes the first time.

That seems to be roughly the time it takes also with caching. That might sound long but it is still 45 minutes quicker than the current valgrind job which only covers a subset of the functional tests whereas this one covers all of them :)

(To be clear: even with the two hour valgrind job I think it easily is worth taking that cost in terms of Travis build time to protect our users from at least a subset of UUM bugs. This kind of (partial) protection is long-overdue :))

Concept ACK

@fanquake

@practicalswift can you drop your boost changes here, and rebase on #18820? We'll get the macOS issues sorted out.

That has been done :) Ready for final review? :)

@MarcoFalke @fanquake Thanks a lot for great and detailed feedback!

All feedback should now be addressed:

• Dropped --disable-asm.
• Now building with ZeroMQ (dropped NO_ZMQ=1 and --disable-zmq).
• Bumped to clang-9 and llvm-9.
• No longer patching src/random.cpp with MemorySanitizer annotations.
• Removed unneccessary bash export of LIBCXX_DIR.

The current version of this PR only touches only .travis.yml and ci/test/.

Should be ready for final review :)

ACK 870f0cd