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

EIP4844: Clarify ratelimit behavior for sidecar with zero blobs #3174

Merged
merged 7 commits into from
Jan 12, 2023

Conversation

terencechain
Copy link
Contributor

@terencechain terencechain commented Dec 23, 2022

Clarify that a peer must not respond a sidecar with empty blobs (i.e, len(sidecar.blobs) == 0) under RPC end point /eth2/beacon_chain/req/blobs_sidecars_by_range/1/. This behavior has caused some confusion on devnet3, so it'd be good to clarify it in the spec. A peer may use the kzg commitment in the block body to distinguish if the slot should have a sidecar or not

After further discussions, we've agreed to favor the responding behavior and further clarify that a peer may not want to rate limit a sidecar with empty blobs

@@ -247,6 +249,8 @@ disconnect and/or temporarily ban such an un-synced or semi-synced client.
Clients MUST respond with at least the first blobs sidecar that exists in the range, if they have it,
and no more than `MAX_REQUEST_BLOBS_SIDECARS` sidecars.

Clients MUST not respond with empty blobs sidecars.
Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify what an "empty" blob sidecar is. It's a BlobSidecar that does not contain any blobs, but it may contain a non-zero beacon_block_root and beacon_block_slot per get_blobs_sidecar.

@terencechain terencechain changed the title Clarify behavior for empty blobs sidecar Clarify behavior for sidecar with no blobs for range request Dec 23, 2022
Copy link
Contributor

@Inphi Inphi left a comment

Choose a reason for hiding this comment

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

thanks!

@@ -247,6 +249,8 @@ disconnect and/or temporarily ban such an un-synced or semi-synced client.
Clients MUST respond with at least the first blobs sidecar that exists in the range, if they have it,
and no more than `MAX_REQUEST_BLOBS_SIDECARS` sidecars.

Clients MUST not respond with empty blobs sidecars.
Copy link
Contributor

@g11tech g11tech Dec 23, 2022

Choose a reason for hiding this comment

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

Suggested change
Clients MUST not respond with empty blobs sidecars.
Clients MAY skip empty blobs sidecars.

@@ -226,6 +226,8 @@ The request MUST be encoded as an SSZ-container.
The response MUST consist of zero or more `response_chunk`.
Each _successful_ `response_chunk` MUST contain a single `BlobsSidecar` payload.

In cases where a slot contains a `BlobSidecar` that does not contain any blobs, but contain non-zero `beacon_block_root` and `beacon_block_slot`, `blobs_sidecar` **may be** skipped.
Copy link
Contributor

@g11tech g11tech Dec 23, 2022

Choose a reason for hiding this comment

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

Suggested change
In cases where a slot contains a `BlobSidecar` that does not contain any blobs, but contain non-zero `beacon_block_root` and `beacon_block_slot`, `blobs_sidecar` **may be** skipped.
In cases where a slot contains a `BlobSidecar` that does not contain any blobs, but contain non-zero `beacon_block_root`, `beacon_block_slot` and a valid `kzg_aggregated_proof`, `blobs_sidecar` **may be** skipped.

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm!

@terencechain terencechain changed the title Clarify behavior for sidecar with no blobs for range request EIP4844: Clarify behavior for sidecar with no blobs for range request Dec 29, 2022
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

Why skip these -- the exist (and would exist at time of gossiping)? doesn't that just make cross validation/verification marginally more difficult?

Is it just to save the 100 bytes or so?

@terencechain
Copy link
Contributor Author

Since we didn't clarify this condition explicitly, it has become the default behavior on devnet3. The saving for 100 bytes is not worth it, but neither cross-validation is more difficult than before (i.e check block body's kzg commitment first, if it's non-empty, then there must exist a sidecar). It does feel cleaner not to have empty objects over the wire since we assume most of them will be empty early on.

@Inphi I think you might have a stronger opinion on this. Anything else to add?

@djrtwo
Copy link
Contributor

djrtwo commented Jan 2, 2023

It does feel cleaner not to have empty objects over the wire since we assume most of them will be empty early on.

I feel a bit otherwise on the cleanliness -- explicit (send the side car no matter what) seems like less cognitive load to understand what's going on. Rather than implicit (drop the side car when no blobs in block) which requires knowing this extra rule to understand.

@Inphi
Copy link
Contributor

Inphi commented Jan 3, 2023

whether we send zero blobs or not, the spec needs to be clear on the behavior.

I'm slightly for empty sidecar elision for a couple reasons;

  • as @terencechain pointed out, we need to check the corresponding kzg commitment either way to determine if a blobs sidecar is expected.
  • implementations that employ per-message rate limits don't have to unnecessarily trip up the limiter for "zero cost* empty blobs sidecars.
  • this is kinda synonymous to the existing behavior for non-existent blocks in missed slots. It would be odd for there to be sidecars, empty or otherwise, if there are missed slots.

@djrtwo
Copy link
Contributor

djrtwo commented Jan 3, 2023

this is kinda synonymous to the existing behavior for non-existent blocks in missed slots. It would be odd for there to be sidecars, empty or otherwise, if there are missed slots.

Except there is a message that exists and was sent with gossip. I find assymetries between different p2p protocols on sending/not sending empty to be the sticking point form me.

  1. explicit better than implicit
  2. symmetry between different calls/functions/etc to aid in expectations and reasoning

implementations that employ per-message rate limits don't have to unnecessarily trip up the limiter for "zero cost* empty blobs sidecars.

I see this but think the above points in aiding spec understanding trump this especially considering that the equillibrium will be non-zero blobs for almost all blocks (1 is the minimum blob gas price so the resource will be utilized)

@Inphi
Copy link
Contributor

Inphi commented Jan 3, 2023

Good points. I'm sympathetic to keeping things symmetric. Though there're a couple asymmetries in Req-Res RPC today, including behavior around missed w/ slots, we don't need to add more more to the pile.

@terencechain terencechain changed the title EIP4844: Clarify behavior for sidecar with no blobs for range request EIP4844: Clarify ratelimit behavior for sidecar with zero blobs Jan 10, 2023
@terencechain
Copy link
Contributor Author

ready again @djrtwo 🙏🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844 scope:networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants