improve UX of topology file parsing corner cases

[crocodile-dentist]

The networking team has discussed with SRE that a few improvements could be made to the current tracing and error handling mechanism when the node is starting up to avoid confusion. The list is as follows:

The node traces an error message in readTopologyFile when it is configured to start in GenesisMode (ConsensusMode key in the config file) and the topology file specifies that bootstrap peers (bootstrapPeers key) should be used during syncing. The severity of this message should be lowered to at most a warning instead.

When the node is configured to run in PraosMode (ConsensusMode key again) and the topology file specifies the key peerSnapshotFile but the file is missing the node shuts down with an error message (cf. readPeerSnapshotFile). For PraosMode, this should be just a warning level message.

When the node is configured to run as a block producer, and the peer snapshot file is provided in the topology file, but useLedgerAfter is -1 or outright missing (ie. DontUseLedgerPeers) or this value points to a slot which is more recent than the slot recorded in the peer snapshot file, the node traces a message to either update the peer snapshot file or enable the use of ledger peers. Outside of testing environments, for BP's, the use of any peers besides trusted local roots is dubious. This warning should be removed in this situation to not cause further confusion, or perhaps replaced to warn that the use of ledger peers and peer snapshot is not recommended for a BP. The node can be considered to be a BP when any of the protocol files are present at startup. This information should be passed to the updateLedgerPeerSnapshot function to properly handle this situation.

During startup, the node should handle this issue by issuing up to three separate traces, one from each table below:

Consensus mode and bootstrap peers interaction

ConsensusMode	bootstrapPeers	Log Severity
GenesisMode	UseBootstrapPeers	Warning - not compatible, reverting to DontUseBootstrapPeers

Potentially a BP and usage of ledger peers

Has Protocol Files (BP-enabled)	useLedgerAfterSlot	Log Severity
Y	Y	Warning - use of ledger peers not recommended for BP

Interaction between parsing of topology file and reading big ledger peer snapshot file

ConsensusMode	peerSnapshotFile	Snapshot present	useLedgerAfterSlot	Has Protocol Files (BP-enabled)	Log Severity
_	Y	_	N	Y	Info - ledger peers and peer snapshot, although specified in the topology file, are disabled in line with recommended BP operation
PraosMode	Y	_	N	N	Warn - peer snapshot file specified but topology disables ledger peers - ignoring file. To turn off this message enable the use of ledger peers or remove peerSnapshotFile from the topology file.
PraosMode	Y	_	N	Y	Warn - peer snapshot file specified but topology disables ledger peers - ignoring file. To turn off this message remove peerSnapshotFile from the topology file.
PraosMode	Y	N	Y	_	Warning - peer snapshot file missing
GenesisMode	_	_	N	N	Warning - it is recommended to use ledger peers and peer snapshot file for relay operations in Genesis mode
GenesisMode	N	_	Y	N	Warning - peer snapshot recommended
GenesisMode	Y	N	Y	_	Error - peer snapshot missing

[coot]

Reading peer snapshot file

When the node is configured to run in PraosMode (ConsensusMode key again) and the topology file specifies the key peerSnapshotFile but the file is missing the node shuts down with an error message (cf. readPeerSnapshotFile). For PraosMode, this should be just a warning level message.

However, when the node is running in GenesisMode (ConsensusMode key) and the peerSnapshotFile is missing, the node should fail with an error (e.g. preserve the current behaviour).

A summary for this warning:

ConsensusMode	Log Severity	process fails
PraosMode	Warning	no
GenesisMode	Error	yes

An additional complication is that cardano-node has two logging systems. Some links & remarks:

• parsing & reading topology file (which contains a location of the snapshot file) and the snapshot file is in Cardano.Node.Configuration.TopologyP2P module. Currently, these messages are hard-coded failures, but they should be turned into trace messages of StartupTrace
• legacy one, contained under Cardano.Tracing, severities of StartupTracer are defined in Cardano.Tracing.Startup module (btw, severities for network messages are declared in Cardano.Tracing.OrphanInstances.Network module);
• new logging framework, contained under Cardano.Node.Tracing, the StartupTracer in the new logging system is configured in the Cardano.Node.Tracing.Tracers.Startup module

Bootstrap Peers

We should also make sure that when bootstrap peers are configured in GenesisMode the node doesn't crash, but only emits an info message (a new message needs to be added to StartupTrace) that it will be ignored.

UseLedgerAfter

When the node is configured to run as a block producer, and the peer snapshot file is provided in the topology file, but useLedgerAfter is -1 or outright missing (ie. DontUseLedgerPeers) or this value points to a slot which is more recent than the slot recorded in the peer snapshot file, the node traces a message to either update the peer snapshot file or enable the use of ledger peers. Outside of testing environments, for BP's, the use of any peers besides trusted local roots is dubious. This warning should be removed in this situation to not cause further confusion, or perhaps replaced to warn that the use of ledger peers and peer snapshot is not recommended for a BP. The node can be considered to be a BP when any of the protocol files are present at startup. This information should be passed to the updateLedgerPeerSnapshot function to properly handle this situation.

I think we should still emit a trace, but only using a Warning severity (this also should be StartupTrace message. Since recently we use this function to check if a node is configured as a BP: hasProtocolFiles.

Tracing

For tracing, in ouroboros-network, cardano-node as well as other places we use contra-tracer library. (Its newer version is also on hackage).

[crocodile-dentist]

@johnalotoski ping for feedback

[johnalotoski]

Yes, I think this does cover all recent working group discussed corner cases. One final one which I'm not sure if it was already addressed in another PR was:

• Add a warning if node is a forger and PeerSharing is enabled explicitly.