Remove CGC from data_availability checker #7033
Conversation
dapplion
force-pushed
the
dedup-cgc-block
branch
from
32018b4 to
629d054
Compare
beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs
Outdated
Show resolved
Hide resolved
| @@ -145,13 +152,16 @@ impl<E: EthSpec> RpcBlock<E> { | |||
| Ok(Self { | |||
| block_root, | |||
| block: inner, | |||
| // Block is before PeerDAS | |||
| custody_columns_count: 0, | |||
| }) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
might be worth having consistent terminology for:
- Blocks
- Post-Deneb Blocks (blocks with blobs)
- Post-PeerDAS Blocks (blocks with custody columns)
Not sure what terminology that would be, but if we don't already have one / can't think of one, we should at least rename the above new function to something like new_with_blobs or something.
There was a problem hiding this comment.
I want to get rid of RpcBlock completely, so this assignment here will become irrelevant. Instead have a chain segment as an enum as you suggest
beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs
Outdated
Show resolved
Hide resolved
| @@ -33,6 +33,8 @@ pub struct NetworkGlobals<E: EthSpec> { | |||
| /// The computed sampling subnets and columns is stored to avoid re-computing. | |||
| pub sampling_subnets: HashSet<DataColumnSubnetId>, | |||
| pub sampling_columns: HashSet<ColumnIndex>, | |||
| /// Constant custody group count (CGC) set at startup | |||
| custody_group_count: u64, | |||
There was a problem hiding this comment.
You think it might be worth using one naming convention (like custody_group_count) everywhere instead of also using custody_columns_count?
There was a problem hiding this comment.
Also this isn't behind any RwLock or anything but it was my understanding that this could change as the node synced more columns / added more validators? Or is that just not implemented yet?
There was a problem hiding this comment.
not implemented yet
ethDreamer
added
waiting-on-author
dapplion
added
ready-for-review
|
This pull request has merge conflicts. Could you please resolve them @dapplion? 🙏 |
|
CI passing now |
|
This pull request has merge conflicts. Could you please resolve them @dapplion? 🙏 |
There was a problem hiding this comment.
@dapplion this is looking good! and thanks for the first round of review @ethDreamer 🙏
I've added a few comments. Let me know what you think!
jimmygchen
added
waiting-on-author
jimmygchen
added
ready-for-merge
|
I've tested this locally with Kurtosis and is working as expected. |
|
This pull request has been removed from the queue for the following reason: Pull request #7033 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (details: 1 review requesting changes and 1 approving review by reviewers with write access.) You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you can post a |
Issue Addressed
Validator custody makes the CGC and set of sampling columns dynamic. Right now this information is stored twice:
If that state becomes dynamic we must make sure it is in sync updating it twice, or guarding it behind a mutex. However, I noted that we don't really have to keep the CGC inside the data availability checker. All consumers can actually read it from the network globals, and we can update
make_availableto read the expected count of data columns from the block.