ethereum / consensus-specs

Ethereum Proof-of-Stake Consensus Specifications
Creative Commons Zero v1.0 Universal
3.53k stars 959 forks source link

Update by reference for two state objects affect each other #3925

Open mkalinin opened 5 days ago

mkalinin commented 5 days ago

Consider the following test:

@with_electra_and_later
@spec_state_test
def test_state_changes_overwritten(spec, state):
    validator_0 = state.validators[0]
    validator_1 = state.validators[1]

    validator_0.exit_epoch = spec.Epoch(0)
    assert state.validators[0].exit_epoch == spec.Epoch(0)

    validator_1.exit_epoch = spec.Epoch(1)

    # Check that both changes have been applied to the state
    assert state.validators[1].exit_epoch == spec.Epoch(1)
    assert state.validators[0].exit_epoch == spec.Epoch(0)

The second assert fails, as validator_1.exit_epoch = spec.Epoch(1) reverts the change made to validator_0 in the line above. If we change the order to 1) update validator_1 2) update validator_0 then the first assert would fail.

mkalinin commented 5 days ago

The cause of the bug is in the following remerkleable behaviour:

  1. state.validators[0] expression creates the following chain of view backings: state.validators_a.validator_0 and keeps it in the validator_0 view, similarly validator_1 denotes state.validators_b.validator_1
  2. validator_0.exit_epoch = spec.Epoch(0) assignment updates validator_0 chain of backings to state_a.validators_a.validator_0_updated, note that the state view is now backed by the state_a backing and the first assert passes. Also, note that validator_1 denotes state_a.validators_b.validator_1: the validators view diverged from the state view
  3. validator_1.exit_epoch = spec.Epoch(1) assignment is applied to validator_1, then it updates validators backing and is finally propagated to the state backing, resulting in the following chain of backings: state_b.validators_b.validator_1 and the state now backed by state_b making the last assert fail

Potential solution would be to cache a child view inside of a parent view instance if a child view is a ComplexView that can be further updated by reference; and on every subsequence calls obtaining the same child view return the cached value.

With this approach the above case would have: validator_0 <- state.validators.validator_0, and validator_1 <- state.validators.validator_1. So, both validator views would refer to the same instance of state.validators. And an update to validator_0 would be propagated to validator_1 backings

mkalinin commented 5 days ago

Another case with similar cause:

@with_electra_and_later
@spec_state_test
def test_state_changes_overwritten_2(spec, state):
    validator_0 = state.validators[0]
    state.validators[0].exit_epoch = spec.Epoch(0)

    assert validator_0.exit_epoch == spec.Epoch(0)

The assert above fails because the update is not applied to the validator_0 view

ralexstokes commented 4 days ago

I wonder if @protolambda has anything to add here