make reconstruction timeout dynamic + prevent reconstruction in nonSupernodes #7628
Conversation
| # Uncategorized helper functions from the spec | ||
| import | ||
| chronos, chronicles, results, taskpools, | ||
| chronos, chronicles, results, taskpools, times, |
There was a problem hiding this comment.
times is a stdlib library. should be on a separate line and prefixed by std/:
import
std/times,
chronos, chronicles, results, taskpools,
| let startTime = Moment.now() | ||
| const reconstructionTimeout = 2.seconds | ||
| let reconstructionTimeout = | ||
| (initDuration(nanoseconds = slotDuration * 100_000_000)).inNanoseconds() |
There was a problem hiding this comment.
100_000_000 is off by a factor of 10.
More generally, initDuration supports milliseconds too, so there's no particular reason to use nanoseconds here.
inNanoSeconds just converts all this back into an int64 anyway, so this whole line is a roundbout method of multiplying by what's supposed to be 1_000_000_000 which invokes initDuration/inNanoseconds for no obvious reason.
That is, one should be able to do something like
let reconstructionTimeout = slotDuration * 1000or similar, rather than round-tripping through these std/times types. Also, as SLOT_DURATION_MS in milliseconds will come in, this will have to support milliseconds, so that's probably the correct granularity. Seconds will have insufficient granularity (already in some cases) and microseconds pointlessly large to handle, and the times library does like to operate in integers.
While the two
if (now - startTime).nanoseconds > reconstructionTimeoutlines use nanoseconds, they could use milliseconds just as easily without the evidently easy-to-typo large numbers.
| let | ||
| currentCgc = node.dataColumnQuarantine.custodyColumns.lenu64 | ||
| nonSupernode = | ||
| currentCgc > node.dag.cfg.NUMBER_OF_CUSTODY_GROUPS div 2 and |
There was a problem hiding this comment.
This boundary condition seems to create an odd hole for node.dataColumnQuarantine.custodyColumns.lenu64 == node.dag.cfg.NUMBER_OF_CUSTODY_GROUPS div 2. It won't be rejected by the earlier
if node.dataColumnQuarantine.custodyColumns.lenu64 <
node.dag.cfg.NUMBER_OF_CUSTODY_GROUPS div 2:
returnand it won't trigger nonSupernode status here (64 > 64 and 64 < 128 == false).
Maybe >= was meant? But see comment below for a slightly broader point, related to this.
| currentCgc < node.dag.cfg.NUMBER_OF_CUSTODY_GROUPS | ||
|
|
||
| if not(node.config.peerdasSupernode) or | ||
| nonSupernode: |
There was a problem hiding this comment.
>>> len(' if not(node.config.peerdasSupernode) or nonSupernode:')
55
so not really sure why it's two lines.
But, beyond the most superficial cosmetics, earlier in the function there's already a
nimbus-eth2/beacon_chain/nimbus_beacon_node.nim
Lines 1705 to 1707 in d71ea30
which is effectively part of this logic too. I'm not sure why these two chunks of custody column checking should exist separately, and it's simpler it they're combined: to achieve a similar effect, check once for
not(node.config.peerdasSupernode) or node.dataColumnQuarantine.custodyColumns.lenu64 < node.dag.cfg.NUMBER_OF_CUSTODY_GROUPS. Nothing about this function's current behavior changes between custody count of 63 and 65 (why 64? is that intentional?), it's just ruling everything under 128 out.
I'm not really sure in what circumstances node.config.peerdasSupernode could hold though and have also node.dataColumnQuarantine.custodyColumns.lenu64 < node.dag.cfg.NUMBER_OF_CUSTODY_GROUPS.
2 participants
No description provided.