Skip to content

Conversation

@kderme
Copy link
Contributor

@kderme kderme commented Mar 15, 2022

The query queryEpochRewardTotal ignores the deposit refunds. However while printing the diff it is not ignored. So if there is some undelared mismath with events, the deposit refunds are also printed even if they don't cause the issue.

@kderme kderme requested a review from erikd as a code owner March 15, 2022 21:02
`on` (\(rwd :& saddr) ->
rwd ^. Db.RewardAddrId ==. saddr ^. Db.StakeAddressId)
where_ (rwd ^. Db.RewardSpendableEpoch ==. val epochNo)
where_ (not_ $ rwd ^. Db.RewardType ==. val Db.RwdDepositRefund)
Copy link
Contributor

@erikd erikd Mar 15, 2022

Choose a reason for hiding this comment

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

Unfortunately, this means we no longer have a way of validating deposit refunds.

I really do not think this is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this function validates anything. This runs after the validation has failed.

We already don't validate deposit refunds.

Copy link
Contributor

@erikd erikd Mar 15, 2022

Choose a reason for hiding this comment

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

The validation failure is at least in part due to the pool deposit refunds. One of the validation failures was:

 [2022-03-03 05:13:40.43 UTC]   e1581eab598e036f2bb6506204b5ad483e47fb817e880b1adfe5c04f09: 
    [(RwdDepositRefund,DbLovelace 500000000)] /= []

where the LHS is from the database and the RHS is from the ledger event.

This used to work.

Copy link
Contributor Author

@kderme kderme Mar 15, 2022

Choose a reason for hiding this comment

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

The validation failure is at least in part due to the pool deposit refunds.

It's not really, the deposit refunds appear in the error message, but are not the reason of failure.

See also this existing comment in queryEpochRewardTotal

            -- For ... reasons ... pool deposit refunds are put into the rewards account
            -- but are not considered part of the total rewards for an epoch.

This was like this before. We do no validations of deposit refunds.

Copy link
Contributor

Choose a reason for hiding this comment

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

The validation detects any difference between what is in the db and what is provided by ledger.

The comment is almost certainly wrong (the code has gone through a lot of iterations and its easy to update the code while forgetting to update the comments). Before the latest update, this validation passed and now it does not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not just a comment. It's also in the query queryEpochRewardTotal

where_ (not_ $ rwd ^. Db.RewardType ==. val Db.RwdDepositRefund)

Before the latest update, this validation passed and now it does not.

Which update do you mean? This fixes an existing mismatch on master: The validation filters out deposits but the comment doesn't.

Copy link
Contributor

@erikd erikd Mar 22, 2022

Choose a reason for hiding this comment

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

I mean whichever version of ledger was used in the 12.0.x release worked correctly. Then the legder people unilaterally changed things and broke db-sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This simply fixes the error message. It doesn't change and has nothing to do with the validation, the new changes in ledger which haven't been in master yet or whether this error message should appear or not.

In master there is no validation of deposits refunds.

@erikd
Copy link
Contributor

erikd commented Apr 28, 2022

This was merged in PR #1094 .

@erikd erikd closed this Apr 28, 2022
@kderme kderme deleted the kderme/ignore-refunds branch July 4, 2022 12:59
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