Remove trackedSubnet check for explicitly named peers in network.Send() #3258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iansuvak
requested review from
danlaine,
joshua-kim and
StephenButtolph
as code owners
StephenButtolph
approved these changes
Aug 2, 2024
Comment on lines
-701
to
-703
| if trackedSubnets := peer.TrackedSubnets(); !trackedSubnets.Contains(subnetID) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
After going through the code - the one potential downside to this PR is:
If a subnet validator forgets to --track-subnet the subnet that it is validating, the nodes querying them will now need to timeout the request rather than immediately marking the request as failed.
I think that is sufficiently niche that it isn't a concern to merge this PR... But it could be something to keep in mind as a future optimization. This is not a change in any security assumptions, as byzantine nodes could always report that they are tracking the subnet but then just never respond to the request.
dhrubabasu
approved these changes
Aug 2, 2024
Co-authored-by: Dhruba Basu <[email protected]> Signed-off-by: Ian Suvak <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this should be merged
This would close ava-labs/awm-relayer#400 . This removes the requirement for an explicitly named peer in
common.SendConfigto be actively tracking the relevant subnet. This doesn't affect the allower functionality or any sampled peers.The particular case this would solve is let nodes respond to
SignatureRequestAppRequestsfrom nodes that haven't explicitly declared that they are tracking the relevant subnet which seems like a valid usecase.The function comment was also updated since it seemed to be out of date, but the update isn't relevant to the functional change since filtering out peers based on their tracked subnets was undocumented functionality.
How this works
It removes the trackedSubnet check when checking explicitly named peers supplied to
getPeersfunction which is called fromnetwork.SendHow this was tested
CI + confirmed that it allows signature aggregation on the awm-relayer E2E tests (both relayer and signature aggregator)