Sanity checking for node configuration on startup

[fraser-iohk]

• This PR adds a a new mechanism for sanity checking node configuration on startup
• These checks are intended to operate as a warning to users that they may have misconfigured their node, and are designed to be easily bypassed by a user who is intentionally configuring their node in an unusual but possibly acceptable way
• For safety reasons, the first change to cardano-node to support this will only log the exception in the standard way, though in future it should cause the node to terminate. We plan to add a way for the cardano-node tracer implementation to distinguish between sanity check issues which are fatal and those which are only warnings.
• The first of these checks is simply to ensure that K, the security parameter, is consistent between all known eras. If eras' security parameters are different, a SanityCheckIssue exception will be traced in the new sanityCheckIssueTracer.

There is an associated branch in the cardano-node repository with changes required to support this additional Tracer.

[nfrisby]

is a Tracer the correct way to report these errors / warnings?

My immediate thought is that the kinds of errors we're concerned with here deserve exceptions.

[fraser-iohk]

My immediate thought is that the kinds of errors we're concerned with here deserve exceptions.

when I discussed this initially with damien and javier, we thought that tracers would be better, since the "errors" we want to raise here shouldn't necessarily be fatal, but probably should at least be communicated to the user as a warning. we thought that implementing them with a Tracer meant that we'd have the flexibility to change it easily and have different issues with varying severity. I don't think exceptions are a bad idea, but it feels a little more awkward to have someone (probably a developer) add catches than to add a filter to a Tracer

edit: putting "errors" in quotes makes it look like I'm being snarky when I'm not trying to, I think that me calling them "errors" in the first place was a mistake

[nfrisby]

I'm "Requesting changes" only because of the dangling dead code comment.

Otherwise, looks very good. Great comments. Idiomatic design. 👍

Regarding tracers, versus exceptions: tracers does seem reasonable, given that these checks are not necessarily fatal.

no more dead code comment :thumbs-up:

[fraser-iohk]

appropriate changes to cardano-node are available on the fraser-iohk/consensus-startup-sanity-checks branch: https://github.com/IntersectMBO/cardano-node/tree/fraser-iohk/consensus-startup-sanity-checks

[amesgen]

appropriate changes to cardano-node are available on the fraser-iohk/consensus-startup-sanity-checks branch: https://github.com/IntersectMBO/cardano-node/tree/fraser-iohk/consensus-startup-sanity-checks

Wasn't the idea (also see #874 (comment)) to throw an exception for InconsistentSecurityParam (as part of the tracer, based on a "fatality" classification function in Consensus)?

[fraser-iohk]

Wasn't the idea (also see #874 (comment)) to throw an exception for InconsistentSecurityParam (as part of the tracer, based on a "fatality" classification function in Consensus)?

yep, that's the goal for the future, but for the first iteration (when stuff might go wrong) I've just made it trace the exception since I don't want to potentially break node use cases without warning the user for at least one version first

[amesgen]

Wasn't the idea (also see #874 (comment)) to throw an exception for InconsistentSecurityParam (as part of the tracer, based on a "fatality" classification function in Consensus)?

yep, that's the goal for the future, but for the first iteration (when stuff might go wrong) I've just made it trace the exception since I don't want to potentially break node use cases without warning the user for at least one version first

Makes sense, can you record that somewhere, maybe in #125?

---

Apart from that, LGTM after formatting, squasing and updating the PR description 👍