Beds 151/add rp mode test #1167
Conversation
Eisei24
force-pushed
the
BEDS-151/Add_RP_mode_test
branch
from
ee05079 to
989033f
Compare
Deploying beaconchain with
|
| Latest commit: |
989033f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dc90713d.beaconchain.pages.dev |
| Branch Preview URL: | https://beds-151-add-rp-mode-test.beaconchain.pages.dev |
| if _, ok := rpInfo.Minipool[res.ValidatorIndex]; !ok { | ||
| rpInfo.Minipool[res.ValidatorIndex] = t.RPMinipoolInfo{ | ||
| NodeFee: res.NodeFee, | ||
| NodeDepositBalance: res.NodeDepositBalance, | ||
| UserDepositBalance: res.UserDepositBalance, | ||
| SmoothingPoolRewards: make(map[uint64]decimal.Decimal), | ||
| } | ||
| } | ||
|
|
||
| node := hexutil.Encode(res.NodeAddress) | ||
| if res.EndTime.Valid && res.SmoothingPoolEth != nil { | ||
| epoch := uint64(utils.TimeToEpoch(res.EndTime.Time)) | ||
| splitReward := res.SmoothingPoolEth.Div(decimal.NewFromUint64(nodeMinipoolCount[node])) | ||
| rpInfo.Minipool[res.ValidatorIndex].SmoothingPoolRewards[epoch] = | ||
| rpInfo.Minipool[res.ValidatorIndex].SmoothingPoolRewards[epoch].Add(splitReward) | ||
| } |
There was a problem hiding this comment.
This looks quite complicated, I don't think we need to handle multiple result rows with the same validator index, do we? Should be unique (unless we're planning for reusable indices maybe, but then the math would be wrong).
But I could be missing something, so as it's also not wrong I'll keep it as is
| // Smoothing pool address is the same for all nodes on the network so take the first result | ||
| SmoothingPoolAddress: queryResult[0].SmoothingPoolAddress, |
There was a problem hiding this comment.
Should use the other query for this information. As its result will have less rows, we'll also return less duplicates. Also makes the first query simpler which is the slower query here.
| From(goqu.L("rocketpool_minipools AS rplm")). | ||
| LeftJoin(goqu.L("validators AS v"), goqu.On(goqu.L("rplm.pubkey = v.pubkey"))). |
There was a problem hiding this comment.
This join is not necessary anymore, the minipools table got a validator_index column recently.
| if rpInfos == nil { | ||
| data.Balances.Total = data.Balances.Total.Add(validatorBalance) | ||
|
|
||
| nonRpDashboardId.Validators = append(nonRpDashboardId.Validators, validator) |
There was a problem hiding this comment.
This does the same as the else block, cleaned this up slightly
| if _, ok := rpInfos.Minipool[entry.ValidatorIndex].SmoothingPoolRewards[epoch]; ok { | ||
| elReward = elReward.Add(rpInfos.Minipool[entry.ValidatorIndex].SmoothingPoolRewards[epoch]) | ||
| } |
There was a problem hiding this comment.
Accessing an non-existing key doesn't crash in go, it just returns the empty object. This would make this nested if more readable imo
| if rpInfos != nil && protocolModes.RocketPool { | ||
| if rpValidator, ok := rpInfos.Minipool[entry.ValidatorIndex]; ok { | ||
| reward = reward.Mul(d.getRocketPoolOperatorFactor(rpValidator)) | ||
| } | ||
| } |
There was a problem hiding this comment.
This code section repeats quite often, thinking about generalizing this - how about defining a method on the RPInfo struct?
| // Calculate smoothing pool rewards | ||
| // Has to be done here in the cl and not el part because here we have the list of all relevant validators | ||
| if rpInfos != nil && protocolModes.RocketPool { | ||
| for validatorIndex, groupId := range validatorGroupMap { | ||
| for epoch, reward := range rpInfos.Minipool[validatorIndex].SmoothingPoolRewards { | ||
| if _, ok := smoothingPoolRewards[epoch]; !ok { | ||
| smoothingPoolRewards[epoch] = make(map[uint64]decimal.Decimal) | ||
| } | ||
| smoothingPoolRewards[epoch][groupId] = smoothingPoolRewards[epoch][groupId].Add(reward) | ||
| } | ||
| } |
There was a problem hiding this comment.
this and the start of the loop are copy pasted, could try to unify
| type QueryResult struct { | ||
| QueryResultBase | ||
| AttestationsSourceReward int64 `db:"attestations_source_reward"` | ||
| AttestationsTargetReward int64 `db:"attestations_target_reward"` | ||
| AttestationsHeadReward int64 `db:"attestations_head_reward"` | ||
| SyncRewards int64 `db:"sync_rewards"` | ||
| BlocksClSlasherReward int64 `db:"blocks_cl_slasher_reward"` | ||
| BlocksClAttestationsReward int64 `db:"blocks_cl_attestations_reward"` | ||
| BlocksClSyncAggregateReward int64 `db:"blocks_cl_sync_aggregate_reward"` | ||
| } | ||
|
|
||
| type QueryResultAdjusted struct { | ||
| QueryResultBase | ||
| AttestationsSourceReward decimal.Decimal | ||
| AttestationsTargetReward decimal.Decimal | ||
| AttestationsHeadReward decimal.Decimal | ||
| SyncRewards decimal.Decimal | ||
| BlocksClSlasherReward decimal.Decimal | ||
| BlocksClAttestationsReward decimal.Decimal | ||
| BlocksClSyncAggregateReward decimal.Decimal | ||
| } |
There was a problem hiding this comment.
CHECK: can these be combined? Is there a clickhouse driver which can fill decimals?
There was a problem hiding this comment.
Answer: yes should be possible. Not aware of any precision loss or something so I merged these
| QueryResultBase: QueryResultBase{ | ||
| ValidatorIndex: entry.ValidatorIndex, | ||
| AttestationsScheduled: entry.AttestationsScheduled, | ||
| AttestationsSourceExecuted: entry.AttestationsSourceExecuted, | ||
| AttestationsTargetExecuted: entry.AttestationsTargetExecuted, | ||
| AttestationsHeadExecuted: entry.AttestationsHeadExecuted, | ||
| SyncScheduled: entry.SyncScheduled, | ||
| SyncExecuted: entry.SyncExecuted, | ||
| Slashed: entry.Slashed, | ||
| BlocksSlashingCount: entry.BlocksSlashingCount, | ||
| BlocksScheduled: entry.BlocksScheduled, | ||
| BlocksProposed: entry.BlocksProposed, | ||
| }, |
| groupSum := queryResultSumMap[row.GroupId] | ||
| groupSum.ValidatorIndices = append(groupSum.ValidatorIndices, row.ValidatorIndex) | ||
| groupSum.AttestationReward = groupSum.AttestationReward.Add(row.AttestationReward) | ||
| groupSum.AttestationIdealReward = groupSum.AttestationIdealReward.Add(row.AttestationIdealReward) | ||
| groupSum.AttestationsExecuted += row.AttestationsExecuted | ||
| groupSum.AttestationsScheduled += row.AttestationsScheduled | ||
| groupSum.BlocksProposed += row.BlocksProposed | ||
| groupSum.BlocksScheduled += row.BlocksScheduled | ||
| groupSum.SyncExecuted += row.SyncExecuted | ||
| groupSum.SyncScheduled += row.SyncScheduled | ||
|
|
||
| clRewardWei := utils.GWeiToWei(big.NewInt(row.ClRewards)) | ||
| if rpInfos != nil && protocolModes.RocketPool { | ||
| if rpValidator, ok := rpInfos.Minipool[row.ValidatorIndex]; ok { | ||
| clRewardWei = clRewardWei.Mul(d.getRocketPoolOperatorFactor(rpValidator)) | ||
| } | ||
| } | ||
| groupSum.ClRewards = groupSum.ClRewards.Add(clRewardWei) | ||
| queryResultSumMap[row.GroupId] = groupSum |
There was a problem hiding this comment.
Not sure if splitting all those sum queries into individual rows in order to apply the correct RP scale factor and summing up in code is the best approach. Mainly because of efficiency reasons:
- It increases the total amount of data that needs to be transferred (e.g. from 11 rows to 2.75 million for the query in
GetValidatorDashboardRewardsfor megatron) - The DB is likely more efficient in batch processing like sums as its specialized for such tasks
- Readability/Maintainablity: All that manual summing is a lot more code to maintain
- All of the above are only done to handle RP Minipool rewards correctly which likely is just a minor subset of all validators, but the mechanism is applied to all validators
If there's no better solution that has to be put up with of course. Also depends how far we want to push for optimization; But shouldn't it be possible to let the DB do it? Either directly (for EL/alloy) or by feeding the list of Minipool indices + NO shares if it's a cross-db query.
Just thoughts for now and maybe it can't be applied to all queries, but even some could help. Could also go for a middleground where this is only applied if the RP mode is set, but stick to the old db-summing otherwise (would help with efficiency but prob not readability)
When reviewing this the important step to keep in mind is that I had to adjust the queries to give me the results for each validator individually instead of aggregates because I then need to multiply the reward with the validator specific rocketpool factor.
EL rewards for minipools in the smoothing pool should be ignored in the query (except in the blocks view), instead the rewards should be taken from the "smoothing pool reward table" split up for each minipool.