Simplify time conversions in Consensus

[dnadales]

This issue captures some changes that can be implemented to reduce or localize slot-to-wallclock conversions in our code.

• 1) We could replaceblockForgeUTCTime with headerForgeUTCTime, since the equivalent header is in scope.
• 2) We could annotate headers that have been validated by ChainSync with their UTC Time, which is calculated here.
• 3) We take the following two independent paths:

  • a) Remove HeaderStateWithTime and use the header's annotation for the historicity check instead.
    The only caveat here is that the MsgAwaitReply part of the historicity check currently relies on having a slot time for the anchor in case the fragment is empty. Possible approaches are:

    1. Stop relying on this and always disengage (or even disconnect) a peer that sends MsgAwaitReply when their candidate fragment is empty (remember that candidate fragments are anchored in a recent immutable tip). This would be more zeleaous than the current historicity check, so it requires proper justification. It seems very plausible to us, but it doesn't seem to immediately follow from any of the "usual" properties like Chain Growth or Existential Chain Quality.
    2. Introduce wrapper on top of AnchoredFragment that also stores extra data (the slot time of the anchor) for use for the candidate fragments. We would need to implement functions like intersect for this that properly update the extra anchor data.¹

  • b) Propagate HeaderWithTime into the Diffusion Layer's interface so that headerForgeUTCTime simply returns the annotation from the header. This will remove an invocation of slotToWallclock, instead reusing the header's time annotation.

Observation: Steps 2 and 3b are not really independent. 2 has a significant performance cost without 3b (because discarding the headers requires a lot of allocation). So, both probably need to be in a PR together, but they could be different commits.

Footnotes

1. With infinite time, it would be neat to generalize the AnchoredFragment API to allow custom extra data for the anchor; but it is rather questionable that this is a useful thing to pursue right now. ↩

[nfrisby]

After our discussion in the call just now, I have a few higher-level thoughts to record/emphasize.

• AnchoredFragment is a specialization of AnchoredSeq. It constrains what the anchor could be (type AnchoredFragment block = AnchoredSeq (WithOrigin SlotNo) (Anchor block) block), and it offers some useful functions, espeically AF.intersect.

• The node's selection and each ChainSync client's candidate fragment are both AnchoredFragments. Notably, AF.intersect is used to update the candidate fragment when the selection has changed (not necessarily every time it changes).

• This makes it somewhat awkward to change the contents of the candidate fragment without also changing the contents of the node's selection, which is an important part of the API. "Just map forget over the internal TVar to hide the extra data" might be OK, but it also seems like it could incur inefficiency.

• The existing HeaderStateHistory uses the selection's available ledger states to compute the necessary times when the candidate fragment is intersected with the selection. We could do the same when the candidate fragment itself carries the times. This also seems inefficient (but only per-MsgFindIntersect).

So, in terms of a final target: I suspect we should be adding times to the ChainDB's internal TVar in addition to the ChainSync clients' candidate fragments. Perhaps the API for the selection should be exists x. (AnchoredFragment x, x -> blk), for the sake of extensibility. (I have not considered the best series of steps to reach that point, which is a focus of this Issue.)

[amesgen]

• This makes it somewhat awkward to change the contents of the candidate fragment without also changing the contents of the node's selection, which is an important part of the API. "Just map forget over the internal TVar to hide the extra data" might be OK, but it also seems like it could incur inefficiency.

Yeah, changing the selection to also track the slot times would make that cleaner (and would be an overall nice goal as you say).

However, it requires changes to ChainSel (in particular: how/where exactly do we get the slot time for blocks we load from disk, both on startup and when switching forks), so it seems potentially nice to have an incremental path ("best series of steps to reach that point" as you say) where the candidate fragments are already annotated with time, but the selection isn't (yet).

#1288 is currently written in that style, and @dnadales also made sure that the BlockFetchConsensusInterface can be adapted accordingly (see IntersectMBO/ouroboros-network#5010).

[amesgen]

FTR: See the PR description of #1288 (comment) for a concrete work item.