Implement Eth1Data RPC Server Functionality #1615

rauljordan · 2019-02-15T22:18:57Z

This is part of #1565 and resolves #1463, and is related to #1456

Description

This PR implements the Eth1Data RPC server functionality in our beacon server for proposers necessary information related to Eth1Data votes to include in their blocks. This PR uses an interface for required information from the powchain service such as CurrentHeight and BlockExists. Those functions will be implemented in a separate PR. This PR only concerns itself with the beacon-chain/rpc package to keep the scope small and easier to review.

This PR implements the logic found in the honest validator specification as of commit hash 375659dc6c92ddcbf1682a024f9afc4fb24335c6 below:

Eth1 Data

block.eth1_data is a mechanism used by block proposers vote on a recent Ethereum 1.0 block hash and an associated deposit root found in the Ethereum 1.0 deposit contract. When consensus is formed, state.latest_eth1_data is updated, and validator deposits up to this root can be processed. The deposit root can be calculated by calling the get_deposit_root() function of the deposit contract using the post-state of the block hash.

Let D be the set of Eth1DataVote objects vote in state.eth1_data_votes where:
- vote.eth1_data.block_hash is the hash of an eth1.0 block that is (i) part of the canonical chain, (ii) >= ETH1_FOLLOW_DISTANCE blocks behind the head, and (iii) newer than state.latest_eth1_data.block_data.
- vote.eth1_data.deposit_root is the deposit root of the eth1.0 deposit contract at the block defined by vote.eth1_data.block_hash.
If D is empty:
- Let block_hash be the block hash of the ETH1_FOLLOW_DISTANCE'th ancestor of the head of the canonical eth1.0 chain.
- Let deposit_root be the deposit root of the eth1.0 deposit contract in the post-state of the block referenced by block_hash
If D is nonempty:
- Let best_vote be the member of D that has the highest vote.eth1_data.vote_count, breaking ties by favoring block hashes with higher associated block height.
- Let block_hash = best_vote.eth1_data.block_hash.
- Let deposit_root = best_vote.eth1_data.deposit_root.
Set block.eth1_data = Eth1Data(deposit_root=deposit_root, block_hash=block_hash).

codecov · 2019-02-16T00:47:18Z

Codecov Report

Merging #1615 into master will decrease coverage by 0.41%.
The diff coverage is 22.22%.

@@            Coverage Diff             @@
##           master    #1615      +/-   ##
==========================================
- Coverage   71.56%   71.15%   -0.42%     
==========================================
  Files          94       94              
  Lines        6500     6562      +62     
==========================================
+ Hits         4652     4669      +17     
- Misses       1439     1482      +43     
- Partials      409      411       +2

codecov · 2019-02-16T00:47:18Z

Codecov Report

Merging #1615 into master will increase coverage by 0.34%.
The diff coverage is 63.15%.

@@            Coverage Diff             @@
##           master    #1615      +/-   ##
==========================================
+ Coverage   70.77%   71.12%   +0.34%     
==========================================
  Files          98       94       -4     
  Lines        7060     6652     -408     
==========================================
- Hits         4997     4731     -266     
+ Misses       1626     1498     -128     
+ Partials      437      423      -14

terencechain

One comment on how we get depositRoot when dataVoteObjects is empty

terencechain · 2019-02-18T21:43:35Z

beacon-chain/rpc/beacon_server.go

