Deprecate some `reorg`-related CLI flags and read from spec by chong-he · Pull Request #9177 · sigp/lighthouse

chong-he · 2026-04-27T04:18:10Z

Issue Addressed

Reconcile CLI reorg config with config.yaml #9123

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

chong-he

Some self-review comments for the reviewers

chong-he · 2026-04-27T12:50:07Z

+            re_org_head_threshold: Some(ReOrgThreshold(20)),
+            re_org_parent_threshold: Some(ReOrgThreshold(160)),
+            re_org_max_epochs_since_finalization: Epoch::new(2),


Not sure if it is a good idea to hard code like this for the default config? The reason is I want to get rid of the definitions of DEFAULT_RE_ORG_HEAD_THRESHOLD etc, so that we can use the spec value as the single source of truth (i.e., in testing/ef_tests/src/cases/fork_choice.rs)

I think we should actually remove these fields from the ChainConfig entirely, so that we are forced to always read them from the ChainSpec

Thanks for the great feedback, I have revised.

chong-he · 2026-04-27T12:55:54Z

        .arg(
            Arg::new("proposer-reorg-cutoff")
                .long("proposer-reorg-cutoff")


There is a proposer_reorg_cutoff_bps in the spec:

lighthouse/consensus/types/src/core/chain_spec.rs

Line 1105 in 6ab48a7

proposer_reorg_cutoff_bps: 1667,

but I think they are referring to different things (the proposer_reorg_cutoff_bps is a pretty newly defined field in the spec) while the proposer-reorg-cutoff CLI flag in Lighthouse is something that has exited for quite some time

So I am leaving this flag unchanged

We can get rid of this one as well. We should switch Lighthouse to using the BPS one (might need some changes to our calculations)

Might need a new method on SlotClock for this (look at the other BPS values to see how we do it)

Revised.

I am not too sure about the use of: Arc::make_mut(&mut spec).reorg_head_weight_threshold to modify the spec in config.rs which requires changes the spec to mut (current it's not mutable in config.rs)

If this works, then currently Lighthouse defaults to re-org at <= 1s, but after the change, it reads from the spec which defaults to re-org at <= 2s. Might be worth mentioning in the next release notes?

michaelsproul · 2026-04-30T04:31:24Z

    }

+    /// Sets the proposer parent re-org threshold
+    pub fn proposer_re_org_parent_threshold(mut self, threshold: Option<ReOrgThreshold>) -> Self {


When we remove the chain config we will also remove these mutators (for tests we can set custom values on the spec).

Removed the function

michaelsproul · 2026-04-30T04:32:25Z

can probably inject the config values into the spec here (and can remove TTD)

I found that the test already uses: let spec = ForkName::Fulu.make_genesis_spec(E::default_spec());
which is the default_spec, which will read from chain_spec.rs. So I believe this is covered by the default spec, i.e., we don't need to mut the spec and do something like: spec.reorg_xx....

michaelsproul · 2026-04-30T04:35:54Z

                .display_order(0)
        )
-        .arg(
-            Arg::new("proposer-reorg-threshold")


We should maybe keep these with a deprecation warning for now, and then can remove in v9.0.0

Added a deprecated note in the help text output

michaelsproul · 2026-05-06T01:58:48Z

+        let slot_duration_millis = self.spec.get_slot_duration().as_millis() as u64;
+        let re_org_cutoff_millis = slot_duration_millis
+            .saturating_mul(self.spec.proposer_reorg_cutoff_bps)
+            .saturating_div(BASIS_POINTS);
+        let re_org_cutoff_duration = Duration::from_millis(re_org_cutoff_millis);


We can use spec.compute_slot_component_duration(self.spec.proposer_reorg_cutoff_bps) to simplify this a little

michaelsproul · 2026-05-06T02:00:00Z

Should probably remove these re-exports now that they are not related to this module/config

michaelsproul · 2026-05-06T02:01:33Z

    parent_distance: u64,
    /// Number of slots between head block and block proposal slot.
    head_distance: u64,
-    re_org_threshold: u64,


It's interesting that the tests still pass after deleting these. I was thinking we could inject them into the mut spec below? Maybe we should add more tests to cover these (possibly in a separate issue)

Is it because the spec uses the default spec already:
let spec = ForkName::Fulu.make_genesis_spec(E::default_spec());
so it will read from the spec?

I added some assert_eq! to make sure they are equal to the spec in the test, but they look a bit of redundant? 2f40297

michaelsproul · 2026-05-06T02:05:35Z

-}
-
-#[test]
-fn disable_proposer_re_orgs() {


This test and the enable_proposer_re_orgs_default should stay if we keep the flag to disable-proposer-reorgs.

michaelsproul · 2026-05-06T02:08:36Z

+        Arc::make_mut(&mut spec).reorg_head_weight_threshold = None;
+        Arc::make_mut(&mut spec).reorg_parent_weight_threshold = None;


Hmm I don't think this is really the right way to do it. make_mut will only edit the spec variable in this function, not the global spec which is used elsewhere. I don't think we can easily edit the global spec at all (once it is behind the Arc).

Two options:

Deprecate the disable-proposer-reorgs flag completely. Users could disable reorgs still by using --testnet-dir with an edited config.yaml.

Add a boolean config on ChainConfig (where the config used to be) for controlling the enabling and disabling.

Probably the 2nd option is better. We'll need to update the code in beacon_chain to use that new flag. We can probably also update the reorg configs to be non-optional in chainspec (they can have defaults, but we can get rid of the Option<..> wrapper for simplicity)

michaelsproul · 2026-05-06T02:09:05Z

+                    let slot_duration_millis = self.spec.get_slot_duration().as_millis() as u64;
+                    let re_org_cutoff_millis = slot_duration_millis
+                        .saturating_mul(self.spec.proposer_reorg_cutoff_bps)
+                        .saturating_div(BASIS_POINTS);
+                    let re_org_cutoff_duration = Duration::from_millis(re_org_cutoff_millis);


Can use the same simplification as mentioned above (the slot component duration thing)

chong-he added 7 commits April 27, 2026 10:58

delete reorg parent and head cli

2614cba

test

edf9451

delete reorg max cli flag

8b277c0

simplify test

8946f48

update cli help text

fe70937

update book

f33e33a

test lint

6d6cd17

chong-he marked this pull request as draft April 27, 2026 04:26

chong-he added 4 commits April 27, 2026 17:41

fix http reorg test

5dc993d

fix error

9fc7249

fix ef-test

739472f

put default

187ebff

chong-he commented Apr 27, 2026

View reviewed changes

chong-he marked this pull request as ready for review April 27, 2026 12:57

chong-he added ready-for-review The code is ready for review backwards-incompat Backwards-incompatible API change labels Apr 27, 2026

michaelsproul reviewed Apr 30, 2026

View reviewed changes

michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Apr 30, 2026

michaelsproul reviewed Apr 30, 2026

View reviewed changes

chong-he and others added 7 commits April 30, 2026 21:11

revise

1a053d3

revise

71d0612

remove reorg cutoff

4373c19

Merge branch 'unstable' into cli-flags-reconcile

d0c4682

remove mut

11cd413

cli

8521fb7

cli fmt and book

ff00dd0

chong-he added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels May 4, 2026

michaelsproul requested changes May 6, 2026

View reviewed changes

michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 6, 2026

chong-he changed the title ~~Delete some reorg-related CLI flags and read from spec~~ Deprecate some reorg-related CLI flags and read from spec May 6, 2026

chong-he added 7 commits May 6, 2026 17:55

revise

652fef7

fix test

0ed9f25

revise

42cb94b

cfg debug

f9b761b

test

34793f2

fmt

401b12e

add some asserts

2f40297

		Arc::make_mut(&mut spec).reorg_head_weight_threshold = None;
		Arc::make_mut(&mut spec).reorg_parent_weight_threshold = None;

Conversation

chong-he commented Apr 27, 2026

Issue Addressed

Additional Info

Uh oh!

chong-he 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!

Choose a reason for hiding this comment

Uh oh!

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!

chong-he May 4, 2026 • 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chong-he May 4, 2026 •

edited

Loading