-
Notifications
You must be signed in to change notification settings - Fork 266
feat(withdrawals): Allow for Disabling Withdrawals [DO NOT MERGE] #2777
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 18 commits
11d3adc
e141967
c416f12
8f81a38
1f9b9c9
eb5df2c
368fd7e
f8b2016
b84c9ea
8c10052
7f0ae9b
cbffee5
6f76b58
0485ae0
23bd6d5
c018962
31f1bcb
b013aea
0cca53e
f366782
dc4ed01
32200e0
8a5b80d
79e3e61
23daa35
f0bd3ac
f5d3115
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 |
|---|---|---|
|
|
@@ -27,6 +27,12 @@ import ( | |
| ) | ||
|
|
||
| // ActiveForkVersionForTimestamp returns the active fork version for a given timestamp. | ||
| // Note that ActiveForkVersionForTimestamp will NOT check for | ||
| // - ElectraDisableWithdrawalsForkTime | ||
| // - ElectraEnableWithdrawalsForkTime | ||
| // | ||
| // As there is no logic that relies on the above forks that goes through ActiveForkVersionForTimestamp. | ||
| // It also gives flexibility for these forks to occur at any point after Electra, e.g. after Electra1. | ||
| func (s spec) ActiveForkVersionForTimestamp(timestamp math.U64) common.Version { | ||
| time := timestamp.Unwrap() | ||
| if time >= s.ElectraForkTime() { | ||
|
|
@@ -38,6 +44,16 @@ func (s spec) ActiveForkVersionForTimestamp(timestamp math.U64) common.Version { | |
| return version.Deneb() | ||
| } | ||
|
|
||
| // WithdrawalsEnabled is a switch that can be used to freeze withdrawals in an emergency scenario. | ||
| // An exception is made for the EVM inflation withdrawal which is always active. | ||
| func (s spec) WithdrawalsEnabled(timestamp math.U64) bool { | ||
| time := timestamp.Unwrap() | ||
| if time >= s.ElectraDisableWithdrawalsForkTime() && time < s.ElectraEnableWithdrawalsForkTime() { | ||
| return false | ||
| } | ||
| return true | ||
| } | ||
|
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. nit: we are really defining a window where withdrawals are disabled, so we may consider a negative logic here and have a WithdrawalDisabled method |
||
|
|
||
| // GenesisForkVersion returns the fork version at genesis. | ||
| func (s spec) GenesisForkVersion() common.Version { | ||
| return s.ActiveForkVersionForTimestamp(math.U64(s.GenesisTime())) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,7 +56,10 @@ const ( | |
| defaultTargetSecondsPerEth1Block = 2 // Berachain specific. | ||
|
|
||
| // Fork-related values. | ||
| defaultElectraForkTime = 9999999999999999 // Set as a future timestamp as not yet determined. | ||
| defaultFarFutureTimestamp = 9999999999999999 // a future timestamp as not yet determined. | ||
| defaultElectraForkTime = defaultFarFutureTimestamp | ||
| defaultElectraDisableWithdrawalsForkTime = defaultFarFutureTimestamp | ||
|
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. To enable and disable on ETH Mainnet, update these values to a relevant timestamp
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. |
||
| defaultElectraEnableWithdrawalsForkTime = defaultFarFutureTimestamp | ||
|
|
||
| // State list length constants. | ||
| defaultEpochsPerHistoricalVector = 8 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,8 +67,10 @@ func (sp *StateProcessor) processOperations( | |
| } | ||
| } | ||
|
|
||
| // After Electra, validators can request withdrawals through execution requests which must be handled. | ||
| // If withdrawals are enabled, process the withdrawals. Otherwise, the withdrawal requests are ignored | ||
| // to prevent withdrawals from excessively queuing up. | ||
|
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. is this true? It seems to me we just let withdrawals be enqueued rn.
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. good catch, i removed the logic but forgot to update comment |
||
| if version.EqualsOrIsAfter(blk.GetForkVersion(), version.Electra()) { | ||
| // After Electra, validators can request withdrawals through execution requests which must be handled. | ||
| requests, err := blk.GetBody().GetExecutionRequests() | ||
| if err != nil { | ||
| return err | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,8 @@ target-seconds-per-eth1-block = 2 | |
| genesis-time = 1_737_381_600 | ||
| deneb-one-fork-time = 1_738_415_507 | ||
| electra-fork-time = 9_999_999_999_999_999 | ||
| electra-disable-withdrawals-fork-time = 9_999_999_999_999_999 | ||
| electra-enable-withdrawals-fork-time = 9_999_999_999_999_999 | ||
|
Comment on lines
+38
to
+39
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. Also update these these value to configure the fork on mainnet |
||
|
|
||
| # State list lengths | ||
| epochs-per-historical-vector = 8 | ||
|
|
||
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.
we should enforce somewhere that Disable Time <= Enable Time
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.
Also wonder if we should make scaffolding for multiple periods of withdrawal pause?
Uh oh!
There was an error while loading. Please reload this page.
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.
Also consider renaming this to
DisableWithdrawalsRequestsWindowStartandDisableWithdrawalsRequestsWindowEnd.I do not believe this is strictly related to Electra.
Also we should stress this is a disabling window
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.
Also, in terms of operations we just need to specify the start window at first, right? We can leave the end to farFuture and reset that once the window is closed
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 should be done as a larger ordering enforcement of forks. e.g. geth https://github.com/ethereum/go-ethereum/blob/516451dc3a514c7c122f28864ea76742a027b858/params/config.go#L702
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.
correct
Uh oh!
There was an error while loading. Please reload this page.
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.
Enforcement of fork timestamps for now can be done in the
(s spec) validate() errorfunction we already have. Good way to make use of that and I would also recommend taking this opportunity to add it in.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.
#2790