Skip to content

Comments

fix: do not always subscribe to all column subnets#7607

Closed
twoeths wants to merge 1 commit intopeerDASfrom
te/peerDAS_do_not_subscribe_to_all_subnets
Closed

fix: do not always subscribe to all column subnets#7607
twoeths wants to merge 1 commit intopeerDASfrom
te/peerDAS_do_not_subscribe_to_all_subnets

Conversation

@twoeths
Copy link
Contributor

@twoeths twoeths commented Mar 20, 2025

Motivation

  • right now we always subscribe to all column subnets which is not necessary because we only need columns that we custody/sample
  • that's why I see 128 logs like this per slot, and this could degrade performance. We only need 8 per slot
 debug: Received gossip dataColumn slot=198378, root=0x9945…49af, curentSlot=198378, peerId=16Uiu2HAm2J4ZRFnf7qWvu8VBRFT66E5XFWnRTwYTahB2oRJom5ji, delaySec=1.5910000801086426, gossipIndex=113, columnIndex=113, pending=data_column, haveColumns=31, expectedColumns=8, recvToValLatency=0.0009999275207519531, recvToValidation=0.0009999275207519531, validationTime=0

Description

  • only subscribe to custody/sampling subnets

@twoeths twoeths requested a review from a team as a code owner March 20, 2025 03:14
Copy link
Member

@matthewkeil matthewkeil left a comment

Choose a reason for hiding this comment

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

This is great!!! We were just talking about this actually on the call with the Base contributor. A couple things jump out at me that we may need to consider.

For validator custody the number that we custody/sample will be variable so we should probably use a centralized source of truth for "what count we use." There is an object called the custodyConfig that is built at startup time and that we likely could update as part of the validator custody scope. Could we pass that into the gossip sub and read the value from that instead of recalculating it?

The second is block publishing. We will need to publish all 128 columns when we produce a block so we would need to figure out how to subscribe and unsubscribe during the epoch that we have publish duties.

This is a great idea though!!

@twoeths
Copy link
Contributor Author

twoeths commented Mar 20, 2025

The second is block publishing. We will need to publish all 128 columns when we produce a block so we would need to figure out how to subscribe and unsubscribe during the epoch that we have publish duties.

to publish it's about to find peers on a topic, not about subscribe to that topic. That's a blocker for this work, will look into that part.

@twoeths twoeths marked this pull request as draft March 20, 2025 07:05
matthewkeil pushed a commit that referenced this pull request Apr 7, 2025
**Motivation**

- we want to have a single CustodyConfig as a preparation for validator
custody config work see
#7607 (review)
- but since we need data on both, I designed to have CustodyConfig
created on BeaconChain (main thread) and NetworkCore (network thread)
- when we have a change on number of connected validators, need to
update both

**Description**

- on main thread store CustodyConfig on BeaconChain
- on network thread, create a wrapped NetworkGlobal to store
CustodyConfig and node id there. In the future we can consider storing
more data there
- add more data to CustodyConfig: sampleGroups, sampledSubnets
- use that CustodyConfig everywhere

---------

Co-authored-by: Tuyen Nguyen <twoeths@users.noreply.github.com>
@dguenther dguenther mentioned this pull request May 2, 2025
25 tasks
@twoeths
Copy link
Contributor Author

twoeths commented May 22, 2025

closing in favor of #7860

@twoeths twoeths closed this May 22, 2025
@twoeths twoeths deleted the te/peerDAS_do_not_subscribe_to_all_subnets branch May 29, 2025 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants