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.
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.
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.
- There is an extra
headin the error message. - Logic looks alright,
slot.epoch + 1does not seem to be overflowable.
There was a problem hiding this comment.
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")) |
| let res = slot.get() | ||
| if res.isErr(): | ||
| return RestApiResponse.jsonError(Http400, InvalidSlotValueError, | ||
| $res.error()) |
There was a problem hiding this comment.
NIT: Indentation of the second line w.r.t. (, on all of these.
There was a problem hiding this comment.
this is true, but also a bit of a chore / best-effort thing as long as we don't have an autoformatter.. the best way to highlight these is through the little suggestion button which gives the right perfomance/effort ratio for fixing them (and avoids typing NIT in ALL-CAPS :))
| @@ -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.
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.
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
arnetheduck
changed the title
atSlot against non-finalized blocksatSlot against non-finalized blocks
syncHorizonwhen responding to validator api requests too farfrom
headheadthe first epoch to allow testing a few more boundary conditions