Allow honest validators to reorg late blocks

[michaelsproul]

Specify a function in fork choice get_proposer_head which returns the parent block upon which a proposer may build while possibly reorging the canonical head (if it was broadcast late and lacks sufficient attestation weight).

The mechanism is designed with safeguards built in so that it does not cause liveness failures:

• Only single-slot reorgs are attempted: block N + 2 may build on block N if block N + 1 arrived late.
• Finalization must be occurring with the expected frequency determined by REORG_MAX_EPOCHS_SINCE_FINALIZATION. The default is to require optimal finalization (every 2 epochs).
• REORG_HEAD_WEIGHT_THRESHOLD is set to 20% (half of the proposer boost) so that an attacker must control up to PROPOSER_BOOST_SCORE - REORG_WEIGHT_THRESHOLD = 20% of attester weight in order to perform an ex-ante reorg. The attack works by releasing attestations for the block at slot N + 1 around the same time as the honest proposer publishes the attempted reorg block at N + 2. The late attestations tip the weight of the N + 1 block from < REORG_WEIGHT_THRESHOLD to > PROPOSER_BOOST_SCORE, meaning that block N + 1 will be preferred to block N + 2.
• REORG_PARENT_WEIGHT_THRESHOLD is set to 160% so that an attacker that is hoarding attestations cannot cause the attempted re-org to fail. For details of this calculation see: https://ethresear.ch/t/selfish-mining-in-pos/15551.

For background on the development of this idea, see: sigp/lighthouse#2860.

Pros

• Completely optional for clients and node operators, can be switched on and off via a flag. Low risk.
• Can be rolled out almost immediately unlike other late block mitigations which are still in R&D.

Cons

• Considerable complexity, particularly regarding the overriding of fork choice updates for the canonical head.
• Obsoleted by (block, slot) fork choice. If a viable implementation of (block, slot) fork choice emerges then reorging late blocks will become the default behaviour.
• Relies on proposer boost, which may be phased out in favour of other fork choice changes (e.g. view-merge).

Thanks to @potuz @mkalinin @terencechain @fradamt @casparschwa @djrtwo for feedback on this idea.

[realbigsean]

Since this is optional, it might be nice to separate it from other parts of the spec a bit more, similar to optimistic sync. We could contain the complexity a bit, especially if it might be phased out in the future.

[fradamt]

What is the circuit-breaker mechanism for when latency is > 4s? Is the intention that checking "single_slot_reorg" and "participation_ok" should act as one, because, when latency is > 4s and blocks do not get attestations in their slot, the parent block would either be too old or not have enough weight?

If that's the case, I think this should be clarified. It might also be nice to have an explicit circuit-breaker that's easier to reason about, and gives clear liveness guarantees (e.g. explicitly turn off the reorg functionality whenever too many proposals are being missed)

[michaelsproul]

@fradamt Yeah the single slot & participation conditions are meant to be the circuit-breaker for asynchrony (although I think the single slot condition alone is sufficient). I think the outcome is that with message delay $\text{4s} &lt; \Delta &lt; \text{16s}$ then every second block will be re-orged and we'll get 50% of blocks on average. Once the delay exceeds 16s then blocks will be forced to build on a parent from at least 2 slots prior and we'll get <50% of blocks on average, without the proposer re-org rules coming into effect at all. In other words the only change in behaviour is for the delay window $\text{4s} &lt; \Delta &lt; \text{16s}$.

It might also be nice to have an explicit circuit-breaker that's easier to reason about, and gives clear liveness guarantees (e.g. explicitly turn off the reorg functionality whenever too many proposals are being missed)

Are you thinking of something like the builder API circuit-breaker conditions about number of missed blocks in the last N blocks and number of epochs since finalization? We could add number-of-skipped-blocks check (while keeping single_slot_reorg), and replace the current participation check with the the epochs-since-finalization check? The main reason I'd like to keep the single_slot_reorg check is that it simplifies the implementation greatly in terms of calculating and caching things.

[potuz]

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.

[michaelsproul]

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

[potuz]

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.

[michaelsproul]

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).

[michaelsproul]

I've rebased this on dev and addressed some of the outstanding review comments. I think there are still several decisions we need to take before merging:

• Q1: Is the current location alongside the other fork choice functions the right location for these specs? As they're optional they could alternatively live in ./fork_choice alongside the safe block specs.
• Q2: How should we address the tension between specifying a precise strategy, and giving implementations the freedom to explore more aggressive strategies? This has implications for how the spec is written, which tests we write, and what inferences we are able to make about re-orgs on live networks.
  • A: max precision: Don't account for any deviations from the spec. Write test cases that enforce when clients should and shouldn't re-org (positive and negative tests, respectively).
  • B: max freedom: Allow clients to deviate from the spec in any way they wish. Write some test cases that describe minimum scenarios in which clients should re-org, but don't specify any negative tests beyond very extreme cases where re-orgs are actually impossible.
  • C: balance: Account for some acceptable deviations from the spec in tests. Write a mix of positive and negative tests while trying to avoid negative tests that would fail on current implementations. Allow implementations to skip some tests if they explore more exotic strategies in future.

I think my current preference for Q2 is option (C). I think the only outstanding difference between Prysm and Lighthouse is our handling of unrealized justification. Lighthouse currently won't re-org a block that updates unrealized justification, while Prysm will, because it assumes it can achieve equivalent justification by packing the block's attestations. I'm not opposed to changing Lighthouse to follow Prysm and specifying this as a case where we should re-org. I think Prysm's strategy is unsafe in general, so would prefer to keep the finalization check in the spec.

[potuz]

Given https://ethresear.ch/t/selfish-mining-in-pos/15551/3 I suggest we add a constant REORG_PARENT_WEIGHT_THRESHOLD with suggested value of uint64(160) and an extra condition

    parent_weight = get_weight(store, parent_root)
    parent_reorg_threshold = calculate_committee_fraction(justified_state, REORG_PARENT_WEIGHT_THRESHOLD)
    parent_strong = parent_weight > parent_reorg_threshold

And require parent_strong = true to reorg.

[dankrad]

I am starting to get more worried about this change -- the reason is that it implements a version of the block/slot fork choice, but without the mitigations that would have to be in place to avoid reorging under poor networking conditions.

Also, we should probably open a discussion about the 4s attestation timeline at this point. In order for attestations, we need (1) block propagation and (2) block validation all to happen in this time slot, which is starting to become more work (e.g. with 4844) and does not feel like the appropriate slot division anymore since the work in the other three thirds remains the same (and is overall less time critical).

[mkalinin]

without the mitigations that would have to be in place to avoid reorging under poor networking conditions.

There is a constraint on the slot of a parent block as a mitigation to poor network connections, i.e. parent_block.slot + 1 == head_block.slot. If a block was missed in a parent slot then no reorg is enforced.

Also, we should probably open a discussion about the 4s attestation timeline at this point.

I think we should raise it on one of the next calls. It would be nice to see the distribution of attestation and aggregates submitting over time into their respective slot phases.

[dankrad]

There is a constraint on the slot of a parent block as a mitigation to poor network connections, i.e. parent_block.slot + 1 == head_block.slot. If a block was missed in a parent slot then no reorg is enforced.

Yes, but that makes any slot whose parent was not reorged subject to a reorg. Seems pretty harsh!

I think we should raise it on one of the next calls. It would be nice to see the distribution of attestation and aggregates submitting over time into their respective slot phases.

Yeah, it's a good idea, I will check if we can get this data

[arnetheduck]

FWIW, we have so far not implemented this in Nimbus (yet), for the simple reason it's not part of the spec.

+1 that having things like this behind flags dubious at best and should be considered a temporary solution to address urgent problems and not a permanent state.

