Generalize block available time metric for PeerDAS #6850
Conversation
dapplion
force-pushed
the
observed-times-peerdas
branch
from
7153ecf to
66ae7ee
Compare
There was a problem hiding this comment.
I've added a few minor comments, overall looks good to me!
I think it's ok to keep the same metric name, it's still means the same thing - the last timestamp we get all the required blobs to make block available (regardless of whether the transmit unit is column sidecar or blob sidecar).
| @@ -565,6 +571,8 @@ impl<T: BeaconChainTypes> DataAvailabilityChecker<T> { | |||
| &self.kzg, | |||
| &pending_components.verified_data_columns, | |||
| &self.spec, | |||
| // TODO(das): what timestamp to use when reconstructing columns? | |||
| timestamp_now(), | |||
There was a problem hiding this comment.
On one hand I think would be a bit more consistent if we put it after reconstruction, and right before we add the columns to put_kzg_verified_data_columns, similar to elsewhere; On the other hand, we can argue that we have these columns once we see the last column that triggered reconstruction. I don't see an issue going either way.
One scenario i can think of is -
- receive column 64, and trigger reconstruction, last_seen timestamp is
now - receive column 65 from gossip, last seen timestamp is
now + t - block is available when reconstruction completes, and last seen timestamp is actually
now + t, not the reconstructed columns that made the block available.
Another scenario
- receive column 64, and trigger reconstruction, last_seen timestamp is
now - receive column 65-127 from gossip, last seen timestamp is
now + t - block is made available before reconstruction completed
- reconstruction completed, but we ignore the columns because they're already in the cache.
- Note that we trigger
make_availableagain here (bug), but it doesnt change the last seem timestamp, stillnow + t
(bug issue: #6439)
There was a problem hiding this comment.
Not suggesting a change here - but if you think it makes sense to use the timestamp before reconstruction (i.e. the last data column that triggered reconstruction to make this block available), we can remove this TODO
| @@ -233,11 +234,17 @@ impl<T: BeaconChainTypes, O: ObservationStrategy> GossipVerifiedDataColumn<T, O> | |||
| #[ssz(struct_behaviour = "transparent")] | |||
| pub struct KzgVerifiedDataColumn<E: EthSpec> { | |||
| data: Arc<DataColumnSidecar<E>>, | |||
| #[ssz(skip_serializing, skip_deserializing)] | |||
There was a problem hiding this comment.
Do we still need Encode and Decode here? might be able to remove it now we don't store overflow cache anymore.
| @@ -293,29 +300,38 @@ impl<E: EthSpec> CustodyDataColumn<E> { | |||
| #[ssz(struct_behaviour = "transparent")] | |||
| pub struct KzgVerifiedCustodyDataColumn<E: EthSpec> { | |||
| data: Arc<DataColumnSidecar<E>>, | |||
| #[ssz(skip_serializing, skip_deserializing)] | |||
jimmygchen
added
waiting-on-author
Issue Addressed
Currently
BlockDelaysmetrics trackall_blobs_observedwhich is has the "intention" to track the time at which the last blob for a block was received. This metric won't register for PeerDAS.Proposed Changes
To generalize this for PeerDAS
all_blobs_observedShould I update the name of the metric?