Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add api node to network if it is a validator #211

Merged
merged 10 commits into from
Mar 5, 2024

Conversation

felipemadero
Copy link
Contributor

Why this should be merged

When the so called API node is inside the validator set for the source blockchain, it is not added itself as it is not a peer of himself (only peers to API node are added). In situations were that validator is needed to fully sign a msg, teleporter can't proceed beyond relayer step.

How this works

The API node adds himself is belonging to the validators set

How this was tested

It was tested on teleporter deploys on fuji for subnets of only one validator.

How is this documented

Copy link
Collaborator

@cam-schultz cam-schultz left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this up, we should definitely account for this case.

However, this is incompatible with the public API, as info.GetNodeID and info.GetNodeIP are disabled. Instead, I think we should move your changes below the existing peer connection loop like so:

get the list of peers
foreach peer {
    attempt to connect to peer
}

// new code starts here
if len(connectedPeers) < len(nodeIDs) {
    attempt manually connect to API node
}

If the relayer is configured to use the public API, then the peer connection loop should succeed, and we will not call info.GetNodeID / info.GetNodeIP. Otherwise, if the info API node is also a validator, it's required for those two methods to be enabled.

Can you please make this change, as well as document the updated API requirements in the README? Thanks.

@felipemadero felipemadero requested a review from cam-schultz March 4, 2024 18:06
@felipemadero
Copy link
Contributor Author

Thanks for bringing this up, we should definitely account for this case.

However, this is incompatible with the public API, as info.GetNodeID and info.GetNodeIP are disabled. Instead, I think we should move your changes below the existing peer connection loop like so:

get the list of peers
foreach peer {
    attempt to connect to peer
}

// new code starts here
if len(connectedPeers) < len(nodeIDs) {
    attempt manually connect to API node
}

If the relayer is configured to use the public API, then the peer connection loop should succeed, and we will not call info.GetNodeID / info.GetNodeIP. Otherwise, if the info API node is also a validator, it's required for those two methods to be enabled.

Can you please make this change, as well as document the updated API requirements in the README? Thanks.

done

felipemadero and others added 5 commits March 4, 2024 17:17
Co-authored-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com>
Signed-off-by: felipemadero <felipe.madero@gmail.com>
Co-authored-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com>
Signed-off-by: felipemadero <felipe.madero@gmail.com>
Co-authored-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com>
Signed-off-by: felipemadero <felipe.madero@gmail.com>
Co-authored-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com>
Signed-off-by: felipemadero <felipe.madero@gmail.com>
@felipemadero felipemadero requested a review from cam-schultz March 4, 2024 20:24
Copy link
Collaborator

@cam-schultz cam-schultz left a comment

Choose a reason for hiding this comment

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

LGTM

@cam-schultz cam-schultz merged commit a45f270 into main Mar 5, 2024
7 checks passed
@cam-schultz cam-schultz deleted the add-api-node-to-network-if-validator branch March 5, 2024 00:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants