Align eth1data Majority Vote with the spec #7200
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7200 +/- ##
==========================================
- Coverage 60.07% 59.58% -0.49%
==========================================
Files 323 418 +95
Lines 27422 30091 +2669
==========================================
+ Hits 16473 17930 +1457
- Misses 8733 9239 +506
- Partials 2216 2922 +706 |
# Conflicts: # beacon-chain/rpc/validator/proposer_test.go
| var chosenTime uint64 | ||
| var chosenNumber *big.Int | ||
| for t, num := range m.BlockNumberByTime { | ||
| if chosenNumber == nil || (t > chosenTime && t <= time) { |
There was a problem hiding this comment.
| if chosenNumber == nil || (t > chosenTime && t <= time) { | |
| if chosenNumber == nil || (t > chosenTime && t <= time) { |
chosenTime is always 0, so the condition t > chosenTime is almost always true( except when t = 0), the time retrieved from this is not accurate.
There was a problem hiding this comment.
I actually had another bug here. The check chosenNumber == nil caused non-deterministic test results. At the beginning of the loop this condition is always true, but there is no guaranteed order for map retrieval, so a block with incorrect time sometimes got assigned to chosenNumber.
| // If latest block is in range and it doesn't undo deposits, then choose that block. | ||
| // Otherwise take the current eth1data. | ||
| currentETH1Data := vs.HeadFetcher.HeadETH1Data() | ||
| if timeOfLastBlockByLatestValidTime >= earliestValidTime { |
There was a problem hiding this comment.
Isn't this already a given ?
earliestValidTime := votingPeriodStartTime - 2*params.BeaconConfig().SecondsPerETH1Block*uint64(eth1FollowDistance)
latestValidTime := votingPeriodStartTime - params.BeaconConfig().SecondsPerETH1Block*uint64(eth1FollowDistance)Unless the assumption is timeOfLastBlockByLatestValidTime untrusted, then the check should be shifted above when we first retrieve it in BlockNumberByTimestamp
What type of PR is this?
Algorithm re-implementation
What does this PR do? Why is it needed?
Original implementation of the eth1data Voting with the Majority algorithm was based on our design doc. One Discord user pointed out a difference between this implementation and the spec. After looking closer at the spec, it turns out that there were other discrepancies between the spec and the code.
This PR aims to align our implementation of the algorithm with the spec. I added several new tests to make sure all potencial cases are covered.
Which issues(s) does this PR fix?
N/A
Other notes for review
I needed to modify the POW chain mock because the naive implementation would not work for some test cases.