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

Put limit to eth/68 methods: GetHeaders, GetBlockBodies, GetReceipts, and GetPooledTransactions #3102

Closed
jangko opened this issue Feb 24, 2025 · 5 comments · Fixed by #3116
Closed
Labels

Comments

@jangko
Copy link
Contributor

jangko commented Feb 24, 2025

https://github.com/ethereum/devp2p/blob/master/caps/eth.md

The spec mentions about soft limit and implementation defined limits. And nimbus EL have no limit implemented on those methods.

Request timeout or other problems might occur because of this unbounded behavior.

@jangko jangko added the EL label Feb 24, 2025
@arnetheduck
Copy link
Member

Apart from the size limits, see bandwidth quota here: https://github.com/status-im/nimbus-eth2/blob/f232e39d92bf96dfb9793c9ecc767dc7ac1dec26/beacon_chain/networking/eth2_network.nim#L516C18-L516C23 - ie if a peer requests too much, we simply slow down their requests to a reasonable rate.

@advaita-saha
Copy link
Contributor

advaita-saha commented Feb 24, 2025

if request.maxResults > uint64(maxHeadersFetch):
debug "GetBlockHeaders (0x03) requested too many headers",

const
maxStateFetch* = 384
maxBodiesFetch* = 128
maxReceiptsFetch* = 256
maxHeadersFetch* = 192

Limits are imposed but these limits are not right as far as I have seen
Geth sends us GetHeaders request for 512 blocks, and we keep rejecting them because of these limits

These limits are not propery scoped out for the eth68 protocol, but needs to match with other clients requirements

@jangko
Copy link
Contributor Author

jangko commented Feb 24, 2025

Implementation defined limit is allowed by the spec. So we have freedom to choose the number of items we want to serve. But indeed the early rejection is wrong. When other client request items more than our limit, we should return at least that limit and not reject that peer request. e.g. Geth request 512 headers, we put limit 192, then we should return 192 headers.

@advaita-saha
Copy link
Contributor

If we are not sending adequete number of headers in response, geth seems to disconnect

TRACE[02-24|12:01:42.609] Fetching skeleton headers                peer=acd83569 from=1999 count=512
DEBUG[02-24|12:01:42.609] Fetching batch of headers                id=acd8356913cc5e06 conn=inbound count=512 fromnum=1999 skip=0 reverse=true
DEBUG[02-24|12:01:42.614] Invalid non-genesis header count         peer=acd83569 have=192 want=512
DEBUG[02-24|12:01:42.614] Message handling failed in `eth`         id=acd8356913cc5e06 conn=inbound err="not enough non-genesis headers delivered"
DEBUG[02-24|12:01:42.614] Removing Ethereum peer                   peer=acd83569 snap=false

@arnetheduck
Copy link
Member

then we should return 192 headers.

this is what we tend to avoid on the eth2 side, unless there exists a specific number in a spec - ie instead of sending fewer responses, we establish a rate at which we want to send responses (ie 192 headers/second) and send them slowly - I suspect the same would work well in eth1.

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

Successfully merging a pull request may close this issue.

3 participants