[WIP] Annotate headers with time #1288

dnadales · 2024-10-22T10:08:57Z

Introduce a new HeaderWithTime data type, which associates the relative slot time the chain sync client mini-protocol received the header.

Introduce a new `HeaderWithTime` data type which associates the relative slot time the header was received by the chain sync client mini-protocol.

...oros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/NodeKernel.hs

amesgen · 2024-10-23T09:50:00Z

ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/Run.hs

+    -- REVIEW: is it ok to drop time here?
+  = peerSimStateDiagramSTMTracerDebug gtBlockTree getCurrentChain (undefined getCandidates) getPoints


I think so; only performance could theoretically be a concern here, but I don't think so.

amesgen · 2024-10-23T09:53:21Z

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Genesis/Governor.hs

+    -- REVIEW: we should avoid this linear computation, but I don't see an alternative to changing the type of the LoE fragment.
+    pure $ dropTime loeFrag


I think the approach here would be to make the logic work on AnchoredFragment (HeaderWithTime blk) (or even AnchoredFragment a where a is suitably constrained). This will e.g. also require changing the type of cdbLoE in the ChainDB.

amesgen · 2024-10-23T09:54:08Z

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Genesis/Governor.hs

@@ -349,7 +359,9 @@ densityDisconnect (GenesisWindow sgen) (SecurityParam k) states candidateSuffixe
          -- If not, it is not qualified to compete by density (yet).
          offersMoreThanK = totalBlockCount > k

