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

Config for web3signer keep-alive #5007

Merged
merged 8 commits into from
Feb 1, 2024

Conversation

michaelsproul
Copy link
Member

Issue Addressed

Addresses #4439 in the context of web3signer connections.

One of the large staking pools was having issues with web3signer connections failing with IncompleteMessage errors.

Proposed Changes

  • Add a flag to control the connection idle timeout. Web3signer uses 30s by default so anything shorter than this should help. In practice the staking pool found success using an extremely short 10ms cutoff, but we didn't experiment with pushing it higher.
  • Add a flag to control connection pooling. Setting the pool size to 0 is reported to help somewhat (see Error: IncompleteMessage: connection closed before message completed hyperium/hyper#2136).

Additional Info

I'm not sure if this is the best way to solve this. Requiring the node operator to fiddle with yet more CLI flags is bad from a UX PoV. We may be better off adopting automatic retry.

@michaelsproul michaelsproul added bug Something isn't working val-client Relates to the validator client binary do-not-merge labels Dec 13, 2023
@michaelsproul michaelsproul changed the base branch from stable to unstable December 13, 2023 22:55
@dapplion dapplion added the work-in-progress PR is a work-in-progress label Jan 29, 2024
@michaelsproul michaelsproul marked this pull request as ready for review January 31, 2024 05:32
@michaelsproul
Copy link
Member Author

Although there may be better approaches than these flags, we have one Lido operator reporting success with these flags on Holesky, to the point where they are requesting custom builds of Lighthouse release that include this change. I think we may as well include this in (un)stable for now and we can reassess in future.

The flag they are using is web3-signer-keep-alive-timeout. I would be open to removing web3-signer-max-idle-connections if we want to minimise the diff size.

@michaelsproul michaelsproul added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress do-not-merge labels Jan 31, 2024
@michaelsproul michaelsproul changed the title Web3signer keep alive Config for web3signer keep alive Jan 31, 2024
@michaelsproul michaelsproul changed the title Config for web3signer keep alive Config for web3signer keep-alive Jan 31, 2024
Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

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

LGTM

@realbigsean realbigsean added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jan 31, 2024
@michaelsproul
Copy link
Member Author

@Mergifyio queue

Copy link

mergify bot commented Feb 1, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 8fb6989

@mergify mergify bot merged commit 8fb6989 into sigp:unstable Feb 1, 2024
30 checks passed
danielramirezch pushed a commit to danielramirezch/lighthouse that referenced this pull request Feb 14, 2024
* Allow tweaking connection pool settings

* Build docker image

* Fix imports

* Merge tag 'v4.6.0' into web3signer-keep-alive

v4.6.0

* Delete temp docker build stuff

* Fix tests

* Merge remote-tracking branch 'origin/unstable' into web3signer-keep-alive

* Update CLI text
@michaelsproul michaelsproul deleted the web3signer-keep-alive branch April 16, 2024 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready-for-merge This PR is ready to merge. val-client Relates to the validator client binary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants