Update EIP-6110: Spec parse_deposit_data

[jochem-brouwer]

The parse_deposit_data is not specced. This PR adds this to the spec. There are two ways to decode and get the relevant data: either hardcoded offsets/sizes or use an ABI decoder. This PR uses the hardcoded approach and has no notion of "ABI" since this would enshrine the ABI in the protocol.

Note that on mainnet the event always returns a fixed-length log and the relevant data is always at the same position and size.

Also note that the to-be-encoded data is not of dynamic bytes size but actually static:

def event_data_to_deposit_request(deposit_event_data) -> bytes:
    deposit_data = parse_deposit_data(deposit_event_data)
    pubkey = Bytes48(deposit_data[0])
    withdrawal_credentials = Bytes32(deposit_data[1])
    amount = deposit_data[2]   # 8 bytes uint64 LE
    signature = Bytes96(deposit_data[3])
    index = deposit_data[4]    # 8 bytes uint64 LE

    return pubkey + withdrawal_credentials + amount + signature + index

TODO:

• Verify the python code
• Clean up the python code
• Verify if the expected event byte length should be checked
• Investigate the Sepolia contract and how/if events differ from mainnet
• Bonus question: where is the Sepolia contract code source in Solidity? (Sepolia contract: 0x7f02c3e3c98b133055b8b348b2ac625669ed295d but has no source verified on etherscan). EDIT: It's here: https://github.com/protolambda/testnet-dep-contract/blob/master/deposit_contract.sol

[eth-bot]

File EIPS/eip-6110.md

Requires 1 more reviewers from @djrtwo, @mkalinin, @petertdavies

[mkalinin]

What is the failure mode for this check? Maybe we should do it here?

[jochem-brouwer]

I have added this also! 4f912ac 😄 👍

[mkalinin]

Can we have PUBKEY_OFFSET, WITHDRAWAL_CREDENTIALS_OFFSET, etc instead of these computations? and then:

pubkey = deposit_event_data[PUBKEY_OFFSET:PUBKEY_OFFSET + 48]
withdrawal_credentials = deposit_event_data[WITHDRAWAL_CREDENTIALS_OFFSET: WITHDRAWAL_CREDENTIALS_OFFSET + 32]
...

[jochem-brouwer]

Have added! Please verify if my offsets and sizes are correct 😄 👍

[benaadams]

Using an ABI isn't particularly important; what is important is the data fields are length prefixed byte arrays. The fixed offsets are a quirk of the current contracts.

Properly parsing (using length prefix) means it would still work if a contract on a different chain output the same data but trimmed leading zeros; however using fixed offsets would break (if the argument is not to use ABI because "it might change"; changing would first break the offsets) (edited)
[3:06 PM]
The data is literally saying the lengths of the data elements; seem irresponsible to ignore it and lay a presumed offset format on it? If it was C that's how you'd get buffer over/under runs.

What if the deposit contract was written in viper or some other language that decided to encode packed? Is valid by the event log specification, would fail to be parsed.

It's also both literally encoding ABI and also ignoring it

[jochem-brouwer]

Yes, good point regarding the ABI, I should not have added this as comment at all. Have removed :)

The goal this PR wants to solve is that parse_deposit_data is currently not specced. This PR should spec it. We have two approaches (and we should decide async or via ACD-E):

• Hardcoded offsets/sizes (this PR)
• Use ABI decoding

Note that the current EIP spec has this in it:

#### Deposit request

The structure denoting the new deposit request consists of the following fields:

1. `pubkey: Bytes48`
2. `withdrawal_credentials: Bytes32`
3. `amount: uint64`
4. `signature: Bytes96`
5. `index: uint64`

