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

Feat: add new --advertise-false-custody-group-count cli flag for PeerDAS testing #7120

Open
wants to merge 4 commits into
base: unstable
Choose a base branch
from

Conversation

startup-dreamer
Copy link

@startup-dreamer startup-dreamer commented Mar 13, 2025

Issue Addressed

Closes: #6973

Proposed Changes

This PR adds a new CLI flag --advertise-false-custody-group-count that allows nodes to advertise a false custody group count in their metadata for PeerDAS testing purposes. This enables testing scenarios where nodes need to mislead peers about their custody group count without altering the actual RPC logic.

Additional Info

When this flag is set:

  • If PeerDAS is scheduled, the node will advertise the specified custody group count in its metadata.
  • If the specified count is outside the valid range (<= number_of_custody_groups), the node will log a warning and use the normal count.
  • If PeerDAS is not scheduled, the flag has no effect.

Future Work

This flag is intended to be temporary and will be removed before the PeerDAS release, as indicated by the TODO comment.

@startup-dreamer startup-dreamer requested a review from jxs as a code owner March 13, 2025 00:10
Copy link

mergify bot commented Mar 13, 2025

This pull request has merge conflicts. Could you please resolve them @startup-dreamer? 🙏

@startup-dreamer startup-dreamer force-pushed the feat/add_advertise_false_custody_group_count_cli_for_peerdas_testing branch from f49f770 to 83a73f5 Compare March 13, 2025 00:15
@jimmygchen jimmygchen added ready-for-review The code is ready for review das Data Availability Sampling labels Mar 13, 2025
@@ -199,8 +199,23 @@ impl<E: EthSpec> Network<E> {

// Construct the metadata
let custody_group_count = ctx.chain_spec.is_peer_das_scheduled().then(|| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this affect both the ENR value and the value returned in the Status ReqResp message?

Copy link
Author

Choose a reason for hiding this comment

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

ENR is built with the metadata (false custody_group_count if provided) and The metadata returned in response to METADATA requests (which is part of the ReqResp protocol) so I'd say yes it affects both.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the user restarts the node with a different value of the false_count flag, will we bump the seq_number of the ENR?

Copy link
Author

@startup-dreamer startup-dreamer Mar 26, 2025

Choose a reason for hiding this comment

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

I rechecked I don't think it bumps the seq_number of ENR, Since the ENR is built from the chain spec and the
subscribe_all_data_column_subnets, not from the false count flag (sorry for prev comment I am still getting familiar with the codebase). Does this make sense @dapplion

// only set `csc` if PeerDAS fork epoch has been scheduled
if spec.is_peer_das_scheduled() {
let custody_subnet_count = if config.subscribe_all_data_column_subnets {
spec.data_column_sidecar_subnet_count
} else {
spec.custody_requirement
};
builder.add_value(PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY, &custody_subnet_count);
}

Copy link
Author

@startup-dreamer startup-dreamer Mar 26, 2025

Choose a reason for hiding this comment

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

But I think I have to give more time to understand this properly.

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Mar 21, 2025
@startup-dreamer startup-dreamer force-pushed the feat/add_advertise_false_custody_group_count_cli_for_peerdas_testing branch from 7e76079 to a111b56 Compare March 22, 2025 22:08
@@ -199,8 +199,23 @@ impl<E: EthSpec> Network<E> {

// Construct the metadata
let custody_group_count = ctx.chain_spec.is_peer_das_scheduled().then(|| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the user restarts the node with a different value of the false_count flag, will we bump the seq_number of the ENR?

Co-authored-by: Lion - dapplion <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
das Data Availability Sampling 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