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

Allow honest validators to reorg late blocks #3034

Merged
merged 11 commits into from
Nov 2, 2023
96 changes: 96 additions & 0 deletions specs/bellatrix/fork-choice.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
- [`ExecutionEngine`](#executionengine)
- [`notify_forkchoice_updated`](#notify_forkchoice_updated)
- [`safe_block_hash`](#safe_block_hash)
- [`should_override_forkchoice_update`](#should_override_forkchoice_update)
- [Helpers](#helpers)
- [`PayloadAttributes`](#payloadattributes)
- [`PowBlock`](#powblock)
Expand Down Expand Up @@ -76,6 +77,101 @@ As per EIP-3675, before a post-transition block is finalized, `notify_forkchoice
The `safe_block_hash` parameter MUST be set to return value of
[`get_safe_execution_payload_hash(store: Store)`](../../fork_choice/safe-block.md#get_safe_execution_payload_hash) function.

##### `should_override_forkchoice_update`

If proposer boost re-orgs are implemented and enabled (see `get_proposer_head`) then additional care
must be taken to ensure that the proposer is able to build an execution payload.

If a beacon node knows it will propose the next block then it SHOULD NOT call
`notify_forkchoice_updated` if it detects the current head to be weak and potentially capable of
being re-orged. Complete information for evaluating `get_proposer_head` _will not_ be available
immediately after the receipt of a new block, so an approximation of those conditions should be
used when deciding whether to send or suppress a fork choice notification. The exact conditions
used may be implementation-specific, a suggested implementation is below.
michaelsproul marked this conversation as resolved.
Show resolved Hide resolved

Let `validator_is_connected` be a function that indicates whether the validator with
`validator_index` is connected to the node (e.g. has sent an unexpired proposer preparation
message).

```python
def validator_is_connected(_validator_index: ValidatorIndex) -> boolean:
...
```

```python
def should_override_forkchoice_update(
store: Store,
head_root: Root,
) -> boolean:
michaelsproul marked this conversation as resolved.
Show resolved Hide resolved
justified_state = store.checkpoint_states[store.justified_checkpoint]
head_block = store.blocks[head_root]
parent_root = head_block.parent_root
parent_block = store.blocks[parent_root]
current_slot = get_current_slot(store)
proposal_slot = head_block.slot + Slot(1)

# Only re-org the head_block block if it arrived later than the attestation deadline.
head_late = store.block_timeliness.get(head_root) is False

# Only suppress the fork choice update if we are confident that we will propose the next block.
parent_state_advanced = store.block_states[parent_root]
process_slots(parent_state_advanced, proposal_slot)
proposer_index = get_beacon_proposer_index(parent_state_advanced)
proposing_reorg_slot = validator_is_connected(proposer_index)

# Do not re-org if the chain is not finalizing with acceptable frequency.
proposal_epoch = compute_epoch_at_slot(proposal_slot)
epochs_since_finalization = proposal_epoch - store.finalized_checkpoint.epoch
finalization_ok = epochs_since_finalization <= REORG_MAX_EPOCHS_SINCE_FINALIZATION

# Single slot re-org.
parent_slot_ok = parent_block.slot + 1 == head_block.slot
time_into_slot = (store.time - store.genesis_time) % SECONDS_PER_SLOT
current_time_ok = (head_block.slot == current_slot or
(proposal_slot == current_slot and
time_into_slot <= SECONDS_PER_SLOT // INTERVALS_PER_SLOT // 2))
hwwhww marked this conversation as resolved.
Show resolved Hide resolved
single_slot_reorg = parent_slot_ok and current_time_ok

# Shuffling stable.
shuffling_stable = proposal_slot % SLOTS_PER_EPOCH != 0

# FFG information of the new head_block will be competitive with the current head.
ffg_competitive = (store.unrealized_justifications[parent_root] ==
store.unrealized_justifications[head_root])

# Check the head weight only if the attestations from the head slot have already been applied.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Check the head weight only if the attestations from the head slot have already been applied.
# Check the head weight only if the attestations from the current slot have already been applied.

This function is phrased in a way that it is agnostic to whether we are calling it on the slot we are about to propose or during the previous slot. However, what worries me is the situation where current_slot = N, we are supposed to propose at N+1, head_slot = N-1 and we call during slot N. In this case, suppose the block N is based on N-2, forking off N-1, it is a timely block, and we haven't yet counted attestations because we are calling this function during N. What will happen is that our head will still be N-1 and every check here will pass. It may well be that later, at the time we need to propose, during N+1, this timely block at N would become our head, but this function here may return true.

Now this is not a problem per-se in this specification since later on, when calling get_proposer_head we will get a misprediction because we will not have passed single_slot_reorg. Anyway, I feel it's better to change this condition here from checking that the attestations for the head block to have been counted to "changing that the attestations for the current slot have been counted if we are calling late in the slot". I do recognize that this is difficult to specify, but perhaps it's better to not be agnostic as to the timing in which this function is being called.

As a side in this long comment: if we are in the situation above, and we call this function during N, then the assert store.proposer_boost_root == Root() will fail in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the head is at N - 1 and we are proposing at N + 1 then this function won't return true. This function infers the proposal_slot from the head slot, so it will check whether we are proposing at slot N:

proposal_slot = head_block.slot + 1

i.e. if we are actually proposing at N+1 and some other block for N already exists, the proposing_reorg_slot check will be False.

Therefore I think the spec already minimises the chance of a misprediction. It's a really nice test case though, I'll make sure it gets a spec test when we add them (I'll also add a test to Lighthouse before then).

Anyway, I feel it's better to change this condition here from checking that the attestations for the head block to have been counted to "changing that the attestations for the current slot have been counted if we are calling late in the slot".

I'm open to making this change, but would like to clarify something about how you envisage Prysm's implementation applying attestations at 10 or 11 secs into the slot will work: are you going to avoid updating the fork choice store's current slot "early"? I.e. if you are 10s into slot N will you apply the attestations for slot N and leave get_current_slot(store) == N? The way everything is written at the moment maintains the invariant that only attestations from get_current_slot(store) - 1 or earlier are applied, supporting Lighthouse's implementation which advances the fork choice slot early at 11.5s into slot N and pre-emptively increments the fork choice slot to N + 1.

As a side in this long comment: if we are in the situation above, and we call this function during N, then the assert store.proposer_boost_root == Root() will fail in this function.

Nice catch on the assert! I think we need to change it so we return False rather than erroring, or assert that the proposer boost is for a block other than head_block:

assert store.proposer_root_root != head_root

Copy link
Contributor

@potuz potuz Jan 7, 2023

Choose a reason for hiding this comment

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

i.e. if we are actually proposing at N+1 and some other block for N already exists, the proposing_reorg_slot check will be False.

Indeed I mixed the previous check, but now I'm worried about something else: validator_is_connected can return true for a large number of validators in the case of a pool. In the above scenario, a pool proposing at N and N+1 will actually pass with the wrong validator the check for proposing_reorg_slot.

are you going to avoid updating the fork choice store's current slot "early"? I.e. if you are 10s into slot N will you apply the attestations for slot N and leave get_current_slot(store) == N ?

Yeah that's my plan, I'm open to change this to make it closer to LightHouse, but it seems very hard on our end cause we track the wallclock on forkchoice and tricking it to change the slot involves bad hacks like changing the genesis time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

validator_is_connected can return true for a large number of validators in the case of a pool. In the above scenario, a pool proposing at N and N+1 will actually pass with the wrong validator the check for proposing_reorg_slot.

@potuz I don't understand what you mean here, can you spell it out in a bit more detail? I thought we already covered this case by demonstrating that the fcU for N - 1 won't be suppressed, and if the head switches to N upon counting attestations towards the end of slot N, then the fcU for N also won't be suppressed because it will fail the parent_slot_ok check (N's parent is N - 2). Hence we'll still build on N via the normal process (no re-org attempts).

# Implementations may want to do this in different ways, e.g. by advancing
# `store.time` early, or by counting queued attestations during the head block's slot.
if current_slot > head_block.slot:
head_weight = get_weight(store, head_root)
reorg_threshold = calculate_committee_fraction(justified_state, REORG_WEIGHT_THRESHOLD)
head_weak = head_weight < reorg_threshold

parent_weight = get_weight(store, parent_root)
parent_threshold = calculate_committee_fraction(justified_state, REORG_PARENT_WEIGHT_THRESHOLD)
parent_strong = parent_weight > parent_threshold
else:
head_weak = True
parent_strong = True

return all([head_late, proposing_reorg_slot, finalization_ok, single_slot_reorg,
shuffling_stable, ffg_competitive, head_weak, parent_strong])
```

> Note that the ordering of conditions is a suggestion only. Implementations are free to
optimize by re-ordering the conditions from least to most expensive and by returning early if
any of the early conditions are `False`.

In case `should_override_forkchoice_update` returns `True`, a node SHOULD instead call
`notify_forkchoice_updated` with parameters appropriate for building upon the parent block. Care
must be taken to compute the correct `payload_attributes`, as they may change depending on the slot
of the block to be proposed (due to withdrawals).

If `should_override_forkchoice_update` returns `True` but `get_proposer_head` later chooses the
canonical head rather than its parent, then this is a misprediction that will cause the node
to construct a payload with less notice. The result of `get_proposer_head` MUST be honoured in
preference to the heuristic method.

## Helpers

### `PayloadAttributes`
Expand Down
92 changes: 83 additions & 9 deletions specs/phase0/fork-choice.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@
- [`get_current_slot`](#get_current_slot)
- [`compute_slots_since_epoch_start`](#compute_slots_since_epoch_start)
- [`get_ancestor`](#get_ancestor)
- [`calculate_committee_fraction`](#calculate_committee_fraction)
- [`get_checkpoint_block`](#get_checkpoint_block)
- [`get_weight`](#get_weight)
- [`get_voting_source`](#get_voting_source)
- [`filter_block_tree`](#filter_block_tree)
- [`get_filtered_block_tree`](#get_filtered_block_tree)
- [`get_head`](#get_head)
- [`get_proposer_head`](#get_proposer_head)
- [`update_checkpoints`](#update_checkpoints)
- [`update_unrealized_checkpoints`](#update_unrealized_checkpoints)
- [Pull-up tip helpers](#pull-up-tip-helpers)
Expand Down Expand Up @@ -76,11 +78,16 @@ Any of the above handlers that trigger an unhandled exception (e.g. a failed ass

### Configuration

| Name | Value |
| ---------------------- | ------------ |
| `PROPOSER_SCORE_BOOST` | `uint64(40)` |
| Name | Value |
| ------------------------------------- | ------------ |
| `PROPOSER_SCORE_BOOST` | `uint64(40)` |
| `REORG_WEIGHT_THRESHOLD` | `uint64(20)` |
hwwhww marked this conversation as resolved.
Show resolved Hide resolved
| `REORG_PARENT_WEIGHT_THRESHOLD` | `uint64(160)`|
hwwhww marked this conversation as resolved.
Show resolved Hide resolved
| `REORG_MAX_EPOCHS_SINCE_FINALIZATION` | `Epoch(2)` |

- The proposer score boost is worth `PROPOSER_SCORE_BOOST` percentage of the committee's weight, i.e., for slot with committee weight `committee_weight` the boost weight is equal to `(committee_weight * PROPOSER_SCORE_BOOST) // 100`.
- The proposer score boost and re-org weight threshold are percentage
values that are measured with respect to the weight of a single committee. See
`calculate_committee_fraction`.

### Helpers

Expand Down Expand Up @@ -115,6 +122,7 @@ class Store(object):
equivocating_indices: Set[ValidatorIndex]
blocks: Dict[Root, BeaconBlock] = field(default_factory=dict)
block_states: Dict[Root, BeaconState] = field(default_factory=dict)
block_timeliness: Dict[Root, boolean] = field(default_factory=dict)
checkpoint_states: Dict[Checkpoint, BeaconState] = field(default_factory=dict)
latest_messages: Dict[ValidatorIndex, LatestMessage] = field(default_factory=dict)
unrealized_justifications: Dict[Root, Checkpoint] = field(default_factory=dict)
Expand Down Expand Up @@ -191,6 +199,14 @@ def get_ancestor(store: Store, root: Root, slot: Slot) -> Root:
return root
```

#### `calculate_committee_fraction`

```python
def calculate_committee_fraction(state: BeaconState, committee_percent: uint64) -> Gwei:
committee_weight = get_total_active_balance(state) // SLOTS_PER_EPOCH
return Gwei((committee_weight * committee_percent) // 100)
```

#### `get_checkpoint_block`

```python
Expand Down Expand Up @@ -225,8 +241,7 @@ def get_weight(store: Store, root: Root) -> Gwei:
proposer_score = Gwei(0)
# Boost is applied if ``root`` is an ancestor of ``proposer_boost_root``
if get_ancestor(store, store.proposer_boost_root, store.blocks[root].slot) == root:
committee_weight = get_total_active_balance(state) // SLOTS_PER_EPOCH
proposer_score = (committee_weight * PROPOSER_SCORE_BOOST) // 100
proposer_score = calculate_committee_fraction(state, PROPOSER_SCORE_BOOST)
return attestation_score + proposer_score
```

Expand All @@ -247,7 +262,6 @@ def get_voting_source(store: Store, block_root: Root) -> Checkpoint:
# The block is not from a prior epoch, therefore the voting source is not pulled up
head_state = store.block_states[block_root]
return head_state.current_justified_checkpoint

```

#### `filter_block_tree`
Expand Down Expand Up @@ -342,6 +356,62 @@ def get_head(store: Store) -> Root:
head = max(children, key=lambda root: (get_weight(store, root), root))
```

#### `get_proposer_head`

_Implementing `get_proposer_head` is optional_.

```python
def get_proposer_head(store: Store, head_root: Root, slot: Slot) -> Root:
justified_state = store.checkpoint_states[store.justified_checkpoint]
head_block = store.blocks[head_root]
parent_root = head_block.parent_root
parent_block = store.blocks[parent_root]

# Only re-org the head block if it arrived later than the attestation deadline.
head_late = store.block_timeliness.get(head_root) is False

# Do not re-org if the chain is not finalizing with acceptable frequency.
epochs_since_finalization = compute_epoch_at_slot(slot) - store.finalized_checkpoint.epoch
finalization_ok = epochs_since_finalization <= REORG_MAX_EPOCHS_SINCE_FINALIZATION

# Only re-org a single slot at most.
single_slot_reorg = parent_block.slot + 1 == head_block.slot and head_block.slot + 1 == slot

# Only re-org if we are proposing on-time.
time_into_slot = (store.time - store.genesis_time) % SECONDS_PER_SLOT
proposing_on_time = time_into_slot <= SECONDS_PER_SLOT // INTERVALS_PER_SLOT // 2

# Do not re-org on an epoch boundary where the proposer shuffling could change.
shuffling_stable = slot % SLOTS_PER_EPOCH != 0

# Ensure that the FFG information of the new head will be competitive with the current head.
ffg_competitive = (store.unrealized_justifications[parent_root] ==
store.unrealized_justifications[head_root])

# Check that the head has few enough votes to be overpowered by our proposer boost.
assert store.proposer_boost_root != head_root # ensure boost has worn off
head_weight = get_weight(store, head_root)
reorg_threshold = calculate_committee_fraction(justified_state, REORG_WEIGHT_THRESHOLD)
head_weak = head_weight < reorg_threshold

# Check that the missing votes are assigned to the parent and not being hoarded.
parent_weight = get_weight(store, parent_root)
parent_threshold = calculate_committee_fraction(justified_state, REORG_PARENT_WEIGHT_THRESHOLD)
parent_strong = parent_weight > parent_threshold

if all([head_late, finalization_ok, single_slot_reorg, proposing_on_time, shuffling_stable,
ffg_competitive, head_weak, parent_strong]):
# We can re-org the current head by building upon its parent block.
return parent_root
else:
return head_root
```

> Note that the ordering of conditions is a suggestion only. Implementations are free to
optimize by re-ordering the conditions from least to most expensive and by returning early if
any of the early conditions are `False`.


#### `update_checkpoints`

```python
Expand Down Expand Up @@ -536,11 +606,15 @@ def on_block(store: Store, signed_block: SignedBeaconBlock) -> None:
# Add new state for this block to the store
store.block_states[block_root] = state

# Add proposer score boost if the block is timely
# Add block timeliness to the store
time_into_slot = (store.time - store.genesis_time) % SECONDS_PER_SLOT
is_before_attesting_interval = time_into_slot < SECONDS_PER_SLOT // INTERVALS_PER_SLOT
is_timely = get_current_slot(store) == block.slot and is_before_attesting_interval
store.block_timeliness[hash_tree_root(block)] = is_timely

# Add proposer score boost if the block is timely and not conflicting with an existing block
is_first_block = store.proposer_boost_root == Root()
if get_current_slot(store) == block.slot and is_before_attesting_interval and is_first_block:
if is_timely and is_first_block:
store.proposer_boost_root = hash_tree_root(block)

# Update checkpoints in store if necessary
Expand Down
21 changes: 14 additions & 7 deletions specs/phase0/validator.md
Original file line number Diff line number Diff line change
Expand Up @@ -274,15 +274,22 @@ A validator has two primary responsibilities to the beacon chain: [proposing blo
A validator is expected to propose a [`SignedBeaconBlock`](./beacon-chain.md#signedbeaconblock) at
the beginning of any `slot` during which `is_proposer(state, validator_index)` returns `True`.

To propose, the validator selects the `BeaconBlock`, `parent` which:

1. In their view of fork choice is the head of the chain at the start of
`slot`, after running `on_tick` and applying any queued attestations from `slot - 1`.
2. Is from a slot strictly less than the slot of the block about to be proposed,
i.e. `parent.slot < slot`.
To propose, the validator selects a `BeaconBlock`, `parent` using this process:

1. Compute fork choice's view of the head at the start of `slot`, after running
`on_tick` and applying any queued attestations from `slot - 1`.
Set `head_root = get_head(store)`.
2. Compute the _proposer head_, which is the head upon which the proposer SHOULD build in order to
incentivise timely block propagation by other validators.
Set `parent_root = get_proposer_head(store, head_root, slot)`.
A proposer may set `parent_root == head_root` if proposer re-orgs are not implemented or have
been disabled.
3. Let `parent` be the block with `parent_root`.

The validator creates, signs, and broadcasts a `block` that is a child of `parent`
that satisfies a valid [beacon chain state transition](./beacon-chain.md#beacon-chain-state-transition-function).
and satisfies a valid [beacon chain state transition](./beacon-chain.md#beacon-chain-state-transition-function).
Note that the parent's slot must be strictly less than the slot of the block about to be proposed,
i.e. `parent.slot < slot`.

There is one proposer per slot, so if there are N active validators any individual validator
will on average be assigned to propose once per N slots (e.g. at 312,500 validators = 10 million ETH, that's once per ~6 weeks).
Expand Down