Skip to content
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.

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

For release 10.3 #1376

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

For release 10.3 #1376

wants to merge 22 commits into from

Conversation

aniketd
Copy link
Contributor

@aniketd aniketd commented Jan 31, 2025

Description

Please include a meaningful description of the PR and link the relevant issues
this PR might resolve.

Also note that:

  • New code should be properly tested (even if it does not add new features).
  • The fix for a regression should include a test that reproduces said regression.

@aniketd aniketd force-pushed the aniketd/release-10-3 branch from c5712c4 to 0279690 Compare February 3, 2025 13:55
@aniketd aniketd force-pushed the aniketd/release-10-3 branch from 0279690 to 2b0f104 Compare February 4, 2025 15:44
Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Some really good progress.
Hopefully my comments unblock you and you can continue progressing further

@@ -392,7 +397,7 @@ mkShelleyGlobals TPraosConfig{..} = SL.Globals {
, slotsPerKESPeriod = tpraosSlotsPerKESPeriod
, stabilityWindow = SL.computeStabilityWindow k tpraosLeaderF
, randomnessStabilisationWindow = SL.computeRandomnessStabilisationWindow k tpraosLeaderF
, securityParameter = k
, securityParameter = nonZeroOr k $ error "The security parameter cannot be zero."
Copy link
Contributor

Choose a reason for hiding this comment

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

Try pushing this NonZero type further into SecurityParam

Suggested change
, securityParameter = nonZeroOr k $ error "The security parameter cannot be zero."
, securityParameter = k

Because then this invariant will propagate throughout the whole system, instead of having a random error call here.

If we push it far enough it will become a deserializaition failure somewhere in a json or cbor decoder, rather than an exception in pure code.

@aniketd aniketd force-pushed the aniketd/release-10-3 branch 2 times, most recently from 1e5ae5a to 58b5304 Compare February 7, 2025 14:24
Comment on lines +75 to +84
-- | The "tip" to prune snapshots from.
--
-- `SecurityParam` has been updated to use `NonZero` but we need to prune from
-- @0@ in some cases.
--
-- Rather than using a plain `Word64` we use this to be able to distinguish that
-- we are indeed using
-- 1. @0@ in places where it is necessary
-- 2. the security parameter as is, in other places
data LedgerDBPruneTip = LedgerDBPruneTipZero | LedgerDBPruneTip SecurityParam
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be in the LedgerDB folder. Also some rewording:

Suggested change
-- | The "tip" to prune snapshots from.
--
-- `SecurityParam` has been updated to use `NonZero` but we need to prune from
-- @0@ in some cases.
--
-- Rather than using a plain `Word64` we use this to be able to distinguish that
-- we are indeed using
-- 1. @0@ in places where it is necessary
-- 2. the security parameter as is, in other places
data LedgerDBPruneTip = LedgerDBPruneTipZero | LedgerDBPruneTip SecurityParam
-- | How many ledger states to keep in the LedgerDB after a prune.
--
-- Rather than using a plain `Word64` we use this to be able to distinguish that
-- we are indeed using
-- 1. @0@ in places where it is necessary, namely when completing the replay of the ImmutableDB blocks as part of opening the ChainDB
-- 2. the security parameter as is, in other places, note it is a strictly positive value
data LedgerDBPruneNum = LedgerDBPruneZero | LedgerDBPruneNum SecurityParam

Copy link
Contributor

Choose a reason for hiding this comment

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

Or LedgerDBPruneN or just LedgerDBPrune

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants