Specify when clients can serve block and sidecars in byRoot RPC methods #3551

tbenr · 2023-11-16T16:47:39Z

Based on conversation in #3547.

Allow clients to respond with blocks and blobs as soon as they pass gossip validation.

Blocks:
The idea is to have it a possibility (MAY) in Phase0 but to be an encouraged (SHOULD) behaviour from Deneb. This to allow mixed client approach: some could add it from Deneb only, other could have it enabled for all forks.

The second rule (SHOULD NOT) is to prevent clients continue serving blocks that turn out to be invalid during state transition.

Blob sidecar:
Similar to blocks: they SHOULD be served as soon as they pass gossip validation rules, even before the corresponding block is received. But as soon as block is received, If it fails gossip rules or state transition, the associated blob sidecars have to stop being served (together with the block itself).

fixes #3547

specs/phase0/p2p-interface.md

specs/deneb/p2p-interface.md

mkalinin · 2023-11-17T13:48:10Z

block import failed but due to data unavailability so block and sidecars i have so far are actually good and can be server

This might be ambiguous. Does it mean that the decision to serve a block depends on the attestation boundary? When does a client run an on_block in the case when not all blobs are received?

tbenr · 2023-11-17T16:22:15Z

This might be ambiguous. Does it mean that the decision to serve a block depends on the attestation boundary? When does a client run an on_block in the case when not all blobs are received?

I'm referring to the second, when on_block fails for data not fully available.

In actual client implementations, we could have block import failing for that reason before receiving a response from the EL (this is not the case in TEKU, where we fail that way when everything else has been validated successfully).

We could add a sentence like:
clients MAY continue serving blocks\sidecars failing ``on_block`` only when blocks\sidecars have been fully processed and the failure reason is due to data unavailable (some sidecars are missing, but the available one have been fully verified against block)."
We can find a better way of expressing it, it is just to show the point.

mkalinin · 2023-11-22T12:52:19Z

We can find a better way of expressing it, it is just to show the point.

Ah, sorry, got confused but I see the point now. Then we should include that into Deneb p2p-interface as in the proposed change the spec doesn’t make any note about a block without fully available blobs.

tbenr · 2023-11-22T22:35:29Z

Ah, sorry, got confused but I see the point now. Then we should include that into Deneb p2p-interface as in the proposed change the spec doesn’t make any note about a block without fully available blobs.

Added, but the problem with that detail is that, if we strictly follow how on_block has been specified, assert is_data_available(hash_tree_root(block), block.body.blob_kzg_commitments) comes before the state transition. So technically is_data_available can fail due to unavailable blobs without having fully validated the block.

specs/phase0/p2p-interface.md

specs/deneb/p2p-interface.md

specs/phase0/p2p-interface.md

tbenr · 2023-11-29T15:17:52Z

@mkalinin I'm thinking about a different phrasing like:

Clients MUST respond with blocks from their view of the current fork choice, from most recent slot back to latest finalized slot.

Additionally, clients SHOULD include blocks that passed gossip rules but have not yet processed by ``fork_choice.on_block``.

Clients MAY continue serving blocks failed ``fork_choice.on_block`` if and only if the failure reason is due to missing blobs. (i.e. some sidecars are missing but the available ones have been fully verified against the fully validated block).

(Similarly for SidecarsByRoot)

This clarifies what is supposed to be served, but I don't have the history of why it wasn't made explicit from the beginning.

djrtwo · 2023-11-29T17:26:59Z

@tbenr -- what do you think about the difference between passing on_block vs just the state transition function? I'm thinking that on_block is by default too restrictive as the MUST filter

djrtwo

ah damn, I just realized I never submitted my review from last week

here you go!

specs/deneb/p2p-interface.md

specs/phase0/p2p-interface.md

mkalinin · 2023-11-30T11:31:01Z

@djrtwo made a good call on referring to the state_transition instead of on_block. Checks run by the on_block on top of the state_transition:

a block is a descendant of the most recent finalized root — this is checked by the gossip rules, so this check is implied in our case
is_data_available returns true

The spec can say SHOULD NOT serve blocks for which one of the following conditions is not satisfied:

state_transition(block) succeeds
kzg proofs of available blobs are valid with respect to blob_kzg_commitments in the block

SHOULD NOT would give a room for implementations to start serving a block after it passes gossip validation but additional checks aren’t yet executed.

djrtwo · 2023-11-30T13:59:26Z

Doing a final review

@tbenr can you rebase this to dev? It has a number of strange commits like you forked prior to most recent

Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>

Co-authored-by: danny <dannyjryan@gmail.com>

specs/deneb/p2p-interface.md

tbenr · 2023-11-30T15:16:09Z

@mkalinin @djrtwo I added
Clients MUST NOT respond with sidecars related to blocks that fail gossip validation rules.

To explicitly drop sidecars when the corresponding blocks arrive late and fail gossip rules.

specs/phase0/p2p-interface.md

mkalinin

I think we can merge after addressing a couple of small things

specs/deneb/p2p-interface.md

Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>

dapplion

Signalling approval to current text here and to the discussion in Serving blocks and blob_sidecars in RPC byRoot right after they passed gossip validation #3547 by extension

Note for implementers: In Lodestar today the flow to query blocks by root is:

Receive block ReqResp response
Run state transition on block
Import to fork-choice / db
now block is available to other ReqResp requesters

I think the requesting logic will have to be modified to:

Receive block ReqResp response
Run gossip validation rules
Make block available to other ReqResp requesters
Run state transition on block
Import to fork-choice / db

djrtwo · 2023-11-30T15:58:12Z

specs/deneb/p2p-interface.md

@@ -300,6 +303,11 @@ Clients MUST support requesting sidecars since `minimum_request_epoch`, where `m
 Clients MUST respond with at least one sidecar, if they have it.
 Clients MAY limit the number of blocks and sidecars in the response.

+Clients SHOULD include a sidecar in the response as soon as it passes the gossip validation rules.
+Clients MUST NOT respond with sidecars that fails gossip validation rules.


Does this need to be explicit? This is the default for all gossip topics wrt other responses and general processing, right?

To me this explicitly states that client must not try to serve this before completing the validation, and if validation fails it must not be considered when serving the RPC call.
So there are no assumptions on clients internal RPC and gossip processing.
I agree that probably for most of current clients (teku for sure) this is true by default.

Is there somewhere a general statement that says "messages must pass gossip validation rules before being considered by any mean by the client"?

There is not other than the fact that gossipsub generally is gating messages from the application layer on these conditions. Not opposed to a general note if it's warranted, but I don't think we need to call out for each specific instance

Fair, let's remove it from here.

specs/deneb/p2p-interface.md

Co-authored-by: danny <dannyjryan@gmail.com>

g11tech · 2023-12-01T20:34:33Z

specs/phase0/p2p-interface.md

@@ -856,6 +856,9 @@ Clients MUST support requesting blocks since the latest finalized epoch.
 Clients MUST respond with at least one block, if they have it.
 Clients MAY limit the number of blocks in the response.

+Clients MAY include a block in the response as soon as it passes the gossip validation rules.
+Clients SHOULD NOT respond with blocks that fail beacon chain state transition.


we can still have this post deneb as well as all clients are verifying blocks without waiting for all blobs to show up

Yes. Deneb updates the first rule but leaves this one unchanged

sounds good, it would have been good to enumerate the rules so as to be super clear, but nevertheless lgtm

mkalinin · 2023-12-04T07:58:33Z

specs/deneb/p2p-interface.md

@@ -300,6 +303,10 @@ Clients MUST support requesting sidecars since `minimum_request_epoch`, where `m
 Clients MUST respond with at least one sidecar, if they have it.
 Clients MAY limit the number of blocks and sidecars in the response.

+Clients SHOULD include a sidecar in the response as soon as it passes the gossip validation rules.
+Clients SHOULD NOT respond with sidecars related to blocks that fail gossip validation rules.
+Clients SHOULD NOT include sidecars related to blocks that fail `state_transition(block)`.


Suggested change

Clients SHOULD NOT include sidecars related to blocks that fail `state_transition(block)`.

Clients SHOULD NOT include sidecars related to blocks that fail beacon chain state transition.

why do we wanna have this removed? seems reasonable to me

Sorry, that was due to my misread, updated the suggestion!

specs/deneb/p2p-interface.md

specs/phase0/p2p-interface.md

specs/deneb/p2p-interface.md

mkalinin reviewed Nov 16, 2023

View reviewed changes

specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved

mkalinin reviewed Nov 16, 2023

View reviewed changes

specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved

mkalinin reviewed Nov 27, 2023

View reviewed changes

specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved

mkalinin reviewed Nov 27, 2023

View reviewed changes

specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved

mkalinin reviewed Nov 27, 2023

View reviewed changes

specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved

djrtwo reviewed Nov 29, 2023

View reviewed changes

hwwhww mentioned this pull request Nov 29, 2023

release v1.4.0-beta.5 #3554

Merged

4 tasks

tbenr and others added 12 commits November 30, 2023 15:20

Specify RPC byRoot blocks-sidecars elegibility

e664d40

fix typo

c46c4ec

Update specs/phase0/p2p-interface.md

6bbdeb8

Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>

Update specs/deneb/p2p-interface.md

ccf9e34

Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>

add failed on_block condition

e2ca4b3

rephrase

d749a49

Update specs/phase0/p2p-interface.md

4c5e5c8

Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>

