Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add option to disable CL #373

Closed
wants to merge 15 commits into from
Closed

feat: add option to disable CL #373

wants to merge 15 commits into from

Conversation

gr8h
Copy link

@gr8h gr8h commented Nov 28, 2023

Add a feature to disable the Consensus Layer (CL)/Beacon node, useful for scenarios where only the Execution Layer (EL) client is needed.

Use Case:
Supporting Aura consensus from Nethermind with an EL client only.

@barnabasbusa
Copy link
Contributor

I think this PR could be made a lot more simple. You probably can do the checks if the CL image is set to null, and check for that.

If CL img is set to null, then don't spin it up, else do.

@barnabasbusa barnabasbusa changed the title Disable CL feat: add option to disable CL Nov 29, 2023
@barnabasbusa
Copy link
Contributor

Could you add the aura config into examples/aura.yaml instead of network config?

examples/aura.yaml Outdated Show resolved Hide resolved
examples/aura.yaml Show resolved Hide resolved
examples/aura.yaml Outdated Show resolved Hide resolved
examples/aura.yaml Outdated Show resolved Hide resolved
examples/aura.yaml Outdated Show resolved Hide resolved
examples/aura.yaml Outdated Show resolved Hide resolved
examples/aura.yaml Outdated Show resolved Hide resolved
examples/aura.yaml Outdated Show resolved Hide resolved
examples/aura.yaml Outdated Show resolved Hide resolved
examples/aura.yaml Outdated Show resolved Hide resolved
@barnabasbusa
Copy link
Contributor

Please run kurtosis lint --format in order to have no linting errors.

@barnabasbusa barnabasbusa linked an issue Nov 29, 2023 that may be closed by this pull request
@h4ck3rk3y
Copy link
Collaborator

@gr8h Thanks for the contribution - some notes

  1. Can we enrich the README about this behavior
  2. While cl_client_image being null does work; this feels like magical behavior which is worse when things aren't documented. How about cl_client_type being set to null? @barnabasbusa does it have to be the image?

@barnabasbusa
Copy link
Contributor

  1. While cl_client_image being null does work; this feels like magical behavior which is worse when things aren't documented. How about cl_client_type being set to null? @barnabasbusa does it have to be the image?

Actually cl_client_type being null is probably better, the image being set to null is not logical indeed.

@gr8h
Copy link
Author

gr8h commented Nov 29, 2023

  1. While cl_client_image being null does work; this feels like magical behavior which is worse when things aren't documented. How about cl_client_type being set to null? @barnabasbusa does it have to be the image?

Actually cl_client_type being null is probably better, the image being set to null is not logical indeed.

Okay, Make sense.

@gr8h
Copy link
Author

gr8h commented Nov 30, 2023

@barnabasbusa one thing i noticed is that the el_extra_params value in the yaml file does not override the values in nethermind_****.start file, it only extend it and it duplicate the params, is this an intended behaviour?

@barnabasbusa
Copy link
Contributor

is this an intended behaviour?

yes it is.

As its supposed to be extra arguments to a sane default, which would mean that the operation is append, not a find and replace.

@barnabasbusa
Copy link
Contributor

Will nethermind be able to take the later values?

@gr8h
Copy link
Author

gr8h commented Nov 30, 2023

Will nethermind be able to take the later values?

I had to change chainspecs path in the constants.star, and removed "--config=none.cfg" on nethermind launch script.

@barnabasbusa
Copy link
Contributor

I'm still a bit confused about what you are trying to do here
As the example config looks like this:

  - el_client_type: nethermind
    el_client_image: nethermind/nethermind:latest
    el_extra_params:
      - --config=/nethermind/configs/validator.cfg
      - --Init.ChainSpecPath=/nethermind/chainspec/chainspec.json
    beacon_extra_params: []
    validator_extra_params: []
    builder_network_params: null
    validator_count: null
    snooper_enabled: false
    ethereum_metrics_exporter_enabled: false
    count: 1

But nowhere here did you define that you want to set the cl_client_type to null
or am I missing something?

if you don't define some parameter, then the defaults will be used
also you have ethereum_metrics-exporter disabled, but you want prometheus and grafana to be spun up?
are you gonna import some dashboards?

Also I'm fairly sure that nethermind:latest is not deneb ready, so why are you triggering deneb on fork 4

wait_for_finalization is not something thats working anymore

also you don't have to define the commands that are default false like

wait_for_finalization: false
global_client_log_level: info
snooper_enabled: false
ethereum_metrics_exporter_enabled: false
parallel_keystore_generation: false

so maybe you want your config to look something like this:

participants:
  - el_client_type: nethermind
    el_client_image: nethermind/nethermind:latest
    el_extra_params:
      - --config=/nethermind/configs/validator.cfg
      - --Init.ChainSpecPath=/nethermind/chainspec/chainspec.json
    cl_client_type: null
    validator_count: 0
  - el_client_type: nethermind
    el_client_image: nethermind/nethermind:latest
    el_extra_params:
      - --config=/nethermind/configs/node.cfg
      - --Init.ChainSpecPath=/nethermind/chainspec/chainspec.json
    cl_client_type: null
    validator_count: 0
    count: 2
additional_services:
  - tx_spammer
  - prometheus_grafana

however, without any validators what kind of consensus mechanism you plan to be using? I see that you have mentioned that Aura consensus mechanism. I'm sorry but I haven't heard of that. Would you be open to creating an "aura-package" instead of trying to add a different consensus mechanism to ethereum-package (which only has ethereum consensus mechanism).

I've ran a test with this config, and as expected, we no longer support non validating nodes.

Using CL MIN_GENESIS_TIME for genesis timestamp
    processing mnemonic 0, for 0 validators
    Writing pubkeys list file...
    generated 0 validators from mnemonic yaml (/tmp/ci-VbeIAQefO0/mnemonics.yaml)
    failed to compute sync committee indices: no active validators to compute sync committee from

@gr8h gr8h closed this Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to Disable the CL/Beacon node
3 participants