dynamic sync committee subscriptions #3308

tersec · 2022-01-21T18:45:51Z

No description provided.

github-actions · 2022-01-21T19:08:35Z

Unit Test Results

    12 files ±0   790 suites ±0 39m 54s ⏱️ - 1m 11s
1 615 tests +1 1 567 ✔️ +1   48 💤 ±0 0 ❌ ±0
9 509 runs +4 9 405 ✔️ +4 104 💤 ±0 0 ❌ ±0

Results for commit b0a2d23. ± Comparison against base commit 61342c2.

♻️ This comment has been updated with latest results.

beacon_chain/nimbus_beacon_node.nim

beacon_chain/spec/network.nim

beacon_chain/nimbus_beacon_node.nim

zah · 2022-01-22T04:46:45Z

beacon_chain/nimbus_beacon_node.nim

+    epochToSyncPeriod = nearSyncCommitteePeriod(epoch)
+
+  if  epochToSyncPeriod.isNone or
+      forkVersionAtEpoch(node.dag.cfg, epoch + 1) ==


Shouldn't the check here use epoch + epochToSyncPeriod.get? Otherwise, if we are in the epochs just before the Altair branch and epochToSyncPeriod was set to 3 for example, we would still not not subscribe (because epoch + 1 is still not an Altair epoch).

beacon_chain/consensus_object_pools/sync_committee_msg_pool.nim

zah · 2022-01-22T05:00:08Z

beacon_chain/nimbus_beacon_node.nim

+        node.network.subscribe(getSyncCommitteeTopic(
+          forkDigests[gossipFork], subcommitteeIdx), basicParams)
+
+  node.network.updateSyncnetsMetadata(nextSyncCommitteeSubnets)


We didn't necessarily connect to all of the nextSyncCommitteeSubnets here though, so this update seems incorrect (due to the randomness in isEpochLeadTime).

beacon_chain/spec/network.nim

…come Co-authored-by: zah <[email protected]>

… lookahead; enforce exception/effect tracking

arnetheduck · 2022-01-22T11:36:22Z

This code needs to be restructured and moved to ActionTracker where it should work similar to attestation duties - this is necessary to support duties coming from the REST API which otherwise will be broken by this approach - so far, the rest api discards sync duties knowing that all subnets are subscribed regardless:

nimbus-eth2/beacon_chain/rpc/rest_validator_api.nim

Line 604 in c7e92bf

return RestApiResponse.jsonMsgResponse(SyncCommitteeSubscriptionSuccess)

this is a blocker, or we break 3rd-party vc:s

tersec · 2022-01-22T11:48:32Z

This code needs to be restructured and moved to ActionTracker where it should work similar to attestation duties - this is necessary to support duties coming from the REST API which otherwise will be broken by this approach - so far, the rest api discards sync duties knowing that all subnets are subscribed regardless:

nimbus-eth2/beacon_chain/rpc/rest_validator_api.nim

Line 604 in c7e92bf

return RestApiResponse.jsonMsgResponse(SyncCommitteeSubscriptionSuccess)

this is a blocker, or we break 3rd-party vc:s

It handles VCs fine:

nimbus-eth2/beacon_chain/rpc/rest_validator_api.nim

Lines 597 to 598 in 8de9457

    
           node.syncCommitteeMsgPool.syncCommitteeSubscriptions[validator_pubkey] = 
        
             item.until_epoch

nimbus-eth2/beacon_chain/nimbus_beacon_node.nim

Lines 711 to 715 in 8de9457

    
           func hasSyncPubKey(node: BeaconNode, epoch: Epoch): auto = 
        
             return func(pubkey: ValidatorPubKey): bool = 
        
               node.syncCommitteeMsgPool.syncCommitteeSubscriptions.getOrDefault( 
        
                   pubkey, GENESIS_EPOCH) >= epoch or 
        
                 pubkey in node.attachedValidators.validators

… nearSyncCommitteePeriod

arnetheduck · 2022-01-24T12:22:28Z

hm, is subscribeAllSubnets handled here? I imagine that we should also add subscribeAllSyncnets, subscribeAllAttnets and change the subscribeAllSubnets to enable both

tersec · 2022-01-24T14:59:09Z

hm, is subscribeAllSubnets handled here? I imagine that we should also add subscribeAllSyncnets, subscribeAllAttnets and change the subscribeAllSubnets to enable both

9983903

This sets enabling --subscribe-all-subnets to override the other two, which allows for an overall more reasonable tradeoff.

Having subscribe-or-not all ${FOO} subnets be different than a third, use-default state creates an actually-ternary setting which would have unintuitive behavior.

As long as one treats not specifying the options at all the same as explicitly specifying their default options, then the only other vaguely reasonable approach here (sync/attnets options override all-subnet options) means that one'd have to specify both sync/attnet and subnet to get anything. These options/switches exist in software, and could be documented, but present complexity up front without increasing expressiveness.

Furthermore, this version allows for backwards compatibility -- anyone already specifying --subscribe-all-subnets gets the status quo behavior of that option.

So, this seems like the least worst version. Alternatively, one could just not allow separately setting sync/attnets, but there are use cases where they're differently appropriate; e.g., a boot node should specify both, while someone mostly looking to boost attestation-based profit slightly might only be interested in the attnets one.

Some software uses a syntax closer to --select-suboptions:a,b where something like --select-subnets:all, --select-subnets:optionA, and --select-subnets:optionB or similar is used. This shows the relationship more implicitly, but also creates an entirely separate parsing sublanguage (which Nimbus already has in some cases) for probably not much gain.

arnetheduck · 2022-01-24T18:32:28Z

beacon_chain/nimbus_beacon_node.nim

-proc trackSyncCommitteeTopics*(node: BeaconNode) =
-  # TODO
-  discard
+proc trackCurrentSyncCommitteeTopics(node: BeaconNode, slot: Slot) {.async.} =


arnetheduck · 2022-01-24T18:32:43Z

beacon_chain/nimbus_beacon_node.nim

+
+  node.network.updateSyncnetsMetadata(currentSyncCommitteeSubnets)
+
+proc trackNextSyncCommitteeTopics(node: BeaconNode, slot: Slot) {.async.} =


dynamic sync committee subscriptions

f17f84f

zah reviewed Jan 22, 2022

View reviewed changes

tersec and others added 3 commits January 22, 2022 07:44

fast-path trivial case rather than rely on RNG with probability 1 out…

f11ffa4

…come Co-authored-by: zah <[email protected]>

use func instead of template; avoid calling async function unnecessarily

019f243

avoid unnecessary sync committee topic computation; use correct epoch…

8de9457

… lookahead; enforce exception/effect tracking

don't over-optimistically update ENR syncnets; non-looping version of…

0c24bdb

… nearSyncCommitteePeriod

allow separately setting --allow-all-{sub,att,sync}nets

9983903

arnetheduck reviewed Jan 24, 2022

View reviewed changes

arnetheduck approved these changes Jan 24, 2022

View reviewed changes

remove unnecessary async

b0a2d23

tersec merged commit 00a3474 into unstable Jan 24, 2022

tersec deleted the vfK branch January 24, 2022 20:41

tersec mentioned this pull request Feb 4, 2022

Implement dynamic sync committee subnet walking #3084

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dynamic sync committee subscriptions #3308

dynamic sync committee subscriptions #3308

tersec commented Jan 21, 2022

github-actions bot commented Jan 21, 2022 •

edited

Loading

zah Jan 22, 2022

tersec Jan 23, 2022

zah Jan 22, 2022

tersec Jan 23, 2022

arnetheduck commented Jan 22, 2022

tersec commented Jan 22, 2022

arnetheduck commented Jan 24, 2022

tersec commented Jan 24, 2022 •

edited

Loading

arnetheduck Jan 24, 2022

tersec Jan 24, 2022

arnetheduck Jan 24, 2022

tersec Jan 24, 2022


		node.network.updateSyncnetsMetadata(currentSyncCommitteeSubnets)

		proc trackNextSyncCommitteeTopics(node: BeaconNode, slot: Slot) {.async.} =

dynamic sync committee subscriptions #3308

dynamic sync committee subscriptions #3308

Conversation

tersec commented Jan 21, 2022

github-actions bot commented Jan 21, 2022 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnetheduck commented Jan 22, 2022

tersec commented Jan 22, 2022

arnetheduck commented Jan 24, 2022

tersec commented Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 21, 2022 •

edited

Loading

tersec commented Jan 24, 2022 •

edited

Loading