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

Enable enr field in GetPeers and GetPeerById #8641

Merged
merged 13 commits into from
Sep 30, 2024

Conversation

zilm13
Copy link
Contributor

@zilm13 zilm13 commented Sep 24, 2024

PR Description

Fixed Issue(s)

Fixes #7473

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

zilm13 and others added 8 commits September 24, 2024 14:50
* spotless
* fix compiler warns
* make tests compile
* Add DiscoveryPeer.getNodeId()
* Use the correct NodeId (from Discovery) instead of wrong Libp2p PeerId when calculating DAS subnets
* Add Eth2Peer.getDiscoveryNodeId() method.
@@ -139,6 +144,18 @@ public Eth2P2PNetwork build() {
// Setup eth2 handlers
final SubnetSubscriptionService attestationSubnetService = new SubnetSubscriptionService();
final SubnetSubscriptionService syncCommitteeSubnetService = new SubnetSubscriptionService();

// TODO a bit hacky solution, subject to be refactored
Copy link
Contributor

Choose a reason for hiding this comment

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

also would prefer to not have 'hacky solution' in a comment...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's draft yet, it was PoC, I wanted to check if it works first, because it's not very straightforward

Comment on lines 151 to 153
LibP2PPeer libP2PPeer = (LibP2PPeer) peer;
PubKey libP2PPubKey = libP2PPeer.getPubKey();
Bytes discoveryNodeIdBytes =
Copy link
Contributor

@rolfyone rolfyone Sep 25, 2024

Choose a reason for hiding this comment

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

we'd also at least want finals... i'd probably just make this a private function that gives a response, or package private and we can easily test it then...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice idea, thank you!

@@ -34,6 +37,16 @@
public class NodeRecordConverter {
private static final Logger LOG = LogManager.getLogger();

public Bytes convertPublicKeyToNodeId(final Bytes publicKey) {
// TODO need to open an additional API in discovery instead of this hack
NodeRecord tempNodeRecord =
Copy link
Contributor

Choose a reason for hiding this comment

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

final

@zilm13 zilm13 marked this pull request as ready for review September 27, 2024 19:59
Copy link
Contributor

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM. i haven't explicitly run it to see it working, but as long as you have im happy...

@zilm13 zilm13 merged commit 659262e into Consensys:master Sep 30, 2024
17 checks passed
@zilm13 zilm13 deleted the get-peers-enr branch September 30, 2024 13:07
zilm13 added a commit to zilm13/teku that referenced this pull request Sep 30, 2024
* Expose discovery NodeId to DiscoveryPeer and LibP2PPeer (Consensys#38)
* Add DiscoveryPeer.getNodeId()
* Add Eth2Peer.getDiscoveryNodeId() method.
* Refactoring, discovery updated to 24.9.1
---------

Co-authored-by: Anton Nashatyrev <Nashatyrev@users.noreply.github.com>
zilm13 added a commit to Nashatyrev/teku that referenced this pull request Oct 1, 2024
* Expose discovery NodeId to DiscoveryPeer and LibP2PPeer (#38)
* Add DiscoveryPeer.getNodeId()
* Add Eth2Peer.getDiscoveryNodeId() method.
* Refactoring, discovery updated to 24.9.1
---------

Co-authored-by: Anton Nashatyrev <Nashatyrev@users.noreply.github.com>
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.

/eth/v1/node/peers endpoint does not include ENR
3 participants