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

EIP-7045: Increase max attestation inclusion slot #3360

Merged
merged 21 commits into from
Jun 21, 2023
Merged

EIP-7045: Increase max attestation inclusion slot #3360

merged 21 commits into from
Jun 21, 2023

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented May 15, 2023

Pretty much a two line change in the state transition spec:

  1. expand attestation slot inclusion range to be the current and next epoch -- change
  2. set target timeliness flag if target correct at all disregarding inclusion delay (other than just process_attestation validly including in it's range) -- change

As well as the modificaiton of the p2p gossip conditions to support this.

The rest is generally copy paste functions and a lot of boiler plate.

As for testing, I've made the relevant process_attestation tests be for the max_inclusion_slot which is spec/fork aware. As well as adding one explicit test for SLOTS_PER_EPOCH inclusion that should be valid for all forks

todo:

  • additional testing
    • integration test that spans a fork boundary. Attestation created in N that can be included at end of epoch N+1 where N+1 is the fork epoch

@djrtwo djrtwo force-pushed the att-slot-range branch 4 times, most recently from d542350 to 786ccbe Compare May 15, 2023 17:30
.circleci/config.yml Outdated Show resolved Hide resolved
@hwwhww hwwhww added the general:enhancement New feature or request label May 16, 2023
@djrtwo djrtwo changed the title [WIP] expand attestation slot range expand attestation slot range May 17, 2023

### Block processing

#### Modified `process_attestation`
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how much it matters, but in the previous feature-specific spec, the mentioned changes propagated up to process_operation than process_block. It's a little unnecessary, but wanted to point that out

@djrtwo djrtwo changed the title expand attestation slot range Increase max attestation inclusion slot May 18, 2023
@djrtwo djrtwo changed the title Increase max attestation inclusion slot EIP-7045: Increase max attestation inclusion slot May 18, 2023
.circleci/config.yml Outdated Show resolved Hide resolved
specs/_features/eip7045/fork.md Outdated Show resolved Hide resolved
specs/deneb/beacon-chain.md Outdated Show resolved Hide resolved
@ppopth
Copy link
Member

ppopth commented Jun 15, 2023

Why do we do this only for the target timeliness flag only but not the source timeliness flag as well? I saw this line

    if is_matching_source and inclusion_delay <= integer_squareroot(SLOTS_PER_EPOCH):

@djrtwo
Copy link
Contributor Author

djrtwo commented Jun 15, 2023

Why do we do this only for the target timeliness flag only but not the source timeliness flag as well? I saw this line

    if is_matching_source and inclusion_delay <= integer_squareroot(SLOTS_PER_EPOCH):

We want validators to be rewarded for correct FFG vote no matter when it makes it on chain -- so anytime in the maximum range.

Source correctness reward "splits the difference" between the timeliness of head and the timeliness of target in the chances to get rewarded when included on chain. This doesn't change that consideration

@arnetheduck
Copy link
Contributor

Raised during the call: are there designated aggregators listening on the right subnet that can pick up these attestations when they come late?

@ralexstokes ralexstokes mentioned this pull request Jun 15, 2023
@hwwhww hwwhww added the Deneb was called: eip-4844 label Jun 16, 2023
@djrtwo
Copy link
Contributor Author

djrtwo commented Jun 18, 2023

Raised during the call: are there designated aggregators listening on the right subnet that can pick up these attestations when they come late?

Proposers are already and would continue to be the aggregators of last resort for late or non-previously-picked-up attestations. This is clearly imperfect due to a given stretch of proposers not having perfect coverage off attestation subnets, but 7045 is a strict improvement by giving both more time and more potential proposers to pick up missing attestations.

Altered networking, late aggregators, etc is orthogonal to the issue of extending this deadline, as the problem currently exists with the 32-slot inclusion deadline and at least is stable if not improvved with the extended deadline here.

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

Note to myself:
The fork-choice on_attestation validation only requires assert target.epoch in [current_epoch, previous_epoch], where the current epoch is the computed with store. So it should be irrelevant to this change.

This PR looks good to me. Just some proofread comments. 👍

specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
specs/deneb/p2p-interface.md Outdated Show resolved Hide resolved
@djrtwo djrtwo merged commit 95f36d9 into dev Jun 21, 2023
@djrtwo djrtwo deleted the att-slot-range branch June 21, 2023 14:31
@djrtwo djrtwo mentioned this pull request Jun 21, 2023
5 tasks
@mkalinin mkalinin mentioned this pull request Jul 17, 2023
12 tasks
arnetheduck added a commit to status-im/eth2.0-specs that referenced this pull request Mar 19, 2024
ethereum#3360 effectively
extends the valid lifetime of an attestation/aggregate to 2 epochs -
this means that an aggregate that was published at the beginning of a
slot now is valid per the gossip rules up to 2 epochs later.

Then net effect of the above change is that peers are allowed to
republish old aggregates and attestations and libp2p will not stop the
spread with the settings we recommend - instead the messages will have
to be stopped with the "attestation cover rule" or similar, even though
they have been observed already.

Significant amounts of this kind of spam have been observed on the
aggregate channel in particular leading to a 5x increase in aggregate
traffic as some clients republish these old messages in spite of the
"attestation cover rule" which should have stopped them - this simple
change will provide an additional layer of protection against such bugs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844 general:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants