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

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

Merged
merged 18 commits into from
Dec 4, 2023
Merged

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

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e664d40
Specify RPC byRoot blocks-sidecars elegibility
tbenr Nov 16, 2023
c46c4ec
fix typo
tbenr Nov 16, 2023
6bbdeb8
Update specs/phase0/p2p-interface.md
tbenr Nov 17, 2023
ccf9e34
Update specs/deneb/p2p-interface.md
tbenr Nov 17, 2023
e2ca4b3
add failed on_block condition
tbenr Nov 22, 2023
d749a49
rephrase
tbenr Nov 22, 2023
4c5e5c8
Update specs/phase0/p2p-interface.md
tbenr Nov 29, 2023
ccca449
apply suggestion
tbenr Nov 29, 2023
88b1818
Update specs/deneb/p2p-interface.md
tbenr Nov 30, 2023
b525e9e
Update specs/deneb/p2p-interface.md
tbenr Nov 30, 2023
662c6eb
remove the last consider
tbenr Nov 30, 2023
bdac932
from on_block to state_transition
tbenr Nov 30, 2023
c661e20
simplify and add a new rule
tbenr Nov 30, 2023
dad09f6
Update specs/phase0/p2p-interface.md
tbenr Dec 1, 2023
eed7a66
Update specs/deneb/p2p-interface.md
tbenr Dec 1, 2023
cf39eed
Update specs/deneb/p2p-interface.md
tbenr Dec 1, 2023
ba09761
remove gossip failure rules
tbenr Dec 1, 2023
d343b6f
Apply suggestions from code review
djrtwo Dec 4, 2023
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
8 changes: 8 additions & 0 deletions specs/deneb/p2p-interface.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,9 @@ Per `context = compute_fork_digest(fork_version, genesis_validators_root)`:

No more than `MAX_REQUEST_BLOCKS_DENEB` may be requested at a time.

*[Modified in Deneb:EIP4844]*
Clients SHOULD include a block in the response as soon as it passes the gossip validation rules.
djrtwo marked this conversation as resolved.
Show resolved Hide resolved

##### BlobSidecarsByRoot v1

**Protocol ID:** `/eth2/beacon_chain/req/blob_sidecars_by_root/1/`
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair, let's remove it from here.

tbenr marked this conversation as resolved.
Show resolved Hide resolved
Clients MUST NOT respond with sidecars related to blocks that fail gossip validation rules.
tbenr marked this conversation as resolved.
Show resolved Hide resolved
Clients SHOULD NOT include sidecars related to blocks that fail `state_transition(block)`.
Copy link
Contributor

@mkalinin mkalinin Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


##### BlobSidecarsByRange v1

**Protocol ID:** `/eth2/beacon_chain/req/blob_sidecars_by_range/1/`
Expand Down
4 changes: 4 additions & 0 deletions specs/phase0/p2p-interface.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,10 @@ 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 MUST NOT respond with blocks that fail gossip validation rules.
Clients SHOULD NOT include blocks that fail `state_transition(block)`.
tbenr marked this conversation as resolved.
Show resolved Hide resolved

`/eth2/beacon_chain/req/beacon_blocks_by_root/1/` is deprecated. Clients MAY respond with an empty list during the deprecation transition period.

##### Ping
Expand Down