Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Align eth1data Majority Vote with the spec #7200

Merged
merged 46 commits into from
Sep 18, 2020
Merged
Changes from 22 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
144df31
align voting with the spec
rkapka Sep 9, 2020
cc7291d
Merge branch 'master' into eth1vote-spec-align
rkapka Sep 9, 2020
be196f9
remove redundant else statements
rkapka Sep 9, 2020
d8673e2
add comment to exported variable
rkapka Sep 9, 2020
05f8636
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 9, 2020
a630b17
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 9, 2020
dccdcb9
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 10, 2020
245b486
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 10, 2020
ca93bf5
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 10, 2020
0b48fcb
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 10, 2020
65fb301
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 10, 2020
8857cae
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 10, 2020
3112536
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 10, 2020
06468dd
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 11, 2020
1ba0b07
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 11, 2020
0f0201c
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 11, 2020
01f4fb0
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 13, 2020
c25d200
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 14, 2020
6d42b7a
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 14, 2020
93d22e7
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 14, 2020
add26cc
Merge branch 'master' into eth1vote-spec-align
rkapka Sep 15, 2020
069f0c1
refactor tests
rkapka Sep 15, 2020
e8e5546
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 15, 2020
f773bff
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 15, 2020
49d6980
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 16, 2020
bed040d
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 16, 2020
5665953
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 16, 2020
de6370a
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 16, 2020
5b4efef
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 16, 2020
fe30f01
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 16, 2020
75278db
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 16, 2020
b233e32
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 16, 2020
63721c7
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 17, 2020
4d077d4
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 17, 2020
ac8b47e
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 17, 2020
273276c
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 17, 2020
79b2469
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 17, 2020
da7eabb
fix mock POWChain
rkapka Sep 17, 2020
e787e4c
move last block's time check and add more test cases
rkapka Sep 17, 2020
6f2e887
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 17, 2020
a1280cb
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 17, 2020
49d2cd5
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 17, 2020
e314009
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 17, 2020
07209c6
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 17, 2020
c8456f1
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 18, 2020
adf9d70
Merge refs/heads/master into eth1vote-spec-align
prylabs-bulldozer[bot] Sep 18, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions beacon-chain/powchain/testing/mock.go
Original file line number Diff line number Diff line change
@@ -31,13 +31,25 @@ type POWChain struct {
GenesisEth1Block *big.Int
}

// GenesisTime represents a static past date - JAN 01 2000.
var GenesisTime = time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC).Unix()

// NewPOWChain creates a new mock chain with empty block info.
func NewPOWChain() *POWChain {
return &POWChain{
HashesByHeight: make(map[int][]byte),
TimesByHeight: make(map[int]uint64),
BlockNumberByTime: make(map[uint64]*big.Int),
}
}

// Eth2GenesisPowchainInfo --
func (m *POWChain) Eth2GenesisPowchainInfo() (uint64, *big.Int) {
blk := m.GenesisEth1Block
if blk == nil {
blk = big.NewInt(0)
blk = big.NewInt(GenesisTime)
}
return uint64(time.Unix(0, 0).Unix()), blk
return uint64(GenesisTime), blk
}

// DepositTrie --
@@ -78,7 +90,14 @@ func (m *POWChain) BlockTimeByHeight(_ context.Context, height *big.Int) (uint64

// BlockNumberByTimestamp --
func (m *POWChain) BlockNumberByTimestamp(_ context.Context, time uint64) (*big.Int, error) {
return m.BlockNumberByTime[time], nil
var chosenTime uint64
var chosenNumber *big.Int
for t, num := range m.BlockNumberByTime {
if chosenNumber == nil || (t > chosenTime && t <= time) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

chosenNumber = num
}
}
return chosenNumber, nil
}

// DepositRoot --
@@ -132,3 +151,11 @@ func (r *RPCClient) BatchCall(b []rpc.BatchElem) error {
}
return nil
}

// InsertBlock adds provided block info into the chain.
func (m *POWChain) InsertBlock(height int, time uint64, hash []byte) *POWChain {
m.HashesByHeight[height] = hash
m.TimesByHeight[height] = time
m.BlockNumberByTime[time] = big.NewInt(int64(height))
return m
}
106 changes: 62 additions & 44 deletions beacon-chain/rpc/validator/proposer.go
Original file line number Diff line number Diff line change
@@ -221,22 +221,25 @@ func (vs *Server) eth1Data(ctx context.Context, slot uint64) (*ethpb.Eth1Data, e
return eth1Data, nil
}