apply suggestion

ccca449

Update specs/deneb/p2p-interface.md

88b1818

Co-authored-by: danny <dannyjryan@gmail.com>

Update specs/deneb/p2p-interface.md

b525e9e

Co-authored-by: danny <dannyjryan@gmail.com>

remove the last consider

662c6eb

from on_block to state_transition

bdac932

tbenr force-pushed the specify_rpc_by_root_elegibility branch from 589d0ac to bdac932 Compare November 30, 2023 14:20

mkalinin reviewed Nov 30, 2023

View reviewed changes

specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved

simplify and add a new rule

c661e20

hwwhww added the scope:networking label Nov 30, 2023

mkalinin reviewed Dec 1, 2023

View reviewed changes

specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved

mkalinin approved these changes Dec 1, 2023

View reviewed changes

mkalinin reviewed Dec 1, 2023

View reviewed changes

specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved

tbenr and others added 2 commits December 1, 2023 08:28

Update specs/phase0/p2p-interface.md

dad09f6

Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>

Update specs/deneb/p2p-interface.md

eed7a66

Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>

dapplion approved these changes Dec 1, 2023

View reviewed changes

tbenr mentioned this pull request Dec 1, 2023

Make blocks and sidecars available before import to RPC requests Consensys/teku#7773

Closed

djrtwo reviewed Dec 1, 2023

View reviewed changes

tbenr and others added 2 commits December 1, 2023 17:29

Update specs/deneb/p2p-interface.md

cf39eed

Co-authored-by: danny <dannyjryan@gmail.com>

remove gossip failure rules

ba09761

g11tech reviewed Dec 1, 2023

View reviewed changes

mkalinin reviewed Dec 4, 2023

View reviewed changes

g11tech approved these changes Dec 4, 2023

View reviewed changes

djrtwo approved these changes Dec 4, 2023

View reviewed changes

specs/deneb/p2p-interface.md Show resolved Hide resolved

specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved

specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved

Apply suggestions from code review

d343b6f

hwwhww added phase0 Deneb was called: eip-4844 labels Dec 4, 2023

hwwhww merged commit 06fe616 into ethereum:dev Dec 4, 2023
26 checks passed

tbenr deleted the specify_rpc_by_root_elegibility branch December 4, 2023 15:58

realbigsean mentioned this pull request Jan 19, 2024

Check availability cache when serving block by root requests sigp/lighthouse#5093

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify when clients can serve block and sidecars in byRoot RPC methods #3551

Specify when clients can serve block and sidecars in byRoot RPC methods #3551

tbenr commented Nov 16, 2023 •

edited

Loading

mkalinin commented Nov 17, 2023

tbenr commented Nov 17, 2023 •

edited

Loading

mkalinin commented Nov 22, 2023

tbenr commented Nov 22, 2023

tbenr commented Nov 29, 2023 •

edited

Loading

djrtwo commented Nov 29, 2023

djrtwo left a comment

mkalinin commented Nov 30, 2023

djrtwo commented Nov 30, 2023

tbenr commented Nov 30, 2023

mkalinin left a comment •

edited

Loading

dapplion left a comment

djrtwo Nov 30, 2023

tbenr Dec 1, 2023

djrtwo Dec 1, 2023

tbenr Dec 1, 2023

g11tech Dec 1, 2023

tbenr Dec 3, 2023

g11tech Dec 4, 2023

mkalinin Dec 4, 2023 •

edited

Loading

g11tech Dec 4, 2023

mkalinin Dec 4, 2023

	Clients SHOULD NOT include sidecars related to blocks that fail `state_transition(block)`.
	Clients SHOULD NOT include sidecars related to blocks that fail beacon chain state transition.

Specify when clients can serve block and sidecars in byRoot RPC methods #3551

Specify when clients can serve block and sidecars in byRoot RPC methods #3551

Conversation

tbenr commented Nov 16, 2023 • edited Loading

mkalinin commented Nov 17, 2023

tbenr commented Nov 17, 2023 • edited Loading

mkalinin commented Nov 22, 2023

tbenr commented Nov 22, 2023

tbenr commented Nov 29, 2023 • edited Loading

djrtwo commented Nov 29, 2023

djrtwo left a comment

Choose a reason for hiding this comment

mkalinin commented Nov 30, 2023

djrtwo commented Nov 30, 2023

tbenr commented Nov 30, 2023

mkalinin left a comment • edited Loading

Choose a reason for hiding this comment

dapplion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkalinin Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbenr commented Nov 16, 2023 •

edited

Loading

tbenr commented Nov 17, 2023 •

edited

Loading

tbenr commented Nov 29, 2023 •

edited

Loading

mkalinin left a comment •

edited

Loading

mkalinin Dec 4, 2023 •

edited

Loading