Skip to content

Round down value when setting validator's effective balance #4229

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
jtraglia merged 8 commits into from
Mar 31, 2025
Merged

Round down value when setting validator's effective balance #4229

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4d9b5b3
Round down EB when setting validator's balance
ensi321 Mar 27, 2025
e145e48
Update `run_test_activation_queue_eligibility`
ensi321 Mar 27, 2025
2dae677
lint
ensi321 Mar 28, 2025
44c2533
Make EB configurable in set_eth1_withdrawal_credential_with_balance
ensi321 Mar 28, 2025
9b34fe5
Lint
ensi321 Mar 28, 2025
caecb3e
Fix some nits
jtraglia Mar 31, 2025
137d023
Remove new test unrelated to PR fix
jtraglia Mar 31, 2025
ea19632
Do the same thing for compounding validators
jtraglia Mar 31, 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
3 changes: 2 additions & 1 deletion tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,8 @@ def test_success_no_max_effective_balance(spec, state):
set_eth1_withdrawal_credential_with_balance(spec, state, validator_index, spec.MAX_EFFECTIVE_BALANCE - 1)
validator = state.validators[validator_index]

assert validator.effective_balance < spec.MAX_EFFECTIVE_BALANCE
# Effective balance should be 1 increment lower than MAX_EFFECTIVE_BALANCE
Copy link
Contributor

@mkalinin mkalinin Mar 28, 2025

Choose a reason for hiding this comment

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

In reality it is not only the case. If balance is greater than X + 0.75 ETH, where X is a round amount of ETH then the EB can either be rounded down or up. But for the test helper it might be fine to have rounding down a default behaviour, the helper can also be extended to set EB explicitly if needed

Copy link
Member

@nflaig nflaig Mar 28, 2025

Choose a reason for hiding this comment

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

the helper can also be extended to set EB explicitly if needed

this is what we discussed as well similar to set_compounding_withdrawal_credential_with_balance, I was thinking we could round based on DOWNWARD_THRESHOLD which would address your concern? but anyhow, being explicit when setting up test data seems favorable to me

Copy link
Contributor

@mkalinin mkalinin Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We can round down when balance + DOWNWARD_THRESHOLD < round_up(balance), and, otherwise, round up

Copy link
Member

@jtraglia jtraglia Mar 28, 2025

Choose a reason for hiding this comment

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

but anyhow, being explicit when setting up test data seems favorable to me

Agreed. I think there should be an explicit effective_balance parameter here.

assert validator.effective_balance == spec.MAX_EFFECTIVE_BALANCE - spec.EFFECTIVE_BALANCE_INCREMENT
assert not spec.is_partially_withdrawable_validator(validator, state.balances[validator_index])

execution_payload = build_empty_execution_payload(spec, state)
Expand Down
7 changes: 6 additions & 1 deletion tests/core/pyspec/eth2spec/test/capella/sanity/test_blocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,12 @@ def test_top_up_to_fully_withdrawn_validator(spec, state):


def _insert_validator(spec, state, balance):
effective_balance = balance if balance < spec.MAX_EFFECTIVE_BALANCE else spec.MAX_EFFECTIVE_BALANCE
effective_balance = (
balance - balance % spec.EFFECTIVE_BALANCE_INCREMENT
if balance < spec.MAX_EFFECTIVE_BALANCE
else spec.MAX_EFFECTIVE_BALANCE
)

validator_index = len(state.validators)
validator = spec.Validator(
pubkey=pubkeys[validator_index],
Expand Down
13 changes: 11 additions & 2 deletions tests/core/pyspec/eth2spec/test/electra/epoch_processing/test_process_registry_updates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def run_test_activation_queue_eligibility(spec, state, validator_index, balance)
next_epoch(spec, state)

state.balances[validator_index] = balance
state.validators[validator_index].effective_balance = balance
state.validators[validator_index].effective_balance = balance - balance % spec.EFFECTIVE_BALANCE_INCREMENT

# ready for entrance into activation queue
mock_deposit(spec, state, validator_index)
Expand All @@ -23,7 +23,7 @@ def run_test_activation_queue_eligibility(spec, state, validator_index, balance)

# validator moved into activation queue if eligible
validator = state.validators[validator_index]
if validator.effective_balance < spec.MIN_ACTIVATION_BALANCE:
if validator.effective_balance <= (spec.MIN_ACTIVATION_BALANCE - spec.EFFECTIVE_BALANCE_INCREMENT):
assert validator.activation_eligibility_epoch == spec.FAR_FUTURE_EPOCH
else:
assert validator.activation_eligibility_epoch < spec.FAR_FUTURE_EPOCH
Expand Down Expand Up @@ -70,3 +70,12 @@ def test_activation_queue_eligibility__greater_than_min_activation_balance(spec,
balance = spec.MIN_ACTIVATION_BALANCE + spec.EFFECTIVE_BALANCE_INCREMENT
set_compounding_withdrawal_credential_with_balance(spec, state, index)
yield from run_test_activation_queue_eligibility(spec, state, index, balance)


@with_electra_and_later
@spec_state_test
def test_activation_queue_eligibility__fractional_greater_than_min_activation_balance(spec, state):
index = 15
balance = spec.MIN_ACTIVATION_BALANCE + (spec.EFFECTIVE_BALANCE_INCREMENT * 0.6)
set_compounding_withdrawal_credential_with_balance(spec, state, index)
yield from run_test_activation_queue_eligibility(spec, state, index, balance)
4 changes: 2 additions & 2 deletions tests/core/pyspec/eth2spec/test/helpers/withdrawals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def set_eth1_withdrawal_credential_with_balance(spec, state, index, balance=None

validator = state.validators[index]
validator.withdrawal_credentials = spec.ETH1_ADDRESS_WITHDRAWAL_PREFIX + b'\x00' * 11 + address
validator.effective_balance = min(balance, spec.MAX_EFFECTIVE_BALANCE)
validator.effective_balance = min(balance - balance % spec.EFFECTIVE_BALANCE_INCREMENT, spec.MAX_EFFECTIVE_BALANCE)
state.balances[index] = balance


Expand Down Expand Up @@ -104,7 +104,7 @@ def set_compounding_withdrawal_credential_with_balance(spec, state, index,
if balance is None:
balance = effective_balance

state.validators[index].effective_balance = effective_balance
state.validators[index].effective_balance = effective_balance - effective_balance % spec.EFFECTIVE_BALANCE_INCREMENT
state.balances[index] = balance


Expand Down
Loading