// eth1DataMajorityVote determines the appropriate eth1data for a block proposal using an extended
// simple voting algorithm - voting with the majority. The algorithm for this method is as follows:
// - Determine the timestamp for the start slot for the current eth1 voting period.
// - Determine the timestamp for the start slot for the previous eth1 voting period.
// - Determine the most recent eth1 block before each timestamp.
// - Subtract the current period's eth1block.number by ETH1_FOLLOW_DISTANCE to determine the voting upper bound.
// - Subtract the previous period's eth1block.number by ETH1_FOLLOW_DISTANCE to determine the voting lower bound.
// eth1DataMajorityVote determines the appropriate eth1data for a block proposal using
// an algorithm called Voting with the Majority. The algorithm works as follows:
// - Determine the timestamp for the start slot for the eth1 voting period.
// - Determine the earliest and latest timestamps that a valid block can have.
// - Determine the first block not before the earliest timestamp. This block is the lower bound.
// - Determine the last block not after the latest timestamp. This block is the upper bound.
// - Filter out votes on unknown blocks and blocks which are outside of the range determined by the lower and upper bounds.
// - If no blocks are left after filtering, use the current period's most recent eth1 block for proposal.
// - Determine the vote with the highest count. Prefer the vote with the highest eth1 block height in the event of a tie.
// - This vote's block is the eth1block to use for the block proposal.
// - If no blocks are left after filtering votes:
// - Use eth1data from the latest valid block.
// - If there are no valid blocks, use the current eth1data from the beacon state.
// - Otherwise:
// - Determine the vote with the highest count. Prefer the vote with the highest eth1 block height in the event of a tie.
// - This vote's block is the eth1 block to use for the block proposal.
func (vs *Server) eth1DataMajorityVote(ctx context.Context, beaconState *stateTrie.BeaconState) (*ethpb.Eth1Data, error) {
ctx, cancel := context.WithTimeout(ctx, eth1dataTimeout)
defer cancel()

slot := beaconState.Slot()
votingPeriodStartTime := vs.slotStartTime(slot)

if vs.MockEth1Votes {
return vs.mockETH1DataVote(ctx, slot)
@@ -246,51 +249,66 @@ func (vs *Server) eth1DataMajorityVote(ctx context.Context, beaconState *stateTr
}
eth1DataNotification = false

slotsPerVotingPeriod := params.BeaconConfig().EpochsPerEth1VotingPeriod * params.BeaconConfig().SlotsPerEpoch
currentPeriodVotingStartTime := vs.slotStartTime(slot)
// Can't use slotStartTime function because slot would be negative in the initial voting period.
previousPeriodVotingStartTime := currentPeriodVotingStartTime -
slotsPerVotingPeriod*params.BeaconConfig().SecondsPerSlot
eth1FollowDistance := int64(params.BeaconConfig().Eth1FollowDistance)
earliestValidTime := votingPeriodStartTime - 2*params.BeaconConfig().SecondsPerETH1Block*uint64(eth1FollowDistance)
latestValidTime := votingPeriodStartTime - params.BeaconConfig().SecondsPerETH1Block*uint64(eth1FollowDistance)

currentPeriodBlockNumber, err := vs.Eth1BlockFetcher.BlockNumberByTimestamp(ctx, currentPeriodVotingStartTime)
lastBlockByEarliestValidTime, err := vs.Eth1BlockFetcher.BlockNumberByTimestamp(ctx, earliestValidTime)
if err != nil {
log.WithError(err).Error("Failed to get block number for current voting period")
log.WithError(err).Error("Failed to get last block by earliest valid time")
return vs.randomETH1DataVote(ctx)
}
previousPeriodBlockNumber, err := vs.Eth1BlockFetcher.BlockNumberByTimestamp(ctx, previousPeriodVotingStartTime)
timeOfLastBlockByEarliestValidTime, err := vs.Eth1BlockFetcher.BlockTimeByHeight(ctx, lastBlockByEarliestValidTime)
if err != nil {
log.WithError(err).Error("Failed to get block number for previous voting period")
log.WithError(err).Error("Failed to get time of last block by earliest valid time")
return vs.randomETH1DataVote(ctx)
}
eth1FollowDistance := int64(params.BeaconConfig().Eth1FollowDistance)
currentPeriodInitialBlock := big.NewInt(0).Sub(currentPeriodBlockNumber, big.NewInt(eth1FollowDistance))
previousPeriodInitialBlock := big.NewInt(0).Sub(previousPeriodBlockNumber, big.NewInt(eth1FollowDistance))
// Increment the earliest block if the original block's time is before valid time.
// This is very likely to happen because BlockTimeByHeight returns the last block AT OR BEFORE the specified time.
if timeOfLastBlockByEarliestValidTime < earliestValidTime {
lastBlockByEarliestValidTime = big.NewInt(0).Add(lastBlockByEarliestValidTime, big.NewInt(1))
}

currentDepositCount, _ := vs.DepositFetcher.DepositsNumberAndRootAtHeight(ctx, currentPeriodInitialBlock)
if currentDepositCount == 0 {
return vs.ChainStartFetcher.ChainStartEth1Data(), nil
lastBlockByLatestValidTime, err := vs.Eth1BlockFetcher.BlockNumberByTimestamp(ctx, latestValidTime)
if err != nil {
log.WithError(err).Error("Failed to get last block by latest valid time")
return vs.randomETH1DataVote(ctx)
}
timeOfLastBlockByLatestValidTime, err := vs.Eth1BlockFetcher.BlockTimeByHeight(ctx, lastBlockByLatestValidTime)
if err != nil {
log.WithError(err).Error("Failed to get time of last block by latest valid time")
return vs.randomETH1DataVote(ctx)
}

if len(beaconState.Eth1DataVotes()) == 0 {
eth1Data, err := vs.defaultEth1DataResponse(ctx, currentPeriodBlockNumber)
if err != nil {
log.WithError(err).Error("Failed to get eth1 data from current period block number")
return vs.randomETH1DataVote(ctx)
}
return eth1Data, nil
lastBlockDepositCount, lastBlockDepositRoot := vs.DepositFetcher.DepositsNumberAndRootAtHeight(ctx, lastBlockByLatestValidTime)
if lastBlockDepositCount == 0 {
return vs.ChainStartFetcher.ChainStartEth1Data(), nil
}

inRangeVotes, err := vs.inRangeVotes(ctx, beaconState, currentPeriodInitialBlock, previousPeriodInitialBlock)
inRangeVotes, err := vs.inRangeVotes(ctx, beaconState, lastBlockByEarliestValidTime, lastBlockByLatestValidTime)
if err != nil {
return nil, err
}
if len(inRangeVotes) == 0 {
eth1Data, err := vs.defaultEth1DataResponse(ctx, currentPeriodBlockNumber)
if err != nil {
log.WithError(err).Error("Failed to get eth1 data from current period block number")
return vs.randomETH1DataVote(ctx)
// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if lastBlockDepositCount >= currentETH1Data.DepositCount {
hash, err := vs.Eth1BlockFetcher.BlockHashByHeight(ctx, lastBlockByLatestValidTime)
if err != nil {
log.WithError(err).Error("Failed to get hash of last block by latest valid time")
return vs.randomETH1DataVote(ctx)
}
return &ethpb.Eth1Data{
BlockHash: hash.Bytes(),
DepositCount: lastBlockDepositCount,
DepositRoot: lastBlockDepositRoot[:],
}, nil
}
return vs.HeadFetcher.HeadETH1Data(), nil
}
return eth1Data, nil
return vs.HeadFetcher.HeadETH1Data(), nil
}

chosenVote := chosenEth1DataMajorityVote(inRangeVotes)
@@ -308,8 +326,8 @@ func (vs *Server) slotStartTime(slot uint64) uint64 {

func (vs *Server) inRangeVotes(ctx context.Context,
beaconState *stateTrie.BeaconState,
currentPeriodInitialBlock *big.Int,
previousPeriodInitialBlock *big.Int) ([]eth1DataSingleVote, error) {
firstValidBlockNumber *big.Int,
lastValidBlockNumber *big.Int) ([]eth1DataSingleVote, error) {

currentETH1Data := vs.HeadFetcher.HeadETH1Data()

@@ -323,10 +341,10 @@ func (vs *Server) inRangeVotes(ctx context.Context,
if eth1Data.DepositCount < currentETH1Data.DepositCount {
continue
}
// previousPeriodInitialBlock.Cmp(height) < 1 filters out all blocks before previousPeriodInitialBlock
// currentPeriodInitialBlock.Cmp(height) > -1 filters out all blocks after currentPeriodInitialBlock
// These filters result in the range [previousPeriodInitialBlock, currentPeriodInitialBlock]
if ok && previousPeriodInitialBlock.Cmp(height) < 1 && currentPeriodInitialBlock.Cmp(height) > -1 {
// firstValidBlockNumber.Cmp(height) < 1 filters out all blocks before firstValidBlockNumber
// lastValidBlockNumber.Cmp(height) > -1 filters out all blocks after lastValidBlockNumber
// These filters result in the range [firstValidBlockNumber, lastValidBlockNumber]
if ok && firstValidBlockNumber.Cmp(height) < 1 && lastValidBlockNumber.Cmp(height) > -1 {
inRangeVotes = append(inRangeVotes, eth1DataSingleVote{eth1Data: *eth1Data, blockHeight: height})
}
}
Loading