Validate peers on startup #231

cam-schultz · 2024-03-13T19:07:09Z

Why this should be merged

Fixes #193

How this works

Manually connects to subnet validators on startup, rather than at message relay time. Each subnet must be connected to enough stake that any of the configured destinations would be able to validate Warp messages sent by the subnet, otherwise the relayer exits. We still need to attempt to connect to peers at message relay time, since the validator set may have changed. This change is more intended to surface any issues earlier, rather than optimizing performance.

In addition to moving this check to startup, this change also refactors some of the peer connection and signature management logic into common helpers.

How this was tested

CI

How is this documented

N/A

cam-schultz · 2024-03-13T20:54:58Z

relayer/message_relayer.go

@@ -483,55 +465,6 @@ func (r *messageRelayer) createSignedMessageAppRequest(requestID uint32) (*avala
 	return nil, errNotEnoughSignatures
 }

-func (r *messageRelayer) getCurrentCanonicalValidatorSet() ([]*avalancheWarp.Validator, uint64, error) {


Signing subnet check moved to messageRelayer constructor; the rest of this function moved to CanonicalValidatorClient.GetCurrentCanonicalValidatorSet

cam-schultz · 2024-03-13T20:55:33Z

relayer/message_relayer.go

 			zap.Error(err),
 		)
 		return nil, err
 	}
-
-	// We make queries to node IDs, not unique validators as represented by a BLS pubkey, so we need this map to track


This logic was moved to AppRequestNetwork.ConnectToCanonicalValidators

cam-schultz · 2024-03-13T20:55:47Z

relayer/message_relayer.go

@@ -156,18 +175,6 @@ func (r *messageRelayer) createSignedMessage() (*avalancheWarp.Message, error) {
 		)
 		return nil, err
 	}
-	signingSubnetID := r.relayer.sourceSubnetID


This was moved to the messageRelayer constructor

cam-schultz · 2024-03-13T20:57:20Z

config/config.go

-	// convenience fields to access the source subnet and chain IDs after initialization
-	sourceSubnetIDs     []ids.ID
-	sourceBlockchainIDs []ids.ID


These were only used to pass to the app request network constructor. With this change, it makes more sense to pass the configuration and iterate over the SourceBlockchain and DestinationBlockchain slices directly, so the parsed IDs were moved there.

cam-schultz · 2024-03-13T20:58:16Z

peers/app_request_network.go

@@ -103,83 +102,99 @@ func NewNetwork(
 		return nil, nil, err
 	}

-	// We need to initially connect to some nodes in the network before peer


There's no need to connect to these beacon nodes, since we explicitly connect to every node that we send AppRequest messages to now.

michaelkaplan13 · 2024-03-14T14:31:51Z

config/config.go

@@ -417,6 +401,18 @@ func (s *SourceBlockchain) Validate(destinationBlockchainIDs *set.Set[string]) e
 		}
 	}

+	// Validate and store the subnet and blockchain IDs for future use


Not for this PR, but we should consider splitting config.go up into multiple files at some point soon. I think SourceBlockchain and DestinationBlockchain could each be put in their own file still in the config package and could help improve navigation/readability.

agreed and some of the similarities between the source/destination can be abstracted

some of the similarities between the source/destination can be abstracted

Viper doesn't play nicely with this unfortunately. There may be a workaround, but I haven't looked to deeply into it.

It doesn't look too bad, but we can definitely defer to a separate ticket https://stackoverflow.com/questions/47185318/multiple-config-files-with-go-viper

To clarify, I think we should keep a single user-facing configuration file, but split it into multiple Go files for easier code readability.

I experimented with Matt's suggestion of composing SourceBlockchain and DestinationBlockchain using a common type, but Viper seemed to have trouble unmarshalling JSON into them. Definitely worth revisiting if we decide to go for those config improvements.

database/json_file_storage.go

main/main.go

michaelkaplan13 · 2024-03-14T14:40:30Z

peers/app_request_network.go

-			break
+	// Manually connect to the validators of each of the source subnets.
+	// We return an error if we are unable to connect to sufficient stake on any of the subnets.
+	// Sufficient stake is determined by the Warp quora of the configured supported destinations,


TIL that "quora" is a word. 😅

michaelkaplan13 · 2024-03-14T14:42:51Z

peers/app_request_network.go

+	// We return an error if we are unable to connect to sufficient stake on any of the subnets.
+	// Sufficient stake is determined by the Warp quora of the configured supported destinations,
+	// or if the subnet supports all destinations, by the quora of all configured destinations.
+	for _, sourceBlockchain := range cfg.SourceBlockchains {


This loop body is pretty large. May want to consider moving parts out to helper functions.

Would also be great to add unit tests for this since the logic is relatively complex. I'm not sure how difficult it is to mock the various clients and arNetwork though.

To add unit tests, I believe we'd have to add mocks for the info client, the P-Chain client, and the underlying Network. All doable, but it might be a large enough change to warrant its own ticket.

peers/app_request_network.go

database/json_file_storage.go

main/main.go

peers/app_request_network.go

geoff-vball · 2024-03-14T15:43:17Z

peers/app_request_network.go

+					logger.Error(
+						"Failed to get subnet ID",
+						zap.Error(err),
+					)
+					return nil, nil, err


In an effort to reduce to code complexity a little bit, I think if we don't require and additional info in the log (ex the blockchain ID), then it would be best skip logging here and to wrap the error:
return nil, nil, fmt.Errorf("failed to get subnet ID: %w", err)
This way, all the important info will get logged together at some point up the stack. As is, we'll get a bunch of disjointed errors that will be harder to correlate.

Doesn't have to be something we do in this PR, but I think it would be good practice to go through and implement at some point.

Agreed that we can have a more unified logging / error handling strategy. I also agree that we should go through and unify the repo once we decide on that strategy. In the meantime, however, I'd like to err on the side of over-verbosity and log where we can, even if some logs are redundant.

geoff-vball · 2024-03-14T15:50:48Z

peers/app_request_network.go

+	ConnectedWeight       uint64
+	TotalValidatorWeight  uint64


I think it would be better to have at least TotalValidatorWeight not be in the struct here, but have a receiver function on ConnectedCanonicalValidators that will iterate through and add up all the weights. This would avoid bugs down the road if anyone mutates in the Validator set.

If we did the same for ConnectedWeight, it would be easier to get the updated weight of all our connections. If we need to use the connected weight many times and don't want to try connecting to all the nodes every time, maybe make the field MostRecentConnectedWeight and then add a receiver method UpdateConnectedWeight

I think those ideas make sense, but I'm going to defer that for later. I think we'll likely want to add an async routine to update the validator set, rather than fetching it in every message relay, at which point we'll want to carefully consider how the global state is managed. For now though, this type is only used in the message relayer, so the risk of it being misused is fairly low.

relayer/message_relayer.go

peers/app_request_network.go

minghinmatthewlam · 2024-03-14T20:50:23Z

peers/app_request_network.go

+// Helper struct to hold connected validator information
+// Warp Validators sharing the same BLS key may consist of multiple nodes,
+// so we need to track the node ID to validator index mapping
+type ConnectedCanonicalValidators struct {


I think we should move this struct to a separate file, and ideally have mockign tests

Eh, I don't think it's worth all that special treatment since this type is POD. It's really just there to simplify calls to ConnectToCanonicalValidators and keep associated data packaged together.

peers/app_request_network.go

geoff-vball · 2024-03-15T22:18:17Z

config/config.go

@@ -417,6 +401,18 @@ func (s *SourceBlockchain) Validate(destinationBlockchainIDs *set.Set[string]) e
 		}
 	}

+	// Validate and store the subnet and blockchain IDs for future use


It doesn't look too bad, but we can definitely defer to a separate ticket https://stackoverflow.com/questions/47185318/multiple-config-files-with-go-viper

cam-schultz added 10 commits March 12, 2024 21:14

