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.

Sign up for GitHub

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

Merged
merged 28 commits into from
Feb 22, 2024
Merged

feat: add endpoint for Altair block reward #6178

Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
6679119
Add block rewards api
ensi321 Dec 5, 2023
f67421b
Add test
ensi321 Dec 5, 2023
0b030f2
Add unit test
ensi321 Dec 11, 2023
5e3f0f5
Lint
ensi321 Dec 11, 2023
c2da457
Address comment
ensi321 Dec 12, 2023
1e47858
Reduce code redundancy
ensi321 Dec 12, 2023
ac9bb42
Read reward cache first before calculate
ensi321 Dec 27, 2023
1fc3201
Lint
ensi321 Dec 27, 2023
f702c2b
Partially address comments
ensi321 Jan 11, 2024
6c14d0c
Accept optional postState to get the reward cache
ensi321 Jan 11, 2024
f331e5a
Update test
ensi321 Jan 11, 2024
db5cc41
lint
ensi321 Jan 11, 2024
edc8c3c
Update packages/beacon-node/src/chain/rewards/blockRewards.ts
ensi321 Jan 22, 2024
f543045
Update packages/beacon-node/src/chain/rewards/blockRewards.ts
ensi321 Jan 22, 2024
b605529
Update packages/beacon-node/src/chain/rewards/blockRewards.ts
ensi321 Jan 22, 2024
45734fa
Update packages/beacon-node/src/chain/rewards/blockRewards.ts
ensi321 Jan 22, 2024
f80daf4
Rename proposerRewards to blockRewards. Fix import
ensi321 Jan 22, 2024
3f8d42c
Remove getBlockRewards from api ignore list
ensi321 Jan 22, 2024
f2bbe95
Fix test
ensi321 Jan 23, 2024
cde10e9
Rename state to preState
ensi321 Jan 25, 2024
a6e2fc4
Add description to fields in BlockRewards
ensi321 Jan 25, 2024
ee42dde
Clean up imports
ensi321 Jan 25, 2024
b6f68d0
Use jsdoc to document properties
nflaig Jan 25, 2024
5c10acf
Apply suggestions from code review
wemeetagain Jan 25, 2024
47e9650
Merge branch 'ChainSafe:unstable' into rewards-api
ensi321 Jan 30, 2024
719ab1d
Add `getPreStateSync()`
ensi321 Jan 30, 2024
51bbf5e
fix: clone states to compute block rewards
twoeths Feb 20, 2024
eceea02
Merge pull request #2 from tuyennhv/tuyennhv/rewards-api
ensi321 Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address comment
  • Loading branch information
@ensi321
ensi321 committed Jan 23, 2024
commit c2da4572d5009925c1876eedfbef334155794bfe
2 changes: 1 addition & 1 deletion packages/beacon-node/src/chain/chain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,7 +994,7 @@ export class BeaconChain implements IBeaconChain {
}

async getBlockRewards(block: allForks.FullOrBlindedBeaconBlock): Promise<BlockRewards> {
const preState = (await this.regen.getPreState(block, {dontTransferCache: false}, RegenCaller.restApi)).clone();
const preState = (await this.regen.getPreState(block, {dontTransferCache: true}, RegenCaller.restApi)).clone();
Copy link
Member

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?

Copy link
Contributor Author

@ensi321 ensi321 Jan 26, 2024 •
edited
Loading

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

Copy link
Member

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

lodestar/packages/api/src/beacon/server/index.ts

Line 41 in 347c95f

lightclient: () => lightclient.getRoutes(config, api.lightclient),

we could then simply not enable rewards APIs by default here

lodestar/packages/beacon-node/src/api/rest/index.ts

Line 19 in 347c95f

api: ["beacon", "config", "events", "node", "validator", "lightclient"],

Copy link
Contributor Author

@ensi321 ensi321 Jan 29, 2024 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor

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

Copy link
Contributor Author

@ensi321 ensi321 Jan 30, 2024 •
edited
Loading

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.

preState.slot = block.slot; // regen.getPreState guarantees pre_state of the same epoch but not the same slot
ensi321 marked this conversation as resolved.
Show resolved Hide resolved
const result = computeBlockRewards(block, preState);
return result;
Expand Down