Skip to content

Conversation

@ackintosh
Copy link
Member

@ackintosh ackintosh commented Aug 24, 2025

Issue Addressed

TTFB_TIMEOUT was deprecated in ethereum/consensus-specs#3767.

Proposed Changes

Remove ttfb_timeout from InboundUpgrade and other related structs.

(Update)
Also removed resp_timeout and also removed the NetworkParams struct since its fields are no longer used. #7925 (comment)

Additional Info

I didn't remove ttfb_timeout field from ChainSpec because the field still exists in the mainnet.yaml file.

@ackintosh ackintosh marked this pull request as ready for review August 24, 2025 21:37
@ackintosh ackintosh requested a review from jxs as a code owner August 24, 2025 21:37
@ackintosh ackintosh added the ready-for-review The code is ready for review label Aug 24, 2025
@ackintosh ackintosh requested a review from pawanjay176 August 24, 2025 21:38
@michaelsproul michaelsproul added the v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky label Aug 24, 2025
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

LGTM. In terms of removing it from the config values, there is now a spec PR that removes both ttfb and resp timeouts ethereum/consensus-specs#4532

I think we should still keep the resp timeout with a value of 10, not sure if we should change it. cc @AgeManning

@AgeManning
Copy link
Member

I agree, we should keep the timeout. It would stall sync and cause a bunch of issues if we don't timeout requests.

@pawanjay176
Copy link
Member

Awesome, @ackintosh can we remove resp_timeout config param as well and use a constant value set to 10 seconds for the stream timeout?
We can merge that and then remove the config values once that gets upstreamed.

@ackintosh
Copy link
Member Author

Thanks @pawanjay176! I removed resp_timeout as well, and also removed the NetworkParams struct since its fields are no longer used. The NetworkParams::max_payload_size field had already been unused before.

Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

LGTM, removed the NetworkParams struct like you mentioned and merged unstable.
Upstream PR also got merged, I think we should remove other references of ttfb_timeout and resp_timeout from the ChainSpec in a separate PR to give time for tooling to catch up. Will merge once CI passes

@pawanjay176 pawanjay176 changed the title Remove ttfb_timeout Remove ttfb_timeout and resp_timeout Sep 3, 2025
@mergify
Copy link

mergify bot commented Sep 3, 2025

Some required checks have failed. Could you please take a look @ackintosh? 🙏

@mergify mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Sep 3, 2025
@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Sep 3, 2025
mergify bot added a commit that referenced this pull request Sep 3, 2025
@mergify mergify bot merged commit 7b5be8b into sigp:unstable Sep 3, 2025
35 of 38 checks passed
@ackintosh ackintosh deleted the remove-ttfb-timeout branch September 3, 2025 21:25
jtraglia pushed a commit to jtraglia/lighthouse that referenced this pull request Sep 16, 2025
`TTFB_TIMEOUT` was deprecated in ethereum/consensus-specs#3767.


  Remove `ttfb_timeout` from `InboundUpgrade` and other related structs.

(Update)
Also removed `resp_timeout` and also removed the `NetworkParams` struct since its fields are no longer used. sigp#7925 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Networking ready-for-merge This PR is ready to merge. syncing v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants