un"hardcode" setting chain configuration

meowsbits commented 4 years ago

Migrated from: https://github.com/etclabscore/multi-geth-fork/issues/67 Original author: @meowsbits

this needs improving:

https://github.com/etclabscore/multi-geth/blob/master/cmd/utils/flags.go#L1567-L1623

having "hardcoded" switches for default chain configs everywhere is too complex and too redundant and leaves too many opportunities for errors and inconsistencies:

no mordor: https://github.com/etclabscore/multi-geth/blob/master/cmd/faucet/faucet.go#L119
missing chains: https://github.com/etclabscore/multi-geth/blob/master/mobile/geth.go#L143-L166

much of this is because of crufty, brittle code (inheriting code where these cases have been carelessly hardcoded with approaching infinite opinionation).

I'm not sure the best pattern for approach yet. Big global? Moar interfaces? Mmmm..

Rel #63

chiro-hiro commented 4 years ago

I'd love this one from go-libp2p: https://github.com/libp2p/go-libp2p/blob/master/config/config.go#L358

A singleton state management looks reasonable to me.

meowsbits commented 4 years ago

Indeed, good idea! A global singleton is one option. One priority to keep in mind is that we'll want to integrate cleanly with the existing parallelized tests (and there are a lot of of those ;)

In fact, core-geth already uses a similar pattern with it's Configurator interface (and interface implementations), for example:

func (c *CoreGethChainConfig) SetEIP155Transition(n *uint64) error {
    c.EIP155Block = setBig(c.EIP155Block, n)
    return nil
}

https://github.com/etclabscore/core-geth/blob/master/params/types/coregeth/chain_config_configurator.go#L190

I'm afraid this issue isn't very well written, and the link has become stale. I'll try to regroup on the main point here:

Problem:

Chain flags like --ropsten and --classic and --kotti each signal the activation of a certain chain configuration.
In many places, the set of these flags occur together, but there is no concept for the group of these options. For example, below.
- Since these flags evolve differently at core-geth than at go-ethereum, these are often places of conflict and can be prone to error.
- Without a concept of "available chain configurations" (as a set), there is not way to communicate this to the user, or to validate the inclusion (or exclusion) of a configuration against a set. For example, we could make some tests a lot more thorough by being able to say "Run this test against all of the test configurations," or, "Run this test against all production configurations," and so forth.

// setBootstrapNodes creates a list of bootstrap nodes from the command line
// flags, reverting to pre-configured ones if none have been specified.
func setBootstrapNodes(ctx *cli.Context, cfg *p2p.Config) {
    urls := params.MainnetBootnodes
    switch {
    case ctx.GlobalIsSet(BootnodesFlag.Name) || ctx.GlobalIsSet(LegacyBootnodesV4Flag.Name):
        if ctx.GlobalIsSet(LegacyBootnodesV4Flag.Name) {
            urls = splitAndTrim(ctx.GlobalString(LegacyBootnodesV4Flag.Name))
        } else {
            urls = splitAndTrim(ctx.GlobalString(BootnodesFlag.Name))
        }
    case ctx.GlobalBool(ClassicFlag.Name):
        urls = params.ClassicBootnodes
    case ctx.GlobalBool(MordorFlag.Name):
        urls = params.MordorBootnodes
    case ctx.GlobalBool(SocialFlag.Name):
        urls = params.SocialBootnodes
    case ctx.GlobalBool(MixFlag.Name):
        urls = params.MixBootnodes
    case ctx.GlobalBool(EthersocialFlag.Name):
        urls = params.EthersocialBootnodes
    case ctx.GlobalBool(LegacyTestnetFlag.Name) || ctx.GlobalBool(RopstenFlag.Name):
        urls = params.RopstenBootnodes
    case ctx.GlobalBool(RinkebyFlag.Name):
        urls = params.RinkebyBootnodes
    case ctx.GlobalBool(KottiFlag.Name):
        urls = params.KottiBootnodes
    case ctx.GlobalBool(GoerliFlag.Name):
        urls = params.GoerliBootnodes
    case ctx.GlobalBool(YoloV1Flag.Name):
        urls = params.YoloV1Bootnodes
    case cfg.BootstrapNodes != nil:
        return // already set, don't apply defaults.
    }

https://github.com/etclabscore/core-geth/blob/master/cmd/utils/flags.go#L805

and here

func dataDirPathForCtxChainConfig(ctx *cli.Context, baseDataDirPath string) string {
    switch {
    case ctx.GlobalBool(RopstenFlag.Name):
        return filepath.Join(baseDataDirPath, "ropsten")
    case ctx.GlobalBool(ClassicFlag.Name):
        return filepath.Join(baseDataDirPath, "classic")
    case ctx.GlobalBool(MordorFlag.Name):
        return filepath.Join(baseDataDirPath, "mordor")
    case ctx.GlobalBool(SocialFlag.Name):
        return filepath.Join(baseDataDirPath, "social")
    case ctx.GlobalBool(MixFlag.Name):
        return filepath.Join(baseDataDirPath, "mix")
    case ctx.GlobalBool(EthersocialFlag.Name):
        return filepath.Join(baseDataDirPath, "ethersocial")
    case ctx.GlobalBool(RinkebyFlag.Name):
        return filepath.Join(baseDataDirPath, "rinkeby")
    case ctx.GlobalBool(KottiFlag.Name):
        return filepath.Join(baseDataDirPath, "kotti")
    case ctx.GlobalBool(GoerliFlag.Name):
        return filepath.Join(baseDataDirPath, "goerli")
    case ctx.GlobalBool(YoloV1Flag.Name):
        return filepath.Join(baseDataDirPath, "yolo-v1")
    }
    return baseDataDirPath
}

https://github.com/etclabscore/core-geth/blob/master/cmd/utils/flags.go#L1294

Possible Solution:

A flag --chain=<name> that would deprecate and eventually supersede the cases of --goerli, --kotti, etc.
- Might use a singleton to handle options and activation.

chiro-hiro commented 4 years ago

The solution looks great but It's also raising another concern where some networks are going to be deprecated. If we add another wrapper on top of configuration for each network so it would be more flexible.

E.g:

package ropsten-config;

func (c *Config) getRopsten() RopstenConfig;

This approach could help us to derive configuration of each network from the "global" configuration.

meowsbits commented 4 years ago

raising another concern where some networks are going to be deprecated.

Can you say more what you mean here?

If I understand correctly, this kind of flexibilty can be accomplished (approximately) with existing functionality:

We take an existing chain config singleton, eg. params.RopstenChainConfig, and can transform it into an equivalent configuration for any other config struct supporting type Configurator interface, here coregeth.CoreGethChainConfig.

func Test1(t *testing.T) {
    ropstenConfig := params.RopstenChainConfig
    var emptyConfig = new(coregeth.CoreGethChainConfig)

    err := confp.Convert(ropstenConfig, emptyConfig)
    if err != nil {
        t.Fatal(err)
    }
    t.Log(emptyConfig.String())
}

=== RUN   Test1
--- PASS: Test1 (0.00s)
    configurator_test.go:259: NetworkID: 3, ChainID: 3 Engine: ethash EIP1014: 4230000 EIP1052: 4230000 EIP1108: 6485846 EIP1283Disable: 4939394 EIP1283: 4230000 EIP1344: 6485846 EIP140: 1700000 EIP145: 4230000 EIP150: 0 EIP152: 6485846 EIP155: 10 EIP160: 10 EIP161abc: 10 EIP161d: 10 EIP170: 10 EIP1884: 6485846 EIP198: 1700000 EIP2028: 6485846 EIP211: 1700000 EIP212: 1700000 EIP213: 1700000 EIP214: 1700000 EIP2200: 6485846 EIP2: 0 EIP658: 1700000 EIP7: 0 EthashEIP100B: 1700000 EthashEIP1234: 4230000 EthashEIP2384: 7117117 EthashEIP649: 1700000 EthashHomestead: 0 
PASS

:pencil2: Just brainstorming here, I had been thinking of something along the lines of something like

package params // or somewhere else? 

type ConfigManager struct {
    // some fields probably, not sure what... maybe including a map of name:config
}

func (m *ConfigManager) ConfigByName(name string) (config ctypes.ChainConfigurator, err error) {
    switch name {
        // ...
    }
}

func (m *ConfigManager) Configs() []ctypes.ChainConfigurator {
    // ...
}

Although, working through the thinking on this now, while it seems convenient and "nice" to have wrappers like this, I think it may be worth stepping back and make sure we understand what problem we're solving.

The primary motivator for this issue is not that existing switch-style code (as above) is ugly, but that it is fragile to upstream merges because of differences in availability of chain options. This could be solved with tests checking that all expected options for core-geth are in place. Granted, this would create yet another case of a "hardcoded list", but it could be far less prone to merge conflict or errors, and would solve the problem without having to diverge more than we already have from upstream.

Another smaller reason being that these ugly sets of switch-style chain option lists occur in several places, and can be difficult/painstaking to make sure all have been updated in case, say, a chain is added or removed. This could be solved again with a few more tests added.

Beyond being "convenient" and "nice," it seems to me that the best argument for some kind of "chain manager"/wrapper would be the ability to programmatically list and iterate all or some chains... and although I have a vague idea that we might be able to build some strong tests on top of that, I'm not really sure yet exactly what or where I would want to do that.

Curious your thoughts.

chiro-hiro commented 4 years ago

Can you say more what you mean here?

I meant, might be in the future we could stop supporting some networks.

If I understand correctly, this kind of flexibilty can be accomplished (approximately) with existing functionality:

We take an existing chain config singleton, eg. params.RopstenChainConfig, and can transform it into an equivalent configuration for any other config struct supporting type Configurator interface, here coregeth.CoreGethChainConfig

Yes, I think those configurations should share the same interface or able to derive an equivalent.

Thanks for your clarification,

To me, singleton management help us manage configuration more consistent meanwhile chain of configuration help us manipulate parameters munch more easier. The configuration wrappers should contain hard-code of chains, networks...etc.

chiro-hiro commented 4 years ago

Here are dummy code:

package configuration

import "sync"

// Configuration structure, might key value based
type Configuration struct {
    items map[string]interface{}
    lock  sync.RWMutex
}

// Config interface shared between packages
type Config interface {
    getByName(key string) interface{}
}

// Option that's function that able to attach config to *config pointer
type Option func(cfg *Configuration) error

// Apply config from opts
func (cfg *Configuration) Apply(opts ...Option) error {
    for _, opt := range opts {
        if opt == nil {
            continue
        }
        if err := opt(cfg); err != nil {
            return err
        }
    }
    return nil
}

// Derive subset of configuration
func (cfg *Configuration) DeriveConfig(keys ...string) Config {}

// GetCfgInstance return singleton instance of configuration
func GetCfgInstance() *Configuration {}

Here are how do I supposed to grouping hardcode:

import "configuration"

package rinkeby-configuration

func ChainHardcode(cfg *configuration.Configuration) error {
    // Apply chain hard code here and do manipulate configuration here
    cfg.Apply(enableEIP155);
}

meowsbits commented 4 years ago

Thanks your time putting this together! This is very helpful.

I think the package-ized chain configuration idea is quite interesting (package rinkeby-configuration), but am not sure exactly what benefits it brings.

In your example, do you mean that type Configuration struct would replace type Configurator interface eventually?

chiro-hiro commented 4 years ago

That's very kind of you!,

But am not sure exactly what benefits it brings.

I'm try to bring up these things:

All chain's hardcode could be "packaged", that would be easier to maintenance.
All options are methods (type Option func(cfg *Configuration) error) that's allowed us to manipulate configuration and handle exceptions cases better.
We could derive a subset of configuration that's needed.

In your example, do you mean that type Configuration struct would replace type Configurator interface eventually?

Yes, I think so.

etclabscore / core-geth

un"hardcode" setting chain configuration #19