-
Notifications
You must be signed in to change notification settings - Fork 1.1k
EIP-7917: Deterministic proposer lookahead #4190
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-7917: Deterministic proposer lookahead #4190
Conversation
59dc696
to
769ea3d
Compare
769ea3d
to
fb3cd63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an immediate step will be targeting fulu
instead of electra
:)
Co-authored-by: Alex Stokes <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job Lin :)
Proposer lookahead is a new constraint for the protocol which for instance constraining approaches with secret leader election that has been explored in the past. If we accept this change, we should explicitly mention that we give up on anything like SSLE unless I misunderstood something |
Thanks for bringing this up! Indeed, SSLE is something to consider - But I am not sure if we have to go so far as to "give up" on SSLE with this change. This PR is just making the existing design of having multiple epochs worth of lookahead (having |
Each new protocol constraint is hard to remove when it becomes a downstream dependency. One of the ways to go would probably be to say that there can be no lookahead if SSLE is introduced in the future, explicitly emphasising that this feature is unreliable long term |
Makes sense, rather than removing the field, just having an empty list for
This makes sense too, and I agree it's worth mentioning in specs and EIP. Thanks for the feedback! |
Is there a security analysis anywhere for this feature? There are good descriptions of the current mechanism on @benjaminion Describing how the preferred solution would be to get proposal rights JIT, which this PR takes us further away from it. Besides a security analysis of knowing the proposing schedule ahead of time, I agree with @mkalinin that this proposal seems to give up on SSLE as it's currently developed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: wondering if it makes sense to update get_beacon_proposer_index
to use state.proposer_lookahead
feels like a nice optimization here
Thanks for the feedback @potuz !
Yeah, security definitely needs consideration, and I’ll add a section in the upcoming EIP. In short, this proposal does not alter the “RANDAO delay” used in the lookahead—the lookahead of epoch And actually, by aligning the RANDAO and effective balances in this way, the proposal removes any chance of validators adjusting their EB after seeing the RANDAO outcome, which is an attack vector to consider. No such attack has been found so far, but this change removes the possibility, hence simplifying the security analysis.
I don’t think we need to "give up" on SSLE. From what I’ve heard, the currently proposed SSLE constructions still include a lookahead, albeit an encrypted one. In those designs, we could reuse the That said, any such changes would introduce additional complexity around preconfs, but that complexity arises regardless of this change. |
Makes total sense! And actually we already do this here: https://github.com/ethereum/consensus-specs/pull/4190/files#diff-bcf6567bc8bd905058f44340eeb4ec690c396543644748b7fb5918e4e4d15141R193 |
RE SSLE: The key feature of secret leader election is: "the proposer at slot N is only known by the proposer at slot N with some lookahead". Then no-one can DoS the proposer, only if the proposer keeps its proposal slot private. This is completely opposite to the pre-confirmation requirement: "users (i.e. everyone) know the proposer at slot N with some lookahead". If we enshrine the users' need to know the proposer ahead of time, under SSLE the proposer will be incentivized to make its proposal public. Implementing SSLE under this reality is technically possible but probably useless. Unless there's some intermediary that's doubly trusted (i.e. the builder) to whom the proposal can reveal its proposal slot, and then the builder somehow handles the pre-conf. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really great @linoscope, thanks for your work on this!
Now the generator passes, and I added some prints and it passes in the first round with 4 epochs created at the beginning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this EIP should also modify get_proposer_head
, should_override_fcu
and is_shuffling_stable
. They may be backported to phase0
perhaps, but they need to be modified nonetheless.
What happens if the looked ahead approver is unable to validate the block (e.g. disconnected)? Also, shouldn't approver be prevented from changing his EB during his epoch (e.g. withdrawal)? |
specs/fulu/beacon-chain.md
Outdated
pending_deposits: List[PendingDeposit, PENDING_DEPOSITS_LIMIT] | ||
pending_partial_withdrawals: List[PendingPartialWithdrawal, PENDING_PARTIAL_WITHDRAWALS_LIMIT] | ||
pending_consolidations: List[PendingConsolidation, PENDING_CONSOLIDATIONS_LIMIT] | ||
proposer_lookahead: List[ValidatorIndex, (MIN_SEED_LOOKAHEAD + 1) * SLOTS_PER_EPOCH] # [New in Fulu:EIP7917] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a list instead of a vector?, if this is a list it is pretty weird to have a tight limit for it. I would go with an SSZ Vector in this case, but if we insist in having a list please set a larger limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, indeed a vector is more natural then a list, as it is fixed in size. Will change to vector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to vector in c8811a0
#### New `compute_proposer_indices` | ||
|
||
```python | ||
def compute_proposer_indices(state: BeaconState, epoch: Epoch, seed: Bytes32, indices: Sequence[ValidatorIndex]) -> List[ValidatorIndex, SLOTS_PER_EPOCH]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All calls to this function explicitly compute seed
at the callsite, it is cleaner to have a different signature only taking the state and epoch and computing the seed within this function.
Moreover, the same applies to the indices argument, this function should be defined as follows
def compute_proposer_indices(state: BeaconState, epoch: Epoch) -> List[ValidatorIndex, SLOTS_PER_EPOCH]:
"""
Return the proposer indices for the given ``epoch``.
"""
start_slot = compute_start_slot_at_epoch(epoch)
seed = get_seed(state, epoch, DOMAIN_BEACON_PROPOSER)
seeds = [hash(seed + uint_to_bytes(Slot(start_slot + i))) for i in range(SLOTS_PER_EPOCH)]
indices = get_active_validator_indices(state, epoch)
return [compute_proposer_index(state, indices, seed) for seed in seeds]
``
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current interface was from a suggestion to make it more in line with the interface of compute_proposer_index
, which takes the seed
and indices
in the argument. Personally don't have strong preference either way, but I do see the benefit of reducing repeated logic in callsites, so right now slightly in favor of reverting back to compute_proposer_indices(state: BeaconState, epoch: Epoch)
interface as @potuz suggests. What do you think @gfukushima ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current spec, compute_proposer_index()
is meant to be a private/internal/utility function that is only used by get_beacon_proposer_index()
.
The usage of compute_proposer_indices()
is more similar to get_beacon_proposer_index()
than compute_proposer_index()
. Delegating the seed calculation to the caller is not really necessary imo.
If we really want to be consistent with the current spec,
we should have get_beacon_proposer_indices(state, epoch)
which calls compute_proposer_indices (state, epoch, seed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks both for the input!
The usage of compute_proposer_indices() is more similar to get_beacon_proposer_index() than compute_proposer_index(). Delegating the seed calculation to the caller is not really necessary imo.
Makes sense.
Made the call to circle back to having seeds
and indicies
in compute_proposer_indicies
here: e9266b2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @ensi321 what I was trying to tell you with my initial comment is that the initial implementation was introducing a circular dependency, having a method in the misc helpers (compute_proposer_indices
) depending on methods of the beacon state accessor (get_seed
,get_active_validator_indices
) would be unusual (if not a bad practice). It is usually the other way around, beacon state accessor can call misc helpers methods. Having said that I think @ensi321 suggestion is the closest I would agree with but with a slight modification: get_beacon_proposer_indices(state, epoch)
should call compute_proposer_indices (state, epoch, seed, indices)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having a method in the misc helpers (compute_proposer_indices) depending on methods of the beacon state accessor (get_seed,get_active_validator_indices) would be unusual (if not a bad practice)
Thanks for the explanation, I see your point, makes sense. Will make this change to remove the circular dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor and introduction of get_beacon_proposer_indices
accessor done here:
79bf80d
Thanks everyone for the input!
Had a nice discussion in Discord, copy-and-pasting my response/conclusion here for visibility: I am having trouble to see whether we can remove
If it's about attestations and justifications, I don't think EIP-7917, which is about proposer lookahead, is relevant? The issue around "having just enough attesations to justify" at epoch boundaries seems still needed even with the new EIP. If anything I think the initial Unless we can say it is 100% safe to remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Regarding
Anyway, I think it can be merged with this and we can find out about this in another PR. We also have some tests that I want to add but I think we can do it in another PR, I don't have rights to create a PR againts this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Hey @leolara, yup let's follow up with more tests. I have a couple more in mind too. |
Good question. I assumed yields with the same keys (e.g., "pre") overwrite the old one but haven't confirmed (perhaps @jtraglia knows).
We did face issue due to the non-deterministic RANDAO-dependent nature of the test before. Namely, tests would succeed when running in
Nice, thanks a lot! Looking forward to reviewing those. I also have some test ideas to potentially add (see if we properly handle active validator set changes), but I think that doesn't need to block this one too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This is the first implementation of EIP-7917. We will want to have a plan deprecate proposer caches in `EpochCache` since Fulu beacon state will carry those information. This will be addressed in a later PR. Pending spec release for spec test Spec PR ethereum/consensus-specs#4190 --------- Co-authored-by: Nico Flaig <[email protected]>
This is the first implementation of EIP-7917. We will want to have a plan deprecate proposer caches in `EpochCache` since Fulu beacon state will carry those information. This will be addressed in a later PR. Pending spec release for spec test Spec PR ethereum/consensus-specs#4190 --------- Co-authored-by: Nico Flaig <[email protected]>
Edit: Ethereum Magicians post for any general discussion around the EIP: https://ethereum-magicians.org/t/eip-7917-deterministic-proposer-lookahead/23259
Currently, the proposer lookahead for the next epoch is unstable because the effective balance (EB) may change before the next epoch due to slashing events, deposits, or withdrawals occurring in the current epoch. The recent increase in the maximum EB further exacerbates this instability, as EB can now exceed the previous 32 ETH cap. This instability complicates the operation of preconf protocols, as described in this document: https://hackmd.io/@linoscope/preconf-lookahead
In this PR, we address this issue by stabilizing the proposer lookahead for the next epoch. We achieve this by calculating and recording the lookahead for epoch
N+1
in the beacon state at the beginning of epochN
.Furthermore, a highly valuable side effect of this approach is that the proposer lookahead becomes available within the beacon state, making it possible to retrieve the lookahead in the EVM using the beacon root and Merkle proofs. Having direct access to lookahead in the EVM greatly simplifies implementing the on-chain components of preconf protocols.
TODO: