jeevanpillay0000
commented
4 years ago Hi, if this is still not finished, I would like to try and implement it.
I noticed that for beacon-chain/core/helpers/slot_epoch.go, @prestonvanloon had already implemented the fix, however, beacon-chain/sync/validate_aggregate_proof.go is yet to be implemented. I would follow a similar structure to the one implemented by @prestonvanloon (perhaps reuse the function he implemented if possible) and then some testing to ensure it works.
Cheers.
nisdas
banescusebi
🐞 Bug Report
Description
The following requirement in the P2P specification can be violated: "The block is not from a future slot (with a `MAXIMUM_GOSSIP_CLOCK_DISPARITY` allowance) -- i.e. validate that `signed_beacon_block.message.slot <= current_slot`" There is an edge case scenario where this condition would be satisfied for a time that is actually in the future. The following is a code snippet from the `VerifySlotTime` function in `beacon-chain/core/helpers/slot_epoch.go`: ``` slotTime := 1000 * (genesisTime + slot*params.BeaconConfig().SecondsPerSlot) currentTime := 1000 * uint64(roughtime.Now().Unix()) tolerance := uint64(timeTolerance.Milliseconds()) // 500ms if slotTime > currentTime+tolerance { … } ``` Let’s say that `slotTime` is 5000s, and actual time represented by `roughtime.Now()` is 5900 ms. However, since `roughtime.Now().Unix()` returns time in seconds - that is, it returns 5s, and `currentTime` ends up being 5000 (same as `slotTime`). However, an actual current time is 5900s, which is 900ms higher than `slotTime`, but 900ms is bigger than 500ms. Therefore, the time difference is actually higher than `MAXIMUM_GOSSIP_CLOCK_DISPARITY`. The same pattern is used in the `validateAggregateAttTime` function inside `beacon-chain/sync/validate_aggregate_proof.go` to verify if the attestation slot is within the last `ATTESTATION_PROPAGATION_SLOT_RANGE` slots. ### Recommended fix/mitigation: If tolerance is less than a second, it makes more sense to do time computations in milliseconds directly, rather than using seconds and then multiplying by 1000. Use [https://golang.org/pkg/time/#Time.UnixNano](https://golang.org/pkg/time/#Time.UnixNano) instead of just `.Unix()`. Same applies to validating other things, e.g., attestations.