Skip to content

Update EIP-6110: Spec parse_deposit_data #9460

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Mar 27, 2025
+43 −4
Merged

Update EIP-6110: Spec parse_deposit_data #9460

Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
232872a
EIP-6110: [draft] spec-out `parse-deposit_data` in python
jochem-brouwer Mar 6, 2025
8e67988
EIP-6110: update the constants / offsets
jochem-brouwer Mar 6, 2025
9b687a3
EIP6110: remove solidity event definition
jochem-brouwer Mar 6, 2025
4f912ac
EIP6110: add length check
jochem-brouwer Mar 6, 2025
3c96c46
Update EIP-6110: read the ""dynamic"" sizes from raw data
jochem-brouwer Mar 18, 2025
9bef0b9
EIP-6110: remove log length check
jochem-brouwer Mar 19, 2025
eb150ed
EIP-6110: harden definition to only target mainnet-sized deposit logs
jochem-brouwer Mar 21, 2025
01e54fc
EIP-6110: also do int conversion for sizes
jochem-brouwer Mar 21, 2025
194f66e
EIP-6110: add explicit DepositEvent layout validator
jochem-brouwer Mar 24, 2025
43cf243
EIP-6110: add length check back in
jochem-brouwer Mar 24, 2025
b1e69fd
Update EIPS/eip-6110.md
jochem-brouwer Mar 25, 2025
31641e8
Update EIPS/eip-6110.md
jochem-brouwer Mar 25, 2025
6b0eb53
EIP-6110: early offset checks to prevent out-of-bounds reading
jochem-brouwer Mar 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions EIPS/eip-6110.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,26 @@ Beginning with the `FORK_BLOCK`, each deposit accumulated in the block **MUST**
in the order they appear in the logs. To illustrate:

```python
def parse_deposit_data(deposit_event_data) -> bytes[]:
"""
Parses deposit data from DepositContract.DepositEvent data
"""
pass
def parse_deposit_data(deposit_event_data):
"""
Parses deposit data from DepositContract.DepositEvent data.
"""

# TODO: verify byte length of deposit_event_data? Should be 576. Should this be part of the spec?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it has to be. The simplest is to require the length of the log data to be exactly this (otherwise, no request is noted).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops forgot to remove this TODO. It is currently at line 115 is_deposit_event = (len(log.topics) > 0 and log.topics[0] == DEPOSIT_EVENT_SIGNATURE_HASH and len(log.data) === 576). I'll remove the TODO :)


PUBKEY_OFFSET = 192
WITHDRAWAL_OFFSET = 288
AMOUNT_OFFSET = 352
SIGNATURE_OFFSET = 416
INDEX_OFFSET = 544

pubkey = deposit_event_data[PUBKEY_OFFSET:PUBKEY_OFFSET + 48]
withdrawal_credentials = deposit_event_data[WITHDRAWAL_OFFSET:WITHDRAWAL_OFFSET + 32]
amount = deposit_event_data[AMOUNT_OFFSET:AMOUNT_OFFSET + 8]
signature = deposit_event_data[SIGNATURE_OFFSET:SIGNATURE_OFFSET + 96]
index = deposit_event_data[INDEX_OFFSET:INDEX_OFFSET + 8]

return [pubkey, withdrawal_credentials, amount, signature, index]

def event_data_to_deposit_request(deposit_event_data) -> bytes:
deposit_data = parse_deposit_data(deposit_event_data)
Expand All @@ -97,7 +112,7 @@ def get_deposit_request_data(receipts)
deposit_requests = []
for receipt in receipts:
for log in receipt.logs:
is_deposit_event = (len(log.topics) > 0 and log.topics[0] == DEPOSIT_EVENT_SIGNATURE_HASH)
is_deposit_event = (len(log.topics) > 0 and log.topics[0] == DEPOSIT_EVENT_SIGNATURE_HASH and len(log.data) === 576)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

Copy link
Contributor

@benaadams benaadams Mar 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@benaadams benaadams Mar 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@benaadams benaadams Mar 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if log.address == DEPOSIT_CONTRACT_ADDRESS and is_deposit_event:
deposit_request = event_data_to_deposit_request(log.data)
deposit_requests.append(deposit_request)
Expand Down
Loading