Do not even propagate future headers #525

nfrisby · 2023-11-28T00:55:37Z

@njd42 and I came up with this idea a couple weeks ago, during a chat about the vagaries of NTP/clocks/etc.

See the commit message of the 'consensus: do not even propagate future headers' commit.

The main concrete benefits:

It's a step towards Chronos and that helps simplify/ground our story around future blocks.
A network-load optimization of sorts: if someone with a horribly out of sync clock sends a block 60 seconds early, the direct neighbors won't even propagate it.

nfrisby · 2023-11-28T01:05:13Z

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

+{-# LANGUAGE StandaloneDeriving #-}
+{-# LANGUAGE TypeApplications #-}
+
+module Ouroboros.Consensus.MiniProtocol.ChainSync.Client.InFutureCheck (


@njd42 Please confirm this logic matches what you and I chatted about a few weeks ago. Thanks!

@njd42 It's being merged now, but I'd still appreciate your thoughts on this module. Thanks.

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

nfrisby · 2023-11-28T01:14:48Z

FYI, the PR is draft because I've so far only propagated the ChainSync client interface change to its test. I'll tackle the rest tomorrow.

Edit: No longer Draft.

amesgen

Looks great, mostly minor comments and a few suggestions. Will review again after the chores (formatting, compilation failures, changelog entries, etc.).

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

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Fragment/InFuture.hs

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

amesgen · 2023-11-28T10:59:39Z

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

Note to other reviewers: The diff here is much smaller when using sth like difftastic.

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

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

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

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

...sensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Network/NodeToNode.hs

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

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

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

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

nfrisby · 2023-11-29T19:03:26Z

Just FYI, I just now rebased onto origin/main---there were no conflicts.

nfrisby · 2023-11-29T20:55:44Z

CI is green 👍

amesgen

LGTM (after squashing), only very minor comments left

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

nfrisby · 2023-11-30T16:54:18Z

FYI: I based a second time to sign the commits.

Main change: remove the LogicalClock, since it's quite a bit of indirection with no real gain. Also, remove -XRecordWildCards.

This an aggressively-simple interpretation of the easy part of Ouroboros Chronos in the presence of The Header-Body Split. (The hard part is the _synchronization beacons_---which need to be propagated promptly, and will be implemented much later.) Other less-aggressive interpretations would propagate future headers/blocks but set them aside. But this seems much simpler and within an RTT or two, assuming eg the NTP clients are well-configured.

This now exercises the real ChainSync InFutureCheck.

njd42 · 2023-12-04T08:48:39Z

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

+            now <- systemTimeCurrent systemTime
+            let ageNow         = now `diffRelTime` onset
+                syntheticDelay = negate ageNow
+            threadDelay $ nominalDelay syntheticDelay   -- TODO leap seconds?


I think that we can resolve the leap second argument now. Worst case is a 0.9s jump (see https://en.wikipedia.org/wiki/DUT1). So the two extrema are:

local clock adjustment is +ve by 0.9s : the delay here would be (up to) 0.9s 'too much' - this is not an operational issue, delays of that magnitude occur during usual operation

local clock adjustment is -ve by 0.9s: this could be perceived by other nodes as a (minor) future arrival and therefore delayed there by a small amounr.

The handling of leap seconds is (and will likely forever be) a bit of mess (see https://en.wikipedia.org/wiki/Leap_second#Issues_created_by_insertion_(or_removal)_of_leap_seconds).

As we only ever take the slotToWallClock time direction we would be safe as this is always (to UTC) unambiguously defined. The mapping in the other direction is (around leap seconds) ambiguous depending on the operating system's handling of leap seconds.

njd42

It all looks good to me - I've added some comments as to why I believe that you have already effectively dealt with the leap second issue (as best as can be done)

After #525, the `cdbFutureBlocks` field became unnecessary, as we now delay headers until they are no longer from the (near) future.

After #525, the `cdbFutureBlocks` field became unnecessary, as we now delay headers until they are no longer from the (near) future. Closes #1260

nfrisby added the enhancement New feature or request label Nov 28, 2023

nfrisby requested review from amesgen and njd42 November 28, 2023 00:55

nfrisby force-pushed the nfrisby/ChainSync-future branch from 916e825 to 330452e Compare November 28, 2023 01:03

nfrisby commented Nov 28, 2023

View reviewed changes

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

nfrisby force-pushed the nfrisby/ChainSync-future branch from 330452e to ee73914 Compare November 28, 2023 01:15

amesgen reviewed Nov 28, 2023

View reviewed changes

nfrisby mentioned this pull request Nov 28, 2023

Add far future blocks to the ChainSync client test #526

Open

nfrisby force-pushed the nfrisby/ChainSync-future branch from aab0db0 to 142a011 Compare November 28, 2023 19:43

nfrisby commented Nov 28, 2023

View reviewed changes

...sensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Network/NodeToNode.hs Outdated Show resolved Hide resolved

nfrisby commented Nov 28, 2023

View reviewed changes

...oros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/NodeKernel.hs Outdated Show resolved Hide resolved

nfrisby commented Nov 28, 2023

View reviewed changes

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

amesgen mentioned this pull request Nov 29, 2023

ChainSync client test: decrease number of tests without any sent headers #529

Closed

amesgen requested changes Nov 29, 2023

View reviewed changes

nfrisby force-pushed the nfrisby/ChainSync-future branch from cb6ba32 to 754ab23 Compare November 29, 2023 19:03

nfrisby force-pushed the nfrisby/ChainSync-future branch from 19b9720 to fa31155 Compare November 29, 2023 19:29

nfrisby marked this pull request as ready for review November 29, 2023 20:56

nfrisby requested a review from a team as a code owner November 29, 2023 20:56

amesgen approved these changes Nov 30, 2023

View reviewed changes

dnadales mentioned this pull request Nov 30, 2023

Block forge should check for pending future blocks in ChainDB #579

Closed

nfrisby force-pushed the nfrisby/ChainSync-future branch from 137f887 to db094ad Compare November 30, 2023 16:41

nfrisby enabled auto-merge November 30, 2023 16:46

nfrisby force-pushed the nfrisby/ChainSync-future branch from db094ad to dcefa64 Compare November 30, 2023 16:53

nfrisby added 2 commits November 30, 2023 09:02

consensus-test: improve legibility in the ChainSync client test driver

53223fd

Main change: remove the LogicalClock, since it's quite a bit of indirection with no real gain. Also, remove -XRecordWildCards.

nfrisby added 2 commits November 30, 2023 09:02

consensus-test: include near-future headers in ChainSync client test

a39cd95

This now exercises the real ChainSync InFutureCheck.

Update changelogs for ChainSync.InFutureCheck

8bb0efc

nfrisby force-pushed the nfrisby/ChainSync-future branch from dcefa64 to 8bb0efc Compare November 30, 2023 17:02

nfrisby added this pull request to the merge queue Nov 30, 2023

Merged via the queue into main with commit 3a1fde0 Nov 30, 2023
12 of 14 checks passed

nfrisby deleted the nfrisby/ChainSync-future branch November 30, 2023 23:32

njd42 reviewed Dec 4, 2023

View reviewed changes

nfrisby linked an issue Jan 5, 2024 that may be closed by this pull request

Block forge should check for pending future blocks in ChainDB #579

Closed

nfrisby mentioned this pull request Jan 10, 2024

Operation serenity Q4 2023 #415

Open

5 tasks

This was referenced Jan 23, 2024

Introduce the Limit on Eagerness #878

Merged

Add the pre-Genesis State Machine for bootstrap peers #808

Merged

dnadales mentioned this pull request Sep 23, 2024

Remove cdbFutureBlocks #1260

Closed

dnadales added a commit that referenced this pull request Sep 27, 2024

Remove cdbFutureBlocks

477aa3f

After #525, the `cdbFutureBlocks` field became unnecessary, as we now delay headers until they are no longer from the (near) future.

dnadales mentioned this pull request Sep 27, 2024

Remove cdbFutureBlocks #1269

Merged

dnadales added a commit that referenced this pull request Nov 8, 2024

Remove cdbFutureBlocks

2a2e41a

After #525, the `cdbFutureBlocks` field became unnecessary, as we now delay headers until they are no longer from the (near) future.

dnadales added a commit that referenced this pull request Nov 11, 2024

Remove cdbFutureBlocks

949f6b2

After #525, the `cdbFutureBlocks` field became unnecessary, as we now delay headers until they are no longer from the (near) future.

github-merge-queue bot pushed a commit that referenced this pull request Nov 11, 2024

Remove cdbFutureBlocks (#1269)

e86b921

After #525, the `cdbFutureBlocks` field became unnecessary, as we now delay headers until they are no longer from the (near) future. Closes #1260

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not even propagate future headers #525

Do not even propagate future headers #525

nfrisby commented Nov 28, 2023 •

edited

Loading

nfrisby Nov 28, 2023

nfrisby Nov 30, 2023

nfrisby commented Nov 28, 2023 •

edited

Loading

amesgen left a comment

amesgen Nov 28, 2023

nfrisby commented Nov 29, 2023

nfrisby commented Nov 29, 2023

amesgen left a comment •

edited

Loading

nfrisby commented Nov 30, 2023

njd42 Dec 4, 2023 •

edited

Loading

njd42 left a comment

Do not even propagate future headers #525

Do not even propagate future headers #525

Conversation

nfrisby commented Nov 28, 2023 • edited Loading

nfrisby Nov 28, 2023

Choose a reason for hiding this comment

nfrisby Nov 30, 2023

Choose a reason for hiding this comment

nfrisby commented Nov 28, 2023 • edited Loading

amesgen left a comment

Choose a reason for hiding this comment

amesgen Nov 28, 2023

Choose a reason for hiding this comment

nfrisby commented Nov 29, 2023

nfrisby commented Nov 29, 2023

amesgen left a comment • edited Loading

Choose a reason for hiding this comment

nfrisby commented Nov 30, 2023

njd42 Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

njd42 left a comment

Choose a reason for hiding this comment

nfrisby commented Nov 28, 2023 •

edited

Loading

nfrisby commented Nov 28, 2023 •

edited

Loading

amesgen left a comment •

edited

Loading

njd42 Dec 4, 2023 •

edited

Loading