From 027d2547cdf379661abb83e44ce7a84f793c52ee Mon Sep 17 00:00:00 2001 From: Zahary Karadjov Date: Wed, 2 Dec 2020 20:15:36 +0200 Subject: [PATCH] Fix a block proposal issue caused by incorrect merkle proofs The key change here is that `addChunksAndGenMerkleProofs` is called with all pending deposits instead of just the deposits included in the block. The later was effectively producing merkle proofs against a different root. --- beacon_chain/eth1_monitor.nim | 27 ++++++++++++++------------- beacon_chain/merkle_minimal.nim | 4 +++- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/beacon_chain/eth1_monitor.nim b/beacon_chain/eth1_monitor.nim index 4634197ff..16adb2313 100644 --- a/beacon_chain/eth1_monitor.nim +++ b/beacon_chain/eth1_monitor.nim @@ -455,7 +455,7 @@ proc advanceMerkleizer(eth1Chain: Eth1Chain, return merkleizer.getChunkCount == depositIndex -proc getDepositsRange(eth1Chain: Eth1Chain, first, last: uint64): seq[Deposit] = +proc getDepositsRange(eth1Chain: Eth1Chain, first, last: uint64): seq[DepositData] = # TODO It's possible to make this faster by performing binary search that # will locate the blocks holding the `first` and `last` indices. # TODO There is an assumption here that the requested range will be present @@ -472,7 +472,7 @@ proc getDepositsRange(eth1Chain: Eth1Chain, first, last: uint64): seq[Deposit] = for i in 0 ..< blk.deposits.lenu64: let globalIdx = firstDepositIdxInBlk + i if globalIdx >= first and globalIdx < last: - result.add Deposit(data: blk.deposits[i]) + result.add blk.deposits[i] proc lowerBound(chain: Eth1Chain, depositCount: uint64): Eth1Block = # TODO: This can be replaced with a proper binary search in the @@ -565,21 +565,22 @@ proc getBlockProposalData*(m: Eth1Monitor, if pendingDepositsCount > 0: if hasLatestDeposits: - let totalDepositsInNewBlock = min(MAX_DEPOSITS, pendingDepositsCount) - var deposits = m.eth1Chain.getDepositsRange( - state.eth1_deposit_index, - state.eth1_deposit_index + totalDepositsInNewBlock) - let depositRoots = mapIt(deposits, hash_tree_root(it.data)) + let + totalDepositsInNewBlock = min(MAX_DEPOSITS, pendingDepositsCount) + deposits = m.eth1Chain.getDepositsRange( + state.eth1_deposit_index, + state.eth1_deposit_index + pendingDepositsCount) + depositRoots = mapIt(deposits, hash_tree_root(it)) var scratchMerkleizer = copy m.eth2FinalizedDepositsMerkleizer if m.eth1Chain.advanceMerkleizer(scratchMerkleizer, state.eth1_deposit_index): let proofs = scratchMerkleizer.addChunksAndGenMerkleProofs(depositRoots) - for i in 0 ..< deposits.lenu64: - deposits[i].proof[0..31] = proofs.getProof(i.int) - deposits[i].proof[32].data[0..7] = - toBytesLE uint64(state.eth1_deposit_index + i + 1) - - swap(result.deposits, deposits) + for i in 0 ..< totalDepositsInNewBlock: + var proof: array[33, Eth2Digest] + proof[0..31] = proofs.getProof(i.int) + proof[32] = default(Eth2Digest) + proof[32].data[0..7] = toBytesLE uint64(result.vote.deposit_count) + result.deposits.add Deposit(data: deposits[i], proof: proof) else: error "The Eth1 chain is in inconsistent state" # This should not really happen result.hasMissingDeposits = true diff --git a/beacon_chain/merkle_minimal.nim b/beacon_chain/merkle_minimal.nim index f9e27a3c2..794fe6acb 100644 --- a/beacon_chain/merkle_minimal.nim +++ b/beacon_chain/merkle_minimal.nim @@ -34,7 +34,9 @@ func attachMerkleProofs*(deposits: var openArray[Deposit]) = template getProof*(proofs: seq[Eth2Digest], idxParam: int): openArray[Eth2Digest] = let idx = idxParam - startIdx = idx * DEPOSIT_CONTRACT_TREE_DEPTH + ## TODO: It's surprising that we have to do +1 here. + ## It seems that `depositContractLimit` is set too high. + startIdx = idx * (DEPOSIT_CONTRACT_TREE_DEPTH + 1) endIdx = startIdx + DEPOSIT_CONTRACT_TREE_DEPTH - 1 proofs.toOpenArray(startIdx, endIdx)