[potuz]

It modifies the rules on how to vote for a block. This is what a bribery protocol to e.g. censor certain transactions permanently would do, too.

Edit: ah I see by voting you mean the vote the proposer is casting by asserting his head was the previous one. Will leave this comment in case someone else also took "attesting" for the meaning of "voting"

I don't see how this modifies voting rules:: attesters in the orphaned committee would not have attested for the late block cause they sent attestations at 4 seconds. Attesters in the next committee are following the usual forkchoice and proposer boost mandates them to vote for the reorging block.

[potuz]

having things like this behind flags dubious at best and should be considered a temporary solution to address urgent problems and not a permanent state.

Just to clarify this, the only reason Prysm has this behind a flag is to protect from an implementation bug that can cause a liveness issue. We do this with most changes that touch core functions like block production. This has nothing to do with the feature being or not merged in the spec. As far as we were concerned this was thoroughly discussed and agreed in ACD albeit some reservations about disclosing because of certain forkchoice attacks that's were present at the time

[hwwhww]

@michaelsproul

Sorry for the late assignment!

I added some basic test cases, updated test format with should_override_forkchoice_update result, and fixed trivial things.

TODOs

• add the mocking validator_is_connected value to test vector and format
• confirm if test format is ok and run with a client
• check if we should output get_proposer_head result. It is labeled optional so I hesitated. also, consider moving get_proposer_head to a separate place (Allow honest validators to reorg late blocks #3034 (comment))

It would be nice to check again in the ACDC call, and then we can move this PR to "last call" status + merge.

[michaelsproul]

Hi @hwwhww, sorry for the slow reply! Thanks for cleaning this up and adding tests, I really appreciate that 😊

I saw that get_proposer_head is optional in phase0 FC, but here, it's not clear to me if should_override_forkchoice_update is MUST to enable. Could you clarify a bit?

I tried to clarify the wording of this section in 3f1bc20. Both functions are optional, and should_override_forkchoice_update should only be enabled when the proposer reorg feature is enabled. This is how Lighthouse and Prysm work currently.

check if we should output get_proposer_head result. It is labeled optional so I hesitated. also, consider moving get_proposer_head to a separate place

I think the tests should include get_proposer_head as well, as it is arguably more important than should_override_forkchoice_update. If clients don't implement it they can simply skip those checks in their fork choice test runner.

I also think we should just leave it in the current location, as it seems like we may want to make this feature mandatory in future (Potuz mentioned that several PBS designs rely on it, but I haven't dug deeper).

confirm if test format is ok and run with a client

I'll try running the test vectors with Lighthouse soon. Maybe once they also include get_proposer_head checks.

[hwwhww]

@michaelsproul thank you!

I updated the test format: https://github.com/ethereum/consensus-specs/blob/d8440f8bb4496d053a36b5fef64420d5149156bb/tests/formats/fork_choice/README.md#checks-step

1. add get_proposer_head
2. add mocking validator_is_connected into should_override_forkchoice_update check.

Now running the fork-choice testgen.

[hwwhww]

Just a note: this change would modify other test vectors.

[hwwhww]

@michaelsproul

FC-only test vectors of commit d8440f8

• minimal_fc_d8440f8.tar.gz
• mainnet_fc_d8440f8.tar.gz

[michaelsproul]

FC-only test vectors of commit d8440f8

Tests are passing on Lighthouse (sigp/lighthouse#4887). I expected at least one test to fail because Lighthouse doesn't have the REORG_PARENT_WEIGHT_THRESHOLD check. This implies the test coverage could be expanded with a case where the reorg would go ahead if it weren't for the low weight of the parent (i.e. where head_weight < REORG_WEIGHT_THRESHOLD && parent_weight < REORG_PARENT_WEIGHT_THRESHOLD).

[hwwhww]

@michaelsproul

LGTM. I'll try to add more test cases in other PRs!