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

Manually tracked peers #521

Merged
merged 7 commits into from
Oct 31, 2024
Merged

Manually tracked peers #521

merged 7 commits into from
Oct 31, 2024

Conversation

michaelkaplan13
Copy link
Collaborator

Why this should be merged

Supercedes #519.

For development and testing purposes, it should be easy to create L1s on the Etna Devnet and Fuji using a single local validator node. Assuming the machine running single validator is not other publicly accessible then the validator will not be discovered by the info.Peers RPC. This allows for a basic work around to still be able to create aggregate signatures from these L1s by providing an explicit list of peers that should be connected to from the aggregator.

Note: Medium term, we should make Network an interface and allow any implementation to be provided to the aggregator. That way use cases can more cleanly/extensibly implement their own validator peer discovery mechanisms. This is just a shorter term work around.

How this works

Adds manuallyTrackedPeers to the appRequestNetwork, and allows it to be set via NewNetwork. The aggregator will always attempt to connect to these peers whether or not they are included in the info.Peers result.

How this was tested

Through usage in the CLI

How is this documented

Comments

@iansuvak
Copy link
Contributor

AppRequestNetwork is already an interface actually so this change can already be made on the tooling side. I'm not opposed to merging this as is but it does introduce an avalanchego dependency on an unmerged tag v1.12.0-initial-poc.5 which I'm not 100% we want to do.

Copy link
Contributor

@bernard-avalabs bernard-avalabs left a comment

Choose a reason for hiding this comment

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

Make sense to me. It may be worthwhile to add an assertion that only allows manually tracked peers to be provided for testing/development.

@@ -165,6 +173,9 @@ func (n *appRequestNetwork) ConnectPeers(nodeIDs set.Set[ids.NodeID]) set.Set[id
return nil
}

// Add manually tracked peers
peers = append(peers, n.manuallyTrackedPeers...)
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the system will behave correctly even if a peer appears more than once in the peers list (i.e., both returned by infoAPI.peers and is a member of the manuallyTrackedPeers).

Copy link
Contributor

Choose a reason for hiding this comment

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

yes it will since we are using sets below and ManuallyTrack is idempotent.

@arturrez
Copy link
Contributor

AppRequestNetwork is already an interface actually so this change can already be made on the tooling side. I'm not opposed to merging this as is but it does introduce an avalanchego dependency on an unmerged tag v1.12.0-initial-poc.5 which I'm not 100% we want to do.

looking at the PR I don't think that v1.12.0-initial-poc.5 is a requirement. If I understand this correctly there is no reason to include it there

@iansuvak
Copy link
Contributor

looking at the PR I don't think that v1.12.0-initial-poc.5 is a requirement. If I understand this correctly there is no reason to include it there

It was a requirement to make the Uptime interface match what is expected by other dependencies. Now that v1.11.12 is out though that interface is merged to master so we can update this to use that version of avalanchego

Copy link
Contributor

@iansuvak iansuvak left a comment

Choose a reason for hiding this comment

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

Looking over the interface again there is no way to make the change purely on the tooling side without making changes both to the AppRequestNetwork interface here as well as exporting the implementing appRequestNetwork struct so I'm fine with merging this as is.

Resolving go.mod conflicts will remove my other concern of using a non-merged tag in main.

@michaelkaplan13 michaelkaplan13 merged commit 9f3254b into main Oct 31, 2024
8 checks passed
@michaelkaplan13 michaelkaplan13 deleted the manually-tracked-peers branch October 31, 2024 17:45
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.

6 participants