-
Notifications
You must be signed in to change notification settings - Fork 157
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
Revert TotalRewardEvent
changes
#2690
Conversation
This is an issue that the generators are not strong enough to discover. The DeltaRewardEvent appears in rare cases when there are no blocks after the 80% of an epoch. For these cases it has to be taken into account while aggregating incremental rewards
This also fixes the test that compares it to the aggregated reward events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, thank you very much @kderme !
@@ -754,6 +754,7 @@ aggDeltaRewardEvents :: [ChainEvent C] -> Map (Credential 'Staking (Crypto C)) ( | |||
aggDeltaRewardEvents events = foldl' accum Map.empty events | |||
where | |||
accum ans (TickEvent (Tick.RupdEvent (RupdEvent _ m))) = Map.unionWith Set.union m ans | |||
accum ans (TickEvent (Tick.NewEpochEvent (DeltaRewardEvent (RupdEvent _ m)))) = Map.unionWith Set.union m ans |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand this event correctly? UNfortunately there is no description about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refering to my commit message
This is an issue that the generators are not strong enough to discover. The DeltaRewardEvent appears in rare
cases when there are no blocks after the 80% of an epoch. For these cases it has to be taken into account while
aggregating incremental rewards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely want to redo the names and structure of the ledger events, they are a mess. And at the same time produce a document.
The Tick.NewEpochEvent (DeltaRewardEvent (RupdEvent _ m)))
should be rare, it's an edge case for when the timing of the reward pulser does not complete in time. The other event, Tick.RupdEvent (RupdEvent _ m))
is the usual one for the pulsing rewards. And indeed it makes sense that you would see that if no blocks happened in the last fifth of the epoch. What you have written looks correct to me.
I am tempted to merge this PR even though one of the non-required CI jobs is yellow. It looks to me like the job actually did successfully complete. |
This reverts changes to the
TotalRewardEvent
done in https://github.com/input-output-hk/cardano-ledger/pull/2615/files#diff-e38c0cafe1544253b0aa549f1fdaeb9b824089696fa5320f0c1dd5b3fa7d7f58R159-R161 and fixes the related test.