Skip to content

JMcK sample tests for review #4147

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

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
35380dc
Adding 2 tests to existing repo:
jamesmckenna77 Feb 27, 2025
e1ca8f1
Adding 2 tests to existing repo:
jamesmckenna77 Feb 27, 2025
b84cfc8
This commit includes:
jamesmckenna77 Feb 28, 2025
bb26606
This commmit is just to remove the yield from the attestation.
jamesmckenna77 Feb 28, 2025
e209c35
For the most part, rename whisk to eip7441
jtraglia Feb 14, 2025
02af1b1
Rename test_whisk to test_eip7441
jtraglia Feb 14, 2025
a3ce0bc
Fix lint
jtraglia Feb 14, 2025
47b598a
Remove WHISK_ prefixes
jtraglia Feb 18, 2025
9d8c876
eip7732 adapt rpc methods
tbenr Feb 4, 2025
a6c70d9
move to separate method
tbenr Feb 5, 2025
eaf057f
fix toc
tbenr Feb 5, 2025
05f921c
feedback
tbenr Feb 12, 2025
5553849
as per feedback
tbenr Feb 18, 2025
d38be4a
Remove python version from circleci cache key (#4134)
jtraglia Feb 19, 2025
1781459
Add light client sync test for forced update before update timeout (#…
bshastry Feb 19, 2025
a6fcc3c
Move deprecated specs to new directory
jtraglia Feb 26, 2025
87ee8fe
Fix typos
jtraglia Feb 26, 2025
c468088
Fix broken link polynomial-commitments-sampling.md (#4140)
rebustron Feb 26, 2025
20f40b6
Adding 2 tests to existing repo:
jamesmckenna77 Feb 27, 2025
9bcae21
Adding 2 tests to existing repo:
jamesmckenna77 Feb 27, 2025
c9cca56
This commit includes:
jamesmckenna77 Feb 28, 2025
9225047
This commmit is just to remove the yield from the attestation.
jamesmckenna77 Feb 28, 2025
d95f784
Merge remote-tracking branch 'origin/jmck_dev' into jmck_dev
jamesmckenna77 Mar 1, 2025
34fe888
1. Moving test_validator_appears_only_once_in_attestation from capell…
jamesmckenna77 Mar 3, 2025
6c413dd
Linting update
jamesmckenna77 Mar 3, 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
from eth2spec.test.context import (
spec_state_test,
with_capella_and_later,
)


@with_capella_and_later
@spec_state_test
def test_validator_appears_only_once_in_attestation(spec, state):
"""
A test to confirm that the validator_index of the validators in an attestation committee are unique and there
are no duplicate validators
"""
slot = state.slot
epoch = slot // spec.SLOTS_PER_EPOCH

# Get the number of committees assigned per slot in the current epoch
slot_committee_count = spec.get_committee_count_per_slot(state, epoch)

# Get the list of validators for each committee in the slot
validators = []
for committee in range(slot_committee_count):
validator_index = spec.get_committee_assignment(state, epoch, committee)

# There are tuples and individual validators in the list, so they need to be extracted
if isinstance(validator_index, tuple):
validators.extend(validator_index[0])
elif isinstance(validator_index, list):
validators.extend(validator_index)
else:
validators.append(validator_index)

# Check that the assigned_validators list is not empty
assert len(validators) > 0

# Confirm that the same validator does not appear more than once in the list of validators
validator_ids = set()
for validator_id in validators:
assert validator_id not in validator_ids
validator_ids.add(validator_id)

yield "committee_assignments", validator_ids
Copy link
Member

Choose a reason for hiding this comment

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

This test is interesting. I have a few questions.

  • Why did you include this in the capella tests?
  • Is this test applicable to previous forks?
  • Is this the easiest way to check if validators only appear once?
  • Why do you yield committee assignments?

Copy link
Author

Choose a reason for hiding this comment

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

  • It is in Capella purely because that was the directory I was working in at the time. Happy to move it to more appropriate location - the phase0 directory looks like the right spot so I'll move it there.
  • This test would indeed be applicable for pretty much every fork/version. Attestations are one of the pillars so I guess I can change that to "@with_all_phases"
  • I yielded the committee_assignments (validator_ids) so that if an issue was encountered, the list, containing evidence any duplicate values would be available. It isn't essential to the test

I'll submit an updated version for both tests.

Copy link
Member

Choose a reason for hiding this comment

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

Hey James. Yes, it would go into the phase0 directory with @with_all_phases. And I see. Unfortunately, we cannot yield committee assignments here as there's a standard format that clients expect. Please refer to other test_process_attestation.py files to see the way things are done. For example:

@with_all_phases
@spec_state_test
@always_bls
def test_invalid_attestation_signature(spec, state):
attestation = get_valid_attestation(spec, state)
next_slots(spec, state, spec.MIN_ATTESTATION_INCLUSION_DELAY)
yield from run_attestation_processing(spec, state, attestation, valid=False)

I believe this is the relevant format file.

Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,31 @@ def test_success_top_up_to_withdrawn_validator(spec, state):
assert has_execution_withdrawal and is_withdrawable and has_non_zero_balance
else:
assert spec.is_fully_withdrawable_validator(validator, balance, current_epoch)


@with_capella_and_later
@spec_state_test
def test_validator_invalid_high_funding_after_withdrawal(spec, state):
"""
Simple extension of the previous test_success_top_up_to_withdrawn_validator in order to add a negative scenario
where a user deposits 2x MAX_EFFECTIVE_BALANCE.
"""
validator_index = 5 # Example validator index

# Fully withdraw the validator
set_validator_fully_withdrawable(spec, state, validator_index)
assert state.balances[validator_index] > 0

next_epoch_via_block(spec, state)
assert state.balances[validator_index] == 0
assert state.validators[validator_index].effective_balance > 0

next_epoch_via_block(spec, state)
assert state.validators[validator_index].effective_balance == 0

# Make a top-up balance to validator - attempt to deposit double the max deposit)
deposit = spec.MAX_EFFECTIVE_BALANCE * 2

# Handle the negative test based on the readme - no post state
if deposit > spec.MAX_EFFECTIVE_BALANCE:
yield 'post', None
Copy link
Member

@jtraglia jtraglia Feb 27, 2025

Choose a reason for hiding this comment

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

This test is flawed for a few reasons.

  • It's possible deposit as much ETH as you want, but the validators effective balance is limited.
  • If it were invalid, you should use run_deposit_processing(..., valid=False) instead manually yielding.
  • It doesn't handle the electra case (spec.MAX_EFFECTIVE_BALANCE_ELECTRA).
  • What readme are you referring to? The test format?

Copy link
Author

Choose a reason for hiding this comment

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

  • Ok, I got it. I need to run_deposit_processing = False to get a proper rejection.
  • I can add an If statement to use electra if the fork is electra
  • I pulled that from /consensus-specs/tests/README.md. It mentions that this method is used to indicate a test that was designed to fail, but maybe I've misread it.

Copy link
Member

Choose a reason for hiding this comment

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

Hey again. No worries, this stuff is pretty complicated. I would suggest updating this test to check that a top up deposit greater than MAX_EFFECTIVE_BALANCE (before and after electra) still works. And that the validator's balance (not its effective balance) is not limited.