+	for _, vote := range beaconState.Eth1DataVotes {
+		eth1Hash := bytesutil.ToBytes32(vote.Eth1Data.BlockHash32)
+		// Verify the block from the vote's block hash exists in the eth1.0 chain and fetch its height.
+		blockExists, blockNumber, err := bs.powChainService.BlockExists(eth1Hash)


instead of blockNumber, we should try to use blockHeight so its consistent with the rest

terencechain · 2019-02-18T21:50:21Z

beacon-chain/rpc/beacon_server.go

+		// at the block defined by vote.eth1_data.block_hash.
+		isBehindFollowDistance := blockNumber.Add(blockNumber, big.NewInt(eth1FollowDistance)).Cmp(currentHeight) == -1
+		isAheadStateLatestEth1Data := blockNumber.Cmp(stateLatestEth1Height) == 1
+		if blockExists && isBehindFollowDistance && isAheadStateLatestEth1Data {


I think this whole check could use some better verbosity. If a Eth1DataVote does not get included, which condition failed and what are the numbers (how many blocks it needs to wait for it to get included.. etc)

terencechain · 2019-02-18T21:52:55Z

beacon-chain/powchain/service.go

+func (w *Web3Service) DepositRoot() [32]byte {
+	return w.depositTrie.Root()
+}
+
 // LatestBlockNumber in the ETH1.0 chain.
 func (w *Web3Service) LatestBlockNumber() *big.Int {


I think LatestBlockHeight sounds better here...

terencechain · 2019-02-18T21:55:56Z

beacon-chain/rpc/beacon_server.go

+		if err != nil {
+			return nil, fmt.Errorf("could not fetch ETH1_FOLLOW_DISTANCE ancestor: %v", err)
+		}
+		depositRoot := bs.powChainService.DepositRoot()


I don't think this is correct. We should get the depositRoot that is referenced by blockHash from ancestorHeight.

bs.powChainService.DepositRoot() just returns you the latest deposit root right?

@terenc3t there is no way to actually do that now, we dont keep a list of all the previous deposit roots. When we get a deposit log we just update the deposit trie and create a new root. Also deposit root is dependant on the merkle index of the deposit not the block number. We could have multiple deposits in a block , generating multiple deposit roots

that's fine we can't do this now but where is the comment or note saying our implementation does diverge from the spec

terencechain · 2019-02-18T22:05:15Z

beacon-chain/rpc/beacon_server.go

+	if err != nil {
+		return nil, fmt.Errorf("could not fetch beacon state: %v", err)
+	}
+	dataVoteObjects := []*pbp2p.Eth1DataVote{}


optional:
s/dataVoteObjects/voteObjects
or
s/dataVoteOBjects/dataVotes

dataVotes sounds better imo

nisdas · 2019-02-19T06:30:48Z

beacon-chain/powchain/service.go

@@ -199,6 +205,18 @@ func (w *Web3Service) LatestBlockHash() common.Hash {
 	return w.blockHash
 }

+// BlockExists --
+// Unimplemented, Work in Progress.


Can you add the issue number referencing the work here as a TODO, same as below

nisdas · 2019-02-19T06:31:32Z

beacon-chain/rpc/beacon_server.go

+	if err != nil {
+		return nil, fmt.Errorf("could not fetch beacon state: %v", err)
+	}
+	dataVoteObjects := []*pbp2p.Eth1DataVote{}


dataVotes sounds better imo

nisdas · 2019-02-19T07:22:02Z

beacon-chain/rpc/beacon_server.go

+	for _, vote := range beaconState.Eth1DataVotes {
+		eth1Hash := bytesutil.ToBytes32(vote.Eth1Data.BlockHash32)
+		// Verify the block from the vote's block hash exists in the eth1.0 chain and fetch its height.
+		blockExists, blockNumber, err := bs.powChainService.BlockExists(eth1Hash)


if blockExists == false we can skip the operations below

nisdas · 2019-02-19T07:24:13Z

beacon-chain/rpc/beacon_server.go

+		// Let dataVoteObjects be the set of Eth1DataVote objects vote in state.eth1_data_votes where:
+		// vote.eth1_data.block_hash is the hash of an eth1.0 block that is:
+		//   (i) part of the canonical chain
+		//   (ii) >= ETH1_FOLLOW_DISTANCE blocks behind the head


the code below represents > ETH1_FOLLOW_DISTANCE blocks behind the head not
>= ETH1_FOLLOW_DISTANCE blocks behind the head .

nisdas · 2019-02-19T07:28:45Z

beacon-chain/rpc/beacon_server.go

+		if err != nil {
+			return nil, fmt.Errorf("could not fetch ETH1_FOLLOW_DISTANCE ancestor: %v", err)
+		}
+		depositRoot := bs.powChainService.DepositRoot()


@terenc3t there is no way to actually do that now, we dont keep a list of all the previous deposit roots. When we get a deposit log we just update the deposit trie and create a new root. Also deposit root is dependant on the merkle index of the deposit not the block number. We could have multiple deposits in a block , generating multiple deposit roots

terencechain · 2019-02-20T15:17:07Z

@rauljordan let me know when this is ready again

rauljordan added 2 commits February 15, 2019 15:51

eth1 data comments

51d3ea5

eth1 data RPC server side impl

fee3597

rauljordan added In Progress Priority: High High priority item labels Feb 15, 2019

rauljordan added this to the Sapphire milestone Feb 15, 2019

rauljordan self-assigned this Feb 15, 2019

rauljordan added 7 commits February 15, 2019 16:55

logic is data objects is empty

b4f6652

finished eth1 server logic

70dc1b0

update comments

6fede2e

fmt

7b7eca1

big int comparisons

d34b82a

begin adding eth1 data tests

787981e

empty data votes test runs

ce1c508

rauljordan added 2 commits February 15, 2019 19:42

all tests complete with every code branch triggered, behavior correct

dbe44e6

fmt, imports

8ce79c1

rauljordan marked this pull request as ready for review February 16, 2019 01:43

rauljordan added Ready For Review and removed In Progress labels Feb 16, 2019

rauljordan requested review from prestonvanloon, nisdas and terencechain and removed request for prestonvanloon, nisdas and terencechain February 16, 2019 01:43

Merge branch 'master' into eth1server

8d81c1e

Merge branch 'master' into eth1server

36ad4a4

terencechain requested changes Feb 18, 2019

View reviewed changes

terencechain reviewed Feb 18, 2019

View reviewed changes

nisdas reviewed Feb 19, 2019

View reviewed changes

rauljordan added 2 commits February 19, 2019 16:28

address comments

ee98205

wrapped up tests

de816c9

rauljordan mentioned this pull request Feb 20, 2019

Fetch Correct Ancestor Deposit Root in ETH1 Data #1656

Closed

rauljordan added 2 commits February 20, 2019 11:37

addressed all comments

3a8c9c6

Merge branch 'master' into eth1server

434cdd5

terencechain approved these changes Feb 20, 2019

View reviewed changes

rauljordan merged commit 9bee695 into master Feb 20, 2019

nisdas deleted the eth1server branch April 20, 2019 03:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Eth1Data RPC Server Functionality #1615

Implement Eth1Data RPC Server Functionality #1615

rauljordan commented Feb 15, 2019

codecov bot commented Feb 16, 2019

codecov bot commented Feb 16, 2019 •

edited

Loading

terencechain left a comment

terencechain Feb 18, 2019

terencechain Feb 18, 2019

terencechain Feb 18, 2019

terencechain Feb 18, 2019

nisdas Feb 19, 2019

terencechain Feb 19, 2019

terencechain Feb 18, 2019

nisdas Feb 19, 2019

nisdas Feb 19, 2019

nisdas Feb 19, 2019

nisdas Feb 19, 2019

nisdas Feb 19, 2019

nisdas Feb 19, 2019

terencechain commented Feb 20, 2019

Implement Eth1Data RPC Server Functionality #1615

Implement Eth1Data RPC Server Functionality #1615

Conversation

rauljordan commented Feb 15, 2019

Description

Eth1 Data

codecov bot commented Feb 16, 2019

Codecov Report

codecov bot commented Feb 16, 2019 • edited Loading

Codecov Report

terencechain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain commented Feb 20, 2019

codecov bot commented Feb 16, 2019 •

edited

Loading