Prototype lightweight checkpointing (non-combinator approach)

[amesgen]

Prototype of lightweight checkpointing, see #448

To be merged, this would need more polish (Haddocks, addressing the inline TODOs) and tests.

Notably, this PR adds a new field to ProtocolParams (CardanoBlock c):

data instance ProtocolParams (CardanoBlock c) = ProtocolParamsCardano {
  ...
  , cardanoCheckpoints            :: Map BlockNo (HeaderHash (CardanoBlock c))
  }

with the semantics described in #447 (comment).

Note that this is not the approach described in #447 (comment); the approach of this PR is likely less invasive/has a smaller diff, but less elegant than the combinator-based appraoch.

[nfrisby]

First impressions

[nfrisby]

What's the hypothesis for why we wouldn't need to propagate?

[amesgen]

This function isn't ever called by protocolInfoCardano (which is the only place actually interesting to us), so I just did the lazy thing and didn't propagate it further for this prototype PR.

[facundominguez]

I would consider addressing this if necessary for testing.

[nfrisby]

same comment as other guard

[amesgen]

Yeah, I am always paralyzed by the choice between when and unless... I used unless here as it is also used in all preceeding checks (even though it would be easy to rewrite these places from eg unless (m <= maxpv) to when (m > maxpv)), and then also used it in all other additionalEnvelopeChecks for consistency.

[facundominguez]

Switched unless to when in 7ca4c63

[facundominguez]

The plan now is to rearrange the PR so Ouroboros.Consensus.HeaderValidation.validateEnvelope invokes the validation of checkpoints. Let's call this function

validateIfCheckpoint :: TopLevelConfig blk -> Header blk -> Except (HeaderEnvelopeError blk) ()

This has the advantage of only needing to call the validation code at one place for ALL types of blocks.

This in turn requires getting the map of checkpoints at this point in the code. This can be done by adding the map as a method to the class ValidateEnvelope:

class ... => ValidateEnvelope blk where 
  ...
  -- | Checkpoints to validate. See 'validateIfCheckpoint'.
  checkpoints :: TopLevelConfig blk -> Map BlockNo (HeaderHash blk)

I wouldn't use a default method for this. It is better to signal to authors of instances that they need to define this method if they care about checkpointing, and they can easily return the empty map when they don't care.

Then the instances of ValidateEnvelope for ShelleyBlock and ByronBlock would need to return the checkpoints in their respective top-level configs.

As testing plan, we would only test validateIfCheckpoint for TestBlock. In particular, we wouldn't write any tests for the instances of ValidateEnvelope for ShelleyBlock and ByronBlock.

[nfrisby]

I suggest we take the "universality"-based approach even farther, and simply add a field topLevelConfigCheckpoints :: Map BlockNo (HeaderHash blk) to TopLevelConfig.

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

Lines 38 to 45 in 982eff4

	-- | The top-level node configuration
	data TopLevelConfig blk = TopLevelConfig {
	topLevelConfigProtocol :: !(ConsensusConfig (BlockProtocol blk))
	, topLevelConfigLedger :: !(LedgerConfig blk)
	, topLevelConfigBlock :: !(BlockConfig blk)
	, topLevelConfigCodec :: !(CodecConfig blk)
	, topLevelConfigStorage :: !(StorageConfig blk)
	}

[amesgen]

Just ensured that there are no roadblocks for this approach: #898 (diff could get much smaller with a compat layer/pattern synonym, but also not really necessary).

For completeness: A (we think relatively minor) disadvantage of #898 is that it adds checkpointing as a built-in concept to the abstract Consensus layer, despite this not being strictly necessary. (It is of course always possible to not do checkpointing by simply not providing any checkpoints.)

A small subtlety I noticed: In the HFC, we often create TopLevelConfigs for each era. In #898, these per-era TopLevelConfigs will contain no checkpoints, and this won't be caught by tests as validateEnvelope is only called at the HF layer.

[nfrisby]

I noticed: In the HFC, we often create TopLevelConfigs for each era

I'm not seeing the "in the HFC" aspect of this in the PR 898 diff. I see topLevelConfigCheckpoints = mempty in plenty of protocolInfo* functions, for example, and some testing functions. But those are not specific to the HFC, right?

... validateEnvelope is only called at the HF layer.

Does that imply that it's harmless for the per-era configs to have an empty checkpoints field? Since our intended semantics is for the checkpoints field to only affect validateEnvelope? Or am I oversimplifying something?

[amesgen]

I noticed: In the HFC, we often create TopLevelConfigs for each era

I'm not seeing the "in the HFC" aspect of this in the PR 898 diff. I see topLevelConfigCheckpoints = mempty in plenty of protocolInfo* functions, for example, and some testing functions. But those are not specific to the HFC, right?

It happens here:

ouroboros-consensus/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/HardFork/Combinator/Basics.hs

Lines 199 to 211 in 982eff4

	distribTopLevelConfig :: All SingleEraBlock xs
	=> EpochInfo (Except PastHorizonException)
	-> TopLevelConfig (HardForkBlock xs)
	-> NP TopLevelConfig xs
	distribTopLevelConfig ei tlc =
	hcpure proxySingle
	(fn_5 (\cfgConsensus cfgLedger cfgBlock cfgCodec cfgStorage ->
	mkTopLevelConfig
	(completeConsensusConfig' ei cfgConsensus)
	(completeLedgerConfig' ei cfgLedger)
	cfgBlock
	cfgCodec
	cfgStorage))

mkTopLevelConfig is touched in #898 and currently always uses mempty (just was the most direct thing to get things to compile).

... validateEnvelope is only called at the HF layer.

Does that imply that it's harmless for the per-era configs to have an empty checkpoints field? Since our intended semantics is for the checkpoints field to only affect validateEnvelope? Or am I oversimplifying something?

Yes, that is exactly right 👍 (Theoretically, some concrete block could of course depend on them in some non-trivial way in eg additionalEnvelopeChecks, but I don't have a realistic use case in mind.)

[amesgen]

Superseded by #898