Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add endpoint for Altair block reward #6178
Changes from 23 commits
6679119
f67421b
0b030f2
5e3f0f5
c2da457
1e47858
ac9bb42
1fc3201
f702c2b
6c14d0c
f331e5a
db5cc41
edc8c3c
f543045
b605529
45734fa
f80daf4
3f8d42c
f2bbe95
cde10e9
a6e2fc4
ee42dde
b6f68d0
5c10acf
47e9650
719ab1d
51bbf5e
eceea02
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 pointregen.getState()
must be called.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.
It looks like this is the only one, can be confirmed by searching for
allowRegen
which is only set to true for debug state apisThe 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
lodestar/packages/api/src/beacon/server/index.ts
Line 41 in 347c95f
we could then simply not enable rewards APIs by default here
lodestar/packages/beacon-node/src/api/rest/index.ts
Line 19 in 347c95f
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.
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 checkstateCache
andcheckpointStateCache
without triggering a regen requestMy 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 whenJobItemQueue
size reaches a certain threshold, it rejects newRegenRequest
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.
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 likeregen.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
, usinggetPreState
means it will reload checkpoint state if neededin 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.
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 incomputeBlockRewards(block, preState, postState)
, and also all the reward calculation inblockRewards.ts
and solely rely on cached valuesUpdate: 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 singleslashing
value. Will still needgetPreStateSync()
for this PRThe other two rewards endpoint (attestation and sync committee) both requirepreState
so we will need to havegetPreStateSync()
implemented at some point. We can avoidgetPreStateSync()
for this endpoint because we have block rewards cached inBeaconStateCache.proposerRewards: RewardCache
but that's not the case for the other endpoints.