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

Eth1 connections #10073

Merged
merged 11 commits into from
Jan 14, 2022
Merged

Conversation

michaelneuder
Copy link
Contributor

What type of PR is this?
Feature

What does this PR do? Why is it needed?
This implements an API to get the info about current ETH1 connections. It was a feature request opened in #9972.

Which issues(s) does this PR fix?
Fixes #9972.

Other notes for review

  1. I added functions to the ChainInfoFetcher interface rather than creating a new interface in the powchain package.
  2. For the beacon-chain/powchain/service_test.go, I was only able to unit test CurrentETH1Endpoint() and ETH1Endpoints(). I couldn't find a test that calls dialETH1Nodes. Since the functions are really quite simple, hopefully living without the coverage is OK. But let me know if there is something I am missing.
  3. For beacon-chain/rpc/prysm/v1alpha1/node/service_test.go I needed a mock that implemented the powchain.ChainInfoFetcher interface. I added it to the testutil package in beacon-chain/rpc/testutil/mock_powchain_info_fetcher.go. The mock just returns the fields it was constructed with or with the zero values.

@michaelneuder michaelneuder requested a review from a team as a code owner January 12, 2022 03:18
@CLAassistant
Copy link

CLAassistant commented Jan 12, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ michaelneuder
✅ rauljordan
❌ Michael Neuder


Michael Neuder seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rauljordan
Copy link
Contributor

Awesome work @michaelneuder this works as expected! I hope you don't mind I pushed a few commits to generate the protos and some Bazel fixes. I have just tested this out locally and it works as expected:

curl http://localhost:3500/eth/v1alpha1/node/eth1/connections
{"currentAddress":"http://localhost:8545","currentConnectionError":"Post \"http://localhost:8545\": dial tcp [::1]:8545: connect: connection refused","addresses":["http://localhost:8545"],"connectionErrors":["Post \"http://localhost:8545\": dial tcp [::1]:8545: connect: connection refused"]}%  

I even found out my eth1 node was down due to your feature, so thanks a lot :)

@rauljordan
Copy link
Contributor

For some reason our CLA assistant wants a second Michael to sign the agreement. Ill see if I can skip that, because it might have been from a commit on another computer you had

rauljordan
rauljordan previously approved these changes Jan 12, 2022
@michaelneuder
Copy link
Contributor Author

michaelneuder commented Jan 13, 2022

cool! thanks for the review!! I am going to try to sort out these failing checks in order to merge.

@rauljordan
Copy link
Contributor

hi @michaelneuder I see some failures in building due to
https://github.com/prysmaticlabs/prysm/runs/4794010970?check_suite_focus=true

Once the PR is green, ill make an exception for the CLA assistant and merge

@michaelneuder
Copy link
Contributor Author

Ok I made a few more changes in regards to the errors we were looking at. I think I messed up the git state a bit, but it seems like all the changes are here. A few notes:

  1. I added GetETH1ConnectionStatus to testing/mock/node_service_mock.go. This fixes one of the interface issues.
  2. I added the mock interfaces for the powchain side to beacon-chain/powchain/testing/mock_powchain.go. This fixed the other interface issue, and also allowed me to remove my mock fetcher, because I can just use mockPOW.POWChain struct in my beacon-chain/rpc/prysm/v1alpha1/node/server_test.go.

Hoping this fixes the checks that we were erroring out on.

rauljordan
rauljordan previously approved these changes Jan 14, 2022
@rauljordan rauljordan merged commit 9166f40 into prysmaticlabs:develop Jan 14, 2022
@rauljordan
Copy link
Contributor

Thank you @michaelneuder this is merged!

Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

It seems I was too late to review this before it was merged in, but there is a resource leak here with regards to dialing the eth1 json-rpc server.


// CurrentETH1ConnectionError returns the error (if any) of the current connection.
func (s *Service) CurrentETH1ConnectionError() error {
_, _, err := s.dialETH1Nodes(s.cfg.currHttpEndpoint)
Copy link
Member

Choose a reason for hiding this comment

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

The clients should be closed after we dial them otherwise there is a resource leak and the connection is
kept active.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, totally missed this. thanks for addressing!

func (s *Service) ETH1ConnectionErrors() []error {
var errs []error
for _, ep := range s.cfg.httpEndpoints {
_, _, err := s.dialETH1Nodes(ep)
Copy link
Member

Choose a reason for hiding this comment

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

Same thing applies here

@prestonvanloon
Copy link
Member

@michaelneuder Could you kindly address the feedback from @nisdas?
We'll need to fix those concerns or revert these changes prior to next release.
Thanks!

@michaelneuder
Copy link
Contributor Author

Sorry missed these comments!

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

Successfully merging this pull request may close these issues.

Display Currently-Used HTTP Connection to Eth1 Node via Healthz or an API Endpoint
5 participants