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
feat: add endpoint for Altair block reward #6178
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6178 +/- ##
============================================
+ Coverage 60.15% 61.69% +1.53%
============================================
Files 407 554 +147
Lines 46511 58090 +11579
Branches 1550 1832 +282
============================================
+ Hits 27980 35840 +7860
- Misses 18499 22213 +3714
- Partials 32 37 +5 |
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
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 pretty good! Just few remarks / questions regarding the units we use for the ssz type
I tested this on my goerli node (slot 7466188)
> curl -s http://localhost:9596/eth/v1/beacon/rewards/blocks/7466188 | jq
{
"data": {
"proposer_index": "2232",
"total": "25570228",
"attestations": "24656876",
"sync_aggregate": "913352",
"proposer_slashings": "0",
"attester_slashings": "0"
},
"execution_optimistic": false
}
I haven't confirmed that the data is correct but 0,0255 ETH consensus reward sounds about right. We might wanna cross check our rewards APIs against other clients (Teku / Lighthouse) at some point.
Not sure if @tuyennhv wants to give this another review, all previous comments have been addressed
@@ -991,4 +992,11 @@ export class BeaconChain implements IBeaconChain { | |||
} | |||
} | |||
} | |||
|
|||
async getBlockRewards(block: allForks.FullOrBlindedBeaconBlock): Promise<BlockRewards> { | |||
const preState = (await this.regen.getPreState(block, {dontTransferCache: true}, RegenCaller.restApi)).clone(); |
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.
A little sketchy that this can trigger a regen.
Is this the only non-debug endpoint that can do that?
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.
A little sketchy that this can trigger a regen.
What's your concern in particular? I don't think getPreState
modifies anything under the hood. To get the preState of a block I don't see a way without calling regen. At some point regen.getState()
must be called.
Is this the only non-debug endpoint that can do that?
This is the only one that I know of cc. @tuyennhv
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.
Is this the only non-debug endpoint that can do that?
It looks like this is the only one, can be confirmed by searching for allowRegen
which is only set to true for debug state apis
What's your concern in particular?
The main concern is probably that you can use this API to easily DoS our public nodes as regen is quite expensive.
What about adding rewards APIs as their own namespace and disabling them by default?
e.g. we also have light client APIs on their own namespace even though those are part of /beacon as per spec
lightclient: () => lightclient.getRoutes(config, api.lightclient), |
we could then simply not enable rewards APIs by default here
api: ["beacon", "config", "events", "node", "validator", "lightclient"], |
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.
What about adding rewards APIs as their own namespace and disabling them by default?
I read the documentation of other CL clients and looks like everyone has rewards endpoint enabled by default. Don’t know if the users expect this endpoint would be enabled on the public nodes.
Alternatively, we can limit the queried block to any block from last finalized checkpoint to head similar to what Teku behaves under prune
mode: https://docs.teku.consensys.io/how-to/use-rewards-api#limitations . This way we only need to check stateCache
and checkpointStateCache
without triggering a regen request
The main concern is probably that you can use this API to easily DoS our public nodes as regen is quite expensive.
My wishful thinking is to have two priority tiers for RegenRequest
. One being essential and one being non-essential. Any debug and reward endpoint that triggers regen should have a low priority such that when JobItemQueue
size reaches a certain threshold, it rejects new RegenRequest
that has low priority.
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.
Alternatively, we can limit the queried block from last finalized checkpoint to head
This is already the case now, it isn't fetching historical states, but this can still trigger block / epoch transitions if the requested state isn't already in a cache. In practice, in healthy network conditions, we have all states between finalized and head in cache. But it can become a problem in periods of non-finality.
We could add and use a regen.getPreStateSync
that acts like regen.getStateSync
in that it only checks cached state values and returns undefined if the state isn't cached. IMO this is a good compromise, since it allows us to serve the data in most cases, but doesn't open us up to any DoS issues.
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.
As an aside, It may be worth thinking deeper about our strategy around serving more expensive queries.
Should we support expensive queries? Behind a feature flag or flags? If so, what does that look like?
Behind extra namespaces for each additional feature-set in the APIs?
What does the architecture of generating expensive data look like? Queue that deprioritizes non-urgent work? Separate worker that handles expensive queries? Other?
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.
yes this is really a concern because with PersistentCheckpointStateCache
, using getPreState
means it will reload checkpoint state if needed
We could add and use a regen.getPreStateSync that acts like regen.getStateSync in that it only checks cached state values and returns undefined if the state isn't cached. IMO this is a good compromise, since it allows us to serve the data in most cases, but doesn't open us up to any DoS issues.
in this specific scenario, if we can get a cached pre state, we can also get post state and get the cached reward from post state, so it's no use to have getPreStateSync()
I think we only want to support getting cached reward in post state for lodestar (it means it'll work for 64-96 blocks for now). If demand arises from any specific use case, we can enhance later
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.
in this specific scenario, if we can get a cached pre state, we can also get post state and get the cached reward from post state, so it's no use to have getPreStateSync()
For the purpose of this endpoint, I echo with @wemeetagain 's idea to only serve blocks which its corresponding postState is cached.
So we should just drop the preState parameter in computeBlockRewards(block, preState, postState)
, and also all the reward calculation in blockRewards.ts
and solely rely on cached values
Update: Looks like we still need the preState to calculate proposer and attester slashing rewards instead of simply checking the cache because RewardCache
combines both into a single slashing
value. Will still need getPreStateSync()
for this PR
so it's no use to have
getPreStateSync()
The other two rewards endpoint (attestation and sync committee) both require preState
so we will need to have getPreStateSync()
implemented at some point. We can avoid getPreStateSync()
for this endpoint because we have block rewards cached in BeaconStateCache.proposerRewards: RewardCache
but that's not the case for the other endpoints.
@@ -71,6 +71,40 @@ export class QueuedStateRegenerator implements IStateRegenerator { | |||
return this.stateCache.get(stateRoot); | |||
} | |||
|
|||
getPreStateSync(block: allForks.BeaconBlock): CachedBeaconStateAllForks | null { |
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.
Please take a closer look at getPreStateSync()
and its usage. Pretty much moved the majority of getPreState()
to this function.
Looks to me checkpointStateCache.getLatest()
wouldn't trigger a reload in the absence of state.
@tuyennhv @wemeetagain @nflaig
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.
yes looks good to me
fix: clone states to compute block rewards
@wemeetagain Would love a second pass from you being merging this |
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.
my main concern about this endpoint triggering regen has been resolved
gave another review, LGTM
🎉 This PR is included in v1.17.0 🎉 |
Description
Introduce a beacon API endpoint to return block reward info according to the spec. This is one of the three reward endpoints mentioned in #5694
Note that the endpoint currently only supports Altair block or later. Attempts to query for phase0 block will return error.
TODO
Will open follow-up PRs for:
finalized
property to the reward response mentioning in Addfinalized
property to HTTP API responses #5693 which is currently blockedSteps to test or reproduce