Skip to content

Conversation

@spencer-tb
Copy link
Contributor

Description

The current EIP-6110 implementation only validates the total length of deposit event data (576 bytes) but does not validate the internal ABI structure as required by the specification. This causes EEST test failures where invalid deposit event errors are not distinctly caught for the appropriate reason.

Solution

This PR implements the deposit event layout validation as specified in EIP-6110's is_valid_deposit_event_data function, where the function returns False for incorrect validation:

  1. Offset Validation: Validates that ABI offsets match the required values.
    if (
        pubkey_offset != 160
        or withdrawal_credentials_offset != 256
        or amount_offset != 320
        or signature_offset != 384
        or index_offset != 512
    ):
        return False
  1. Size Validation: Validates that field sizes match the required values.
    return (
        pubkey_size == 48
        and withdrawal_credentials_size == 32
        and amount_size == 8
        and signature_size == 96
        and index_size == 8
    )

Hive eest/consume-engine

Following this PR, the failing EIP-6110 tests now pass:
https://hive.ethpandaops.io/#/test/fusaka/1758206019-3d8e1ef2d1c0829ec602a80ba1b23b1d?testnumber=18233

withdrawalCredRequestOffset = pubkeyRequestOffset + 48
amountRequestOffset = withdrawalCredRequestOffset + 32
signatureRequestOffset = amountRequestOffset + 8
indexRequestOffset = signatureRequestOffset + 96
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably revert the name changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumped! Thx

@Sahil-4555
Copy link

Sahil-4555 commented Sep 29, 2025

Deposit Event ABI Layout (576 bytes)

Byte Range Content Size Notes
[0:32] pubkey_offset 32 8 bytes offset + 24 bytes zero padding
[32:64] withdrawal_credentials_offset 32 8 bytes offset + 24 bytes zero padding
[64:96] amount_offset 32 8 bytes offset + 24 bytes zero padding
[96:128] signature_offset 32 8 bytes offset + 24 bytes zero padding
[128:160] index_offset 32 8 bytes offset + 24 bytes zero padding
[160:192] pubkey_length 32 8 bytes length (48) + 24 bytes zero padding
[192:224] pubkey[0:32] 32 32 bytes of pubkey
[224:256] pubkey[32:48] + padding 32 16 bytes of pubkey + 16 bytes zero padding
[256:288] withdrawal_credentials_length 32 8 bytes length (32) + 24 bytes zero padding
[288:320] withdrawal_credentials 32 32 bytes of data
[320:352] amount_length 32 8 bytes length (8) + 24 bytes zero padding
[352:384] amount 32 8 bytes of uint64 + 24 bytes zero padding
[384:416] signature_length 32 8 bytes length (96) + 24 bytes zero padding
[416:448] signature[0:32] 32 32 bytes of signature
[448:480] signature[32:64] 32 32 bytes of signature
[480:512] signature[64:96] 32 32 bytes of signature
[512:544] index_length 32 8 bytes length (8) + 24 bytes zero padding
[544:576] index 32 8 bytes of uint64 + 24 bytes zero padding

@MariusVanDerWijden
Copy link
Member

I don't know, this adds a lot of dead code to something that WILL NEVER change in order to get tests to pass.
I am slightly leaning against this tbh, but want to discuss in the next triage

@spencer-tb
Copy link
Contributor Author

We merged ethereum/execution-spec-tests#2177 in EEST so the tests do pass now. Happy for you guys to close the PR if thats the decision.

@fjl fjl changed the title core/types/deposit: add extra validation for deposit requests core/types: add extra validation for deposit requests Oct 7, 2025
@fjl
Copy link
Contributor

fjl commented Oct 7, 2025

We will close this after another round of internal discussion. We do have the length check, so in order to cause this issue, a contract would have to emit incorrect data of correct size. I just find that very unlikely.

@fjl fjl closed this Oct 7, 2025
@spencer-tb
Copy link
Contributor Author

The only other component I can think of is if it could affect L2s that fork geth! Thanks

@jonbarrientos02-bit
Copy link

jonbarrientos02-bit commented Oct 7, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants