Skip to content

consensus: annotate headers with their slot's RelativeTime #1288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 23, 2025
Merged

consensus: annotate headers with their slot's RelativeTime #1288

merged 1 commit into from
Apr 23, 2025

Conversation

dnadales
Copy link
Member

@dnadales dnadales commented Oct 22, 2024 •
edited by nfrisby
Loading

Introduce a new HeaderWithTime data type, which annotates each received header with the relative time of its slot as calculated by the ChainSync client.

Partially addresses #1301

amesgen
amesgen reviewed Oct 23, 2024
View reviewed changes
@amesgen amesgen mentioned this pull request Nov 6, 2024
Open
5 tasks
@dnadales dnadales force-pushed the dnadales/annotate-headers-with-time branch from b76d77c to 2b11f09 Compare November 19, 2024 13:11
amesgen
amesgen reviewed Nov 21, 2024
View reviewed changes
@dnadales dnadales force-pushed the dnadales/annotate-headers-with-time branch from 1b5a867 to 6f9c71b Compare November 27, 2024 15:52
@dnadales dnadales mentioned this pull request Dec 2, 2024
Closed
7 tasks
@nfrisby nfrisby force-pushed the dnadales/annotate-headers-with-time branch 2 times, most recently from 7296216 to f40c927 Compare December 10, 2024 23:26
@nfrisby
Copy link
Contributor

nfrisby commented Dec 12, 2024

I pushed up some commits that improve some names&Haddock and also hide HeaderWithTime from some tracers (to avoid extra work for downstream integration etc).

@nfrisby nfrisby force-pushed the dnadales/annotate-headers-with-time branch from ccc97d4 to 566d1e5 Compare December 12, 2024 16:28
@nfrisby
Copy link
Contributor

nfrisby commented Dec 12, 2024 •
edited
Loading

I changed the representation of the fragment pair based on the conversation we had in today's office hours.

I confirmed that the statemachine test fails (nicely) when I insert bugs in either ChainSel or Background's maintenance of the fragment with times. I introduced a bespoke strict pair type for clarity and to most easily satisfy the NoThunks tests (which are also enabled by the Cabal flags that enable the new invariant checking logic for the fragment pair).

@nfrisby nfrisby force-pushed the dnadales/annotate-headers-with-time branch 5 times, most recently from c625852 to 4cb2c5e Compare December 18, 2024 17:34
@nfrisby
Copy link
Contributor

nfrisby commented Dec 18, 2024

I squashed in order to rebase. I also minized the diff. The next step is to split it into two or three separate commits.

@nfrisby nfrisby force-pushed the dnadales/annotate-headers-with-time branch 6 times, most recently from 2effa46 to ee2f301 Compare December 18, 2024 21:17
nfrisby
nfrisby reviewed Dec 18, 2024
View reviewed changes
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/HeaderValidation.hs

instance ( HasHeader (Header blk)
, StandardHash (HeaderWithTime blk)
, Typeable blk
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the root cause of all the additional Typeable blk contexts in this commit. It's required to satisfy the superclass on the (upstream) HasHeader. I don't see how to avoid it, sadly.

@nfrisby nfrisby force-pushed the dnadales/annotate-headers-with-time branch 2 times, most recently from ba54272 to b4424eb Compare December 18, 2024 21:21
@nfrisby nfrisby changed the title [WIP] Annotate headers with time consensus: annotate headers with their slot's RelativeTime Dec 18, 2024
@nfrisby nfrisby marked this pull request as ready for review December 18, 2024 21:21
@geo2a geo2a mentioned this pull request Apr 2, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Apr 2, 2025
@geo2a
Split out ImmutableEraParams (#1441)
b0a9ee2 
This PR extracts some small changes from
#1288 in order
to make the rebase of
#1288 onto main
more manageable.
@geo2a geo2a force-pushed the dnadales/annotate-headers-with-time branch from b4424eb to 75abfdf Compare April 3, 2025 10:03
@geo2a
Copy link
Contributor

geo2a commented Apr 3, 2025

I've made progress on this PR by rebasing on top of the many changes in main.

@geo2a geo2a dismissed amesgen’s stale review April 3, 2025 10:29

I'm still making some progress there and do not want it to be merged yet.

@geo2a geo2a force-pushed the dnadales/annotate-headers-with-time branch from 870aa07 to c8cdf94 Compare April 3, 2025 10:30
@geo2a geo2a mentioned this pull request Apr 8, 2025
Merged
@geo2a geo2a force-pushed the dnadales/annotate-headers-with-time branch 2 times, most recently from 29a098c to 9a2a73e Compare April 9, 2025 15:57
@geo2a geo2a requested review from nfrisby and amesgen April 9, 2025 15:57
@geo2a
Copy link
Contributor

geo2a commented Apr 9, 2025 •
edited
Loading

@nfrisby @amesgen could you have another look at this PR? I think I've incorporated all the changes suggested in the comments.

Perhaps we should, after all, keep this change parked until UTXO-HD is on main.

amesgen
amesgen approved these changes Apr 10, 2025
View reviewed changes
Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (but, once again, we might not want to merge this immediately, cf. #1288 (comment))

...ensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/BlockFetch/ClientInterface.hs
-> SlotForgeTimeOracle m blk
-- ^ Slot forge time, see 'headerForgeUTCTime' and 'blockForgeUTCTime'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just remove SlotForgeTimeOracle completely

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl.hs
atomically $ LgrDB.setCurrent lgrDB ledger
varChain <- newTVarIO chain
varChain <- newTVarWithInvariantIO checkInternalChain $ InternalChain chain chainWithTime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the invariant that is being checked in checkInternalChain seems sufficiently lightweight that we could test it by default if e.g. assertions are enabled (which they are when running our tests in CI). However, TVar invariants are not checked by default (only in the nightly tests, which have their own problems, chiefly that we have no notifications for when they fail), as they include the nothunks tests, which are extremely slow.

I don't think this should block this PR though.

@geo2a geo2a moved this from 🏗 In progress to 👀 In review in Consensus Team Backlog Apr 15, 2025
@geo2a geo2a force-pushed the dnadales/annotate-headers-with-time branch from 9a2a73e to ca47595 Compare April 23, 2025 09:56
@geo2a geo2a requested a review from jorisdral as a code owner April 23, 2025 09:56
@geo2a @dnadales
@nfrisby @amesgen
consensus: remember and use slot-to-time translation from ChainSel
eb8f344 
- Define HeaderWithTime
- In ChainDB, maintain a parallel selection with time annotations
- Remove SlotForgeTimeOracle, as we no longer need to translate time in the BlockFetch client
- Update BlockFetch client to operate on chains that use HeaderWithTime
- Note that HeaderWithTime is hidden from the tracer events, at least for now.

Co-authored-by: Damian Nadales <[email protected]>
Co-authored-by: Nicolas Frisby <[email protected]>
Co-authored-by: amesgen <[email protected]>
@geo2a geo2a force-pushed the dnadales/annotate-headers-with-time branch from ca47595 to eb8f344 Compare April 23, 2025 10:08
@geo2a geo2a added this pull request to the merge queue Apr 23, 2025
Merged via the queue into main with commit 79d9770 Apr 23, 2025
19 checks passed
@geo2a geo2a deleted the dnadales/annotate-headers-with-time branch April 23, 2025 12:54
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Consensus Team Backlog Apr 23, 2025
@jasagredo jasagredo mentioned this pull request May 6, 2025
Closed
This was referenced May 7, 2025
Merged
Closed
github-merge-queue bot pushed a commit that referenced this pull request May 8, 2025
@geo2a
consensus: more carefully hide HeaderWithTime from traces (#1498)
292d711 
This is a follow-up to #1288 that ensures

- do not expose `HeaderWithTime` in `TraceGDDEvent` via `GDDDebugInfo`
- do not store the chain fragment with `HeaderWithTime` in
`DensityBounds`

adding the "no changelog'' label, as the relevant changelog fragments
are already included into "main"

Draft PR to for integration with the node:
IntersectMBO/cardano-node#6211
@amesgen amesgen mentioned this pull request May 12, 2025
Open
@amesgen amesgen mentioned this pull request Jun 2, 2025
Open
@amesgen amesgen mentioned this pull request Jul 15, 2025
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amesgen amesgen amesgen approved these changes

@jasagredo jasagredo Awaiting requested review from jasagredo jasagredo is a code owner

@fraser-iohk fraser-iohk Awaiting requested review from fraser-iohk fraser-iohk is a code owner

@geo2a geo2a Awaiting requested review from geo2a geo2a is a code owner

@nfrisby nfrisby Awaiting requested review from nfrisby nfrisby is a code owner

@jorisdral jorisdral Awaiting requested review from jorisdral

Assignees

@nfrisby nfrisby

@geo2a geo2a

Labels
technical debt Technical debt
Projects
Consensus Team Backlog
Status: ✅ Done
Milestone
Unknown quarter
Development

Successfully merging this pull request may close these issues.
4 participants
@dnadales @nfrisby @geo2a @amesgen