Remove `cdbFutureBlocks` #1269

dnadales · 2024-09-27T10:42:22Z

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

Closes #1260

nfrisby

Nice! Love the +186/-516 ratio 😁

I didn't Approve yet because of the ongoing discussion on the REVIEW comment, etc.

docs/website/contents/for-developers/HandlingBlocksFromTheFuture.md

...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs

amesgen

Looks good!

Inspecting rg -i future -t haskell resulted in the following places that should also be adapted:

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

Line 689 in 16fa875

-- future. Although the chain DB does not adopt future blocks, if the

The ChainDB no longer does this; rather, it now has this as a precondition, so what we are doing here is good.

ouroboros-consensus/ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Node/ErrorPolicy.hs

Lines 74 to 80 in 16fa875

    
             -- When the system clock moved back, we have to restart the node, 
        
             -- because the ImmutableDB validation might have to truncate some 
        
             -- blocks from the future. Note that a full validation is not 
        
             -- required, as the default validation (most recent epoch) will keep 
        
             -- on truncating epochs until a block that is not from the future is 
        
             -- found. 
        
           , ErrorPolicy $ \(_ :: SystemClockMovedBackException) -> Just shutdownNode

This comment is incorrect both with and without this PR; the ImmutableDB does not truncate blocks from the future. (This comment is quite old (IntersectMBO/ouroboros-network@eed741a), I haven't checked whether it might have been correct back then).

Same thing here, the comment has been copy-pasted:

ouroboros-consensus/ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Node/RethrowPolicy.hs

Lines 68 to 74 in 16fa875

    
             -- When the system clock moved back, we have to restart the node, 
        
             -- because the ImmutableDB validation might have to truncate some 
        
             -- blocks from the future. Note that a full validation is not 
        
             -- required, as the default validation (most recent epoch) will keep 
        
             -- on truncating epochs until a block that is not from the future is 
        
             -- found. 
        
           <> mkRethrowPolicy (\_ctx (_ :: SystemClockMovedBackException) -> shutdownNode)

ouroboros-consensus/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/API.hs

Line 681 in 16fa875

| InFutureExceedsClockSkew !(RealPoint blk)

can be removed (or maybe even the entire InvalidBlockReason type)

docs/website/contents/for-developers/HandlingBlocksFromTheFuture.md

ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/StateMachine.hs

...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs

dnadales · 2024-10-11T20:11:12Z

I converted it to a draft till the consensus packages needed for Node 10.0 have been released.

amesgen

LGTM (after (maybe partial) squashing), only two minor comments

docs/website/contents/for-developers/HandlingBlocksFromTheFuture.md

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/API.hs

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

…ure are handled

... instead of the new tip. Chain selection uses `Query.getTipPoint` instead of `chainSelectionForBlock`.

The precondition states we never add blocks from the future.

The `ChainDB` state machine model does not need to take the block slots into account, as a result this part of the logic has been removed from the model and tests.

…m the future

we might forge atop a block form the future

... since it was only wrapping up an `ExtValidationError` value.

The handling of blocks from the future in the ChainDB was removed in #1269.

dnadales requested review from nfrisby, jasagredo, amesgen and fraser-iohk as code owners September 27, 2024 10:42

dnadales force-pushed the dnadales/1260-remove-cdbFutureBlocks branch 4 times, most recently from 6a29209 to d3b359c Compare September 27, 2024 11:28

nfrisby reviewed Sep 30, 2024

View reviewed changes

amesgen reviewed Oct 8, 2024

View reviewed changes

dnadales marked this pull request as draft October 11, 2024 20:10

dnadales force-pushed the dnadales/1260-remove-cdbFutureBlocks branch from d8ae619 to cedff47 Compare November 8, 2024 16:07

dnadales marked this pull request as ready for review November 8, 2024 16:14

amesgen approved these changes Nov 8, 2024

View reviewed changes

docs/website/contents/for-developers/HandlingBlocksFromTheFuture.md Outdated Show resolved Hide resolved

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/API.hs Outdated Show resolved Hide resolved

dnadales added 3 commits November 11, 2024 16:21

Add changelog fragments

b593b9c

Remove cdbFutureBlocks

949f6b2

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

Remove obsolete module Ouroboros.Consensus.Fragment.InFuture

232f607

dnadales force-pushed the dnadales/1260-remove-cdbFutureBlocks branch from cedff47 to d7ce177 Compare November 11, 2024 15:40

dnadales added 8 commits November 11, 2024 16:48

Add a section in our documentation describing how blocks from the fut…

78343cb

…ure are handled

Change chainSelectionForBlock so that it returns ()

28dde8d

... instead of the new tip. Chain selection uses `Query.getTipPoint` instead of `chainSelectionForBlock`.

Add a precondition to addBlock and addBlockAsync

5d01e90

The precondition states we never add blocks from the future.

Remove the AddFutureBlock command

4220761

The `ChainDB` state machine model does not need to take the block slots into account, as a result this part of the logic has been removed from the model and tests.

Correct a comment stating that the ImmutableDB truncates blocks fro…

8c05749

…m the future

Remove InFutureExceedsClockSkew

40ac1b9

Correct the comment stating under which conditions ...

3f88dc0

we might forge atop a block form the future

Remove the InvalidBlockReason data type

21ab657

... since it was only wrapping up an `ExtValidationError` value.

dnadales force-pushed the dnadales/1260-remove-cdbFutureBlocks branch from d7ce177 to 21ab657 Compare November 11, 2024 15:49

dnadales enabled auto-merge November 11, 2024 16:00

dnadales added this pull request to the merge queue Nov 11, 2024

Merged via the queue into main with commit e86b921 Nov 11, 2024
17 checks passed

dnadales deleted the dnadales/1260-remove-cdbFutureBlocks branch November 11, 2024 17:26

amesgen mentioned this pull request Nov 22, 2024

UTxO-HD targeting main #1267

Open

amesgen added a commit that referenced this pull request Nov 22, 2024

BlockFetch client test: remove in-future check todo

3f01af4

The handling of blocks from the future in the ChainDB was removed in #1269.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `cdbFutureBlocks` #1269

Remove `cdbFutureBlocks` #1269

dnadales commented Sep 27, 2024

nfrisby left a comment

amesgen left a comment

dnadales commented Oct 11, 2024

amesgen left a comment •

edited

Loading

	-- When the system clock moved back, we have to restart the node,
	-- because the ImmutableDB validation might have to truncate some
	-- blocks from the future. Note that a full validation is not
	-- required, as the default validation (most recent epoch) will keep
	-- on truncating epochs until a block that is not from the future is
	-- found.
	, ErrorPolicy $ \(_ :: SystemClockMovedBackException) -> Just shutdownNode

Remove cdbFutureBlocks #1269

Remove cdbFutureBlocks #1269

Conversation

dnadales commented Sep 27, 2024

nfrisby left a comment

Choose a reason for hiding this comment

amesgen left a comment

Choose a reason for hiding this comment

dnadales commented Oct 11, 2024

amesgen left a comment • edited Loading

Choose a reason for hiding this comment

Remove `cdbFutureBlocks` #1269

Remove `cdbFutureBlocks` #1269

amesgen left a comment •

edited

Loading