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

Expose ENR custody columns #6271

Closed
wants to merge 1 commit into from
Closed

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Aug 16, 2024

Issue Addressed

Part of

Proposed Changes

  • Add custody_columns method to NetworkGlobals

@dapplion dapplion added ready-for-merge This PR is ready to merge. das Data Availability Sampling das-unstable-merge ready-for-review The code is ready for review and removed ready-for-merge This PR is ready to merge. labels Aug 16, 2024
@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 19, 2024
pub fn custody_columns(&self, spec: &ChainSpec) -> Vec<ColumnIndex> {
let enr = self.local_enr();
let node_id = enr.node_id().raw().into();
let custody_subnet_count = enr.custody_subnet_count::<E>(spec);
Copy link
Member

Choose a reason for hiding this comment

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

Just a small suggestion wrt Enr::custody_subnet_count, I think we should make that simply return an error on invalid/non existing enr instead of returning the default value. Basically change the function signature to

fn custody_subnet_count<E: EthSpec>(&self, spec: &ChainSpec) -> Result<u64, &'static str>;

Swallowing the error at the accessor function and returning the default might lead to confusing errors.

Copy link
Member

@jimmygchen jimmygchen Aug 19, 2024

Choose a reason for hiding this comment

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

Good that you raised this again - we actually thought about this when this was initially implemented, and the original reason to swallow it is wanting to have the fallback logic in one place, and don't want to do any error handling due to invalid input from peers, but maybe there's better approach and may even be useful to log an error?

Copy link
Member

Choose a reason for hiding this comment

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

We could just do the fallback logic at the calling site. The accessor function on that trait doesn't seem like the right place to do fallback logic.
Consider a situation where a client has a bug in their encoding for the enr custody subnet field, in that case, we would always swallow the error which isn't what we want to do.

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-merge This PR is ready to merge. labels Aug 20, 2024
@jimmygchen
Copy link
Member

The main das branch has been merged and this change has already been included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
das Data Availability Sampling das-unstable-merge waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants