Don't write empty blobs to db #3848
Conversation
emhane
requested review from
realbigsean and
pawanjay176
and removed request for
realbigsean
emhane
added
ready-for-review
emhane
force-pushed
the
empty_blobs_db
branch
2 times, most recently
from
b3b580d
to
8dd2e07
Compare
| 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.
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.
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.
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.
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