Configuration support fully specified source to destination mapping #59

gwen917 · 2023-10-13T17:20:21Z

Why this should be merged

Allow relayer only relays the message between specific subnets, instead of relaying all the messages between source subnets to destination subnets
Add AllowedDestinations chainIDs in source subnets to specify the allowed destinations.

How this was tested

Unit tests, CI

config/config.go

messages/teleporter/message_manager.go

config/config.go

michaelkaplan13 · 2023-10-17T14:12:33Z

config/config.go

-	WSEndpoint        string                           `mapstructure:"ws-endpoint" json:"ws-endpoint"`
-	MessageContracts  map[string]MessageProtocolConfig `mapstructure:"message-contracts" json:"message-contracts"`
+	SubnetID            string                           `mapstructure:"subnet-id" json:"subnet-id"`
+	ChainID             string                           `mapstructure:"chain-id" json:"chain-id"`


Is this "ChainID" the referring to the EVM chain ID, or the Avalanche-specific blockchain ID?

@cam-schultz may know best here, but if it's the blockchain ID, lets rename it to avoid future confusion.

Yes, this is the Avalanche blockchain ID.

This is an unfortunate overloading of terms, but I agree we should make the distinction where we can. FWIW, subnet-evm isn't consistent either: ethclient.Client.ChainID returns the blockchainID, while the Warp precompile has the unambiguous method getBlockchainID. I don't think the EVM chain ID comes into play in subnet-evm or awm-relayer.

There are 479 hits for chainID in this repo, however... Do you mind if we save this for a separate PR to not pollute the diff?

Similar to the above ambiguity in naming, DevRel brought up the confusion of destinationChainID in TeleporterMessenger, should we change try to change all references of chainID in favor of blockchainID so it'd be destinationBlockchainID? Is this enough of a differentiation for people to not get confused with EVM chainID?

afaik ethclient.Client.ChainID returns the eth chainID. I don't think the eth client has any way of knowing the blockchainID.

Also note that in the go code, chainID is a big.Int and blockchainID is a [32]byte

Created an issue here: #70

config/config.go

messages/teleporter/message_manager.go

michaelkaplan13 · 2023-10-17T14:33:52Z

messages/teleporter/message_manager.go

+				zap.String("warpMessageID", warpMessageInfo.WarpUnsignedMessage.ID().String()),
+				zap.String("teleporterMessageID", teleporterMessage.MessageID.String()),
+			)
+			return false, nil


In the case above where the destination chain ID does not have a destination client, we return false and a non-nil error. I noticed this case is different in that we return false, nil. Inspecting the code, it looks like it's incorrect to return an error in the case above (unless I'm missing another check on whether or not the destination chain ID is supported elsewhere).

@cam-schultz could you confirm this?

Yeah, that case should not be an error. With the upcoming subnet-evm changes (as well as anycasting support in the future), I think ShouldSendMessage is the right place to check against the list of configured destinations.

Thinking about it some more, we can clean up this implementation by using the configured allowed destinations as a filter in on the list of destination clients in NewRelayer before the call to NewMessageManager. We'd then pass in the filtered list of destination clients. In relayer.go, we'd add something like:

var filteredDestinationClients map[ids.ID]ethclient.Client allowedDestinationChainIDs, err := sourceSubnetInfo.GetAllowedDestinations() if len(m.allowedDestinations) > 0 { filteredDestinationClients := make(map[ids.ID]ethclient.Client) for _, id := range m.allowedDestinations { filteredDestinationClients[id] = destinationClients[id] } } else { filteredDestinationClients = destinationClients } ... messageManager, err := messages.NewMessageManager(logger, addressHash, config, filteredDestinationClients, allowedDestinationChainIDs)

This would move logic for deciding which destinations are and are not valid to the relayer constructor which ones once, rather than the message manager ShouldSendMessage implementation, which runs for each message. When processing a message log, we could then reference the allowed destinations for that source subnet here

would agree that we should move the destination client filtering logic to NewRelayer to prevent checking each message.

bernard-avalabs

Generally makes sense to me. I left a few minor comments.

messages/teleporter/message_manager.go

bernard-avalabs · 2023-10-19T15:38:27Z

messages/teleporter/message_manager.go

@@ -105,6 +108,22 @@ func (m *messageManager) ShouldSendMessage(warpMessageInfo *vmtypes.WarpMessageI
 	if !ok {
 		return false, fmt.Errorf("relayer not configured to deliver to destination. destinationChainID=%s", destinationChainID.String())
 	}
+
+	// If supportedDestinationss is empty, then all destinations are allowed


This seems opposite than the expected behavior (one normally assumes that if the allow list is empty, then nothing is allowed).

This is similar to how the "allowedRelayerAddresses" field is handled in TeleporterMessenger: https://github.com/ava-labs/teleporter/blob/main/contracts/src/Teleporter/TeleporterMessenger.sol#L126

The expected use case here is that if the supportedDestinations JSON key is not provided, we don't filter any of the configured destinations. This would be useful if the relayer operator wants messages from subnet A to be relayed to anybody, but messages from subnet B to only be relayed to subnet C, for example.

minghinmatthewlam · 2023-10-23T22:14:03Z

README.md

@@ -25,6 +25,85 @@ The relayer binary accepts a path to a JSON configuration file as the sole argum
 ./build/awm-relayer --config-file path-to-config
 ```

+### Configuration


Love this new README section 🙌

minghinmatthewlam · 2023-10-23T22:21:47Z

config/config.go

+//
+
+// GetSourceIDs returns the Subnet and Chain IDs of all subnets configured as a source
+func GetSourceIDs() ([]ids.ID, []ids.ID, error) {


why did we move this to using a global config variable vs a receiver function?

After it's constructed, the configuration is a static singleton, so this pattern makes access more straightforward. The alternative is passing the option from the config down the call stack to where it's used, unnecessarily complicating interfaces along the way.

I have a preference, though not a strong preference, to stay away from the global variables and to instead pass in the configs, similar to previously how we had global var for destination clients. This also helps prevent in the future from accidentally ovewriting values in the global config

On second thought, I agree we should pass the config down the callstack rather than have a global variable. I've made this change.

config/config.go

cam-schultz · 2023-10-24T01:44:07Z

mostly LGTM, left some questions. If we have a case of source chains A,B,C, and destiantion chains D,E,F. Now we want to add chain G as an allowed destination for only A, we'd also have to add the allow destiantion field of DEF for chains B and C right? Since the chains B and C would otherwise deliver to chain G automatically.

Yes, that's correct. If no supported destinations are listed, all configured destination subnets are implicitly allowed. Otherwise, to disallow just a single destination, all other destinations must be listed.

config/config.go

michaelkaplan13 · 2023-10-26T19:10:47Z

config/config.go

+				s.SubnetID,
+				blockchainID)
+		}
+		s.supportedDestinations.Add(chainID)


I'd consider moving setting the supportedDestinations value to a initialize method to be more obvious, but no strong preference either way

I think that might be more trouble than it's worth. To do so properly, we'd want to iterate through all supported destinations in the validate method, then do so again to cache them in initialize. Here, we do it in a single pass.

config/config.go

relayer/relayer.go

michaelkaplan13

One small comment and one question, but otherwise LGTM

relayer/relayer_test.go

config/config.go

michaelkaplan13

LGTM

geoff-vball

LGTM!

gwen917 added 2 commits October 13, 2023 12:04

add allowed destination chain id list

9ab0d98

add unit tests

618a185

gwen917 requested review from michaelkaplan13, cam-schultz, minghinmatthewlam, geoff-vball and bernard-avalabs as code owners October 13, 2023 17:20

fix comments

fddb357

cam-schultz reviewed Oct 13, 2023

View reviewed changes

gwen917 added 4 commits October 16, 2023 10:39

fix typo and config validation

8b5eacd

fix PR comments

62d4fac

fix PR comments

4c0d019

fix

4240369

gwen917 linked an issue Oct 16, 2023 that may be closed by this pull request

Configuration support fully specified source to destination mapping #53

Closed

michaelkaplan13 reviewed Oct 17, 2023

View reviewed changes

cam-schultz added 7 commits October 17, 2023 19:57

add global cfg

199b3e7

add source chain ID to log

6f56a22

combine source subnet config validation

724178f

add config readme

5b6a211

rename supported destinations

89ab862

calculare allowed destinations on startup

836bce8

clarify error msg

c22b7bd

cam-schultz requested a review from michaelkaplan13 October 17, 2023 21:45

bernard-avalabs reviewed Oct 19, 2023

View reviewed changes

cam-schultz added 3 commits October 19, 2023 20:18

fix typo

be3f30f

Merge branch 'main' into p2p-config

b3318d6

readme cleanup

5e75b5f

minghinmatthewlam reviewed Oct 23, 2023

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

cam-schultz added 5 commits October 24, 2023 01:47

cleanup readme

6973bdb

validate dst chains in source validation

c80f97b

clarify comment

947d5d7

correct json tag

9ae1307

Merge branch 'main' into p2p-config

5b5decb

cam-schultz requested review from minghinmatthewlam and bernard-avalabs October 25, 2023 14:03

cam-schultz added 5 commits October 25, 2023 18:34

Merge branch 'main' into p2p-config

50dd208

remove default p-chain node

c05368e

denote api methods in readme

6c8a869

remove global config

77fe281

move supported destination check to relayer

cb82c07

michaelkaplan13 reviewed Oct 26, 2023

View reviewed changes

cam-schultz added 3 commits October 26, 2023 14:52

check supported destination unit test

50548ac

lazily initialize source id fields

468fa3f

remove redundant validation step

03b790d

cam-schultz requested a review from michaelkaplan13 October 26, 2023 20:15

Merge branch 'main' into p1p-config

5630533

michaelkaplan13 reviewed Oct 26, 2023

View reviewed changes

relayer/relayer_test.go Outdated Show resolved Hide resolved

config/config.go Outdated Show resolved Hide resolved

cam-schultz added 2 commits October 26, 2023 16:11

use require in test

87634cb

store source ids on initialization

0e5eee6

cam-schultz requested a review from michaelkaplan13 October 26, 2023 21:15

michaelkaplan13 reviewed Oct 27, 2023

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

combine duplicate iterations

8498771

cam-schultz requested a review from michaelkaplan13 October 27, 2023 14:45

michaelkaplan13 approved these changes Oct 27, 2023

View reviewed changes

geoff-vball approved these changes Oct 27, 2023

View reviewed changes

cam-schultz merged commit d79025f into main Oct 27, 2023
7 checks passed

cam-schultz deleted the p2p-config branch October 27, 2023 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration support fully specified source to destination mapping #59

Configuration support fully specified source to destination mapping #59

gwen917 commented Oct 13, 2023

michaelkaplan13 Oct 17, 2023

cam-schultz Oct 17, 2023

cam-schultz Oct 17, 2023

minghinmatthewlam Oct 25, 2023 •

edited

Loading

geoff-vball Oct 25, 2023

cam-schultz Oct 25, 2023

michaelkaplan13 Oct 17, 2023

cam-schultz Oct 17, 2023

cam-schultz Oct 17, 2023

minghinmatthewlam Oct 23, 2023

cam-schultz Oct 25, 2023

bernard-avalabs left a comment

bernard-avalabs Oct 19, 2023

cam-schultz Oct 19, 2023

minghinmatthewlam Oct 23, 2023

minghinmatthewlam Oct 23, 2023

cam-schultz Oct 24, 2023

minghinmatthewlam Oct 25, 2023

cam-schultz Oct 25, 2023

cam-schultz commented Oct 24, 2023

michaelkaplan13 Oct 26, 2023

cam-schultz Oct 26, 2023

michaelkaplan13 left a comment

michaelkaplan13 left a comment

geoff-vball left a comment

Configuration support fully specified source to destination mapping #59

Configuration support fully specified source to destination mapping #59

Conversation

gwen917 commented Oct 13, 2023

Why this should be merged

How this was tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minghinmatthewlam Oct 25, 2023 • edited Loading

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

bernard-avalabs left a 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

cam-schultz commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkaplan13 left a comment

Choose a reason for hiding this comment

michaelkaplan13 left a comment

Choose a reason for hiding this comment

geoff-vball left a comment

Choose a reason for hiding this comment

minghinmatthewlam Oct 25, 2023 •

edited

Loading