This is how the EIP-7685 request_data is being encoded. (This would also raise the question for me: why does the Deposit event not encode this directly in the event signature? But this is a side-question, we can't change this anyways, and neither can we change that on mainnet these bytes are always at the same offset and same size for all event data of the deposit event)

[jochem-brouwer]

I have updated the OP of this PR also regarding this.

[benaadams]

But this EIP is encoding that anything decoding using the specified ABI should fail if junky data was passed but the offset would "pass" which seems sloppy? Since the data would be wrong for anyone parsing the event upstream (and would the data even be correct in that circumstance?)

[jochem-brouwer]

I'm not entirely sure if I got this comment right but I think this applies here as well: #9460 (comment)

[benaadams]

What happens when its the correct contract, correct signature but log.data is not exactly 576 bytes?

[jochem-brouwer]

It skips the log. Note that the current spec tells us how to encode the request (no changes to it in this PR):

1. `pubkey: Bytes48`
2. `withdrawal_credentials: Bytes32`
3. `amount: uint64`
4. `signature: Bytes96`
5. `index: uint64`

Note that if these value lengths are ABI encoded you would still end up with the 576 length event, because:

• 5 offset Bytes32 (160 bytes total)
• 5 size Bytes32 (160 bytes total)
• pubkey: 48 bytes, but will get rounded up to 64 bytes
• withdrawal_credentials: 32 bytes
• amount: will get rounded up to 32 bytes
• signature: 96 bytes
• index: will get rounded up to 32 bytes

So, in total (5+5) * 32 + 64 + 32 + 32 + 96 + 32 = 576 bytes. If we would take, for instance, the pubkey and would now encode the actual bytes data as more than 64 bytes, then we would thus get extra 32 bytes extra in the log data, however, this cannot be encoded in the request as Bytes48 anymore (unless we want to ignore any data).

[benaadams]

Note that the current spec tells us how to encode the request

The spec does not specify lengths and offsets, it specifies that the input values are variable https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/deposit-contract.md#staking-deposit-contract

The deposit contract has a public deposit function to make deposits. It takes as arguments bytes calldata pubkey, bytes calldata withdrawal_credentials, bytes calldata signature, bytes32 deposit_data_root

and also that the logs are variable

Every deposit emits a DepositEvent log for consumption by the beacon chain. The deposit contract does little validation, pushing most of the validator onboarding logic to the beacon chain.

The L1 smart contract also specify that the types are variable https://etherscan.io/address/0x00000000219ab540356cbb839cbe05303d7705fa#code
Both in the input values

    function deposit(
        bytes calldata pubkey,
        bytes calldata withdrawal_credentials,
        bytes calldata signature,
        bytes32 deposit_data_root
    ) override external payable {

and in the log

event DepositEvent(
        bytes pubkey,
        bytes withdrawal_credentials,
        bytes amount,
        bytes signature,
        bytes index
    );

Where is it specified that they are fixed sized buffers of Bytes48, Bytes32, uint64, Bytes96, uint64?

[jochem-brouwer]

Sorry I should have been more clear, I was not referring to the solidity spec, but to the encoding spec of the actual request (so the request which gets in the block header) of this EIP. See: https://eips.ethereum.org/EIPS/eip-6110#execution-layer and then to the "Deposit request" part:

[benaadams]

Ok but those are post parsing the log data? i.e. parse_deposit_data and is about ho to convert to fixed sizes (for output)

Before that where is the len(log.data) === 576 guarantee in any prior spec?

[jochem-brouwer]

There technically is none, but, the deposit event hash is now hardcoded in clients to filter out non-DepositEvent logs (so the Sepolia bug)

So this is given:

      event DepositEvent(
        bytes pubkey,
        bytes withdrawal_credentials,
        bytes amount,
        bytes signature,
        bytes index
    );

Note: keccak256("DepositEvent(bytes,bytes,bytes,bytes,bytes)="0x649bbc62d0e31342afea4e5cd82d4049e7e1ee912fc0889aa790803be39038c5"

By ABI encoding as mentioned here: #9460 (comment) if we keep the way how requests are encoded, then if more data would get added to one of these parameters, it would not fit in the requests data (and we thus need to edit how the request itself gets encoded in the header)

[jochem-brouwer]

Side-note, this extra check is taken from Geth's code: https://github.com/ethereum/go-ethereum/blob/cd78b65cdaa9e00866962e38685c4f93ac9e8444/core/types/deposit.go#L28

Note that on mainnet the deposit event length should be 576 bytes always (implied by the solidity implementation, did not check bytecode)

[benaadams]

keccak256("DepositEvent(bytes,bytes,bytes,bytes,bytes)="0x649bbc62d0e31342afea4e5cd82d4049e7e1ee912fc0889aa790803be39038c5"

Yes and bytes are variable length values; not fixed length. So the ABI does not have fixed position offsets and lengths.

The L1 code does have require statements specifying the length of the params; however we know from 3 other chains (Sepolia, Chaido and Gnosis) that the internal L1 contract implementation is not a guarenetted spec

[benaadams]

e.g. if the contract accepted 1ETH deposits 0xDE0B6B3A7640000 and didn't force it to a Uint256 size in the log output (as L1 contract does) or maybe output in wei rather than gwei; but output packed it would currently be valid and parsable.

However the log would also fail the len(log.data) === 576 check, which first appears in this PR and nothing else prior; and the offset for SIGNATURE_OFFSET and INDEX_OFFSET would be incorrect.

[benaadams]

Note that on mainnet the deposit event length should be 576 bytes

What the mainnet contract does is no guarantee of what ABI compliant contracts that follow the spec do; or Sepolia wouldn't have broken.

What is in all prior specs is that these are all variable length fields; that the size is 576 is an implementation detail of the mainnet contract, however it is not guaranteed in any spec; which in fact specify they are variable length fields.

Mainnet output in gwei for example is a particularly of mainnet asset and the beacon chain requirements, but only L1 uses mainnet CLs/beacon chain.