src and dst cfgs store ids

7a4c847

canonical vdrs in own package

0fbcb79

connect peers on startup

0c0cdfd

ConnectToCanonicalValidators helper

55b6b24

add helper type

c02cf8b

fix linter

25e2472

connect to signing subnet

6bb878f

use struct var

b4e681c

primary network handling

5db104b

clean up peer utils

67e2410

cam-schultz requested review from michaelkaplan13, minghinmatthewlam, gwen917, geoff-vball and bernard-avalabs as code owners March 13, 2024 19:07

cam-schultz self-assigned this Mar 13, 2024

Merge branch 'main' into validate-peers

5e0b9f1

cam-schultz changed the title ~~Validate peers~~ Validate peers on startup Mar 13, 2024

cam-schultz commented Mar 13, 2024

View reviewed changes

do not connect to beacon nodes

253e247

cam-schultz commented Mar 13, 2024

View reviewed changes

remove unused var

3976dbc

michaelkaplan13 reviewed Mar 14, 2024

View reviewed changes

geoff-vball reviewed Mar 14, 2024

View reviewed changes

minghinmatthewlam reviewed Mar 14, 2024

View reviewed changes

peers/app_request_network.go Outdated Show resolved Hide resolved

minghinmatthewlam reviewed Mar 14, 2024

View reviewed changes

peers/app_request_network.go Outdated Show resolved Hide resolved

minghinmatthewlam reviewed Mar 14, 2024

View reviewed changes

cam-schultz added 7 commits March 14, 2024 21:53

pass source chain cfg to db ctr

f4bc236

update comment

5ee36e3

pr feedback

f3d64ad

add vdr client to network struct

0cb6139

refactor connection loop

9311703

remove comment

a2c0e86

cache chain->subnet mapping

46e8fb8

michaelkaplan13 reviewed Mar 15, 2024

View reviewed changes

peers/app_request_network.go Outdated Show resolved Hide resolved

michaelkaplan13 reviewed Mar 15, 2024

View reviewed changes

peers/app_request_network.go Outdated Show resolved Hide resolved

cam-schultz added 2 commits March 15, 2024 20:04

supported dsts already parsed

9fa3f75

add helpers

f504c2f

cam-schultz mentioned this pull request Mar 15, 2024

Add AppRequestNetwork unit tests #237

Open

Merge branch 'main' into validate-peers

d48e359

cam-schultz requested review from michaelkaplan13, minghinmatthewlam and geoff-vball March 15, 2024 20:59

michaelkaplan13 approved these changes Mar 15, 2024

View reviewed changes

geoff-vball approved these changes Mar 19, 2024

View reviewed changes

cam-schultz merged commit 54888f9 into main Mar 19, 2024
6 of 7 checks passed

cam-schultz deleted the validate-peers branch March 19, 2024 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate peers on startup #231

Validate peers on startup #231

cam-schultz commented Mar 13, 2024

cam-schultz Mar 13, 2024

cam-schultz Mar 13, 2024

cam-schultz Mar 13, 2024

cam-schultz Mar 13, 2024

cam-schultz Mar 13, 2024

michaelkaplan13 Mar 14, 2024

minghinmatthewlam Mar 14, 2024

cam-schultz Mar 14, 2024

geoff-vball Mar 15, 2024

cam-schultz Mar 19, 2024

michaelkaplan13 Mar 14, 2024

michaelkaplan13 Mar 14, 2024

michaelkaplan13 Mar 14, 2024

cam-schultz Mar 15, 2024

geoff-vball Mar 14, 2024

cam-schultz Mar 15, 2024

geoff-vball Mar 14, 2024

cam-schultz Mar 15, 2024

minghinmatthewlam Mar 14, 2024

cam-schultz Mar 15, 2024

geoff-vball Mar 15, 2024

Validate peers on startup #231

Validate peers on startup #231

Conversation

cam-schultz commented Mar 13, 2024

Why this should be merged

How this works

How this was tested

How is this documented

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment