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

Regular Sync Inserts PostChainStart Deposits Again Into the State #2199

Closed
rauljordan opened this issue Apr 7, 2019 · 5 comments · Fixed by #2232
Closed

Regular Sync Inserts PostChainStart Deposits Again Into the State #2199

rauljordan opened this issue Apr 7, 2019 · 5 comments · Fixed by #2232
Labels
Bug Something isn't working Priority: Critical Highest, immediate priority item

Comments

@rauljordan
Copy link
Contributor

This is part of #1586. We have a major bug in which after restarting a node, regular sync processes previous post-chainstart deposits again into the state, leading validators to double their money. We should not allow this to be the case.

@rauljordan rauljordan added Bug Something isn't working Priority: Critical Highest, immediate priority item labels Apr 7, 2019
@prestonvanloon
Copy link
Member

This happens consistently under the following conditions:

  • Some post chainstart validators have their deposits included in blocks, but they are not yet activated
  • In a multi-node scenario (possibly single node as well)

@prestonvanloon
Copy link
Member

i think there are three (or more) problems here:

  • pending deposits are not properly pruned after sync startup
  • pending deposits are not verified not to have been already included in the blockchain when requesting via RPC
  • pending deposits are not verified not to have been already included in the blockchain when processing a block with new deposits

@prestonvanloon
Copy link
Member

prestonvanloon commented Apr 10, 2019

I recommend solving from the bottom up. So in this order:

  • reject blocks that have duplicate deposits (deposits included in a block that have already been included)
  • prevent serving duplicate deposits via RPC
  • properly prune pending deposits on sync

@prestonvanloon
Copy link
Member

prestonvanloon commented Apr 11, 2019

We should include the spec change from PR ethereum/consensus-specs#594 & PR ethereum/consensus-specs#709 to solve the missing precondition for including a new validator deposit

This change has a deposit index that must be equal to the state.deposit_index.

https://github.com/ethereum/eth2.0-specs/blob/dev/specs/core/0_beacon-chain.md#process_deposit

def process_deposit(state: BeaconState, deposit: Deposit) -> None:
    """
    Process a deposit from Ethereum 1.0.
    Note that this function mutates ``state``.
    """
    # Deposits must be processed in order
    assert deposit.index == state.deposit_index

    ...

@prestonvanloon
Copy link
Member

prestonvanloon commented Apr 11, 2019

Pending deposits can be dropped on the following condition:

  • deposit.index < finalized_state.deposit_index

Pending deposits can only be served via RPC on the following condition:

  • deposit.index >= current_state.deposit_index

New blocks can be considered valid with deposits only if deposits are processed in order where:

  • deposit.index == state.deposit_index

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Priority: Critical Highest, immediate priority item
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants