Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Feb 1, 2022

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

@DrahtBot DrahtBot added the Tests label Feb 1, 2022
@Sjors
Copy link
Member

Sjors commented Feb 1, 2022

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).

@maflcko
Copy link
Member Author

maflcko commented Feb 1, 2022

Thanks, fixed. NO_BDB=1 is already set.

@Sjors
Copy link
Member

Sjors commented Feb 1, 2022

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?

@maflcko
Copy link
Member Author

maflcko commented Feb 1, 2022

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.

@Sjors
Copy link
Member

Sjors commented Feb 1, 2022

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.

@maflcko
Copy link
Member Author

maflcko commented Feb 1, 2022

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.

@maflcko
Copy link
Member Author

maflcko commented Feb 1, 2022

Looks like this fixed itself, but we should still consider doing this anyway in the future.

@maflcko maflcko closed this Feb 1, 2022
@maflcko maflcko deleted the 2202-cibdbmsan branch February 1, 2022 18:24
@fanquake
Copy link
Member

fanquake commented Feb 2, 2022

Concept ACK. I had a similar change in an MSAN branch.

@bitcoin bitcoin locked and limited conversation to collaborators Feb 2, 2023
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.

4 participants