IntersectMBO / IntersectMBO/ouroboros-consensus

Simplify time conversions in Consensus

Open
#1,301 5 comments 0 reactions 0 assignees View on GitHub
Dominant language
Haskell
Stars
67
Forks
43
Avg merge
5d 13h
Merged PRs (30d)
43

Description

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

- [ ] 1) We could replace[`blockForgeUTCTime`](https://github.com/input-output-hk/ouroboros-network/blob/92778e3bb243ab68015f1f1dc1acf13718e7b1e1/ouroboros-network-api/src/Ouroboros/Network/BlockFetch/ConsensusInterface.hs#L152) with [`headerForgeUTCTime`](https://github.com/input-output-hk/ouroboros-network/blob/92778e3bb243ab68015f1f1dc1acf13718e7b1e1/ouroboros-network-api/src/Ouroboros/Network/BlockFetch/ConsensusInterface.hs#L145), 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](https://github.com/intersectmbo/ouroboros-consensus/blob/f4d877b3f6c3163863582740bf5bba7a6730cbee/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client/InFutureCheck.hs#L131).
- [ ] 3) We take the following two independent paths:
- [ ] a) Remove [`HeaderStateWithTime`](https://github.com/intersectmbo/ouroboros-consensus/blob/f4d877b3f6c3163863582740bf5bba7a6730cbee/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/HeaderStateHistory.hs#L147) 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.[^1]
- [ ] 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.

[^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.

Contributor guide

Open the contributing guide

Research direction

Start with ConsensusInterface.hs, InFutureCheck.hs, and HeaderStateHistory.hs, tracing the existing slot-to-wallclock conversions and HeaderStateWithTime use. Review the ChainSync historicity check and Diffusion Layer interface before choosing between header annotations and an anchored-fragment wrapper. Done means the selected conversion paths are localized without weakening the MsgAwaitReply historicity behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
haskell
Domain
blockchain, distributed-systems
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.