Implement EIP-7251 #5507
Implement EIP-7251 #5507
Conversation
ethDreamer
added
work-in-progress
| VariableList<PendingBalanceDeposit, T::PendingBalanceDepositsLimit>, | ||
| #[superstruct(only(Electra))] | ||
| pub pending_partial_withdrawals: | ||
| VariableList<PartialWithdrawal, T::PendingPartialWithdrawalsLimit>, |
There was a problem hiding this comment.
Updated to PendingPartialWithdrawal
| | &BeaconState::Altair(_) | ||
| | &BeaconState::Merge(_) | ||
| | &BeaconState::Capella(_) | ||
| | &BeaconState::Deneb(_) => { |
There was a problem hiding this comment.
Why not do fork >= deneb { new logic } else { current logic }? No change required for the next fork.
There was a problem hiding this comment.
In general I try to make fork-related patterns enumerate the possibilities so as to draw our attention to things that have changed from one fork to the next. When we implement new forks, the compiler will draw your attention to places where things change. Match statements of this kind also turn the code into a kind of living documentation of where things change in which fork.
Consider the activation churn limit for example.
There was a problem hiding this comment.
Before joining LH, I hated this. There's an insane amount of boilerplate that you force everyone to deal with when they may not have to. A new fork developer will know where and when to change code, and it is almost always covered by spec tests.
There was a problem hiding this comment.
I think we should put in effort to reduce this boiler plate, this is an example external contrib EIP implementation where it's really hard to even find the changes related to the EIP: https://github.com/kevinbogner/lighthouse/pull/3/files . Trying to merge this in post-electra boiler plate would probably be more painful than re-implementing it.
From our POV it's a once-per fork pain but it can be a per-POC pain for any LH contributors. Maybe adding a generic and permanent "NextFork" would help here too. Kind of ugly though.
There was a problem hiding this comment.
leaning on fork order does make sense, we're attempting to embedding fork order into superstructs: sigp/superstruct#33
the places we've been burned on not doing complete matches are usually in networking code, not consensus code. We can evaluate on a case-by-case basis but I think so far it's been easier to default to complete matches only.
There was a problem hiding this comment.
state transition / networking is a good place to draw the line, since state transition is covered by spec tests. In Lodestar we use fork comparisons and have never had related bugs to forgetting this for forks.
| .pending_partial_withdrawals_mut() | ||
| .map(|ppw| Vec::from(std::mem::replace(ppw, Vec::new().into()))) | ||
| { | ||
| pending_partial_withdrawals.drain(0..partial_withdrawals_count); |
There was a problem hiding this comment.
This could be quite expensive with tree states done naively. It's possible to do a specialized operation on milhouse that splits the tree at some index while potentially preserving part of the hash cache @michaelsproul
@ethDreamer can you add a TODO(EIP7251) to not forget?
| balance.safe_sub(balance.safe_rem(spec.effective_balance_increment)?)?, | ||
| spec.max_effective_balance, | ||
| ); | ||
| let new_effective_balance = match state_fork { |
There was a problem hiding this comment.
Would be more maintainable to to Ord on the fork here
|
|
||
| if let Some(participation_cache) = maybe_participation_cache { | ||
| // TODO: consider changes to progressive balances cache |
There was a problem hiding this comment.
Annotated all todos with TODO(EIP7251) or TODO(maxeb)
| mod pending_balance_deposits; | ||
| mod pending_consolidations; | ||
|
|
||
| pub fn process_epoch<T: EthSpec>( |
There was a problem hiding this comment.
Hold for epoch-single pass and build on top of it
| earliest_exit_epoch, | ||
| consolidation_balance_to_consume, | ||
| earliest_consolidation_epoch, | ||
| pending_balance_deposits: VariableList::default(), |
There was a problem hiding this comment.
missing logic to skim balances of already 0x02 vals
| } | ||
| } | ||
|
|
||
| /// TODO: consider removing fork guard? |
There was a problem hiding this comment.
Instead of matching on the fork you should match on self.pending_balance_deposits_mut() returning Ok()
| if self.has_compounding_withdrawal_credential(spec) { | ||
| spec.max_effective_balance_eip7251 | ||
| } else { | ||
| // FIXME: does this need to be made fork-aware? |
There was a problem hiding this comment.
Depending on where it's used, yes. 0x02 validators before the fork will behave as if they had 32 ETH maxeb
Issue Addressed
Implement EIP-7251