config: load networking overrides from RuntimeConfig - #8943
config: load networking overrides from RuntimeConfig#8943SyedMuhamadYasir wants to merge 3 commits into
Conversation
| iterator compute_subscribed_subnets*( | ||
| node_id: UInt256, epoch: Epoch, | ||
| epochsPerSubnetSubscription: uint64 = EPOCHS_PER_SUBNET_SUBSCRIPTION, | ||
| subnetsPerNode: uint64 = SUBNETS_PER_NODE): SubnetId = |
There was a problem hiding this comment.
Having epochsPerSubnetSubscription and subnetsPerNode be optional parameters isn't really correct given the premise of this PR.
Also, epochsPerSubnetSubscription is only used in UInt256 form, so it's better to convert once and store that UInt256 in action_tracker than a uint64 which keeps having to be converted at each use (node_id mod epochsPerSubnetSubscription.u256).
There was a problem hiding this comment.
Agreed. Updated in f1ec4bd. epochsPerSubnetSubscription and subnetsPerNode are now required parameters rather than optional defaults. ActionTracker converts the subscription period once during initialization and stores it as UInt256, so the subnet calculation consumes that directly instead of converting it on each use. I also updated the affected call sites and tests.
| test "negative epoch": | ||
| reject "3c1e98bf132530c669723f58aa3d395be0d0bfaa653152eecb04605e203bfeb500:-1000" | ||
|
|
||
|
|
| cfg.TERMINAL_BLOCK_HASH == | ||
| plainCfg.TERMINAL_BLOCK_HASH | ||
| singleQuotedCfg.TERMINAL_BLOCK_HASH == | ||
| plainCfg.TERMINAL_BLOCK_HASH |
There was a problem hiding this comment.
This does a lot of validation on TERMINAL_BLOCK_HASH which hasn't been used in years:
- remove TTD monitoring #4486
- stop calling
exchangeTransitionConfiguration#5889 - rm non-runtimeconfig NUMBER_OF_COLUMNS from local testnet settings #8589
At this point
$ rg TERMINAL_BLOCK_HASH beacon_chain/ research/ tests/
beacon_chain/spec/presets.nim
105: TERMINAL_BLOCK_HASH*: Hash32
106: TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH*: Epoch # Not actively used, but part of the spec
259: TERMINAL_BLOCK_HASH:
478: TERMINAL_BLOCK_HASH:
692: TERMINAL_BLOCK_HASH:
beacon_chain/rpc/rest_config_api.nim
178: TERMINAL_BLOCK_HASH:
179: $cfg.TERMINAL_BLOCK_HASH,
180: TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH:
181: Base10.toString(uint64(cfg.TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH)),
is all that's left, that is, it's a configuration stub. My inclination actually would be to add an explicit ignoredValues entry for something like this and get it removed from the configurations as well as the one sort-of-user, the config API which reports a value that neither Nimbus nor almost any other CL client these days can do much with.
I recently checked for other purposes and even support for running pre-Pectra has been removed from all but a couple CLs.
This is akin to the MAX_REQUEST_BLOCKS case -- from a Nimbus perspective it's a garbage value which can float freely and there's no real validation to be done.
There was a problem hiding this comment.
So there's no reason to actually "parse" this thing, really. It's just a black box string which is read in and then regurgitated for the config endpoint (and even that might be removable).
There was a problem hiding this comment.
Agreed. In f1ec4bd I dropped the generic quoted-Hash32 parsing change and its parser-specific tests. TERMINAL_BLOCK_HASH is now treated as an ignored legacy config value, so Nimbus accepts the key without parsing or validating it and retains the default runtime value. I left removal of the dormant RuntimeConfig field/config-API output out of this PR to keep the networking change focused, but I can fold that cleanup in here if you would prefer.
|
Another |
Accept quoted Hash32 configuration values and move selected behavioral networking settings to typed RuntimeConfig fields while preserving existing defaults and compile-time structural limits. Update runtime consumers and permanent tests, including the generated AllTests-mainnet inventory.
Address review feedback by making runtime subnet parameters explicit, storing the subnet subscription period in UInt256 form, treating the unused terminal block hash as a legacy ignored config value, and covering the proposer-duties v2 gossip disparity consumer.
31d9c26 to
f1ec4bd
Compare
Summary
Make a small set of networking parameters available through Nimbus's existing typed
RuntimeConfigpath instead of requiring compile-time overrides:EPOCHS_PER_SUBNET_SUBSCRIPTIONSUBNETS_PER_NODEMAXIMUM_GOSSIP_CLOCK_DISPARITYMAX_REQUEST_BLOCKSThis is a network-agnostic runtime-config alternative to the compile-time
intdefineapproach in #7618, which helped scope the compatibility gap. It does not add custom-network-specific branches, names, chain IDs, or magic values.TERMINAL_BLOCK_HASHis no longer consumed by Nimbus, so it is treated as an ignored legacy config value rather than adding genericHash32parsing behavior for an inert field.Design
The change keeps Nimbus's existing boundary between behavioral runtime configuration and structural/type-level limits.
Runtime consumers use direct typed
RuntimeConfigfields; there is no dynamic registry or string lookup in protocol hot paths, and stock defaults remain unchanged.In particular, modern Deneb sync continues to use compile-time
MAX_REQUEST_BLOCKS_DENEB. RuntimeMAX_REQUEST_BLOCKSis accepted, stored, and exposed through the config API; Nimbus does not consume it for sync.MAXIMUM_GOSSIP_CLOCK_DISPARITYis propagated to the relevant gossip/validation consumers, including the proposer-duties v2 path added in #8963.Subnet stability behavior uses the configured
EPOCHS_PER_SUBNET_SUBSCRIPTIONandSUBNETS_PER_NODEvalues. These values are explicit atActionTrackerinitialization; the subscription period is converted once and stored inUInt256form for the subnet calculation.Tests
Permanent tests cover:
TERMINAL_BLOCK_HASHMAX_REQUEST_BLOCKSaccepts anyuint64The generated
AllTests-mainnet.mdinventory is updated accordingly.Review validation after rebasing onto
unstableatfb921544be650d846cdccfca22b1a4b128c710fc:nimbus_beacon_nodebuild passed with-d:disableMarchNative -d:disableLTOAn earlier revision also passed the broad
make testgate with 14,302 tests, 0 failures, 0 errors.Upstream CI remains authoritative for the normal build matrix.