Proposer LMD Score Boosting by adiasg · Pull Request #2730 · ethereum/consensus-specs

adiasg · 2021-11-19T20:48:42Z

This PR implements a fix for the LMD balancing attacks, as described here.

Note: Old PR for this feature was #2353.

Proposer Score Boost

A new "proposer score boost" has been introduced, which boosts the LMD score of blocks that are received in a timely manner. The boost increases the score of the block by committee_weight // 4 for that slot. If a block for slot X is received in the first SECONDS_PER_SLOT // INTERVALS_PER_SLOT (= 4) seconds of the slot, then the block receives the boost for the duration of slot X. After slot X is over, the boost is removed and the usual LMD score calculations from only attestations are done.

The boost is not applied to blocks that are not received in the first SECONDS_PER_SLOT // INTERVALS_PER_SLOT seconds of their slots, and this PR does not affect LMD score calculations for such blocks.

In the above example:

block B is received in a timely manner and qualifies for the boost. B's score is boosted for only slot X.
block C is not received in a timely manner and does not qualify for the boost.

Summary of Changes

Parameters:
- Added new parameter ATTESTATION_OFFSET_QUOTIENT, which is used to specify the time at which attestations are produced within a slot
Store:
- Added new field store.proposer_score_boost, which is a special LatestMessage that stores the boost
Fork Choice:
- Updated get_latest_attesting_balance to take into account store.proposer_score_boost
- Updated on_block & on_tick to appropriately set/reset store.proposer_score_boost
Testing:
- Added test for store.proposer_score_boost accounting
- Updated existing tests to work correctly after introducing proposer score boosting

adiasg · 2021-11-20T17:40:16Z

Thanks @joachimneu for pointing out that the proposer score boost should allow for more granularity.
The parameter PROPOSER_SCORE_BOOST is now a percentage value.

djrtwo

nice!

A handful of formatting comments. Logic looks generally good

specs/merge/fork-choice.md

specs/phase0/fork-choice.md

specs/phase0/validator.md

specs/phase0/fork-choice.md

Co-authored-by: Danny Ryan <dannyjryan@gmail.com>

djrtwo

nice! minor comments. and one substnative one

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_get_head.py

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py

djrtwo

flagging this as "requesting changes" so we don't merge before changing propser score boost to 70

hwwhww

I pushed commit of configuration invariant checks. (Sorry for ruining it for a couple minutes)

I have a question about the is_before_attesting_interval calculation.
Generally, the approach looks good to me.

specs/merge/fork-choice.md

specs/phase0/fork-choice.md

hwwhww · 2021-11-23T08:51:44Z

specs/phase0/validator.md

 A validator is expected to create, sign, and broadcast an attestation during each epoch. The `committee`, assigned `index`, and assigned `slot` for which the validator performs this role during an epoch are defined by `get_committee_assignment(state, epoch, validator_index)`.

-A validator should create and broadcast the `attestation` to the associated attestation subnet when either (a) the validator has received a valid block from the expected block proposer for the assigned `slot` or (b) one-third of the `slot` has transpired (`SECONDS_PER_SLOT / 3` seconds after the start of `slot`) -- whichever comes _first_.
+A validator should create and broadcast the `attestation` to the associated attestation subnet when either (a) the validator has received a valid block from the expected block proposer for the assigned `slot` or (b) `1 / INTERVALS_PER_SLOT` of the `slot` has transpired (`SECONDS_PER_SLOT / INTERVALS_PER_SLOT` seconds after the start of `slot`) -- whichever comes _first_.


nitpicking: we use SECONDS_PER_SLOT // INTERVALS_PER_SLOT in code and have set SECONDS_PER_SLOT % INTERVALS_PER_SLOT == 0. It could be a bit off when SECONDS_PER_SLOT % INTERVALS_PER_SLOT != 0.

Is SECONDS_PER_SLOT % INTERVALS_PER_SLOT == 0 an invariant that should be checked in test_config_invariants.py?

note that this is division and not integer division.

I don't think this makes sense as a strict config invariant but maybe deserves a note

EDITED: Sorry, I meant "we use SECONDS_PER_SLOT // INTERVALS_PER_SLOT in code ".
^^^^^ typo fixed