-      pure (peer, DensityBounds {clippedFragment, offersMoreThanK, lowerBound, upperBound, hasBlockAfter, latestSlot, idling})
+      pure (peer, DensityBounds {
+               clippedFragment = (dropTime clippedFragment), -- REVIEW: should we change the type of the clipped fragment?


amesgen · 2024-10-23T09:55:39Z

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Genesis/Governor.hs

+  -> [(peer, AnchoredFragment (HeaderWithTime blk))] -- REVIEW: add a comment here? (in addition to the haddock above)
+  -> AnchoredFragment (HeaderWithTime blk)           -- REVIEW: add a comment here?


Yeah, moving the Haddocks to the individual parameters from the docs above sounds good, but seems orthogonal to this PR.

It is indeed; however, I don't think we'll ever do if we don't improve the haddocks while working on other tasks. I can use a separate commit for this, of course :)

amesgen · 2024-10-23T09:58:18Z

...ensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/BlockFetch/ClientInterface.hs

+    readCandidateChains = undefined getCandidates
+       -- REVIWE: Should we change the 'BlockFetchConsensusInterface' s that 'readCandidateChains' return 'HeaderWithTime'?


That's exactly the idea of step 3b) in the corresponding draft issue 👍 As BlockFetchConsensusInterface is polymorphic in what type header is used for headers, it doesn't even require changing BlockFetchConsensusInterface for this purpose. Concretely, mkBlockFetchConsensusInterface would return BlockFetchConsensusInterface peer (HeaderWithTime blk) blk m.

I see. This would change the type of readCurrentChain, which I'm unsure it is intended (since now the current chain will need to incorporate the times).

amesgen · 2024-10-23T10:00:01Z

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/HeaderValidation.hs

+-- 'ValidatedHeader' however this seems to be a misnomer since the
+-- header is not validated at the point at which we construct this
+-- value in 'Ouroboros.Consensus.MiniProtocol.ChainSync.Client'.


I think that depends on where you construct HeaderWithTime/ValidatedHeader.

(Generally, HeaderWithTime sounds good though 👍)

amesgen · 2024-10-23T10:10:27Z

...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs

          $ (   case isPipeliningEnabled version of
-                    ReceivingTentativeBlocks    -> drop 1
+                    ReceivingTentativeBlocks    -> drop 1 -- REVIEW: we could also use this opportunity to explain this.


Does the comment above clear it up?

ouroboros-consensus/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs

Lines 2008 to 2012 in 43e04df

-- As of block diffusion pipelining, their tip header might be

-- tentative. Since they do not yet have a way to explicitly say

-- whether it is tentative, we assume it is and therefore skip their

-- tip here. TODO once it's explicit, only skip it if it's annotated as

-- tentative

Maybe we could move the comment directly above the drop 1?

amesgen · 2024-10-23T10:11:55Z

ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/BlockFetch/Client.hs

@@ -281,7 +281,7 @@ runBlockFetchTest BlockFetchClientTestSetup{..} = withRegistry \registry -> do
        BlockFetchClientInterface.mkBlockFetchConsensusInterface
          (TestBlockConfig numCoreNodes)
          chainDbView
-          getCandidates
+          (undefined getCandidates) -- TODO: adapt 'getCandidates' if the change to 'mkBlockFetchConsensusInterface' is correct.


For this test, the annotated times should be irrelevant.

amesgen · 2024-10-23T10:14:24Z

ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/ChainSync/Client.hs

+    -- TODO: this should be a library function if the idea of dropping time on the candidate fragment makes sense.
+    dropTime = undefined


Dropping time here seems fine 👍

From ``` BlockFetchConsensusInterface (ConnectionId addrNTN) (Header blk) blk m ``` to ``` BlockFetchConsensusInterface (ConnectionId addrNTN) (HeaderWithTime blk) blk m ``` The next commits will start implementing the `undefined` in the code.

This is semantically identical (as the LoE fragment is anchored in a recent immutable tip), but it is nice to use a more familiar function, and it is also potentially useful for a planned change.

... from `AnchoredFragment (Header blk)` to `AnchoredFragment (HeaderWithTime blk)`.

... which contains an `AddBogusTime` type class, which allows to add bogus times to headers. This type class and its instances are meant to be used in test code.

... from `AnchoredFragment (Header TestBlock))` to `AnchoredFragment (HeaderWithTime TestBlock))`.

... in the density disconnect tests.

[WIP] Annotate headers with time

837e663

Introduce a new `HeaderWithTime` data type which associates the relative slot time the header was received by the chain sync client mini-protocol.

amesgen reviewed Oct 23, 2024

View reviewed changes

dnadales added 5 commits October 25, 2024 13:37

Add a ProjectHeader type class.

4ad52f1

Generalize preferAnchoredCandidate.

257d8b4

Fixup!

1e010dc

Generalize compareAnchoredFragments

247bd80

amesgen mentioned this pull request Nov 6, 2024

Simplify time conversions in Consensus #1301

Open

5 tasks

dnadales and others added 5 commits November 14, 2024 12:13

Adapt the code to the changes to the BlockFetchConsensusInterface

444812f

ChainSel: stop using cross for sanitizing LoE

6b7a3fb

This is semantically identical (as the LoE fragment is anchored in a recent immutable tip), but it is nice to use a more familiar function, and it is also potentially useful for a planned change.

ChainSel: improve docs for sanitizeLoEFrag

a181566

Change the type of LoE fragments

fdb333b

... from `AnchoredFragment (Header blk)` to `AnchoredFragment (HeaderWithTime blk)`.

Add a Test.Util.HeaderValidation module

2b11f09

... which contains an `AddBogusTime` type class, which allows to add bogus times to headers. This type class and its instances are meant to be used in test code.

dnadales force-pushed the dnadales/annotate-headers-with-time branch from b76d77c to 2b11f09 Compare November 19, 2024 13:11

dnadales added 3 commits November 19, 2024 14:22

Change the type of StaticCandidates.suffixes

f36cf62

... from `AnchoredFragment (Header TestBlock))` to `AnchoredFragment (HeaderWithTime TestBlock))`.

Change the type of StaticCandidates.loeFrag

1b8973d

... from `AnchoredFragment (Header TestBlock))` to `AnchoredFragment (HeaderWithTime TestBlock))`.

Remove the use of dropTime

23718d1

... in the density disconnect tests.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Annotate headers with time #1288

[WIP] Annotate headers with time #1288

dnadales commented Oct 22, 2024

amesgen Oct 23, 2024

amesgen Oct 23, 2024

amesgen Oct 23, 2024

amesgen Oct 23, 2024

dnadales Oct 23, 2024

amesgen Oct 23, 2024

dnadales Oct 23, 2024

amesgen Oct 23, 2024

amesgen Oct 23, 2024

amesgen Oct 23, 2024

amesgen Oct 23, 2024

		-- REVIEW: is it ok to drop time here?
		= peerSimStateDiagramSTMTracerDebug gtBlockTree getCurrentChain (undefined getCandidates) getPoints

		-- REVIEW: we should avoid this linear computation, but I don't see an alternative to changing the type of the LoE fragment.
		pure $ dropTime loeFrag

		-> [(peer, AnchoredFragment (HeaderWithTime blk))] -- REVIEW: add a comment here? (in addition to the haddock above)
		-> AnchoredFragment (HeaderWithTime blk) -- REVIEW: add a comment here?

		readCandidateChains = undefined getCandidates
		-- REVIWE: Should we change the 'BlockFetchConsensusInterface' s that 'readCandidateChains' return 'HeaderWithTime'?

	-- As of block diffusion pipelining, their tip header might be
	-- tentative. Since they do not yet have a way to explicitly say
	-- whether it is tentative, we assume it is and therefore skip their
	-- tip here. TODO once it's explicit, only skip it if it's annotated as
	-- tentative

		-- TODO: this should be a library function if the idea of dropping time on the candidate fragment makes sense.
		dropTime = undefined

[WIP] Annotate headers with time #1288

Are you sure you want to change the base?

[WIP] Annotate headers with time #1288

Conversation

dnadales commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment