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.

Sign up for GitHub

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

Jump to bottom

Expose ENR custody columns #6271

Closed
wants to merge 1 commit into from
Closed

Expose ENR custody columns #6271

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions beacon_node/lighthouse_network/src/types/globals.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
//! A collection of variables that are accessible outside of the network thread itself.
use crate::discovery::enr::Eth2Enr;
use crate::peer_manager::peerdb::PeerDB;
use crate::rpc::{MetaData, MetaDataV2};
use crate::types::{BackFillState, SyncState};
Expand All @@ -7,7 +8,7 @@ use crate::EnrExt;
use crate::{Enr, GossipTopic, Multiaddr, PeerId};
use parking_lot::RwLock;
use std::collections::HashSet;
use types::{ChainSpec, ColumnIndex, EthSpec};
use types::{ChainSpec, ColumnIndex, DataColumnSubnetId, EthSpec};

pub struct NetworkGlobals<E: EthSpec> {
/// The current local ENR.
Expand Down Expand Up @@ -111,10 +112,12 @@ impl<E: EthSpec> NetworkGlobals<E> {
}

/// Compute custody data columns the node is assigned to custody.
pub fn custody_columns(&self, _spec: &ChainSpec) -> Vec<ColumnIndex> {
let _enr = self.local_enr();
//TODO(das): implement ENR changes
vec![]
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 •
edited
Loading

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.

DataColumnSubnetId::compute_custody_columns::<E>(node_id, custody_subnet_count, spec)
.collect()
}

/// TESTING ONLY. Build a dummy NetworkGlobals instance.
Expand Down
Loading