note that this is division and not integer division.

Yes. Clarify: so the value that the validator guide describes may be different from the value from the fork choice rule code when SECONDS_PER_SLOT % INTERVALS_PER_SLOT != 0. INTERVALS_PER_SLOT := 3 is fine for our minimal and mainnet config.

Another option is also using integer division here: "... SECONDS_PER_SLOT // INTERVALS_PER_SLOT seconds after the start of slot..."

ah, right. Then it is valuable to have these work well together.

hwwhww

The on_tick call seems incorrect for test vectors.
Proposed fix that you can review and cherry-pick: https://github.com/ethereum/consensus-specs/compare/proposer-score-boost-test-fix

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py

hwwhww

one more on_tick usage issue. otherwise, it looks good.

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_get_head.py

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py

Add `proposer_boost_root` field to test vector "checks" step

@michaelsproul

## Issue Addressed Resolves: #2741 Includes: #2853 so that we can get ssz static tests passing here on v1.1.6. If we want to merge that first, we can make this diff slightly smaller ## Proposed Changes - Changes the `justified_epoch` and `finalized_epoch` in the `ProtoArrayNode` each to an `Option<Checkpoint>`. The `Option` is necessary only for the migration, so not ideal. But does allow us to add a default logic to `None` on these fields during the database migration. - Adds a database migration from a legacy fork choice struct to the new one, search for all necessary block roots in fork choice by iterating through blocks in the db. - updates related to ethereum/consensus-specs#2727 - We will have to update the persisted forkchoice to make sure the justified checkpoint stored is correct according to the updated fork choice logic. This boils down to setting the forkchoice store's justified checkpoint to the justified checkpoint of the block that advanced the finalized checkpoint to the current one. - AFAICT there's no migration steps necessary for the update to allow applying attestations from prior blocks, but would appreciate confirmation on that - I updated the consensus spec tests to v1.1.6 here, but they will fail until we also implement the proposer score boost updates. I confirmed that the previously failing scenario `new_finalized_slot_is_justified_checkpoint_ancestor` will now pass after the boost updates, but haven't confirmed _all_ tests will pass because I just quickly stubbed out the proposer boost test scenario formatting. - This PR now also includes proposer boosting ethereum/consensus-specs#2730 ## Additional Info I realized checking justified and finalized roots in fork choice makes it more likely that we trigger this bug: ethereum/consensus-specs#2727 It's possible the combination of justified checkpoint and finalized checkpoint in the forkchoice store is different from in any block in fork choice. So when trying to startup our store's justified checkpoint seems invalid to the rest of fork choice (but it should be valid). When this happens we get an `InvalidBestNode` error and fail to start up. So I'm including that bugfix in this branch. Todo: - [x] Fix fork choice tests - [x] Self review - [x] Add fix for ethereum/consensus-specs#2727 - [x] Rebase onto Kintusgi - [x] Fix `num_active_validators` calculation as @michaelsproul pointed out - [x] Clean up db migrations Co-authored-by: realbigsean <seananderson33@gmail.com>

adiasg mentioned this pull request Nov 19, 2021

Proposer LMD Score Boosting #2353

Closed

Merge branch 'fc-dev-validate_target_epoch_scope-patch' into dev

97e6d5c

djrtwo reviewed Nov 22, 2021

View reviewed changes

adiasg commented Nov 22, 2021

View reviewed changes

specs/phase0/fork-choice.md Outdated Show resolved Hide resolved

djrtwo mentioned this pull request Nov 22, 2021

minor Merge and forkchoice updates release #2733

Merged

3 tasks

djrtwo and others added 9 commits November 22, 2021 11:11

Merge branch 'dev' of github.com:ethereum/eth2.0-specs into dev

7736c8b

Add proposer score boosting & related tests

2d161b4

Update validator guide with ATTESTATION_OFFSET_QUOTIENT

281c1b2

Add parameter for score boost value

47fa6d1

Add datatype to new parameters

504d82c

Make PROPOSER_SCORE_BOOST a percentage value

b0fb861

Apply suggestions from code review

3b20e3e

