beacon-kit icon indicating copy to clipboard operation
beacon-kit copied to clipboard

feat(withdrawals): Allow for Disabling Withdrawals [DO NOT MERGE]

Open rezbera opened this issue 7 months ago • 4 comments

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

DO NOT MERGE till needed

rezbera avatar May 19 '25 23:05 rezbera

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files

Impacted file tree graph

@@            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%) :arrow_up:
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%) :arrow_up:
state-transition/core/state_processor_staking.go 61.42% <ø> (-0.28%) :arrow_down:

... and 1 file with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar May 19 '25 23:05 codecov[bot]

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

I have added this @abi87 @calbera

rezbera avatar May 20 '25 19:05 rezbera

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

rezbera avatar May 21 '25 10:05 rezbera

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

rezbera avatar May 26 '25 18:05 rezbera