-
Notifications
You must be signed in to change notification settings - Fork 227
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
harden REST API atSlot
against non-finalized blocks
#3538
Conversation
* harden validator API against pre-finalized slot requests * check `syncHorizon` when responding to validator api requests too far from `head` * limit state-id based requests to one epoch ahead of `head`
if res.slot + uint64(2 * SLOTS_PER_EPOCH) < slot: | ||
let head = node.dag.head | ||
|
||
if slot > head.slot and not node.isSynced(head): |
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.
node.isSynced
returns false
when head
is more than syncHorizon
behind the wall clock, it does not take into account the requested slot
.
The old logic allowed slot
to be syncHorizon
ahead of head
even while not yet fully synced.
The new logic no longer allows even requesting a single slot ahead of head
while sync has not completed.
Intended?
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, while syncing we don't want to allow spurious requests for slots that will cause expensive empty-slots replays of the state
beacon_chain/rpc/rest_utils.nim
Outdated
# head to avoid long empty slot replays (in particular a second epoch | ||
# transition) | ||
if stateIdent.slot.epoch + 1 > node.dag.head.slot.epoch: | ||
return err("Requesting state too far ahead of head current head") |
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.
- There is an extra
head
in the error message. - Logic looks alright,
slot.epoch + 1
does not seem to be overflowable.
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.
actually it was overflowing because of the magic far_future_epoch handling (that at some point should be applied to +
as well...)
max( | ||
getStateField(node.dag.headState, current_justified_checkpoint).epoch, | ||
node.dag.finalizedHead.slot.epoch) | ||
ok(node.dag.head.atEpochStart(justifiedEpoch).toBlockSlotId().expect("not nil")) |
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.
Good catch!
let res = slot.get() | ||
if res.isErr(): | ||
return RestApiResponse.jsonError(Http400, InvalidSlotValueError, | ||
$res.error()) |
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.
NIT: Indentation of the second line w.r.t. (
, on all of these.
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.
@@ -400,6 +406,9 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) = | |||
return RestApiResponse.jsonError(Http400, InvalidSlotValueError, | |||
$res.error()) | |||
res.get() | |||
if qslot <= node.dag.finalizedHead.slot: |
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.
The altair check of other endpoints is inside the block fetching qslot
-- should this additional check be located similarly, as in, first loading into rslot
, then validating against the allowed range, then assigning to qslot
? It doesn't matter semantically, it's more of a style / consistency thing.
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.
indeed, there's a myriad of these little improvements that one could make with infinite time at hand..
make rest test create a new genesis with the tests running roughly in the first epoch to allow testing a few more boundary conditions
atSlot
against non-finalized blocksatSlot
against non-finalized blocks
syncHorizon
when responding to validator api requests too farfrom
head
head
the first epoch to allow testing a few more boundary conditions