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

Ambit claim for ledger event rewards #1080

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Ambit claim for ledger event rewards #1080

wants to merge 2 commits into from

Conversation

erikd
Copy link
Contributor

@erikd erikd commented Mar 11, 2022

No description provided.

@erikd erikd added the do not merge Test or debug only label Mar 11, 2022
@erikd erikd force-pushed the erikd/ambit-claim branch 2 times, most recently from 19d3fd1 to 5bed79a Compare March 11, 2022 07:21
* MIR payments from the reserves.
* Refunds of the stake pool deposit payment when a pool is de-registered.

Currently, the ledger provides the following reward related events:

Choose a reason for hiding this comment

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

It's probably worth noting that the list below is the name of the wrapped events from the Cardano API. The corresponding ledger events are currently nested according to the ledger rule hierarchy (though we may change this is the near future, a clean up is on order)

  • ShelleyLedgerEventTICK . NewEpochEvent . DeltaRewardEvent . RupdEvent
  • ShelleyLedgerEventTICK . NewEpochEvent . MirEvent
  • ShelleyLedgerEventTICK . NewEpochEvent . EpochEvent . PoolReapEvent
  • ShelleyLedgerEventTICK . NewEpochEvent . TotalRewardEvent

* `LEDeltaReward`
* `LEMirTransfer`
* `LERetiredPools`
* `LETotalRewards`

Choose a reason for hiding this comment

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

Suggested change
* `LETotalRewards`
* `LERewardEvent`

* A single `LETotalRewards` event should be emitted for every epoch in the Shelley era and later,
even if there are no rewards for that epoch (in which case the event reward map will be `mempty`).
* `LETotalRewards` should include all payments to stake addresses for a given epoch. That means
all staking rewards (member and owner), all MIR payments and all stake pool deposit refunds.

Choose a reason for hiding this comment

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

There is currently no such LETotalRewards event in the Cardano API, do you meant the LERewardEvent? The MIR payments and pool deposit refunds come from totally different ledger rules, we cannot make the event described here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is currently no such LETotalRewards event in the Cardano API,

db-sync currently does not use cardano-api for anything ledger event related.

Is there any accomodation ledger might make the use of these events by the current one and only consumer of these events easier?

Choose a reason for hiding this comment

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

currently does not use cardano-api for anything ledger event related.

oh! Where does LETotalRewards come from? It's not from cardano-ledger.

Is there any accomodation ledger might make the use of these events by the current one and only consumer of these events easier?

Absolutely! We just can't glue these events together in the way you are proposing, not with serious consequences

* `LETotalRewards` should include all payments to stake addresses for a given epoch. That means
all staking rewards (member and owner), all MIR payments and all stake pool deposit refunds.
* `LEDeltaReward` should only ever contain pool membership or pool ownership rewards.
* `LEDeltaReward` may contain rewards to stake addresses that have been de-registered.

Choose a reason for hiding this comment

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

Here are some extra details that might be helpful.

In the Alonzo era, and previous eras:

  • any stake address inside the stake distribution snapshot which is deregestered at the time the reward calculation begins will not be included in this event.
  • any stake address inside the stake distribution snapshot which is deregestered after the reward calculation begins, and is not registered at the time of the epoch boundary, will have it's lovelace given to the treasury.

In the Babbage era:

  • any stake address inside the stake distribution snapshot which is not registered at the time of the epoch boundary will have it's lovelace given to the treasury.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Era specific behavior for any event is a huge pain in the neck for db-sync.

* `LEDeltaReward` may contain rewards to stake addresses that have been de-registered.
* The sum of all `LEDeltaReward`, `LEMirTransfer` and `LERetiredPools` amounts for an epoch should
always be the same as the sum of `LETotalRewards` event amounts for that epoch.
* `LETotalRewards` will not contain rewards to stake addresses that have been de-registered.

Choose a reason for hiding this comment

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

This requirement (that the total reward event sum is equal to the sum of the other events) is inconsistent with the idea that the LETotalRewards does not contain the de-registered accounts, but the LEDeltaReward might include them.

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 we have a different idea of the meaning of the word "total"?

Choose a reason for hiding this comment

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

The sum of all LEDeltaReward, LEMirTransfer and LERetiredPools amounts for an epoch should
always be the same as the sum of LETotalRewards event amounts for that epoch.

LEDeltaReward may contain rewards to stake addresses that have been de-registered.

LETotalRewards will not contain rewards to stake addresses that have been de-registered.

These three requirements are inconsistent. A reward for a stake addresses that has been de-registered would be in the sum of "all LEDeltaReward, LEMirTransfer and LERetiredPools amounts" by the second quote above, but not in the sum of "LETotalRewards event amounts" by the third quote above. Therefore the sums will not be the same in the presence of stake addresses that have been de-registered.

* The sum of all `LEDeltaReward`, `LEMirTransfer` and `LERetiredPools` amounts for an epoch should
always be the same as the sum of `LETotalRewards` event amounts for that epoch.
* `LETotalRewards` will not contain rewards to stake addresses that have been de-registered.
* The `LETotalRewards` must be the last reward related event for a given epoch to be emitted from

Choose a reason for hiding this comment

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

The ledger will make no guarantees about the ordering of the events corresponding to a given block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I can theoretically get an LEDeltaReward for epoch N after I get the LETotalRewards for that same epoch? That does not make a lot of sense. More importantly, it makes the LETotalRewards useless from the POV of db-sync using it for validation.

Choose a reason for hiding this comment

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

Yes that is theoretically possible. The event is still useful, we use it in the ledger property tests to ensure that the delta rewards are what we expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal of all of these events is to be used by db-sync which is currently the only consumer of ledger events. If its not useful for db-sync is not useful.

@erikd
Copy link
Contributor Author

erikd commented Mar 15, 2022

@JaredCorduan My attempt to ask for what would make my life easier has obviously failed. How about you actually document what you can provide me with? It would be important to document everything I can rely on and as much as possible about what I cannot rely on. Hopefully the things I can rely on is a lot longer than the list of things I cannot rely on.

Feel free to add that to the current document, below the text I wrote or add it here as a comment.

@JaredCorduan
Copy link

@JaredCorduan My attempt to ask for what would make my life easier has obviously failed. How about you actually document what you can provide me with? It would be important to document everything I can rely on and as much as possible about what I cannot rely on. Hopefully the things I can rely on is a lot longer than the list of things I cannot rely on.

Feel free to add that to the current document, below the text I wrote or add it here as a comment.

I don't think it failed at all! I think we are making great progress! the ledger team's top priority is Babbage right now, but after that we should have time to document the events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Test or debug only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants