ci: Remove bdb build from msan task #24228

1. 
   MarcoFalke commented at 3:03 pm on February 1, 2022: member

   This removes BDB from the CI msan task, because:

   • It is a common source for failures, as the script doesn’t have fallbacks on network errors
   • Most wallet code is also covered via sqlite and any bdb-only wallet code can be checked with valgrind
2. DrahtBot added the label Tests on Feb 1, 2022
3. 
   Sjors commented at 4:08 pm on February 1, 2022: member

   Concept ACK

   IIUC we do still check BDB functionality in plenty of other tests, so this just loses the memory sanitizer coverage. Since we shoved the legacy (BDB) wallet into a box (LegacyScriptPubKeyMan), we probably won’t be touching its internals (often). Just have to remember, when we do, to be a bit extra careful.

   Are you sure this actually removes BDB from that task? E.g. BDB_PREFIX is still used in BITCOIN_CONFIG. Maybe explicitly opt-out with --without-bdb (also for clarity).

4. ci: Remove bdb build from msan task fa40842513
5. MarcoFalke force-pushed on Feb 1, 2022
6. 
   MarcoFalke commented at 4:31 pm on February 1, 2022: member

   Thanks, fixed. NO_BDB=1 is already set.
7. 
   Sjors commented at 4:48 pm on February 1, 2022: member

   Thanks, fixed. NO_BDB=1 is already set.

   That’s odd; then what was the point of building it? Only compile time checks for dependency itself? But not any of the call sites, and no runtime checks?

8. 
   MarcoFalke commented at 5:15 pm on February 1, 2022: member

   Building bdb from depends was never supported with msan, which is why it used the script. This works, as configure will pick up the non-depends bdb. However, now that the non-depends bdb is removed, there is no need to specify --without-bdb, as bdb isn’t even built.
9. 
   Sjors commented at 5:16 pm on February 1, 2022: member

   Oh wait, NO_BDB only affects the depends itself, but you can still build it from script. With other depends it doesn’t work that way, because it changes the configure settings to match. Fairly confusing, hence my suggestion to make it explicit with --without-bdb.
10. 
    MarcoFalke commented at 5:26 pm on February 1, 2022: member

    I think it is better to keep it as is now, because:

    • No bdb build is known to pass, so there should be no way to re-enable one without making CI red.
    • If there was one that passes, having to modify a single place seems less confusing than having to modify two places at the same time.
11. 
    MarcoFalke commented at 6:24 pm on February 1, 2022: member

    Looks like this fixed itself, but we should still consider doing this anyway in the future.
12. MarcoFalke closed this on Feb 1, 2022

    ---

13. MarcoFalke deleted the branch on Feb 1, 2022
14. 
    fanquake commented at 12:32 pm on February 2, 2022: member

    Concept ACK. I had a similar change in an MSAN branch.
15. DrahtBot locked this on Feb 2, 2023

Contributors
MarcoFalke Sjors fanquake

Labels
Tests