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

Add ForkDigest filter #123

Closed
wants to merge 1 commit into from
Closed

Add ForkDigest filter #123

wants to merge 1 commit into from

Conversation

migalabs
Copy link

Description

We ran an instance of the Nodewatch eth2-crawler for 24 hours (latest commit in the master branch c161d309fcbf7cd4455c736e089def94b3919463), and looked at the ForkDigest exposed by each peer. We noticed that only 83.1% of them correspond to the Altair mainnet, 2.5% correspond to Phase 0 (before Altair) and 14.4% to other testnets (See figure below).

We are not aware of the purpose of the Nodewatch eth2-crawler and the collected data. We have not been able to find any documentation about the networks it operates on. Our conclusion is that, if the purpose of the crawler is to show all the peers participating in any of the Eth2 networks (including testnets), then this information should be explicitly mentioned in the website/dashboard, as anyone who reads it could think that it only applies to the Eth2 mainnet (Altair). However, if the purpose of your crawler is to gather data from the peers connected just to the Eth2 mainnet (Altair fork), then the filter proposed in this PR should be applied.

Please note that the filter by client version that you implemented in your commit of October 22nd c63e61b1940b2c56542926597744bed21e024e74 is not enough, as there could be nodes with an updated client version that is compatible with the new fork (Altair), but the node is not necessarily participating in the Eth2 mainnet, but could be connected to other testnet (e.g., prater).

image

Changes

  • Adding condition to check the ForkDigest before storing Peer in the database
  • Only Peers with the defined ForkDigest will be stored

Closes: #?

There are no issues related to this Pull Request

@CLAassistant
Copy link

CLAassistant commented Nov 18, 2021

CLA assistant check
All committers have signed the CLA.

@infosecual
Copy link

The EF is happy with supporting mainnet only. If we can spin up separate instances to monitor testnets when needed.

Copy link

@infosecual infosecual left a comment

Choose a reason for hiding this comment

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

LGTM. Will defer to NodeWatch team to approve.

@dhyaniarun1993
Copy link
Member

Thanks @migalabs. We are looking into this.

@sadiq1971
Copy link
Member

LGTM. But need same check on existing peers on db to filter out the testnet peers.

Copy link
Member

@dhyaniarun1993 dhyaniarun1993 left a comment

Choose a reason for hiding this comment

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

@migalabs Please sign the contributors license

@migalabs
Copy link
Author

@migalabs Please sign the contributors license

It should be already signed, let us know if there are further steps to follow

@wemeetagain
Copy link
Member

IMO it might be nice for the crawler to grab all eth2 peers, and only filter afterwards in the database queries.

I'd argue that storing / querying eth2 peers across all testnets is a small additional overhead, and that it gives you flexibility to perform possibly interesting queries down the line.

@dhyaniarun1993
Copy link
Member

@migalabs Please sign the contributors license

It should be already signed, let us know if there are further steps to follow

@migalabs Please sign the contributors license. It's still pending

@migalabs
Copy link
Author

@migalabs Please sign the contributors license

It should be already signed, let us know if there are further steps to follow

@migalabs Please sign the contributors license. It's still pending

Hey @dhyaniarun1993 ! Sorry for the long delay, but from our side, everything seems to be fine. This is what the CLA status for us looks like:
image
Is there anything else that we need to fill up or sing?

@sadiq1971
Copy link
Member

Done it here #206

@sadiq1971 sadiq1971 closed this Nov 1, 2022
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.

7 participants