Repository: cosmos/relayer
Pull request #980 (2022-09-09 23:29:44)

Fix unnecessary retention of irrelevant messages
This wasn't causing a problem directly, since the messages would be filtered out later, but I noticed that messages were being retained for matching channels on unrelated chains.

Example:
Configure path `stride-cosmoshub` with channel filter `allowlist` `channel-0`
Messages were being retained on `cosmoshub` when the counterparty channel was `channel-0`. This was including chains that are not stride, that happened to also have `channel-0` for their channel to the hub.

Including chain-id in the check resolves this


Patch:
diff --git a/relayer/channel.go b/relayer/channel.go
index 23a0169..0ec4899 100644
--- a/relayer/channel.go
+++ b/relayer/channel.go
@@ -47,11 +47,11 @@ func (c *Chain) CreateOpenChannels(
 
 	srcPathChain := pathChain{
 		provider: c.ChainProvider,
-		pathEnd:  processor.NewPathEnd(pathName, c.PathEnd.ChainID, c.PathEnd.ClientID, "", []processor.ChannelKey{}),
+		pathEnd:  processor.NewPathEnd(pathName, c.PathEnd.ChainID, c.PathEnd.ClientID, "", []processor.ChainChannelKey{}),
 	}
 	dstPathChain := pathChain{
 		provider: dst.ChainProvider,
-		pathEnd:  processor.NewPathEnd(pathName, dst.PathEnd.ChainID, dst.PathEnd.ClientID, "", []processor.ChannelKey{}),
+		pathEnd:  processor.NewPathEnd(pathName, dst.PathEnd.ChainID, dst.PathEnd.ClientID, "", []processor.ChainChannelKey{}),
 	}
 
 	// Timeout is per message. Four channel handshake messages, allowing maxRetries for each.
@@ -120,11 +120,11 @@ func (c *Chain) CloseChannel(
 ) error {
 	srcPathChain := pathChain{
 		provider: c.ChainProvider,
-		pathEnd:  processor.NewPathEnd(pathName, c.PathEnd.ChainID, c.PathEnd.ClientID, "", []processor.ChannelKey{}),
+		pathEnd:  processor.NewPathEnd(pathName, c.PathEnd.ChainID, c.PathEnd.ClientID, "", []processor.ChainChannelKey{}),
 	}
 	dstPathChain := pathChain{
 		provider: dst.ChainProvider,
-		pathEnd:  processor.NewPathEnd(pathName, dst.PathEnd.ChainID, dst.PathEnd.ClientID, "", []processor.ChannelKey{}),
+		pathEnd:  processor.NewPathEnd(pathName, dst.PathEnd.ChainID, dst.PathEnd.ClientID, "", []processor.ChainChannelKey{}),
 	}
 
 	// Timeout is per message. Two close channel handshake messages, allowing maxRetries for each.
diff --git a/relayer/connection.go b/relayer/connection.go
index e188a68..1882341 100644
--- a/relayer/connection.go
+++ b/relayer/connection.go
@@ -28,11 +28,11 @@ func (c *Chain) CreateOpenConnections(
 
 	srcpathChain := pathChain{
 		provider: c.ChainProvider,
-		pathEnd:  processor.NewPathEnd(pathName, c.PathEnd.ChainID, c.PathEnd.ClientID, "", []processor.ChannelKey{}),
+		pathEnd:  processor.NewPathEnd(pathName, c.PathEnd.ChainID, c.PathEnd.ClientID, "", []processor.ChainChannelKey{}),
 	}
 	dstpathChain := pathChain{
 		provider: dst.ChainProvider,
-		pathEnd:  processor.NewPathEnd(pathName, dst.PathEnd.ChainID, dst.PathEnd.ClientID, "", []processor.ChannelKey{}),
+		pathEnd:  processor.NewPathEnd(pathName, dst.PathEnd.ChainID, dst.PathEnd.ClientID, "", []processor.ChainChannelKey{}),
 	}
 
 	// Timeout is per message. Four connection handshake messages, allowing maxRetries for each.
diff --git a/relayer/processor/path_end.go b/relayer/processor/path_end.go
index 73be970..9ba05b5 100644
--- a/relayer/processor/path_end.go
+++ b/relayer/processor/path_end.go
@@ -10,11 +10,17 @@ type PathEnd struct {
 
 	// Can be either "allowlist" or "denylist"
 	Rule       string
-	FilterList []ChannelKey // which channels to allow or deny
+	FilterList []ChainChannelKey // which channels to allow or deny
+}
+
+type ChainChannelKey struct {
+	ChainID             string
+	CounterpartyChainID string
+	ChannelKey          ChannelKey
 }
 
 // NewPathEnd constructs a PathEnd, validating initial parameters.
-func NewPathEnd(pathName string, chainID string, clientID string, rule string, filterList []ChannelKey) PathEnd {
+func NewPathEnd(pathName string, chainID string, clientID string, rule string, filterList []ChainChannelKey) PathEnd {
 	return PathEnd{
 		PathName:   pathName,
 		ChainID:    chainID,
@@ -29,34 +35,34 @@ const (
 	RuleDenyList  = "denylist"
 )
 
-func (pe PathEnd) checkChannelMatch(listChannelID, listPortID string, channelKey ChannelKey) bool {
+func (pe PathEnd) checkChannelMatch(listChainID, listChannelID, listPortID string, channelKey ChainChannelKey) bool {
 	if listChannelID == "" {
 		return false
 	}
-	if listChannelID == channelKey.ChannelID {
+	if listChannelID == channelKey.ChannelKey.ChannelID && listChainID == channelKey.ChainID {
 		if listPortID == "" {
 			return true
 		}
-		if listPortID == channelKey.PortID {
+		if listPortID == channelKey.ChannelKey.PortID {
 			return true
 		}
 	}
-	if listChannelID == channelKey.CounterpartyChannelID {
+	if listChannelID == channelKey.ChannelKey.CounterpartyChannelID && listChainID == channelKey.CounterpartyChainID {
 		if listPortID == "" {
 			return true
 		}
-		if listPortID == channelKey.CounterpartyPortID {
+		if listPortID == channelKey.ChannelKey.CounterpartyPortID {
 			return true
 		}
 	}
 	return false
 }
 
-func (pe PathEnd) shouldRelayChannelSingle(channelKey ChannelKey, listChannel ChannelKey, allowList bool) bool {
-	if pe.checkChannelMatch(listChannel.ChannelID, listChannel.PortID, channelKey) {
+func (pe PathEnd) shouldRelayChannelSingle(channelKey ChainChannelKey, listChannel ChainChannelKey, allowList bool) bool {
+	if pe.checkChannelMatch(listChannel.ChainID, listChannel.ChannelKey.ChannelID, listChannel.ChannelKey.PortID, channelKey) {
 		return allowList
 	}
-	if pe.checkChannelMatch(listChannel.CounterpartyChannelID, listChannel.CounterpartyPortID, channelKey) {
+	if pe.checkChannelMatch(listChannel.CounterpartyChainID, listChannel.ChannelKey.CounterpartyChannelID, listChannel.ChannelKey.CounterpartyPortID, channelKey) {
 		return allowList
 	}
 	return !allowList
@@ -66,7 +72,7 @@ func (pe PathEnd) shouldRelayChannelSingle(channelKey ChannelKey, listChannel Ch
 // if port ID is non-empty on allowlist channel, allow only that specific port
 // if port ID is empty on blocklist channel, block all ports
 // if port ID is non-empty on blocklist channel, block only that specific port
-func (pe PathEnd) ShouldRelayChannel(channelKey ChannelKey) bool {
+func (pe PathEnd) ShouldRelayChannel(channelKey ChainChannelKey) bool {
 	if pe.Rule == RuleAllowList {
 		for _, allowedChannel := range pe.FilterList {
 			if pe.shouldRelayChannelSingle(channelKey, allowedChannel, true) {
diff --git a/relayer/processor/path_end_runtime.go b/relayer/processor/path_end_runtime.go
index 1023dad..0f187cd 100644
--- a/relayer/processor/path_end_runtime.go
+++ b/relayer/processor/path_end_runtime.go
@@ -95,7 +95,7 @@ func (pathEnd *pathEndRuntime) mergeMessageCache(messageCache IBCMessagesCache,
 	channelHandshakeMessages := make(ChannelMessagesCache)
 
 	for ch, pmc := range messageCache.PacketFlow {
-		if pathEnd.info.ShouldRelayChannel(ch) {
+		if pathEnd.info.ShouldRelayChannel(ChainChannelKey{ChainID: pathEnd.info.ChainID, ChannelKey: ch}) {
 			if inSync && pathEnd.metrics != nil {
 				for eventType, pCache := range pmc {
 					pathEnd.metrics.AddPacketsObserved(pathEnd.info.PathName, pathEnd.info.ChainID, ch.ChannelID, ch.PortID, eventType, len(pCache))
diff --git a/relayer/processor/path_processor.go b/relayer/processor/path_processor.go
index 211dc9e..b05c0d2 100644
--- a/relayer/processor/path_processor.go
+++ b/relayer/processor/path_processor.go
@@ -166,9 +166,9 @@ func (pp *PathProcessor) SetChainProviderIfApplicable(chainProvider provider.Cha
 
 func (pp *PathProcessor) IsRelayedChannel(chainID string, channelKey ChannelKey) bool {
 	if pp.pathEnd1.info.ChainID == chainID {
-		return pp.pathEnd1.info.ShouldRelayChannel(channelKey)
+		return pp.pathEnd1.info.ShouldRelayChannel(ChainChannelKey{ChainID: chainID, ChannelKey: channelKey})
 	} else if pp.pathEnd2.info.ChainID == chainID {
-		return pp.pathEnd2.info.ShouldRelayChannel(channelKey)
+		return pp.pathEnd2.info.ShouldRelayChannel(ChainChannelKey{ChainID: chainID, ChannelKey: channelKey})
 	}
 	return false
 }
diff --git a/relayer/strategies.go b/relayer/strategies.go
index f214890..32fd780 100644
--- a/relayer/strategies.go
+++ b/relayer/strategies.go
@@ -44,11 +44,11 @@ func StartRelayer(
 
 	switch processorType {
 	case ProcessorEvents:
-		var filterSrc, filterDst []processor.ChannelKey
+		var filterSrc, filterDst []processor.ChainChannelKey
 
 		for _, ch := range filter.ChannelList {
-			ruleSrc := processor.ChannelKey{ChannelID: ch}
-			ruleDst := processor.ChannelKey{CounterpartyChannelID: ch}
+			ruleSrc := processor.ChainChannelKey{ChainID: src.ChainProvider.ChainId(), ChannelKey: processor.ChannelKey{ChannelID: ch}}
+			ruleDst := processor.ChainChannelKey{CounterpartyChainID: src.ChainProvider.ChainId(), ChannelKey: processor.ChannelKey{CounterpartyChannelID: ch}}
 			filterSrc = append(filterSrc, ruleSrc)
 			filterDst = append(filterDst, ruleDst)
 		}


Test patch:
diff --git a/relayer/processor/path_end_test.go b/relayer/processor/path_end_test.go
index 7f6dac4..b978f7d 100644
--- a/relayer/processor/path_end_test.go
+++ b/relayer/processor/path_end_test.go
@@ -8,6 +8,7 @@ import (
 )
 
 const (
+	testChain0   = "test-chain-0"
 	testChannel0 = "test-channel-0"
 	testPort0    = "test-port-0"
 	testChannel1 = "test-channel-1"
@@ -18,157 +19,227 @@ const (
 func TestAllowAllChannels(t *testing.T) {
 	mockPathEnd := processor.PathEnd{}
 
-	mockAllowedChannel := processor.ChannelKey{
-		ChannelID: testChannel0,
-		PortID:    testPort0,
+	mockAllowedChannel := processor.ChainChannelKey{
+		ChannelKey: processor.ChannelKey{
+			ChannelID: testChannel0,
+			PortID:    testPort0,
+		},
 	}
 	require.True(t, mockPathEnd.ShouldRelayChannel(mockAllowedChannel), "does not allow channel to be relayed, even though allow list and block list are empty")
 
 	// test counterparty
-	mockAllowedChannel2 := processor.ChannelKey{
-		CounterpartyChannelID: testChannel1,
-		CounterpartyPortID:    testPort1,
+	mockAllowedChannel2 := processor.ChainChannelKey{
+		ChannelKey: processor.ChannelKey{
+			CounterpartyChannelID: testChannel1,
+			CounterpartyPortID:    testPort1,
+		},
 	}
 	require.True(t, mockPathEnd.ShouldRelayChannel(mockAllowedChannel2), "does not allow counterparty channel to be relayed, even though allow list and block list are empty")
 }
 
 //
 func TestAllowAllPortsForChannel(t *testing.T) {
-	mockAllowList := []processor.ChannelKey{
-		{ChannelID: testChannel0},
-	}
+	mockAllowList := []processor.ChainChannelKey{{
+		ChainID:    testChain0,
+		ChannelKey: processor.ChannelKey{ChannelID: testChannel0},
+	}}
 	mockPathEnd := processor.PathEnd{
 		Rule:       processor.RuleAllowList,
 		FilterList: mockAllowList,
 	}
 
-	mockAllowedChannel := processor.ChannelKey{
-		ChannelID: testChannel0,
-		PortID:    testPort0,
+	mockAllowedChannel := processor.ChainChannelKey{
+		ChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			ChannelID: testChannel0,
+			PortID:    testPort0,
+		},
 	}
 	require.True(t, mockPathEnd.ShouldRelayChannel(mockAllowedChannel), "does not allow channel to be relayed, even though channelID is in allow list")
 
 	// test counterparty
-	mockAllowedChannel2 := processor.ChannelKey{
-		CounterpartyChannelID: testChannel0,
-		CounterpartyPortID:    testPort0,
+	mockAllowedChannel2 := processor.ChainChannelKey{
+		CounterpartyChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			CounterpartyChannelID: testChannel0,
+			CounterpartyPortID:    testPort0,
+		},
 	}
 	require.True(t, mockPathEnd.ShouldRelayChannel(mockAllowedChannel2), "does not allow counterparty channel to be relayed, even though channelID is in allow list")
 
-	mockBlockedChannel := processor.ChannelKey{
-		ChannelID: testChannel1,
-		PortID:    testPort1,
+	mockBlockedChannel := processor.ChainChannelKey{
+		ChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			ChannelID: testChannel1,
+			PortID:    testPort1,
+		},
 	}
 	require.False(t, mockPathEnd.ShouldRelayChannel(mockBlockedChannel), "allows channel to be relayed, even though channelID is not in allow list")
 
-	mockBlockedChannel2 := processor.ChannelKey{
-		CounterpartyChannelID: testChannel1,
-		CounterpartyPortID:    testPort1,
+	mockBlockedChannel2 := processor.ChainChannelKey{
+		CounterpartyChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			CounterpartyChannelID: testChannel1,
+			CounterpartyPortID:    testPort1,
+		},
 	}
 	require.False(t, mockPathEnd.ShouldRelayChannel(mockBlockedChannel2), "allows channel to be relayed, even though channelID is not in allow list")
 }
 
 func TestAllowSpecificPortForChannel(t *testing.T) {
-	mockAllowList := []processor.ChannelKey{
-		{ChannelID: testChannel0, PortID: testPort0},
+	mockAllowList := []processor.ChainChannelKey{
+		{
+			ChainID: testChain0,
+			ChannelKey: processor.ChannelKey{
+				ChannelID: testChannel0,
+				PortID:    testPort0,
+			},
+		},
 	}
 	mockPathEnd := processor.PathEnd{
 		Rule:       processor.RuleAllowList,
 		FilterList: mockAllowList,
 	}
 
-	mockAllowedChannel := processor.ChannelKey{
-		ChannelID: testChannel0,
-		PortID:    testPort0,
+	mockAllowedChannel := processor.ChainChannelKey{
+		ChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			ChannelID: testChannel0,
+			PortID:    testPort0,
+		},
 	}
 	require.True(t, mockPathEnd.ShouldRelayChannel(mockAllowedChannel), "does not allow channel to be relayed, even though channelID is in allow list")
 
 	// test counterparty
-	mockAllowedChannel2 := processor.ChannelKey{
-		CounterpartyChannelID: testChannel0,
-		CounterpartyPortID:    testPort0,
+	mockAllowedChannel2 := processor.ChainChannelKey{
+		CounterpartyChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			CounterpartyChannelID: testChannel0,
+			CounterpartyPortID:    testPort0,
+		},
 	}
 	require.True(t, mockPathEnd.ShouldRelayChannel(mockAllowedChannel2), "does not allow counterparty channel to be relayed, even though channelID is in allow list")
 
-	mockBlockedChannel := processor.ChannelKey{
-		ChannelID: testChannel0,
-		PortID:    testPort1,
+	mockBlockedChannel := processor.ChainChannelKey{
+		ChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			ChannelID: testChannel0,
+			PortID:    testPort1,
+		},
 	}
 	require.False(t, mockPathEnd.ShouldRelayChannel(mockBlockedChannel), "allows channel to be relayed, even though portID is not in allow list")
 
-	mockBlockedChannel2 := processor.ChannelKey{
-		CounterpartyChannelID: testChannel0,
-		CounterpartyPortID:    testPort1,
+	mockBlockedChannel2 := processor.ChainChannelKey{
+		CounterpartyChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			CounterpartyChannelID: testChannel0,
+			CounterpartyPortID:    testPort1,
+		},
 	}
 	require.False(t, mockPathEnd.ShouldRelayChannel(mockBlockedChannel2), "allows channel to be relayed, even though portID is not in allow list")
 }
 
 func TestBlockAllPortsForChannel(t *testing.T) {
-	mockBlockList := []processor.ChannelKey{
-		{ChannelID: testChannel0},
+	mockBlockList := []processor.ChainChannelKey{
+		{
+			ChainID: testChain0,
+			ChannelKey: processor.ChannelKey{
+				ChannelID: testChannel0,
+			},
+		},
 	}
 	mockPathEnd := processor.PathEnd{
 		Rule:       processor.RuleDenyList,
 		FilterList: mockBlockList,
 	}
 
-	mockBlockedChannel := processor.ChannelKey{
-		ChannelID: testChannel0,
-		PortID:    testPort0,
+	mockBlockedChannel := processor.ChainChannelKey{
+		ChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			ChannelID: testChannel0,
+			PortID:    testPort0,
+		},
 	}
 	require.False(t, mockPathEnd.ShouldRelayChannel(mockBlockedChannel), "allows channel to be relayed, even though channelID is in block list")
 
 	// test counterparty
-	mockBlockedChannel2 := processor.ChannelKey{
-		CounterpartyChannelID: testChannel0,
-		CounterpartyPortID:    testPort0,
+	mockBlockedChannel2 := processor.ChainChannelKey{
+		CounterpartyChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			CounterpartyChannelID: testChannel0,
+			CounterpartyPortID:    testPort0,
+		},
 	}
 	require.False(t, mockPathEnd.ShouldRelayChannel(mockBlockedChannel2), "allows counterparty channel to be relayed, even though channelID is in block list")
 
-	mockAllowedChannel := processor.ChannelKey{
-		ChannelID: testChannel1,
-		PortID:    testPort1,
+	mockAllowedChannel := processor.ChainChannelKey{
+		ChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			ChannelID: testChannel1,
+			PortID:    testPort1,
+		},
 	}
 	require.True(t, mockPathEnd.ShouldRelayChannel(mockAllowedChannel), "does not allow channel to be relayed, even though channelID is not in block list")
 
-	mockAllowedChannel2 := processor.ChannelKey{
-		CounterpartyChannelID: testChannel1,
-		CounterpartyPortID:    testPort1,
+	mockAllowedChannel2 := processor.ChainChannelKey{
+		CounterpartyChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			CounterpartyChannelID: testChannel1,
+			CounterpartyPortID:    testPort1,
+		},
 	}
 	require.True(t, mockPathEnd.ShouldRelayChannel(mockAllowedChannel2), "does not allow counterparty channel to be relayed, even though channelID is not in block list")
 }
 
 func TestBlockSpecificPortForChannel(t *testing.T) {
-	mockBlockList := []processor.ChannelKey{
-		{ChannelID: testChannel0, PortID: testPort0},
+	mockBlockList := []processor.ChainChannelKey{
+		{
+			ChainID: testChain0,
+			ChannelKey: processor.ChannelKey{
+				ChannelID: testChannel0,
+				PortID:    testPort0,
+			},
+		},
 	}
 	mockPathEnd := processor.PathEnd{
 		Rule:       processor.RuleDenyList,
 		FilterList: mockBlockList,
 	}
 
-	mockBlockedChannel := processor.ChannelKey{
-		ChannelID: testChannel0,
-		PortID:    testPort0,
+	mockBlockedChannel := processor.ChainChannelKey{
+		ChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			ChannelID: testChannel0,
+			PortID:    testPort0,
+		},
 	}
 	require.False(t, mockPathEnd.ShouldRelayChannel(mockBlockedChannel), "allows channel to be relayed, even though channelID/portID is in block list")
 
 	// test counterparty
-	mockBlockedChannel2 := processor.ChannelKey{
-		CounterpartyChannelID: testChannel0,
-		CounterpartyPortID:    testPort0,
+	mockBlockedChannel2 := processor.ChainChannelKey{
+		CounterpartyChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			CounterpartyChannelID: testChannel0,
+			CounterpartyPortID:    testPort0,
+		},
 	}
 	require.False(t, mockPathEnd.ShouldRelayChannel(mockBlockedChannel2), "allows counterparty channel to be relayed, even though channelID/portID is in block list")
 
-	mockAllowedChannel := processor.ChannelKey{
-		ChannelID: testChannel0,
-		PortID:    testPort1,
+	mockAllowedChannel := processor.ChainChannelKey{
+		ChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			ChannelID: testChannel0,
+			PortID:    testPort1,
+		},
 	}
 	require.True(t, mockPathEnd.ShouldRelayChannel(mockAllowedChannel), "does not allow channel to be relayed, even though portID is not in block list")
 
-	mockAllowedChannel2 := processor.ChannelKey{
-		CounterpartyChannelID: testChannel0,
-		CounterpartyPortID:    testPort1,
+	mockAllowedChannel2 := processor.ChainChannelKey{
+		CounterpartyChainID: testChain0,
+		ChannelKey: processor.ChannelKey{
+			CounterpartyChannelID: testChannel0,
+			CounterpartyPortID:    testPort1,
+		},
 	}
 	require.True(t, mockPathEnd.ShouldRelayChannel(mockAllowedChannel2), "does not allow counterparty channel to be relayed, even though portID is not in block list")
 }