-
Notifications
You must be signed in to change notification settings - Fork 266
feat(chain): implement BRIP-0008 hysteresis update for Fulu fork #3081
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
bc4f5b1
095433f
36c3ee0
75c2286
21e8648
4e3c20d
dc79669
a29b833
4c9b65b
3cdc138
adeb522
b8bad56
4d88d47
872e2dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -48,15 +48,17 @@ type HysteresisSpec interface { | |||||
| // HysteresisQuotient returns the quotient used in effective balance | ||||||
| // calculations to create hysteresis. This provides resistance to small | ||||||
| // balance changes triggering effective balance updates. | ||||||
| HysteresisQuotient() math.U64 | ||||||
| // The value is fork-gated by timestamp (updated in Fulu per BRIP-0008). | ||||||
| HysteresisQuotient(timestamp math.U64) math.U64 | ||||||
|
|
||||||
| // HysteresisDownwardMultiplier returns the multiplier used when checking | ||||||
| // if the effective balance should be decreased. | ||||||
| HysteresisDownwardMultiplier() math.U64 | ||||||
|
|
||||||
| // HysteresisUpwardMultiplier returns the multiplier used when checking | ||||||
| // if the effective balance should be increased. | ||||||
| HysteresisUpwardMultiplier() math.U64 | ||||||
| // The value is fork-gated by timestamp (updated in Fulu per BRIP-0008). | ||||||
| HysteresisUpwardMultiplier(timestamp math.U64) math.U64 | ||||||
| } | ||||||
|
|
||||||
| type DepositSpec interface { | ||||||
|
|
@@ -109,6 +111,9 @@ type ForkSpec interface { | |||||
|
|
||||||
| // Electra1ForkTime returns the time at which the Electra1 fork takes effect. | ||||||
| Electra1ForkTime() uint64 | ||||||
|
|
||||||
| // FuluForkTime returns the time at which the Fulu fork takes effect. | ||||||
| FuluForkTime() uint64 | ||||||
| } | ||||||
|
|
||||||
| type BlobSpec interface { | ||||||
|
|
@@ -269,6 +274,7 @@ func (s spec) validate() error { | |||||
| s.Data.Deneb1ForkTime, | ||||||
| s.Data.ElectraForkTime, | ||||||
| s.Data.Electra1ForkTime, | ||||||
| s.Data.FuluForkTime, | ||||||
| } | ||||||
| for i := 1; i < len(orderedForkTimes); i++ { | ||||||
| prev, cur := orderedForkTimes[i-1], orderedForkTimes[i] | ||||||
|
|
@@ -332,15 +338,23 @@ func (s spec) EffectiveBalanceIncrement() math.Gwei { | |||||
| return math.Gwei(s.Data.EffectiveBalanceIncrement) | ||||||
| } | ||||||
|
|
||||||
| func (s spec) HysteresisQuotient() math.U64 { | ||||||
| func (s spec) HysteresisQuotient(timestamp math.U64) math.U64 { | ||||||
| fv := s.ActiveForkVersionForTimestamp(timestamp) | ||||||
| if version.EqualsOrIsAfter(fv, version.Fulu()) { | ||||||
| return math.U64(s.Data.HysteresisQuotientFulu) | ||||||
| } | ||||||
| return math.U64(s.Data.HysteresisQuotient) | ||||||
| } | ||||||
|
|
||||||
| func (s spec) HysteresisDownwardMultiplier() math.U64 { | ||||||
| return math.U64(s.Data.HysteresisDownwardMultiplier) | ||||||
| } | ||||||
|
|
||||||
| func (s spec) HysteresisUpwardMultiplier() math.U64 { | ||||||
| func (s spec) HysteresisUpwardMultiplier(timestamp math.U64) math.U64 { | ||||||
| fv := s.ActiveForkVersionForTimestamp(timestamp) | ||||||
| if version.EqualsOrIsAfter(fv, version.Fulu()) { | ||||||
| return math.U64(s.Data.HysteresisUpwardMultiplierFulu) | ||||||
| } | ||||||
| return math.U64(s.Data.HysteresisUpwardMultiplier) | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -447,6 +461,11 @@ func (s spec) Electra1ForkTime() uint64 { | |||||
| return s.Data.Electra1ForkTime | ||||||
| } | ||||||
|
|
||||||
| // FuluForkTime returns the timestamp of the Fulu fork. | ||||||
| func (s spec) FuluForkTime() uint64 { | ||||||
| return s.Data.FuluForkTime | ||||||
| } | ||||||
|
|
||||||
| // EpochsPerHistoricalVector returns the number of epochs per historical vector. | ||||||
| func (s spec) EpochsPerHistoricalVector() uint64 { | ||||||
| return s.Data.EpochsPerHistoricalVector | ||||||
|
|
@@ -518,10 +537,12 @@ func (s spec) ValidatorSetCap() uint64 { | |||||
| // inflation amount of native EVM balance through a withdrawal every block. | ||||||
| func (s spec) EVMInflationAddress(timestamp math.U64) common.ExecutionAddress { | ||||||
| fv := s.ActiveForkVersionForTimestamp(timestamp) | ||||||
| switch fv { | ||||||
| case version.Deneb1(), version.Electra(), version.Electra1(): | ||||||
| switch { | ||||||
| case version.EqualsOrIsAfter(fv, version.Fulu()): | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we not check for exact version here? If we add Fulu1 (or future fork) then we would silently return here instead of panicking which is how the switch was written explicitly before.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well I'd argue it's more correct now. For example the value we set in Deneb1 Also adjusted other functions to work like this in 4d88d47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that, but I am concerned there may be a slight chance that we miss updating all call sites when adding a non compatible fork. Before this PR, we would see a panic immedietly and then have to evaluate if its valid to current handling (adding the fork to the case) , or whether it needs a separate handling (like here for fulu).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a chain spec value needs to be updated via hard fork, there needs to be an explicit change via a PR. Hence why we don't create a new variable for every value of the chain spec in every new fork. If a chain spec value is being called on an unsupported fork version, then we should panic. IMO this convention means we actually always implicitly "evaluate" the necessary conditions. all that being said, its pretty nitpicky. We can make this change in another PR throughout the repo to maintain the same standard everywhere. Will revert back to old way on this PR for now. |
||||||
| return s.Data.EVMInflationAddressFulu | ||||||
| case version.EqualsOrIsAfter(fv, version.Deneb1()): | ||||||
| return s.Data.EVMInflationAddressDeneb1 | ||||||
| case version.Deneb(): | ||||||
| case version.Equals(fv, version.Deneb()): | ||||||
| return s.Data.EVMInflationAddressGenesis | ||||||
| default: | ||||||
| panic(fmt.Sprintf("EVMInflationAddress not supported for this fork version: %d", fv)) | ||||||
|
|
@@ -532,10 +553,12 @@ func (s spec) EVMInflationAddress(timestamp math.U64) common.ExecutionAddress { | |||||
| // be minted to the EVMInflationAddress via a withdrawal every block. | ||||||
| func (s spec) EVMInflationPerBlock(timestamp math.U64) math.Gwei { | ||||||
| fv := s.ActiveForkVersionForTimestamp(timestamp) | ||||||
| switch fv { | ||||||
| case version.Deneb1(), version.Electra(), version.Electra1(): | ||||||
| switch { | ||||||
| case version.EqualsOrIsAfter(fv, version.Fulu()): | ||||||
| return math.Gwei(s.Data.EVMInflationPerBlockFulu) | ||||||
| case version.EqualsOrIsAfter(fv, version.Deneb1()): | ||||||
| return math.Gwei(s.Data.EVMInflationPerBlockDeneb1) | ||||||
| case version.Deneb(): | ||||||
| case version.Equals(fv, version.Deneb()): | ||||||
| return math.Gwei(s.Data.EVMInflationPerBlockGenesis) | ||||||
| default: | ||||||
| panic(fmt.Sprintf("EVMInflationPerBlock not supported for this fork version: %d", fv)) | ||||||
|
|
||||||
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.
Looking at the code its unclear why HysteresisUpwardMultiplier takes an argument but HysteresisDownwardMultiplier does not. Since HysteresisDownwardMultiplier does not change in the fork there is no need to, but maybe add an inline comment (or pass a unused timetsamp)
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.
Added comment in 3cdc138