Missing values in `/eth/v1/config/spec`

[barnabasbusa]

Description

There are a few values that are missing from the output of /eth/v1/config/spec beacon api.

Version

latest unstable - 556e917

Present Behaviour

Currently missing config values are:
REORG_HEAD_WEIGHT_THRESHOLD (20)
REORG_PARENT_WEIGHT_THRESHOLD (160)
REORG_MAX_EPOCHS_SINCE_FINALIZATION (2)
EPOCHS_PER_SUBNET_SUBSCRIPTION (256)
ATTESTATION_SUBNET_COUNT (64)
ATTESTATION_SUBNET_EXTRA_BITS (0)

Missing preset values:
UPDATE_TIMEOUT (8192) (diff value for minimal)

Missing domain types:
DOMAIN_BLS_TO_EXECUTION_CHANGE (0x0a000000)

Expected Behaviour

All values should be returned with the correct values.

[0xmrree]

@michaelsproul yo can i take this?

[michaelsproul]

Yeah that'd be great! The reorg config might need relocating from
its current home

[0xmrree]

@michaelsproul apologies for delay just starting on this now. I see that for those re org params they are currently be sourced from cli args then passed into fork choice algo. Should my pr include refactoring to remove these cli args and then hook up fork choice to ChainSpec which will source the reorg stuff from yaml net configs? - ill assume yes given your comment above

[michaelsproul]

I would actually quite like to keep the CLI parameters if we can establish a straight-forward way to have them override the YAML values. Lighthouse pioneered the reorg feature before other clients, which is why we have flags for these parameters, that predate their inclusion in the YAML config.

The ideal behaviour would be:

1. Make these parameters mandatory (or provide defaults) in ChainSpec so they are always present
2. If no CLI flag is set, use the value from the ChainSpec for configuration of the reorg feature (maybe we keep the fields in two places: ChainSpec and ChainConfig)
3. If a CLI flag is set, have it override the YAML config for the purposes of behaviour (how the node handles reorgs) and for API output

[michaelsproul]

I'd be open to doing this change in two PRs. The non-reorg related flags could be in a small standalone PR, and the reorg flags could be in a follow up PR

[0xmrree]

Ah this makes total sense that you guys had it first, I was wondering why stuff was optional. To make sure I understand if no CLI flag is set, just use the .yaml config vlaue. If cli flag is set then update Config/ChainSpec so that /eth/v1/config/spec reflects the overridden config?

[0xmrree]

Also regarding the attestation subnet selection stuff. I see that spec for phase 0 states subnet subscription rotates based on epoch while lighthouse does not. Am I missing something?

phase 0 spec:

