Skip to content

Conversation

@rmoreliovlabs
Copy link
Contributor

Description

I have implemented the analysis documentation that explores various options for enabling SNAP feature discovery among connected peers. You can view the detailed analysis here: SNAP Feature Discovery Analysis

Motivation and Context

Establishing a decentralized approach for RSKj nodes to seamlessly discover and establish connections with SNAP-sync-provider peers during their initial connection phase. This enhancement will focus on integrating a new capability into the HelloMessage and HandshakeHandler classes, thereby facilitating efficient peer discovery and connectivity within the network

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Tests for the changes have been added (for bug fixes / features)
  • Requires Activation Code (Hard Fork)
  • Other information:

@rmoreliovlabs rmoreliovlabs marked this pull request as draft July 22, 2024 13:17
@rmoreliovlabs rmoreliovlabs force-pushed the feature/SNAP_capability_discovery branch 2 times, most recently from df4836c to b7c2bfc Compare July 22, 2024 15:37
@rmoreliovlabs rmoreliovlabs force-pushed the feature/SNAP_capability_discovery branch from b7c2bfc to 2dffc6a Compare July 22, 2024 19:33
@rmoreliovlabs rmoreliovlabs marked this pull request as ready for review July 22, 2024 20:04
Copy link
Contributor

@asoto-iov asoto-iov left a comment

Choose a reason for hiding this comment

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

As a summary I think that before approve we should discuss about:

  • having a snap sync versioning.
  • Review if it has sense to finish the handshake if the RSK protocol is not supported.

Copy link
Contributor

@Vovchyk Vovchyk left a comment

Choose a reason for hiding this comment

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

now we would also need to filer snap-capable peers in here. And for that we'd need to extend Peer interface with new method - boolean isSnapCapable(), wdyt?

@asoto-iov
Copy link
Contributor

it is still missing what Vlad mentioned here: #2628 (review)

@rmoreliovlabs rmoreliovlabs requested a review from asoto-iov August 5, 2024 14:30
Copy link
Collaborator

@fmacleal fmacleal left a comment

Choose a reason for hiding this comment

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

Good job. I have a few comments, some improvements suggestion and also other related with tests. Let me know what you think. 🙂

@rmoreliovlabs rmoreliovlabs requested a review from fmacleal August 13, 2024 12:39
Copy link
Collaborator

@fmacleal fmacleal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@asoto-iov asoto-iov left a comment

Choose a reason for hiding this comment

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

Please review the new comment. I think it is needed to review the getBestSnapPeer candidate filtering to avoid unnecessary communication with non snap capable peers.

@sonarqubecloud
Copy link

@fmacleal fmacleal merged commit 80483ff into snap-v4-final Aug 21, 2024
@aeidelman aeidelman added this to the Lovell 7.1.0 milestone May 13, 2025
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