Co-authored-by: Danny Ryan <dannyjryan@gmail.com>

This reverts commit 4c726cd.

859bbf4

Apply Danny's code review

88c76ab

djrtwo force-pushed the proposer-score-boost branch from 2de8b3b to 88c76ab Compare November 22, 2021 18:15

minor formatting cleanups

cebe6ba

djrtwo force-pushed the proposer-score-boost branch from 44e064a to cebe6ba Compare November 22, 2021 18:31

djrtwo added 2 commits November 22, 2021 11:31

simplify on_tick proposer boost update

282d85b

toc

ea09df5

djrtwo force-pushed the proposer-score-boost branch from 1e985b5 to ea09df5 Compare November 22, 2021 18:39

djrtwo reviewed Nov 22, 2021

View reviewed changes

adiasg added 2 commits November 22, 2021 14:44

Apply Danny's code review & suggestions

1d835c5

Rename test

d85d439

djrtwo approved these changes Nov 22, 2021

View reviewed changes

add PROPOSER_SCORE_BOOST to configuration yaml files

64b4ca2

djrtwo suggested changes Nov 23, 2021

View reviewed changes

hwwhww force-pushed the proposer-score-boost branch from b823455 to e2c396c Compare November 23, 2021 08:29

Add configuration value checks

bdd7b07

hwwhww force-pushed the proposer-score-boost branch from e2c396c to bdd7b07 Compare November 23, 2021 08:44

hwwhww reviewed Nov 23, 2021

View reviewed changes

hwwhww added the forkchoice label Nov 23, 2021

hwwhww reviewed Nov 23, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py Outdated Show resolved Hide resolved

adiasg and others added 5 commits November 23, 2021 07:02

Apply HWW code's review - fix is_before_attesting_interval

a0b5a80

Apply HWW code's review - properly update test steps

ecbe919

Set PROPOSER_SCORE_BOOST to 70%

2a5c9d8

Merging local branch to remote latest

6f95637

Add proposer_boost_root field to test vector "checks" step

2ba0586

hwwhww approved these changes Nov 23, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_get_head.py Outdated Show resolved Hide resolved

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_on_block.py Show resolved Hide resolved

djrtwo added 2 commits November 23, 2021 11:16

Merge pull request #2736 from ethereum/proposer-score-boost-add-check

40f1c85

Add `proposer_boost_root` field to test vector "checks" step

pr feedback

975931b

djrtwo approved these changes Nov 23, 2021

View reviewed changes

djrtwo merged commit 395fdd4 into dev Nov 23, 2021

djrtwo deleted the proposer-score-boost branch November 23, 2021 18:39

michaelsproul mentioned this pull request Nov 30, 2021

Proposer boosting in fork choice sigp/lighthouse#2838

Closed

dapplion mentioned this pull request Nov 30, 2021

Fork choice consensus updates ChainSafe/lodestar#3438

Closed

2 tasks

realbigsean mentioned this pull request Dec 1, 2021

[Merged by Bors] - v1.1.6 Fork Choice changes sigp/lighthouse#2822

Closed

6 tasks

tbenr mentioned this pull request Dec 3, 2021

[Merge-Phase0] update to specs version 1.1.6 Consensys/teku#4743

Closed

4 tasks

twoeths mentioned this pull request Dec 17, 2021

Proposer LMD Score Boosting ChainSafe/lodestar#3535

Closed

casparschwa mentioned this pull request May 4, 2022

how does pos ethereum prevent bribery for late block proposal? flashbots/mev-boost#111

Open

1 task

adiasg added the security General protocol security-related items label May 11, 2023

Conversation

adiasg commented Nov 19, 2021 • edited by djrtwo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposer Score Boost

Summary of Changes

Uh oh!

adiasg commented Nov 20, 2021

Uh oh!

djrtwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djrtwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djrtwo left a comment

Choose a reason for hiding this comment

Uh oh!

hwwhww left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hwwhww Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djrtwo Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

hwwhww Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

djrtwo Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

hwwhww left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hwwhww left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adiasg commented Nov 19, 2021 •

edited by djrtwo

Loading

hwwhww Nov 23, 2021 •

edited

Loading

hwwhww left a comment •

edited

Loading