-
Notifications
You must be signed in to change notification settings - Fork 738
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
Don't write empty blobs to db #3848
Conversation
b3b580d
to
8dd2e07
Compare
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.
Nice! there's a .DS_Store
file committed here, would you mind deleting it? Looks good otherwise!
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.
LGTM overall. Just some clarifications.
blobs_sent += 1; | ||
self.send_network_message(NetworkMessage::SendResponse { | ||
peer_id, | ||
response: Response::BlobsByRange(Some(Arc::new(blob))), | ||
response: Response::BlobsByRange(response_data), |
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.
Just to understand this better, are we sending a stream terminator if the sidecar contains empty blobs? Is this a change in the spec?
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.
That doesn't look right...good catch! The goal is to not send an empty blobs sidecar. I will interpret this as not sending a response at all for a block with no kzg_commitments. Relating to this pr ethereum/consensus-specs#3174 (comment)
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.
I would hold off making this change until the linked PR gets resolved. I personally don't have a preference between sending empty blobs v/s not sending empty blobs.
However, if we go down the not sending empty blobs route, then we also need to ensure that we handle that case on the receiving end to account for missing blobs in the response.
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.
Nice catch Pawan!
However, if we go down the not sending empty blobs route, then we also need to ensure that we handle that case on the receiving end to account for missing blobs in the response.
We do handle handle it either way on the receiving end right now. Diva implemented that last week. Other clients should be able to handle empty blobs too even if they skip empty blobs when they serve them so I thinks it's OK to send empty blobs for now.
Issue Addressed
Don't write empty blobs to db
Proposed Changes
Blob verification doesn't appear to happen on blobs loaded from db, so verification will occur where it is supposed to (i.e. where this is called
lighthouse/beacon_node/beacon_chain/src/kzg_utils.rs
Line 14 in 786d983