[Peras 9] Peras voting rules prep: parameters, arrival times, and CertDB updates

[agustinmista]

This PR introduces some general changes and QoL improvements in anticipation to the introduction of the Peras voting rules. These are separated into hopefully digestible atomic commits:

1. Introduce Ouroboros.Consensus.Peras.Params module to keep track of Peras protocol parameters in a single place,
2. Add a small helper function onPerasRoundNo,
3. Tweak HasPerasCertX field projection typeclasses,
4. Introduce WithArrivalTime combinator to wrap values with a RelativeTime,
5. Wrap validated Peras certs with their arrival time (and adapt tests), and
6. Store latest certificate seen in the PerasCertDB and add a getLatestCertSeen method to retrieve it.

Please refer to the corresponding commit messages for more information.

[tbagrel1]

LGTM!

I think we may change the title of the PR though (and maybe part of its description) to better reflect that most changes are related to adding timestamps on certs and propagate the necessary stuff to make it work.

[nbacquey]

Besides my comments, it seems ok to undraft

[agustinmista]

FTR: actual changes here are minimal (masked by new indentation needed by forAll):

• Parameterize the property with the seed used to derive a mocked up SystemTime via forAll, and
• Instantiate a mocked up SystemTime and pass it around to the places needing it.

[jasagredo]

Enabling "Hide whitespace" in PR-review view config proves useful here.

[dnadales]

How does the code that calls this function deal with a potential exception?

[agustinmista]

My understanding is that certificate validation errors should be treated as fatal and trigger the disconnection from the peer.

@tbagrel1 @nbacquey do you know where such an exception could end up getting caught? I remember Alex mentioning that unhandled exceptions in a N2N app would eventually trigger a disconnection from the peer, but I would like my memory to be challenged 😅

[tbagrel1]

In ObjectDiffusionV2 we have withInboundPipelinedPeer which is a bracket function to properly remove peer data from the global peer data map when an exception arise. But this is a slightly orthogonal concern, since the exception still bubbles up after.

I think that exception handling for the thread in charge of running the mini-protocol is defined higher-up in the call tree, probably even above ouroboros-network. From my understanding it's a per-peer disconnection handler, not a per-protocol one, so theoretically we should not have anything specific to do for the new Peras protocol.

Still, I'm on the hunt for the exact repo/location where this handling happens. I'll answer the thread again when I find it :)

[tbagrel1]

Apparently:

• https://github.com/IntersectMBO/ouroboros-network/blob/main/network-mux/src/Network/Mux.hs#L453 catches an exception and rethrow it. This is supposed to ultimately shutdown the whole mux, as stated by this comment, but so far this is only a stated intention, as no disconnection has been done yet
• The exception is caught/handled here: https://github.com/IntersectMBO/ouroboros-network/blob/main/ouroboros-network/framework/lib/Ouroboros/Network/ConnectionHandler.hs#L282, and let runRethrowPolicy decide of what to do (which cmd to run). From https://github.com/IntersectMBO/ouroboros-network/blob/main/ouroboros-network/framework/lib/Ouroboros/Network/RethrowPolicy.hs#L119, we see that the command when we have an IOError is to ShutdownPeer, so classifyExceptions rethrows a ExceptionInHandler wrapping the underlying exception. The connection handlers with installed classifyExceptions handlers are returned as part of makeConnectionHandler
• Connection handlers are used by Ouroboros.Network.ConnectionManager.Core.with. In acquireOutboundConnectionImpl for example, when we don't already have a connection to the peer, we run forkConnectionHandler in a bracketOnError function responsible for closing the socket cleanly if an exception is raised. forkConnectionHandler also has its own cleanup procedure which might close the socket too but I don't know if this is this one or the one from bracketOnError above that is triggered by a Miniprotocol exception.
• At a higher level, ConnectionManager.Core.with is used by Ouroboros.Network.Diffusion.{run,runM,mkRemoteThread,mkLocalThread}

This should be taken with a pinch of salt; I may have missed an important handler. But in the end it seems that exceptions from mini-protocols just bubble-up until they reach the connection manager.

[jasagredo]

Is this datatype expected to dissolve once we start the node? in the sense that its fields will be used in some computation and all will be evaluated?

Otherwise the fields should probably be strict.

[agustinmista]

Fixed in 61b3b2e

[agustinmista]

Actually, we can't have strict fields if we want to conserve the error for parameters with yet-to-be-defined default values. I made the ones with default values strict, and left the others lazy.

[jasagredo]

Then please add a TODO to be fixed eventually.

[jasagredo]

Is the access to the PerasCertDB single threaded? If not, I think we should consider migrating to quickcheck-state-machine and defining some parallel properties to get confidence there cannot be a deadlock. That or using the new-ish support for parallel properties in quickcheck-dynamic but I don't know how polished is that.

Was there a particular reason to use quickcheck-dynamic from the start?

[fraser-iohk]

could we call this PerasBlockMinimumSlotAge? do we need it to be shortened?