def compute_subscribed_subnet(node_id: NodeID, epoch: Epoch, index: int) -> SubnetID:
    node_id_prefix = node_id >> (NODE_ID_BITS - ATTESTATION_SUBNET_PREFIX_BITS)
    node_offset = node_id % EPOCHS_PER_SUBNET_SUBSCRIPTION
    permutation_seed = hash(
        uint_to_bytes(uint64((epoch + node_offset) // EPOCHS_PER_SUBNET_SUBSCRIPTION))
    )
    permutated_prefix = compute_shuffled_index(
        node_id_prefix,
        1 << ATTESTATION_SUBNET_PREFIX_BITS,
        permutation_seed,
    )
    return SubnetID((permutated_prefix + index) % ATTESTATION_SUBNET_COUNT)

lighthouse:

    /// Computes the set of subnets the node should be subscribed to. We subscribe to these subnets
    /// for the duration of the node's runtime.
    #[allow(clippy::arithmetic_side_effects)]
    pub fn compute_attestation_subnets(
        raw_node_id: [u8; 32],
        spec: &ChainSpec,
    ) -> impl Iterator<Item = SubnetId> {
        // The bits of the node-id we are using to define the subnets.
        let prefix_bits = spec.attestation_subnet_prefix_bits as u32;

        let node_id = U256::from_be_slice(&raw_node_id);
        // calculate the prefixes used to compute the subnet and shuffling
        let node_id_prefix = (node_id >> (NODE_ID_BITS - prefix_bits))
            .as_le_slice()
            .get_u64_le();

        // Get the constants we need to avoid holding a reference to the spec
        let &ChainSpec {
            subnets_per_node,
            attestation_subnet_count,
            ..
        } = spec;

        (0..subnets_per_node)
            .map(move |idx| SubnetId::new((node_id_prefix + idx as u64) % attestation_subnet_count))
    }

In addition, I see that there is no configuration for extra prefix bits currently.

For this issue I would assume I just add epochs_per_subnet_subscription and attestation_subnet_extra_bits even though its not used.

[michaelsproul]

Ah this makes total sense that you guys had it first, I was wondering why stuff was optional. To make sure I understand if no CLI flag is set, just use the .yaml config vlaue. If cli flag is set then update Config/ChainSpec so that /eth/v1/config/spec reflects the overridden config?

Yep this sounds right. Modifying the ChainSpec might not really be feasible after initialistion (because it is immutable + Arced), so likely we should modify the existing variable in ChainConfig, and then ensure that the API always serves the value from ChainConfig.

I see that spec for phase 0 states subnet subscription rotates based on epoch while lighthouse does not. Am I missing something?

@0xmrree Nice catch, Lighthouse is intentionally not rotating the long-lived subnet subscriptions. We made that change in this PR:

• Simple Subnet Management #6146

There was a PR to the spec but it got closed for inactivity, I've followed up to see if we should reopen it.

• Simplified attestation subnet mapping ethereum/consensus-specs#3735

For this issue I would assume I just add epochs_per_subnet_subscription and attestation_subnet_extra_bits even though its not used.

I guess that's the best way to do it. We will likely never use epochs_per_subnet_subscription (due to the intentional change). We could use attestation_subnet_extra_bits (even though it is zero) to derive the value of attestation_subnet_prefix_bits. In which case I think we should maybe stop reading that value from config.yaml, and stop serving it on the config API (i.e. remove it as a field from ChainSpec and derive it instead – put it behind a method).

@barnabasbusa can you confirm that attestation_subnet_prefix_bits is not meant to be part of API output any more?

[barnabasbusa]

https://github.com/ethereum/consensus-specs/blob/master/configs/mainnet.yaml#L182 @michaelsproul I would expect all values that are listed here to be also listed by the beacon api output.

Whether this value should be in the spec is a different question.

[0xmrree]

newbie question - My understanding was that Ethereum essentially is the protocol in the spec and the clients implement the spec how they please but they all have consensus to implement the same protocol outlined in the spec. Why is it ok in this instance to not implement the rotating subnets even though the spec says so? Not challenging here, I want to understand the framework

[michaelsproul]

Why is it ok in this instance to not implement the rotating subnets even though the spec says so? Not challenging here, I want to understand the framework

Unlike the consensus state transition (process_block/process_epoch/process_slot), there doesn't need to be perfect agreement between clients on network-level behaviour. In the case of these long-lived subnets, a node can't tell why another node is subscribed to a subnet (or not), and the spec doesn't ascribe any penalty for non-compliance (because it is impossible to detect). Most clients also include a flag like --subscribe-all-subnets, which leads to lots of nodes subscribed to subnets that they "shouldn't be" according to their node ID.

[0xmrree]

In which case I think we should maybe stop reading that value from config.yaml, and stop serving it on the config API (i.e. remove it as a field from ChainSpec and derive it instead – put it behind a method).

Tbh I'm not sure I follow that we should stop reading from config.yaml if we actually start using attestation_subnet_extra_bits such that setting it to non zero would change the subnets we subscribe too. (Which is the expected behavior of changing that value in the config according to spec) My understanding would be this would support it coming from the config.yaml - but I am down to do what you think is best.

[michaelsproul]

@0xmrree I think it's strange that the config.yaml includes 3 parameters, when only 2 of them are necessary to determine the 3rd. It begs the question of how Lighthouse should behave if e.g. attestation_subnet_prefix_bits and attestation_subnet_extra_bits are inconsistent. If we do continue to read the unnecessary value attestation_subnet_prefix_bits, then we should check that it is consistent (i.e. it equals ceillog2(ATTESTATION_SUBNET_COUNT) + ATTESTATION_SUBNET_EXTRA_BITS). My initial proposal was just that we ignore it, because we don't currently have much validation of the fields of ChainSpec (because they are mostly designed without redundancy). I think this redundancy only exists for backwards-compatibility.