Last minute cleanups #1244
Last minute cleanups #1244
Conversation
JustinDrake
added
the
milestone:June 30 freeze 🥶
| | `MIN_EPOCHS_TO_INACTIVITY_PENALTY` | `2**2` (= 4) | epochs | 25.6 minutes | | ||
|
|
||
| * `MAX_EPOCHS_PER_CROSSLINK` should be a small constant times `SHARD_COUNT // SLOTS_PER_EPOCH`. | ||
| | `MIN_ATTESTATION_INCLUSION_DELAY` | `Slot(2**0)` (= 1) | 6 seconds | |
There was a problem hiding this comment.
None of these should be typed as Slot or Epoch. "5 slots" is not actually a Slot. It's a count of something.
There was a problem hiding this comment.
Is the thing that is being counted here not a Slot though?
There was a problem hiding this comment.
Slot was called SlotNumber, as the ID to identify a certain slot; Epoch and Shard are as well.
| | `EPOCHS_PER_HISTORICAL_VECTOR` | `2**16` (= 65,536) | epochs | ~0.8 years | | ||
| | `EPOCHS_PER_SLASHINGS_VECTOR` | `2**13` (= 8,192) | epochs | ~36 days | | ||
| | `HISTORICAL_ROOTS_LIMIT` | `2**24` (= 16,777,216) | historical roots | ~26,131 years | | ||
| | `EPOCHS_PER_HISTORICAL_VECTOR` | `Epoch(2**16)` (= 65,536) | ~0.8 years | |
There was a problem hiding this comment.
I think I disagree:
- The name
EPOCHS_PER_...is consistent with theEpochtype. EPOCHS_PER_HISTORICAL_VECTORnaturally occurs inEpochsummations such asepoch + EPOCHS_PER_HISTORICAL_VECTOR - MIN_SEED_LOOKAHEAD.EPOCHS_PER_HISTORICAL_VECTORnaturally occurs inEpochmodular reductions such asepoch % EPOCHS_PER_HISTORICAL_VECTOR.
There was a problem hiding this comment.
I think in instances where there is a PER in the name, dimensional analysis dictates that they are not Epochs, but `Epochs * HistoricalVectors**(-1).
There was a problem hiding this comment.
SLOTS_PER_EPOCH is defined as "the number of slots in an epoch". That's of type Slot, not Slot/Epoch.
There was a problem hiding this comment.
Disagree. The use of SLOTS_PER_EPOCH in slot_to_epoch and in epoch_start_slot stand counter to your argument.
If SLOTS_PER_EPOCH is a Slot then epoch_start_slot's return type is Epoch*Slot and epoch_start_slot is dimensionless.
There was a problem hiding this comment.
Historical vector here doesn't represent a type, but a non-typed scalar. So you could say we are multiplying by something that doesn't change the type, only the length. Imho I think the debate is silly, and naming is consistent and clear.
There was a problem hiding this comment.
Hum, OK. Would you agree that Epochs * HistoricalVectors**(-1) is the same as Epochs because HistoricalVectors is dimensionless? 😂
There was a problem hiding this comment.
I don't care too much either way. Need to go to sleep soon as I can't think straight anymore 💤
There was a problem hiding this comment.
😂Fine. :) I am not entirely convinced, but I sort of agree enough on the dimensionlessness HistoricalVectors.
There was a problem hiding this comment.
Agreed the need for sleep is real.
protolambda
force-pushed
the
last-minute-cleanups
branch
from
b6bb725 to
ab2001e
Compare
|
Opened #1251 to get merged before the freeze. 😄 |
|
closing in favor of #1251. Sorry! we can debate this very aesthetic item tomorrow. |
#1244 without "5 slots is a `Slot`"
BLS_WITHDRAWAL_PREFIX_BYTE(should beBLS_WITHDRAWAL_PREFIX)fork_version: bytes=b'\x00' * 4(should befork_version: Version=Version())nin bothint_to_bytesandinteger_squareroot)uint64overintin a few placesBLS_WITHDRAWAL_PREFIXaBytes1JUSTIFICATION_BITS_LENGTHin the constants (as opposed to configurations)assert index_count <= 2**40(there for cryptographic security)