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.

Sign up for GitHub

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

Jump to bottom

Revert TotalRewardEvent changes #2690

Merged
merged 2 commits into from
Mar 16, 2022
Merged

Revert TotalRewardEvent changes #2690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32aa5a0
Include DeltaRewardEvent in test
kderme Mar 15, 2022
7cde151
Revert changes to the TotalRewardEvent
kderme Mar 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions eras/shelley/impl/src/Cardano/Ledger/Shelley/LedgerState.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ module Cardano.Ledger.Shelley.LedgerState
updateStakeDistribution,
applyRUpd,
applyRUpd',
filterAllRewards,
createRUpd,
completeRupd,
startStep,
Expand Down Expand Up @@ -1189,17 +1190,12 @@ applyRUpd' ::
(EpochState era, Map (Credential 'Staking (Crypto era)) (Set (Reward (Crypto era))))
applyRUpd'
ru
(EpochState as ss ls pr pp _nm) = (EpochState as' ss ls' pr pp nm', registered)
es@(EpochState as ss ls pr pp _nm) = (EpochState as' ss ls' pr pp nm', registered)
where
utxoState_ = _utxoState ls
delegState = _delegationState ls
dState = _dstate delegState
(regRU, unregRU) =
Map.partitionWithKey
(\k _ -> eval (k ∈ dom (rewards dState)))
(rs ru)
totalUnregistered = fold $ aggregateRewards pr unregRU
registered = filterRewards pr regRU
(registered, totalUnregistered) = filterAllRewards (rs ru) es
registeredAggregated = aggregateRewards pp registered
as' =
as
Expand All @@ -1220,6 +1216,24 @@ applyRUpd'
}
nm' = nonMyopic ru

filterAllRewards ::
( HasField "_protocolVersion" (Core.PParams era) ProtVer
) =>
Map (Credential 'Staking (Crypto era)) (Set (Reward (Crypto era))) ->
EpochState era ->
(Map (Credential 'Staking (Crypto era)) (Set (Reward (Crypto era))), Coin)
filterAllRewards rs' (EpochState _as _ss ls pr _pp _nm) =
(registered, totalUnregistered)
where
delegState = _delegationState ls
dState = _dstate delegState
(regRU, unregRU) =
Map.partitionWithKey
(\k _ -> eval (k ∈ dom (rewards dState)))
rs'
totalUnregistered = fold $ aggregateRewards pr unregRU
registered = filterRewards pr regRU

decayFactor :: Float
decayFactor = 0.9

Expand Down
4 changes: 2 additions & 2 deletions eras/shelley/impl/src/Cardano/Ledger/Shelley/Rules/NewEpoch.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,10 @@ newEpochTransition = do
let updateRewards ru'@(RewardUpdate dt dr rs_ df _) = do
let totRs = sumRewards (esPrevPp es) rs_
Val.isZero (dt <> (dr <> toDeltaCoin totRs <> df)) ?! CorruptRewardUpdate ru'
let (es', _regRU) = applyRUpd' ru' es
let (es', regRU) = applyRUpd' ru' es
-- This event (which is only generated once per epoch) must be generated even if the
-- map is empty (db-sync depends on it).
tellEvent (TotalRewardEvent e rs_)
tellEvent $ TotalRewardEvent e regRU
pure es'
es' <- case ru of
SNothing -> pure es
Expand Down
21 changes: 12 additions & 9 deletions eras/shelley/test-suite/test/Test/Cardano/Ledger/Shelley/Rewards.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ import Cardano.Ledger.Shelley.LedgerState
circulation,
completeRupd,
createRUpd,
filterAllRewards,
rewards,
updateNonMyopic,
)
Expand Down Expand Up @@ -104,7 +105,7 @@ import Cardano.Ledger.Shelley.Rewards
mkApparentPerformance,
mkPoolRewardInfo,
)
import Cardano.Ledger.Shelley.Rules.NewEpoch (NewEpochEvent (TotalRewardEvent))
import Cardano.Ledger.Shelley.Rules.NewEpoch (NewEpochEvent (DeltaRewardEvent, TotalRewardEvent))
import Cardano.Ledger.Shelley.Rules.Rupd (RupdEvent (..))
import qualified Cardano.Ledger.Shelley.Rules.Tick as Tick
import Cardano.Ledger.Shelley.TxBody (PoolParams (..), RewardAcnt (..))
Expand Down Expand Up @@ -750,10 +751,11 @@ newEpochEventsProp tracelen propf = withMaxSuccess 10 $
Just SourceSignalTarget {target} -> propf (concat (runShelleyBase $ getEvents tr)) (chainNes target)
_ -> True === True

aggDeltaRewardEvents :: [ChainEvent C] -> Map (Credential 'Staking (Crypto C)) (Set (Reward (Crypto C)))
aggDeltaRewardEvents events = foldl' accum Map.empty events
aggIncrementalRewardEvents :: [ChainEvent C] -> Map (Credential 'Staking (Crypto C)) (Set (Reward (Crypto C)))
aggIncrementalRewardEvents 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
Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

accum ans _ = ans

getMostRecentTotalRewardEvent :: [ChainEvent C] -> Map (Credential 'Staking (Crypto C)) (Set (Reward (Crypto C)))
Expand All @@ -770,21 +772,22 @@ eventsMirrorRewards :: [ChainEvent C] -> NewEpochState C -> Property
eventsMirrorRewards events nes = same eventRew compRew
where
(compRew, eventRew) =
case (nesRu nes) of
SNothing -> (total, aggevent)
case nesRu nes of
SNothing -> (total, aggFilteredEvent)
SJust pulser ->
( Map.unionWith (Set.union) (rs completed) total,
Map.unionWith (Set.union) lastevent aggevent
( Map.unionWith Set.union (rs completed) total,
Map.unionWith Set.union lastevent aggevent
)
where
(completed, lastevent) = complete pulser
total = getMostRecentTotalRewardEvent events
aggevent = aggDeltaRewardEvents events
aggevent = aggIncrementalRewardEvents events
(aggFilteredEvent, _) = filterAllRewards aggevent (nesEs nes)
same x y = withMaxSuccess 1 $ counterexample message (x === y)
where
message =
( "events don't mirror rewards "
++ tersemapdiffs "Map differences: aggregated events on the left, computed on the right." x y
++ tersemapdiffs "Map differences: aggregated filtered events on the left, computed on the right." x y
)

ppAgg :: Map (Credential 'Staking (Crypto C)) (Set (Reward (Crypto C))) -> PDoc
Expand Down