Skip to content

Remove TARGET_NUMBER_OF_PEERS config variable #3852

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

Closed
dapplion wants to merge 2 commits into from
Closed

Remove TARGET_NUMBER_OF_PEERS config variable #3852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf28e72
Remove TARGET_NUMBER_OF_PEERS config variable
dapplion Jul 23, 2024
d439251
Update specs/_features/eip7594/das-core.md
dapplion Jul 24, 2024
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
1 change: 0 additions & 1 deletion configs/mainnet.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,6 @@ DATA_COLUMN_SIDECAR_SUBNET_COUNT: 32
MAX_REQUEST_DATA_COLUMN_SIDECARS: 16384
SAMPLES_PER_SLOT: 8
CUSTODY_REQUIREMENT: 1
TARGET_NUMBER_OF_PEERS: 70

# [New in Electra:EIP7251]
MIN_PER_EPOCH_CHURN_LIMIT_ELECTRA: 128000000000 # 2**7 * 10**9 (= 128,000,000,000)
Expand Down
1 change: 0 additions & 1 deletion configs/minimal.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ DATA_COLUMN_SIDECAR_SUBNET_COUNT: 32
MAX_REQUEST_DATA_COLUMN_SIDECARS: 16384
SAMPLES_PER_SLOT: 8
CUSTODY_REQUIREMENT: 1
TARGET_NUMBER_OF_PEERS: 70

# [New in Electra:EIP7251]
MIN_PER_EPOCH_CHURN_LIMIT_ELECTRA: 64000000000 # 2**6 * 10**9 (= 64,000,000,000)
Expand Down
5 changes: 1 addition & 4 deletions specs/_features/eip7594/das-core.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ The following values are (non-configurable) constants used throughout the specif
| - | - | - |
| `SAMPLES_PER_SLOT` | `8` | Number of `DataColumnSidecar` random samples a node queries per slot |
| `CUSTODY_REQUIREMENT` | `1` | Minimum number of subnets an honest node custodies and serves samples from |
| `TARGET_NUMBER_OF_PEERS` | `70` | Suggested minimum peer count |

### Containers

Expand Down Expand Up @@ -289,9 +288,7 @@ The particular columns that a node custodies are selected pseudo-randomly as a f

At each slot, a node needs to be able to readily sample from *any* set of columns. To this end, a node SHOULD find and maintain a set of diverse and reliable peers that can regularly satisfy their sampling demands.

A node runs a background peer discovery process, maintaining at least `TARGET_NUMBER_OF_PEERS` of various custody distributions (both `custody_size` and column assignments). The combination of advertised `custody_size` size and public node-id make this readily and publicly accessible.

`TARGET_NUMBER_OF_PEERS` should be tuned upward in the event of failed sampling.
A node runs a background peer discovery process, maintaining at least 70 peers of various custody distributions (both `custody_size` and column assignments). The combination of advertised `custody_size` size and public node-id make this readily and publicly accessible. A node may increase its peer count in the event of failed sampling.
Copy link
Copy Markdown
Member

@haxxpop haxxpop Jul 24, 2024

Choose a reason for hiding this comment

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

I think there is really no reason to put the number at all. How about just specifying that you should have enough peers to cover all the columns?

Copy link
Copy Markdown
Member Author

@dapplion dapplion Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay to me to not recommend a number at all, but would like to get the thumbs up of researchers before. The purpose of this PR is only to remove the config variable, not the number itself.

Copy link
Copy Markdown
Member

@jtraglia jtraglia Jul 25, 2024

Choose a reason for hiding this comment

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

I think removing it makes sense. We should wait for @fradamt to comment.

Copy link
Copy Markdown
Contributor

@fradamt fradamt Aug 5, 2024

Choose a reason for hiding this comment

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

Removing the config variable sounds good. As for the rest, I would maybe lean towards having a paragraph somewhere which gives some indications on how to think about the number of peers in relation to sampling, rather than giving a specific number. For example in the spirit of this discussion

Copy link
Copy Markdown
Member

@haxxpop haxxpop Aug 6, 2024

Choose a reason for hiding this comment

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

However, I think it's okay to make another PR to remove this number and rephrase this paragraph. We can merge it as it is.


*Note*: while high-capacity and super-full nodes are high value with respect to satisfying sampling requirements, a node SHOULD maintain a distribution across node capacities as to not centralize the p2p graph too much (in the extreme becomes hub/spoke) and to distribute sampling load better across all nodes.

Expand Down