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

electra: Misc beacon chain operations tests #3980

Merged
merged 5 commits into from
Nov 4, 2024

Conversation

mkalinin
Copy link
Collaborator

This PR introduces a few tests for process_withdrawal_request and process_attestation

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

LGTM 👍 thanks!

@jtraglia jtraglia mentioned this pull request Oct 22, 2024
20 tasks
Comment on lines 88 to 113
@with_electra_and_later
@spec_state_test
@with_presets([MINIMAL], "need multiple committees per slot")
@always_bls
def test_multiple_committees(spec, state):
attestation_data = build_attestation_data(spec, state, slot=state.slot, index=0)
attestation = spec.Attestation(data=attestation_data)

# fill the attestation with two committees
fill_aggregate_attestation(spec, state, attestation, signed=True, committee_index=0)
fill_aggregate_attestation(spec, state, attestation, signed=True, committee_index=1)

next_slots(spec, state, spec.MIN_ATTESTATION_INCLUSION_DELAY)

yield from run_attestation_processing(spec, state, attestation)


@with_electra_and_later
@spec_state_test
@with_presets([MINIMAL], "need multiple committees per slot")
@always_bls
def test_one_committee_with_gap(spec, state):
attestation = get_valid_attestation(spec, state, index=1, signed=True)
next_slots(spec, state, spec.MIN_ATTESTATION_INCLUSION_DELAY)

yield from run_attestation_processing(spec, state, attestation)
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Let's move these tests to phase0?
  2. For test_multiple_committees, I believe we have similar tests in phase0 sanity or fork choice that cover this case. But fine to add it. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests make sense for Electra because an attestation can have more than one committee. Do you think they are applicable for pre-Electra forks? If yes, I am more than happy to move them.

For test_multiple_committees, I believe we have similar tests in phase0 sanity or fork choice that cover this case. But fine to add it. 👍

During a quick look I haven’t found one, but realized that a subsequent call to fill_aggregate_attestation resets the aggregation bits to zero which isn’t a desirable test scenario. I will work on to fix this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, indeed, there is a state_transition_with_full_block that is called from some FC tests, and in Electra it will produce a block containing an on chain attestation of a new type with all committees in a single bitfield. I think it is still makes sense to have a separate state test covering this case, so decided to submit a fix rather than remove it

@mkalinin
Copy link
Collaborator Author

mkalinin commented Nov 4, 2024

This PR should be ready to merge now

@jtraglia jtraglia merged commit 49b6840 into ethereum:dev Nov 4, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants