Hard Forks Could Render Deposits Invalid

[nisdas]

Following on #3849, @paulhauner brought up an interesting case where in the case of a changed fork version in the case of hardforks, this would lead to valid deposits being rejected. Due to failing signature verification.

prysm/beacon-chain/core/blocks/block_operations.go

Lines 966 to 972 in 9bf55e5

	domain := helpers.Domain(beaconState.Fork, helpers.CurrentEpoch(beaconState), params.BeaconConfig().DomainDeposit)
	depositSig := deposit.Data.Signature
	if err := verifySigningRoot(deposit.Data, pubKey, depositSig, domain); err != nil {
	// Ignore this error as in the spec pseudo code.
	log.Errorf("Skipping deposit: could not verify deposit data signature: %v", err)
	return beaconState, nil
	}

.

Although currently for our testnet we only stick to one fork version, in mainnet the fork version would be incremented each time there is a hardfork.

This would be problematic in the case of processing deposit logs from the eth1chain. For example when processing a deposit log whose deposit references a fork version of

[]byte{0,0,0,0}

but the current fork version in the state would be

[]byte{0,0,0,1}

So using the current fork version, signature verification of the deposit would fail. We would need to find a way to co-ordinate processing of deposit logs according to the appropriate fork version. This issue is part of a bigger conversation of how to co-ordinate hard forks in prysm.

[paulhauner]

Hey :) I'm not sure if I'm missing the point here, but there should be no need to "co-ordinate processing of deposit logs according to the appropriate fork version".

The fork version for a deposit is always [0, 0, 0, 0], regardless of any beacon_state.fork.current_version.

[nisdas]

below is the spec's definition of get_domain.

def get_domain(state: BeaconState,
               domain_type: int,
               message_epoch: Epoch=None) -> int:
    """
    Return the signature domain (fork version concatenated with domain type) of a message.
    """
    epoch = get_current_epoch(state) if message_epoch is None else message_epoch
    fork_version = state.fork.previous_version if epoch < state.fork.epoch else state.fork.current_version

A different fork version in the state will lead to a different domain, which when verifying a signature for a deposit( which was made with an old fork version) will lead to the verification failing.

So when starting up the beacon node, and processing all the deposit logs from the deposit contract, we have to take into mind what the fork version was in the state when the deposit was made, otherwise that deposit will fail verification.

This isnt so much of an issue with the spec, more of how we currently process deposits. When processing a deposit log, we extract the deposit data convert it into a deposit object and then run it through ProcessDeposit

prysm/beacon-chain/core/blocks/block_operations.go

Line 957 in 9bf55e5

func ProcessDeposit(beaconState *pb.BeaconState, deposit *ethpb.Deposit, valIndexMap map[[48]byte]int) (*pb.BeaconState, error) {

. If it passes the processing we then can proceed with keeping the deposit in our pending_deposits pool , from which proposers can then package them into blocks.

[paulhauner]

In process_deposit, the spec declares:

# Note: Deposits are valid across forks, thus the deposit domain is retrieved directly from `compute_domain`.
domain = compute_domain(DOMAIN_DEPOSIT)

The second parameter to compute_domain is omitted, so it defaults to Version() which I understand to be [0, 0, 0, 0].

Edit: my understanding of the value of Version() is confirmed here: ethereum/consensus-specs#1430

[nisdas]

ah, that is a bug then in our code. From the spec https://github.com/ethereum/eth2.0-specs/blob/master/specs/core/0_beacon-chain.md#deposits

However, we use get_domain instead of compute_domain which is probably the source of confusion. Thanks for explaining this and bringing this up, will open a PR for this in a bit

[nisdas]

Closing this issue, since its not relevant with the current ProcessDeposit in the spec.

[paulhauner]

No worries! :)