Disable MESS

[meowsbits]

This PR drafts the disablement of MESS/ECBP1100, anticipated to occur on ETC concurrently with the Spiral network upgrade.

Following convention, a configuration override flag --ecbp1100.disable=[number] is provided.

As-is, this draft uses runtime and reflect at the Configurator implementation level to handle the actual enable/disable logic (9e274d0). Alternatively, we could consider instead adding conditions at the places where the configuration is referenced to handle it, like the example below.

diff --git a/core/forkchoice.go b/core/forkchoice.go
index d07cdf6aa4..55d898c3a0 100644
--- a/core/forkchoice.go
+++ b/core/forkchoice.go
@@ -162,7 +162,8 @@ func (f *ForkChoice) ReorgNeeded(current *types.Header, extern *types.Header) (b
                        return reorg, nil
                }
        }
-       if !f.chain.Config().IsEnabled(f.chain.Config().GetECBP1100Transition, current.Number) {
+       if !f.chain.Config().IsEnabled(f.chain.Config().GetECBP1100Transition, current.Number) ||
+               f.chain.Config().IsEnabled(f.chain.Config().GetECBP1100DisableTransition, current.Number) {
                return reorg, nil
        }

[ziogaschr]

Nice hack in order to check if both are enabled, either way it will not stay for too long.

p.s.: I hope I don't read back this comment in 1-2 years and the code is still here :)

[ziogaschr]

I wonder if we also have to handle cases where --ecbp1100.nodisable is set.

Shall we force disable and raise a warning? This is what we prefer, on the other way, it's against the user's preference.
I think if user decided to use --ecbp1100.nodisable we can only show a warning but we can't force a disable.

[ziogaschr]

@meowsbits this is probably better to be removed (I added it).

A node operator might indeed want the nodisable functionality, so if he still wants to enable ECBP-1100 he has to do --ecbp1100.disable=69420123 --ecbp1100.nodisable.

The thing is that with the above check, this is not allowed.

[meowsbits]

Good point on the nodisable flag. That feature was on request, and was intended to turn off the MinPeers and StaleHead safety mechanisms. The concern was that the safety mechanisms could be used to knock a node off the canonical network, assuming that the canonical network followed MESS. If was MESS was on, they wanted it to stay on.

MESS is only enabled if a peer has greater than or equal to the MinimumSyncPeers peers. In Core-Geth this value is by default 5.
MESS is disabled if, once synced, a node's head block is not changed within a time limit (ie becomes stale). In Core-Geth this value is by default 30 * 13 seconds
https://github.com/ethereumclassic/ECIPs/blob/master/_specs/ecip-1100.md#implementation

So its my understanding that conceptually there's no conflict with these flags, although you're definitely right to point to it.

[ziogaschr]

Oh, so this will not allow MESS to be disabled only once it has been enabled.
Is this right? If yes, then there is no conflict indeed.

[diega]

🚢

[meowsbits]

The naming/conceptual conflict of ecbp1100.nodisable and GetECBP1100DisableTransition is annoying and confusing.

IMO ecbp1100.nodisable could have been better named ecbp1100.nosafety, but now is unchangeable since it holds an API place in the flags and config TOML marshalling:

	ECBP1100NoDisableFlag = &cli.BoolFlag{
		Name:  "ecbp1100.nodisable",
		Usage: "Short-circuit ECBP-1100 (MESS) disable/safety mechanisms; (yields a permanent-once-activated state, deactivating auto-shutoff mechanisms)",
	}

	// ECBP1100NoDisable overrides
	// When this value is *true, ECBP100 will not (ever) be disabled; when *false, it will never be enabled.
	ECBP1100NoDisable *bool `toml:",omitempty"`

This configuration value is used as:

func (bc *BlockChain) EnableArtificialFinality(enable bool, logValues ...interface{}) {
	// Short circuit if AF state is enabled and nodisable=true.
	if bc.artificialFinalityNoDisable != nil && atomic.LoadInt32(bc.artificialFinalityNoDisable) == 1 &&
		bc.IsArtificialFinalityEnabled() && !enable {
		log.Warn("Preventing disable artificial finality", "enabled", true, "nodisable", true)
		return
	}

How about renaming our new GetECBP1100DisableTransition and her associated fields to ECBP1100Deactivate instead to lessen the confusion? "Activation" being common language for turning on fork features ("block activation number"), and we're talking about "turning off" the feature.

[ziogaschr]

Let's not forget to set the block_number here or we wait for this PR to be merged and set it at #595

[meowsbits]

I just set it here == Spiral 907295d

[diega]

🚢²