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.

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

Allow /api/eth/v1/validator/duties/sync/{epoch} to be called for epochs in the next sync committee period #3133

Merged
merged 2 commits into from
Nov 30, 2021

Conversation

zah
Copy link
Member

@zah zah commented Nov 28, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Nov 28, 2021

Unit Test Results

     12 files  ±0     756 suites  ±0   39m 37s ⏱️ - 47m 57s
1 515 tests ±0  1 477 ✔️ ±0  38 💤 ±0  0 ±0 
9 032 runs  ±0  8 952 ✔️ ±0  80 💤 ±0  0 ±0 

Results for commit bddcdfe. ± Comparison against base commit 4570f6e.

♻️ This comment has been updated with latest results.

else:
return RestApiResponse.jsonError(Http400, EpochFromFutureError)
else:
# The slot at the start of the sync committee period is likely to have a
Copy link
Member

Choose a reason for hiding this comment

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

strictly, we store head.atSlot(earliest).blck.parent.atSlot(earliest) - ie we store the state without the last block applied whereas this asks for the state at the slot with the block applied - the former is "enough" to compute the sync committee (which is done in epoch processing which happens before block processing) - there should perhaps be a helper for this in chaindag but there isn't right now

Copy link
Member Author

@zah zah Nov 28, 2021

Choose a reason for hiding this comment

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

While this trick is useful for the non-finalized states, should we consider storing "REST native" state snapshots for the finalized portion of the history? The era files reforms might give us a good opportunity to do this.

Copy link
Member

Choose a reason for hiding this comment

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

The era files also are specc:ed using the "slot state" without the block applied - for all intents and purposes, the "block" part belongs to the "new" epoch already - processing it destroys some state fields that would be messy to reconstruct otherwise (randao, balances that led to the effective balances etc)

It's not so much a trick as a natural way to reason about states and epochs: the epoch processing ends the previous epoch (the slot number is updated after the epoch processing is done - the block processing is done with the new slot number).

Applying that last block is pretty fast as well - the heavy lifting (epoch transition) has already been done - for rest requests like sync committees, proposers or attestation duties, it doesn't matter if the block has been applied or not, so all such rest processing would do well to use the state without the block.

Copy link
Member

Choose a reason for hiding this comment

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

Another important point about REST: whenever there's a request for per-epoch information, the current implementation rewinds the state to the exact block/slot: this is also unnecessary - it could use any state from the same epoch regardless of which blocks have been applied already, and thus avoid replays in many cases (cc @cheatfate) - this is an optimization we do not yet implement anywhere except through EpochRef. The example here is that you ask for the sync committee using slot 15 in the epoch: any state from [0-32) can satisfy it.

Copy link
Member Author

@zah zah Nov 29, 2021

Choose a reason for hiding this comment

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

Can you clarify why the information destroyed by block application remains useful for the states that are already finalized? All your other points seem equally valid if we were storing the state after block application (i.e. you can serve epoch properties such as the sync committee quickly from it).

By "REST Native", I was specifically referring to the states returned in the /eth/v/1beacon/states/{state_id}/* family of requests, which is arguably the most "natural" way to reason about states.

Copy link
Member

Choose a reason for hiding this comment

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

A good example is latest_block_header - it gives the basic information about the block that last influenced the epoch to become what it became (it determines the proposer shuffling, sync committees, effective balances, fork choice, deposits and so on).

I agree it's not immensely useful (ie it's possible to live without it), but doing things this way also gives symmetry to the storage, just like half-open ranges in general are easier to reason about because many algorithms on top of them are more simply expressed.

Regarding "REST native", I would actually work towards introducing more "standardised" ways of addressing slot/epoch states because broadly, these are more useful - it is also the case that most states you fetch from REST will require some block application - at that point, whether we replay one block more or less doesn't make much difference and it's a lot easier to treat all state requests (finalized and non-finalized) the same.

Copy link
Member

Choose a reason for hiding this comment

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

which is arguably the most "natural" way to reason about states.

I would argue that it's not at all a "natural" way to reason about states, and it's highly inefficient from a REST point of view: when addressed with state root or block+slot, most {state_id.}/* requests map N urls to M responses, with N = 64*M - ie completely wrong for caching purposes where you ideally want a 1:1 relationship.

I suspect the reasons for this are historical above all - the requests were created at a time when the spec was still poorly understood, else these kind of issues would have been addressed more elegantly than through the current "dependent root" mechanism.

Copy link
Member Author

Choose a reason for hiding this comment

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

It depends on how we define "natural". If it means something along the lines of "according to the principle of least surprise", then the current spec is certainly closer to the Ethereum traditions and thus less surprising. I haven't seen much use of the block+slot-offset addressing scheme in conversations either.

There is also another fundamental reason why the "dependent root" treatment is popular. It works as a proper instance of content-addressable storage. If you have a state root from somewhere, you can use any provider to download the state and you can then verify that it matches the root that you have. Not so with the block+slot scheme.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it remains the case that in order to serve state-root-based requests, we need a state root -> block+slot mapping for states that we do not fully store in the database, regardless of whether we store them with or without block applied, because it's not reasonable to expect that we'll store every state in there (even with StateDiff and era files, we'll not be storing every state).

In fact, what all these requests have in common is that unless you redundantly store each and every state separately, for all these requests you necessarily need to reduce the query to a "canonical" state range where the request can be satisfied and then choose the most efficient loading strategy - it doesn't really matter if we store the state with or without block, it's the reduction that is interesting - for block proposers for example, any state from the requested epoch will do - for sync committees, there's two days of states to choose from and so on.

"dependent root"

I'm not sure what you mean by your comment - the "dependent root" is used to identify the shuffling that was used to produce attestation duties (https://ethereum.github.io/beacon-APIs/#/Validator/getAttesterDuties) - it's used to detect significant vs insignificant reorgs from the point of view of the VC, or, in other words, when a state changed significantly for a particular data set. The concept can be generalised to block producers, sync committees etc - internally in clients, it's used as a key for caches.

@zah zah merged commit 3aa8040 into unstable Nov 30, 2021
@zah zah deleted the rest-fixes-2021-11-28 branch November 30, 2021 01:14

return RestApiResponse.jsonResponse(duties)
# We use a local proc in order to:
# * avoid code duplication
Copy link
Member

Choose a reason for hiding this comment

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

lol, when the means become the end ;)

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.

None yet

2 participants