feat(withdrawals): Allow for Disabling Withdrawals [DO NOT MERGE] by rezzmah · Pull Request #2777 · berachain/beacon-kit

rezzmah · 2025-05-19T23:21:35Z

This PR sets up the relevant boilerplate such that withdrawals can be disabled if needed

DO NOT MERGE till needed

codecov · 2025-05-19T23:23:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.64%. Comparing base (6254669) to head (f5d3115).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2777      +/-   ##
==========================================
+ Coverage   60.53%   60.64%   +0.10%     
==========================================
  Files         351      351              
  Lines       16235    16260      +25     
  Branches       22       22              
==========================================
+ Hits         9828     9861      +33     
+ Misses       5653     5645       -8     
  Partials      754      754

Files with missing lines	Coverage Δ
chain/helpers.go	`100.00% <100.00%> (ø)`
chain/spec.go	`76.00% <100.00%> (+1.21%)`	⬆️
config/spec/devnet.go	`100.00% <100.00%> (ø)`
config/spec/mainnet.go	`100.00% <100.00%> (ø)`
config/spec/testnet.go	`100.00% <100.00%> (ø)`
state-transition/core/state/statedb.go	`64.21% <100.00%> (+1.05%)`	⬆️
state-transition/core/state_processor_staking.go	`61.42% <ø> (-0.28%)`	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

abi87 · 2025-05-20T12:01:09Z

+			 fv := s.ActiveForkVersionForTimestamp(timestamp)
+			 switch fv {
+			 case version.Electra1():
+				 return false
+			 default:
+			 	return true


I get the idea but not sure this would be backward, if we activate Pectra and some withdrawals are made before this hard fork. We'd have to go by slot I think? Or timestamp

abi87

I would expect a proper hard fork, with a (list of) timestamps specified in specs for when withdrawals should be activate/disactivated right?

abi87 · 2025-05-21T09:04:51Z

+	// ElectraDisableWithdrawalsForkTime is the time at which withdrawals were first disabled (if disabled).
+	ElectraDisableWithdrawalsForkTime uint64 `mapstructure:"electra-disable-withdrawals-fork-time"`
+	// ElectraEnableWithdrawalsForkTime is the time at which withdrawals were enabled after disabling
+	ElectraEnableWithdrawalsForkTime uint64 `mapstructure:"electra-enable-withdrawals-fork-time"`


we should enforce somewhere that Disable Time <= Enable Time

Also wonder if we should make scaffolding for multiple periods of withdrawal pause?

Also consider renaming this to DisableWithdrawalsRequestsWindowStart and DisableWithdrawalsRequestsWindowEnd.
I do not believe this is strictly related to Electra.
Also we should stress this is a disabling window

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

we should enforce somewhere that Disable Time <= Enable Time

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

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

correct

Enforcement of fork timestamps for now can be done in the (s spec) validate() error function we already have. Good way to make use of that and I would also recommend taking this opportunity to add it in.

abi87 · 2025-05-21T09:05:39Z

+func (s spec) WithdrawalsEnabled(timestamp math.U64) bool {
+	time := timestamp.Unwrap()
+	if time >= s.ElectraDisableWithdrawalsForkTime() && time < s.ElectraEnableWithdrawalsForkTime() {
+		return false
+	}
+	return true
+}


nit: we are really defining a window where withdrawals are disabled, so we may consider a negative logic here and have a WithdrawalDisabled method

abi87 · 2025-05-21T09:08:52Z


+	// 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.


is this true? It seems to me we just let withdrawals be enqueued rn.
I am not against dropping requests but I don't think we are doing this now

good catch, i removed the logic but forgot to update comment

abi87

Some points:

request to rename disabling window
question about disabling not just servicing of withdrawals but enqueing of requests as well (it's in a comment but not in the code)

Finally I wonder if we should prepare the specs to allow for multiple stopping window, but making two lists of Disabling window start and disabling window ends

rezzmah · 2025-05-21T10:24:38Z

Some points:

request to rename disabling window

question about disabling not just servicing of withdrawals but enqueing of requests as well (it's in a comment but not in the code)

Finally I wonder if we should prepare the specs to allow for multiple stopping window, but making two lists of Disabling window start and disabling window ends

Addressed 1 and 2.

With regards to 3, we can always retroactively refactor the process if we end up doing a first withdrawal fork

calbera

General question. Do these 2 chain spec values imply we can "re-use" them to do a withdrawals freeze in the future? I.e. say we recognize an issue at t=100. Then we say we will disable at t=150 and enable at t=250. Now later on if t=1000, and we need to freeze again, we could just set the disable=1100 and enable to 1200.

Is this the general thinking?

rezzmah · 2025-05-26T18:38:15Z

General question. Do these 2 chain spec values imply we can "re-use" them to do a withdrawals freeze in the future? I.e. say we recognize an issue at t=100. Then we say we will disable at t=150 and enable at t=250. Now later on if t=1000, and we need to freeze again, we could just set the disable=1100 and enable to 1200.

Is this the general thinking?

No, there is no intention to re-use these specific variables. As you're aware, that would cause issues with syncing. If we reach the unfortunate position where we need multiple freezes, we can refactor the fork retroactively similar to the way we did forking by timestamp, such that it allows for multiple freezes.

I'd rather not over-engineer upfront for an unlikely scenario

rezzmah added 2 commits May 20, 2025 08:24

fix issue

11d3adc

add withdrawals_enabled

e141967

rezzmah had a problem deploying to test-e2e May 19, 2025 23:21 — with GitHub Actions Error

rezzmah added 2 commits May 20, 2025 09:43

Added check

c416f12

extending withdrawals disabling

8f81a38

rezzmah had a problem deploying to test-e2e May 19, 2025 23:52 — with GitHub Actions Error

Remove electra changes

1f9b9c9

rezzmah had a problem deploying to test-e2e May 19, 2025 23:57 — with GitHub Actions Error

fix

eb5df2c

rezzmah had a problem deploying to test-e2e May 20, 2025 00:00 — with GitHub Actions Error

Update statedb.go

368fd7e

rezzmah had a problem deploying to test-e2e May 20, 2025 00:22 — with GitHub Actions Error

Update statedb.go

f8b2016

rezzmah had a problem deploying to test-e2e May 20, 2025 00:41 — with GitHub Actions Error

Update statedb.go

b84c9ea

rezzmah had a problem deploying to test-e2e May 20, 2025 00:43 — with GitHub Actions Error

comments linting

8c10052

rezzmah had a problem deploying to test-e2e May 20, 2025 00:55 — with GitHub Actions Error

Added test for freezing withdrawals

7f0ae9b

rezzmah had a problem deploying to test-e2e May 20, 2025 02:05 — with GitHub Actions Error

Added test

cbffee5

rezzmah temporarily deployed to test-e2e May 20, 2025 02:31 — with GitHub Actions Inactive

rezzmah changed the title ~~[draft] Allow for Disabling Withdrawals~~ feat(withdrawals): Allow for Disabling Withdrawals May 20, 2025

abi87 reviewed May 20, 2025

View reviewed changes

Add Disable and Enable Fork

6f76b58

rezzmah had a problem deploying to test-e2e May 20, 2025 19:30 — with GitHub Actions Error

Update defaults.go

0485ae0

rezzmah had a problem deploying to test-e2e May 20, 2025 19:32 — with GitHub Actions Error

Process Withdrawal Requests even if withdrawals disabled

31f1bcb

rezzmah temporarily deployed to test-e2e May 21, 2025 05:50 — with GitHub Actions Inactive

Merge branch 'main' into prep-for-em-fork

b013aea

rezzmah had a problem deploying to test-e2e May 21, 2025 08:25 — with GitHub Actions Error

abi87 reviewed May 21, 2025

View reviewed changes

abi87 suggested changes May 21, 2025

View reviewed changes

Fix comment based on updated logic

0cca53e

rezzmah had a problem deploying to test-e2e May 21, 2025 09:36 — with GitHub Actions Error

Flip to withdrawals disabled

f366782

rezzmah temporarily deployed to test-e2e May 21, 2025 10:12 — with GitHub Actions Inactive

Merge branch 'main' into prep-for-em-fork

dc4ed01

rezzmah had a problem deploying to test-e2e May 21, 2025 20:08 — with GitHub Actions Error

merge conflict fix

32200e0

rezzmah temporarily deployed to test-e2e May 21, 2025 20:11 — with GitHub Actions Inactive

calbera reviewed May 22, 2025

View reviewed changes

Merge branch 'main' into prep-for-em-fork

8a5b80d

rezzmah had a problem deploying to test-e2e May 27, 2025 23:46 — with GitHub Actions Error

rezzmah mentioned this pull request May 28, 2025

chore(forks): Enforce Fork Ordering #2790

Merged

Merge branch 'main' into prep-for-em-fork

79e3e61

rezzmah had a problem deploying to test-e2e May 28, 2025 00:21 — with GitHub Actions Error

Add spec enforcement of ordering

23daa35

rezzmah temporarily deployed to test-e2e May 28, 2025 00:30 — with GitHub Actions Inactive

Merge branch 'main' into prep-for-em-fork

f0bd3ac

calbera temporarily deployed to test-e2e May 28, 2025 17:24 — with GitHub Actions Inactive

Merge branch 'main' into prep-for-em-fork

f5d3115

rezzmah had a problem deploying to test-e2e June 19, 2025 21:16 — with GitHub Actions Failure

Conversation

rezzmah commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abi87 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abi87 May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calbera May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abi87 left a comment

Choose a reason for hiding this comment

Uh oh!

rezzmah commented May 21, 2025

Uh oh!

calbera left a comment

Choose a reason for hiding this comment

Uh oh!

rezzmah commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rezzmah commented May 19, 2025 •

edited

Loading

codecov bot commented May 19, 2025 •

edited

Loading

abi87 May 21, 2025 •

edited

Loading

calbera May 22, 2025 •

edited

Loading

rezzmah commented May 26, 2025 •

edited

Loading