From 811889c8fcd059caab59cf15edaaa4d7f9642e3f Mon Sep 17 00:00:00 2001 From: Andrew Gouin Date: Mon, 12 Sep 2022 15:35:38 -0600 Subject: [PATCH] Fix unnecessary retention of irrelevant messages (#980) --- relayer/channel.go | 8 +- relayer/connection.go | 4 +- relayer/processor/path_end.go | 28 ++-- relayer/processor/path_end_runtime.go | 2 +- relayer/processor/path_end_test.go | 197 ++++++++++++++++++-------- relayer/processor/path_processor.go | 4 +- relayer/strategies.go | 6 +- 7 files changed, 163 insertions(+), 86 deletions(-) diff --git a/relayer/channel.go b/relayer/channel.go index 23a016922..0ec489974 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 e188a683d..1882341de 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 73be970ea..9ba05b542 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 1023dad2a..0f187cdaa 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_end_test.go b/relayer/processor/path_end_test.go index 7f6dac4c3..b978f7dd5 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") } diff --git a/relayer/processor/path_processor.go b/relayer/processor/path_processor.go index 211dc9e02..b05c0d231 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 f21489016..32fd7803f 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) }