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.

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

Jump to bottom

Allow honest validators to reorg late blocks #3034

Merged
merged 11 commits into from
Nov 2, 2023
Merged

Allow honest validators to reorg late blocks #3034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
45a3615
Allow honest validators to reorg late blocks
michaelsproul Sep 12, 2023
651db2f
Refactoring
hwwhww Oct 19, 2023
0f61819
Update other specs accordingly and re-order "Proposer head and reorg …
hwwhww Oct 20, 2023
24b4d46
Add `get_proposer_head` and `should_override_forkchoice_update` tests
hwwhww Oct 23, 2023
22215b8
Fix `parent_state_advanced` side effect and reorder the conditions
hwwhww Oct 24, 2023
3f8d8fe
Update config files
hwwhww Oct 24, 2023
35d444b
Update test format and output `should_override_forkchoice_update` result
hwwhww Oct 24, 2023
3f1bc20
Clarifications for proposer boost reorgs
michaelsproul Oct 26, 2023
9ce8e3d
Update test format. Add `get_proposer_head`
michaelsproul Oct 26, 2023
b9285b8
Add `validator_is_connected`
hwwhww Oct 26, 2023
d8440f8
Merge branch 'dev' into pr3034
hwwhww Oct 26, 2023
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
7 changes: 7 additions & 0 deletions configs/mainnet.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,13 @@ MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT: 8
# ---------------------------------------------------------------
# 40%
PROPOSER_SCORE_BOOST: 40
# 20%
REORG_HEAD_WEIGHT_THRESHOLD: 20
# 160%
REORG_PARENT_WEIGHT_THRESHOLD: 160
# `2` epochs
REORG_MAX_EPOCHS_SINCE_FINALIZATION: 2


# Deposit contract
# ---------------------------------------------------------------
Expand Down
6 changes: 6 additions & 0 deletions configs/minimal.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,12 @@ MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT: 4
# ---------------------------------------------------------------
# 40%
PROPOSER_SCORE_BOOST: 40
# 20%
REORG_HEAD_WEIGHT_THRESHOLD: 20
# 160%
REORG_PARENT_WEIGHT_THRESHOLD: 160
# `2` epochs
REORG_MAX_EPOCHS_SINCE_FINALIZATION: 2


# Deposit contract
Expand Down
7 changes: 6 additions & 1 deletion pysetup/spec_builders/bellatrix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@ def get_execution_state(_execution_state_root: Bytes32) -> ExecutionState:
def get_pow_chain_head() -> PowBlock:
pass"""
pass
def validator_is_connected(validator_index: ValidatorIndex) -> bool:
# pylint: disable=unused-argument
return True"""

@classmethod
def execution_engine_cls(cls) -> str:
Expand Down
89 changes: 87 additions & 2 deletions specs/bellatrix/fork-choice.md
View file
Open in desktop
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,86 @@ 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(validator_index: ValidatorIndex) -> bool` 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 should_override_forkchoice_update(store: Store, head_root: Root) -> bool:
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 = is_head_late(store, head_root)

# Shuffling stable.
shuffling_stable = is_shuffling_stable(proposal_slot)

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

# Do not re-org if the chain is not finalizing with acceptable frequency.
finalization_ok = is_finalization_ok(store, proposal_slot)

# 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].copy()
process_slots(parent_state_advanced, proposal_slot)
proposer_index = get_beacon_proposer_index(parent_state_advanced)
proposing_reorg_slot = validator_is_connected(proposer_index)

# Single slot re-org.
parent_slot_ok = parent_block.slot + 1 == head_block.slot
proposing_on_time = is_proposing_on_time(store)

# Note that this condition is different from `get_proposer_head`
current_time_ok = (head_block.slot == current_slot
or (proposal_slot == current_slot and proposing_on_time))
single_slot_reorg = parent_slot_ok and current_time_ok

# 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:

consensus-specs/specs/bellatrix/fork-choice.md

Line 111 in 56f20f7

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 •
edited
Loading

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_weak = is_head_weak(store, head_root)
parent_strong = is_parent_strong(store, parent_root)
else:
head_weak = True
parent_strong = True

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

*Note*: 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 preferred over
the result of `should_override_forkchoice_update` (when proposer reorgs are enabled).

## Helpers

### `PayloadAttributes`
Expand Down Expand Up @@ -191,11 +272,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
8 changes: 6 additions & 2 deletions specs/capella/fork-choice.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,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
8 changes: 6 additions & 2 deletions specs/deneb/fork-choice.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,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
Loading