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

Shift networking configuration into configs #4401

Closed
AgeManning opened this issue Jun 14, 2023 · 1 comment
Closed

Shift networking configuration into configs #4401

AgeManning opened this issue Jun 14, 2023 · 1 comment

Comments

@AgeManning
Copy link
Member

Description

The following consensus PRs shift some of the network constants into the configuration files:

We should move these constants also.

Our configuration setup is located here: https://github.com/sigp/lighthouse/blob/stable/consensus/types/src/eth_spec.rs

We need to modify this to incorporate constants and in the associated code. Please hit me up for further info/help if needed :)

@AgeManning
Copy link
Member Author

AgeManning commented Jun 20, 2023

The configurations we need to add are:

name default value location in code to replace
gossip_max_size 10485760 here EDIT: We use the 10x version as the default and divide by 10 for the pre-merge version.
max_request_blocks 1024 here
epochs_per_subnet_subscription 256 - I think this is already there N/A
min_epochs_for_block_requests 33024 I dont think we have this, but it should go in this function and the blocks_by_root. @pawanjay176 can help here
max_chunk_size 1048576 here
ttfb_timeout 5 here
resp_timeout 10 here
attestation_propagation_slot_range 32 This is currently not configureable, but we can add it here - check with @michaelsproul
maximum_gossip_clock_disparity 500 here @michaelsproul might be able to help with this one
message_domain_invalid_snappy 0x00000000 we don't use it, but it's here
message_domain_valid_snappy 0x01000000 here
subnets_per_node 2 - already there I think N/A

So what we need to do, is add these fields into the ChainSpec: https://github.com/sigp/lighthouse/blob/stable/consensus/types/src/chain_spec.rs#L32

We should add them into the networking section here: https://github.com/sigp/lighthouse/blob/stable/consensus/types/src/chain_spec.rs#L165

Then for each of the hard-coded networks, i.e mainnet, put in the default values: https://github.com/sigp/lighthouse/blob/stable/consensus/types/src/chain_spec.rs#L473

For all the networks, just use the defaults listed above.

Then for all the places where we have these values being used in the code, replace them with the values from the chain spec.

Most places should have access to a generic EthSpec type. You can get ChainSpec values via: EthSpec::default_spec().

So for example, the first one is gossip_max_size. The location for that variable is here. We should remove that constant (and the one underneath it, as its just 10x the value.

Then in the code that we are using it, like here: https://github.com/sigp/lighthouse/blob/stable/beacon_node/lighthouse_network/src/config.rs#L47 and here: https://github.com/sigp/lighthouse/blob/stable/beacon_node/lighthouse_network/src/config.rs#L457 we need to accept an EthSpec.

So, ideally the function could require the generic, i.e

pub fn gossipsub_config<TSpec:EthSpec>(...) { ... }

It gets used here: so you can pass the TSpec into the function.

That should resolve the first one.

@pawanjay176 should be able to help with all of these.

bors bot pushed a commit that referenced this issue Aug 1, 2023
## Issue Addressed
Addresses [#4401](#4401)

## Proposed Changes
Shift some constants into ```ChainSpec``` and remove the constant values from code space.

## Additional Info

I mostly used ```MainnetEthSpec::default_spec()``` for getting ```ChainSpec```. I wonder Did I make a mistake about that.


Co-authored-by: armaganyildirak <[email protected]>
Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Age Manning <[email protected]>
Co-authored-by: Diva M <[email protected]>
bors bot pushed a commit that referenced this issue Aug 2, 2023
## Issue Addressed
Addresses [#4401](#4401)

## Proposed Changes
Shift some constants into ```ChainSpec``` and remove the constant values from code space.

## Additional Info

I mostly used ```MainnetEthSpec::default_spec()``` for getting ```ChainSpec```. I wonder Did I make a mistake about that.


Co-authored-by: armaganyildirak <[email protected]>
Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Age Manning <[email protected]>
Co-authored-by: Diva M <[email protected]>
bors bot pushed a commit that referenced this issue Aug 2, 2023
## Issue Addressed
Addresses [#4401](#4401)

## Proposed Changes
Shift some constants into ```ChainSpec``` and remove the constant values from code space.

## Additional Info

I mostly used ```MainnetEthSpec::default_spec()``` for getting ```ChainSpec```. I wonder Did I make a mistake about that.


Co-authored-by: armaganyildirak <[email protected]>
Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Age Manning <[email protected]>
Co-authored-by: Diva M <[email protected]>
bors bot pushed a commit that referenced this issue Aug 3, 2023
## Issue Addressed
Addresses [#4401](#4401)

## Proposed Changes
Shift some constants into ```ChainSpec``` and remove the constant values from code space.

## Additional Info

I mostly used ```MainnetEthSpec::default_spec()``` for getting ```ChainSpec```. I wonder Did I make a mistake about that.


Co-authored-by: armaganyildirak <[email protected]>
Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Age Manning <[email protected]>
Co-authored-by: Diva M <[email protected]>
bors bot pushed a commit that referenced this issue Aug 3, 2023
## Issue Addressed
Addresses [#4401](#4401)

## Proposed Changes
Shift some constants into ```ChainSpec``` and remove the constant values from code space.

## Additional Info

I mostly used ```MainnetEthSpec::default_spec()``` for getting ```ChainSpec```. I wonder Did I make a mistake about that.


Co-authored-by: armaganyildirak <[email protected]>
Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Age Manning <[email protected]>
Co-authored-by: Diva M <[email protected]>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
Addresses [sigp#4401](sigp#4401)

Shift some constants into ```ChainSpec``` and remove the constant values from code space.

I mostly used ```MainnetEthSpec::default_spec()``` for getting ```ChainSpec```. I wonder Did I make a mistake about that.

Co-authored-by: armaganyildirak <[email protected]>
Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Age Manning <[email protected]>
Co-authored-by: Diva M <[email protected]>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
Addresses [sigp#4401](sigp#4401)

Shift some constants into ```ChainSpec``` and remove the constant values from code space.

I mostly used ```MainnetEthSpec::default_spec()``` for getting ```ChainSpec```. I wonder Did I make a mistake about that.

Co-authored-by: armaganyildirak <[email protected]>
Co-authored-by: Paul Hauner <[email protected]>
Co-authored-by: Age Manning <[email protected]>
Co-authored-by: Diva M <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant