Add deposit index to beacon state #2232

0xKiwi · 2019-04-11T05:53:37Z

Resolves #2199

Currently, we have an issue that we don't properly make sure deposits aren't duplicated. The spec has recently implemented a deposit_index in the beacon state that maintains what deposit index has been processed for the sake of ensuring duplicate deposits aren't sent in.

Changes made:

Beacon State now has DepositIndex which is:

Initialized as 0.
Incremented with each deposit.
Used as the reference for pruning pending deposits.

MerkleBranchHash32S -> MerkleProofHash32S

prestonvanloon · 2019-04-11T05:55:53Z

beacon-chain/sync/initial-sync/sync_state.go

@@ -47,17 +46,7 @@ func (s *InitialSync) processState(msg p2p.Message) {
 		return
 	}

-	exists, blkNum, err := s.powchain.BlockExists(ctx, bytesutil.ToBytes32(finalizedState.LatestEth1Data.BlockHash32))


Why remove this? dont we still want to verify that the block exists?

Good catch, adding it back. My mistake.

prestonvanloon · 2019-04-11T05:57:37Z

beacon-chain/core/blocks/block_operations.go

+	// Deposits must be processed in order
+	if deposit.MerkleTreeIndex != beaconState.DepositIndex {
+		return fmt.Errorf(
+			"expected deposit merkle branch index to match beacon state deposit index, wanted: %d, received: %d",


Merkle branch index or merkle tree index?

Good catch, thank you. Changed to merkle tree index.

beacon-chain/rpc/beacon_server.go

nisdas · 2019-04-11T06:15:58Z

beacon-chain/db/pending_deposits.go

@@ -113,7 +113,7 @@ func (db *BeaconDB) PrunePendingDeposits(ctx context.Context, b *big.Int) {
 	// added deposit and works backwards.
 	idx := -1
 	for i := depositListSize - 1; i >= 0; i-- {
-		if db.pendingDeposits[i].block.Cmp(b) == -1 {
+		if db.pendingDeposits[i].deposit.MerkleTreeIndex < merkleTreeIndex {


this relies on the pending deposits being sorted by their indexes, right now they are sorted by the block in which the deposit is logged in

Good point!

nisdas · 2019-04-11T06:21:41Z

beacon-chain/core/state/state.go

@@ -146,6 +147,8 @@ func GenesisBeaconState(
 		if err != nil {
 			return nil, fmt.Errorf("could not process validator deposit: %v", err)
 		}
+
+		state.DepositIndex++


you should not increment it here, it should be in the ProcessDepositfunction

nisdas · 2019-04-11T06:32:11Z

beacon-chain/chaintest/tests/state-tests/block-processing.yaml

@@ -17,18 +17,18 @@ test_cases:
      num_slots: 64 # Testing advancing state to exactly slot == SlotsPerEpoch
      deposits:
        - slot: 9223372036854775809
-          amount: 32
-          merkle_index: 0
+          amount: 0


wait , what is going on here ? why is this 1.
the same below. We always use 32 eth

codecov · 2019-04-11T07:12:44Z

Codecov Report

Merging #2232 into master will decrease coverage by 0.7%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #2232      +/-   ##
==========================================
- Coverage   69.57%   68.86%   -0.71%     
==========================================
  Files         116      116              
  Lines        9080     8882     -198     
==========================================
- Hits         6317     6117     -200     
+ Misses       2111     2108       -3     
- Partials      652      657       +5

0xKiwi · 2019-04-11T09:46:09Z

Progress so far, I believe it's mostly done except with some bugs on the RPC side. Too late now so I'll resume tomorrow.

Also, @nisdas and I don't think the RPC should only return

deposit.index < current_state.deposit_index

Shouldn't it at least include the current deposit index, so the validators can send it?
And would the validators even need the previous deposits, doesn't it only need current and upcoming?

…fix-deposits-sync

prestonvanloon

Looks amazing. Let's try in cluster before merge.

terencechain

👍

nisdas · 2019-04-12T04:31:29Z

beacon-chain/rpc/beacon_server.go

+	var pendingDeps []*pbp2p.Deposit
+	for _, dep := range allPendingDeps {
+		if dep.MerkleTreeIndex >= beaconState.DepositIndex {
+			pendingDeps = append(pendingDeps, dep)


this isnt ordered, we want the indexes to be sorted in an ascending order.

nvm , ignore it, its sorted by the PendingDeposits function

nisdas

Great job on this, looks good to me

nisdas · 2019-04-12T04:49:23Z

beacon-chain/rpc/beacon_server.go

+	var pendingDeps []*pbp2p.Deposit
+	for _, dep := range allPendingDeps {
+		if dep.MerkleTreeIndex >= beaconState.DepositIndex {
+			pendingDeps = append(pendingDeps, dep)


nvm , ignore it, its sorted by the PendingDeposits function

…fix-deposits-sync

…v/prysm into fix-deposits-sync

rauljordan · 2019-04-13T22:11:42Z

Looks great, awesome job on this non-trivial feature!

…v/prysm into fix-deposits-sync

Begin adding Deposit Index to beacon state

78ec625

prestonvanloon reviewed Apr 11, 2019

View reviewed changes

beacon-chain/rpc/beacon_server.go Show resolved Hide resolved

nisdas reviewed Apr 11, 2019

View reviewed changes

0xKiwi added 2 commits April 11, 2019 02:58

Move deposit index increment to ProcessDeposit

9a7c2b1

Fix initial deposits bug

4a4c901

0xKiwi added 2 commits April 11, 2019 04:31

Fix bug in RPC

2bfd174

More bug fixes

7e80d71

0xKiwi added 2 commits April 11, 2019 19:27

Fix RPC bug, ensure deposits are processed in the right order

9fbb59a

Fix tests

93e7310

0xKiwi marked this pull request as ready for review April 11, 2019 23:43

0xKiwi requested review from prestonvanloon, nisdas, rauljordan and terencechain and removed request for prestonvanloon April 11, 2019 23:45

Merge branch 'master' of https://github.com/prysmaticlabs/prysm into …

7861bb6

…fix-deposits-sync

prestonvanloon previously approved these changes Apr 12, 2019

View reviewed changes

terencechain previously approved these changes Apr 12, 2019

View reviewed changes

nisdas reviewed Apr 12, 2019

View reviewed changes

nisdas previously approved these changes Apr 12, 2019

View reviewed changes

0xKiwi added 3 commits April 12, 2019 01:49

Merge branch 'master' into fix-deposits-sync

5fa340d

Merge branch 'master' of https://github.com/prysmaticlabs/prysm into …

461783b

…fix-deposits-sync

Fix merge

276b784

0xKiwi dismissed nisdas’s stale review via 276b784 April 12, 2019 15:46

0xKiwi dismissed stale reviews from terencechain and prestonvanloon via 276b784 April 12, 2019 15:46

prestonvanloon mentioned this pull request Apr 12, 2019

Validator balances are not consistent after reorgs #2229

Closed

0xKiwi and others added 3 commits April 13, 2019 00:39

Attempt to fix pruning deposits on regular sync

a240c41

Fix pending deposits on node restart

2048a58

Merge branch 'master' into fix-deposits-sync

f2d284c

rauljordan previously approved these changes Apr 13, 2019

View reviewed changes

0xKiwi added 2 commits April 13, 2019 17:49

Fix test

45835c6

Merge branch 'fix-deposits-sync' of https://github.com/IvanTheGreatDe…

1a0bb9c

…v/prysm into fix-deposits-sync

0xKiwi dismissed rauljordan’s stale review via 1a0bb9c April 13, 2019 21:50

prestonvanloon previously approved these changes Apr 13, 2019

View reviewed changes

Fix tests properly

99f0b1c

0xKiwi dismissed prestonvanloon’s stale review via 99f0b1c April 13, 2019 22:08

Merge branch 'master' into fix-deposits-sync

2edbab3

rauljordan previously approved these changes Apr 13, 2019

View reviewed changes

0xKiwi added 2 commits April 13, 2019 18:19

Gofmt

c3ec45b

Merge branch 'fix-deposits-sync' of https://github.com/IvanTheGreatDe…

62853a8

…v/prysm into fix-deposits-sync

0xKiwi dismissed rauljordan’s stale review via 62853a8 April 13, 2019 22:19

prestonvanloon previously approved these changes Apr 13, 2019

View reviewed changes

prestonvanloon and others added 3 commits April 14, 2019 08:24

Merge branch 'master' into fix-deposits-sync

eca5287

Remove unneeded logs

45e625f

Merge branch 'fix-deposits-sync' of https://github.com/IvanTheGreatDe…

9f4f8ca

…v/prysm into fix-deposits-sync

0xKiwi dismissed prestonvanloon’s stale review via 9f4f8ca April 13, 2019 22:57

Fix build

71efd8b

prestonvanloon approved these changes Apr 14, 2019

View reviewed changes

prestonvanloon merged commit 6227948 into prysmaticlabs:master Apr 14, 2019

0xKiwi deleted the fix-deposits-sync branch June 6, 2019 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deposit index to beacon state #2232

Add deposit index to beacon state #2232

0xKiwi commented Apr 11, 2019 •

edited

Loading

prestonvanloon Apr 11, 2019

0xKiwi Apr 11, 2019 •

edited

Loading

prestonvanloon Apr 11, 2019

0xKiwi Apr 11, 2019

nisdas Apr 11, 2019

0xKiwi Apr 11, 2019

nisdas Apr 11, 2019

nisdas Apr 11, 2019

codecov bot commented Apr 11, 2019 •

edited

Loading

0xKiwi commented Apr 11, 2019

prestonvanloon left a comment

terencechain left a comment

nisdas Apr 12, 2019

nisdas Apr 12, 2019

nisdas left a comment

nisdas Apr 12, 2019

rauljordan commented Apr 13, 2019

Add deposit index to beacon state #2232

Add deposit index to beacon state #2232

Conversation

0xKiwi commented Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

0xKiwi Apr 11, 2019 • edited Loading

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

codecov bot commented Apr 11, 2019 • edited Loading

Codecov Report

0xKiwi commented Apr 11, 2019

prestonvanloon left a comment

Choose a reason for hiding this comment

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

nisdas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauljordan commented Apr 13, 2019

0xKiwi commented Apr 11, 2019 •

edited

Loading

0xKiwi Apr 11, 2019 •

edited

Loading

codecov bot commented Apr 11, 2019 •

edited

Loading