Make stake calculation consistent in `dijkstra` by moving `SNAP` to the end of the `EPOCH` by aniketd · Pull Request #5986 · IntersectMBO/cardano-ledger

aniketd · 2026-08-06T16:24:17Z

Description

Addresses a part of #5014: add more tests and implement the switch of moving SNAP to the end of the EPOCH in dijkstra.

Best way to analyse the changes is to diff the EPOCH rule between conway and dijkstra.

Checklist

The test demonstrates the inconsistency in calculations by failing the correct expected condition. This test is expected to pass once the EPOCH rule for dijkstra is implemented with consistent stake calculations.

Copilot

Pull request overview

This PR addresses inconsistent voting stake calculation (issue #5014) by changing Dijkstra’s epoch-boundary processing so the SNAP stake snapshot is taken after refunds/withdrawals are applied, aligning SPO voting stake with DRep voting stake. It also adds regression tests (Conway) and corresponding “fixed behavior” tests (Dijkstra), plus the necessary exports and dependency bumps to share logic.

Changes:

Implement a Dijkstra-specific EPOCH STS rule that runs SNAP at the end of the epoch transition (after POOLREAP + governance refunds/withdrawals).
Add Dijkstra imp tests asserting SPO voting stake matches DRep voting stake in scenarios involving refunds/withdrawals; add Conway imp tests reproducing #5014.
Export helper functions from cardano-ledger-conway needed by Dijkstra, and adjust bounds/changelogs accordingly.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
eras/dijkstra/impl/testlib/Test/Cardano/Ledger/Dijkstra/Imp/SnapSpec.hs	New Dijkstra imp tests asserting SPO and DRep voting stake are consistent after refunds/withdrawals.
eras/dijkstra/impl/testlib/Test/Cardano/Ledger/Dijkstra/Imp.hs	Wires the new SnapSpec into the Dijkstra imp test suite.
eras/dijkstra/impl/src/Cardano/Ledger/Dijkstra/Rules/Epoch.hs	New Dijkstra `EPOCH` transition implementing “SNAP-at-end” ordering.
eras/dijkstra/impl/src/Cardano/Ledger/Dijkstra/Rules.hs	Ensures the new Dijkstra EPOCH rule module is linked/loaded.
eras/dijkstra/impl/src/Cardano/Ledger/Dijkstra/Era.hs	Introduces a Dijkstra-specific `EPOCH` rule type and points `EraRule "EPOCH"` to it.
eras/dijkstra/impl/CHANGELOG.md	Documents the new Dijkstra EPOCH behavior and rationale (#5014).
eras/dijkstra/impl/cardano-ledger-dijkstra.cabal	Adds the new module/test module and bumps `cardano-ledger-conway` lower bound to include needed exports.
eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Imp/SnapSpec.hs	Adds tests reproducing the lag described in #5014 (baseline behavior).
eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Epoch.hs	Exports helper functions used by Dijkstra’s EPOCH implementation.
eras/conway/impl/CHANGELOG.md	Notes newly exported functions in `cardano-ledger-conway`.

Suppressed comments (1)

eras/dijkstra/impl/testlib/Test/Cardano/Ledger/Dijkstra/Imp/SnapSpec.hs:118

In this combined scenario, the proposal id is discarded (_ <- submitProposal ...), so the test can’t assert that the proposal actually expired (and its deposit was refunded), and similarly the withdrawal action’s enactment/removal isn’t asserted. Because the core stake assertion is just equality, the test could pass even if the intended refunds/withdrawal never took effect. Capturing the ids and asserting expectMissingGovActionId after passNEpochs 2 makes the test validate the setup.

      submitTx_ $ mkBasicTx mkBasicTxBody & bodyTxL . treasuryDonationTxBodyL .~ Coin 1_000_000
      _ <- submitProposal =<< mkProposalWithAccountAddress InfoAction returnAddr
      poolToRetire <- freshKeyHash
      registerPoolWithAccountAddress poolToRetire returnAddr
      passEpoch
      curEpochNo <- getsNES nesELL
      submitTxAnn_ "Retire the temporary pool" $
        mkBasicTx mkBasicTxBody
          & bodyTxL . certsTxBodyL
            .~ SSeq.singleton (RetirePoolTxCert poolToRetire (addEpochInterval curEpochNo (EpochInterval 2)))
      modifyPParams $ \pp -> pp & ppGovActionLifetimeL .~ EpochInterval 30
      govActionId <- submitTreasuryWithdrawals [(returnAddr, Coin 1_000_000)]
      submitYesVote_ (DRepVoter drep) govActionId
      submitYesVoteCCs_ committeeCs govActionId
      passNEpochs 2
      drepVotingStake <- getDRepVotingStake drep

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+      govActionId <- submitTreasuryWithdrawals [(returnAddr, Coin 1_000_000)]
+      submitYesVote_ (DRepVoter drep) govActionId
+      submitYesVoteCCs_ committeeCs govActionId
+      passNEpochs 2
+      drepVotingStake <- getDRepVotingStake drep
+      spoVotingStake <- getSpoVotingStake pool


teodanciu

After a most educative incursion into stake calculation, I finally understand what this PR is solving.
It looks to me as if it's following what was discussed, and it's testing the fix.
But what worries me is that it's testing only the fix.
I believe this change will have consequences for the rewards calculation (since the post-refund balances will now be snapshotted), and also on pool removal (because POOLREAP runs before SNAP) - so the pool will disappear from mark immediately I believe?
Also, I'm not sure how (if at all, likely ont) TICKF is affected.
Conformance tests are also failing - do we need the spec to be updated as well?

I suppose we can write more tests in future PRs, I find it difficult to understand the consequences without seeing the differences in the consumers of the snapshots, so I I will leave it to someone more knowledgeable to approve!

teodanciu · 2026-08-14T13:58:46Z

+        & lsCertStateL .~ certState2
+        & lsUTxOStateL .~ utxoState2
+  snapshots1 <-
+    trans @(EraRule "SNAP" era) $ TRC (Shelley.SnapEnv ledgerState1 curPParams, snapshots0, ())


Why before and not after the HARDFORK rule? If HARDFORK modifies the state, then the pulser will get the accounts from the post-HARDFORK state and the stakePoolDistr from the pre-HARDFORK snapshot, so kinda similar problem to what we have today no?

Is there a specific reason to have it here that I'm missing?

teodanciu · 2026-08-14T14:01:35Z

+        & lsCertStateL .~ certState2
+        & lsUTxOStateL .~ utxoState2
+  snapshots1 <-
+    trans @(EraRule "SNAP" era) $ TRC (Shelley.SnapEnv ledgerState1 curPParams, snapshots0, ())


Aren't curPParams stale here? The govstate has been replaced at this point, no?

teodanciu · 2026-08-14T14:14:02Z

+    -- Conway's reproduction (Conway.Imp.SnapSpec.conwayOnlySpec) asserts the SPO trails the DRep by
+    -- the refunded deposit: (drepVotingStake <-> spoVotingStake) `shouldBe` govActionDeposit.
+    -- The Dijkstra SNAP fix removes that one-epoch lag, so here the two stakes are equal instead.


I wonder if instead of duplicating the conditions of the test in Conway, we could have a protocol-version switch in conway (that would be called in Dijkstra too), that clearly shows the difference in behavior?
Not so much to avoid duplication, but because I think it would be a nice documentation to see the change between protocol versions.

teodanciu · 2026-08-14T14:18:35Z

+    -- the refunded deposit: (drepVotingStake <-> spoVotingStake) `shouldBe` govActionDeposit.
+    -- The Dijkstra SNAP fix removes that one-epoch lag, so here the two stakes are equal instead.
+    impAnn "SPO voting stake reflects the refund immediately, like the DRep stake" $
+      spoVotingStake `shouldBe` drepVotingStake


Wonder if we can make this check stronger, because the property I believe is: spoVotingStake should be leader-election stake + activeProposalDeposits delegated to that pool.
I think it would be more difficult for this to pass for the wrong reason, than checking against the drepVotingStake.

teodanciu · 2026-08-14T14:33:41Z

+      getDRepVotingStake drep = do
+        drepDistr <- getsNES $ nesEsL . epochStateDRepPulsingStateL . psDRepDistrG
+        pure $ fromCompact $ drepDistr Map.! DRepCredential drep
+  it "SPO voting stake equals DRep voting stake after a refunded deposit" $ do


I haven't tried it, but it seems to me that this kind of checks would lend themselves to some property tests instead or in addition to crafting these scenarios (the property being: "for every account delegated to both a pool and a DRep, its contribution to the two distributions agrees").
I don't know how feasible it is atm, since we can't generate arbitrary correct ledger states.

Another thing we could try - I don't know if it's feasible - but to have some sort of check-hook in Dijkstra imp test to check the invariant, so we get to test it whenever we run imp tests?

aniketd added 5 commits August 6, 2026 16:48

Add stake calculation snapspec to dijkstra.

50385b6

The test demonstrates the inconsistency in calculations by failing the correct expected condition. This test is expected to pass once the EPOCH rule for dijkstra is implemented with consistent stake calculations.

Copy conway EPOCH rule over to dijkstra verbatim

888f759

Add test variations for stake calculations

854b66f

Move SNAP transition to end of EPOCH in dijkstra

318817b

Update changelog and bump dependency

9e93be8

aniketd requested review from neilmayhew and teodanciu August 6, 2026 16:24

aniketd requested a review from a team as a code owner August 6, 2026 16:24

Copilot AI lite review requested due to automatic review settings August 6, 2026 16:24

Copilot started reviewing on behalf of aniketd August 6, 2026 16:28 View session

Copilot AI reviewed Aug 6, 2026

View reviewed changes

teodanciu reviewed Aug 14, 2026

View reviewed changes

Navigation Menu

Uh oh!

Make stake calculation consistent in `dijkstra` by moving `SNAP` to the end of the `EPOCH` - #5986

Make stake calculation consistent in `dijkstra` by moving `SNAP` to the end of the `EPOCH`#5986
aniketd wants to merge 5 commits into
masterfrom
aniketd/stake-calculation-consistency

aniketd commented Aug 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

teodanciu left a comment

Uh oh!

teodanciu Aug 14, 2026

Uh oh!

teodanciu Aug 14, 2026

Uh oh!

teodanciu Aug 14, 2026

Uh oh!

teodanciu Aug 14, 2026

Uh oh!

teodanciu Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

aniketd commented Aug 6, 2026

Description

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

teodanciu left a comment

Choose a reason for hiding this comment

Uh oh!

teodanciu Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

teodanciu Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

teodanciu Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

teodanciu Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

teodanciu Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants