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

Bump GOSSIP_MAX_SIZE from 10MiB to 15MiB #4041

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

Giulio2002
Copy link

@Giulio2002 Giulio2002 commented Dec 6, 2024

GOSSIP_MAX_SIZE is on the fence for worst case block and it will be especially if we consider an increase in gas limit which seems to be strongly pushed by the community. 15 MiB gives large space for an increase up 60mn gas.

@g11tech
Copy link
Contributor

g11tech commented Dec 6, 2024

this text also needs to be updated or removed from https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/p2p-interface.md?

with the addition of ExecutionPayload to BeaconBlocks, there is a dynamic field -- transactions -- which can validly exceed the GOSSIP_MAX_SIZE limit (1 MiB) put in place at Phase 0, so GOSSIP_MAX_SIZE has increased to 10 Mib on the network. At the GAS_LIMIT (~30M) currently seen on mainnet in 2021, a single transaction filled entirely with data at a c

@Giulio2002
Copy link
Author

@g11tech is it better now?

@arnetheduck
Copy link
Contributor

A PR description here with some math behind this specific number would be helpful here, along with some reasoning about how this is going to affect distribution times, latency, overall bandwidth given gossip amplification factors etc.

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@arnetheduck
Copy link
Contributor

Also, notably, changes like this should happen at a hard fork, else we risk creating a split network based off nodes that have and have not upgraded (and would therefore penalise the upgraded nodes that are sending "too large" messages - in the worst case of half the validators having upgraded, this would lead to non-finality effectively) - the way we've dealt with this in the past is to have a separate constant for the "next" hard fork that will use the new limit for messages in the new hard fork.

Finally, req/resp is governed by similar rules - we upgrade the two constants together.

@Giulio2002
Copy link
Author

Giulio2002 commented Dec 6, 2024

Also, notably, changes like this should happen at a hard fork, else we risk creating a split network based off nodes that have and have not upgraded (and would therefore penalise the upgraded nodes that are sending "too large" messages - in the worst case of half the validators having upgraded, this would lead to non-finality effectively) - the way we've dealt with this in the past is to have a separate constant for the "next" hard fork that will use the new limit for messages in the new hard fork.

Finally, req/resp is governed by similar rules - we upgrade the two constants together.

I think it is the same constant - also as you know we are already exchanging messages on a private channel about this issue. please keep the conversation there.

@arnetheduck
Copy link
Contributor

I think it is the same constant

MAX_CHUNK_SIZE

@Giulio2002
Copy link
Author

MAX_CHUNK_SIZE

done

@jtraglia jtraglia mentioned this pull request Dec 6, 2024
13 tasks
@ppopth
Copy link
Member

ppopth commented Dec 9, 2024

15 MiB gives large space for an increase up 60mn gas

Do we already have an EIP to increase the gas limit from 30mn to 60mn? If not, changing this before the gas limit seems weird.

@Giulio2002
Copy link
Author

Giulio2002 commented Dec 9, 2024

15 MiB gives large space for an increase up 60mn gas

Do we already have an EIP to increase the gas limit from 30mn to 60mn? If not, changing this before the gas limit seems weird.

You change this first so that everything does not break :D. not the other way around

@ppopth
Copy link
Member

ppopth commented Dec 10, 2024

@Giulio2002 can you provide some calculation that it can really exceed 10MiB?

We have an EIP that is trying to reduce the block size https://eips.ethereum.org/EIPS/eip-7623 which is opposite to this PR.

Could you please elaborate more?

The current spec also says that with calldata gas of 16 and gas limit 30M, the max block size is actually 2MiB which is already lower than 10Mib.

@Giulio2002
Copy link
Author

@Giulio2002 can you provide some calculation that it can really exceed 10MiB?

We have an EIP that is trying to reduce the block size https://eips.ethereum.org/EIPS/eip-7623 which is opposite to this PR.

Could you please elaborate more?

The current spec also says that with calldata gas of 16 and gas limit 30M, the max block size is actually 2MiB which is already lower than 10Mib.

Yeah, the spec is wrong. it is not safe to increase the gas limit to >38 million because of this. 15 MiB is okay and was discussed with all other CL clients

@Giulio2002
Copy link
Author

also just for reference - prysm merged their PR with the change: prysmaticlabs/prysm#14692

@nisdas
Copy link
Contributor

nisdas commented Dec 11, 2024

@Giulio2002 The max gossip size in prysm is still 10 MiB , we haven't changed it. We only allow the uncompressed size to be up to 20 Mib

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants