From 7610f804acb26d911242e4574952700c131b054c Mon Sep 17 00:00:00 2001 From: dimkouv Date: Tue, 7 Jan 2025 14:59:13 +0200 Subject: [PATCH 1/7] P2 - delete unused RMNRemote reader --- .mockery.yaml | 1 - internal/reader/rmn_remote.go | 47 ----- mocks/internal_/reader/rmn_remote.go | 266 --------------------------- 3 files changed, 314 deletions(-) delete mode 100644 internal/reader/rmn_remote.go delete mode 100644 mocks/internal_/reader/rmn_remote.go diff --git a/.mockery.yaml b/.mockery.yaml index fb4b9c9fa..c2cfe79d8 100644 --- a/.mockery.yaml +++ b/.mockery.yaml @@ -8,7 +8,6 @@ packages: github.com/smartcontractkit/chainlink-ccip/internal/reader: interfaces: HomeChain: - RMNRemote: CCIP: github.com/smartcontractkit/chainlink-ccip/internal/plugincommon: interfaces: diff --git a/internal/reader/rmn_remote.go b/internal/reader/rmn_remote.go deleted file mode 100644 index 78bd20d4a..000000000 --- a/internal/reader/rmn_remote.go +++ /dev/null @@ -1,47 +0,0 @@ -package reader - -import ( - rmntypes "github.com/smartcontractkit/chainlink-ccip/commit/merkleroot/rmn/types" - - cciptypes "github.com/smartcontractkit/chainlink-ccip/pkg/types/ccipocr3" -) - -type RMNRemote interface { - GetFSign() uint64 - GetSignersInfo() []rmntypes.RemoteSignerInfo - GetRmnReportVersion() string - GetRmnRemoteContractAddress() string - GetRmnHomeConfigDigest() cciptypes.Bytes32 -} - -type RmnRemotePoller struct { - rmnRemoteConfig rmntypes.RemoteConfig -} - -func NewRMNRemotePoller() RMNRemote { - return &RmnRemotePoller{ - rmnRemoteConfig: rmntypes.RemoteConfig{}, - } -} - -func (r *RmnRemotePoller) GetFSign() uint64 { - panic("implement me") -} - -func (r *RmnRemotePoller) GetSignersInfo() []rmntypes.RemoteSignerInfo { - panic("implement me") -} - -func (r *RmnRemotePoller) GetRmnReportVersion() string { - panic("implement me") -} - -func (r *RmnRemotePoller) GetRmnRemoteContractAddress() string { - panic("implement me") -} - -func (r *RmnRemotePoller) GetRmnHomeConfigDigest() cciptypes.Bytes32 { - panic("implement me") -} - -var _ RMNRemote = (*RmnRemotePoller)(nil) diff --git a/mocks/internal_/reader/rmn_remote.go b/mocks/internal_/reader/rmn_remote.go deleted file mode 100644 index 7399f4248..000000000 --- a/mocks/internal_/reader/rmn_remote.go +++ /dev/null @@ -1,266 +0,0 @@ -// Code generated by mockery v2.43.2. DO NOT EDIT. - -package reader - -import ( - ccipocr3 "github.com/smartcontractkit/chainlink-ccip/pkg/types/ccipocr3" - mock "github.com/stretchr/testify/mock" - - types "github.com/smartcontractkit/chainlink-ccip/commit/merkleroot/rmn/types" -) - -// MockRMNRemote is an autogenerated mock type for the RMNRemote type -type MockRMNRemote struct { - mock.Mock -} - -type MockRMNRemote_Expecter struct { - mock *mock.Mock -} - -func (_m *MockRMNRemote) EXPECT() *MockRMNRemote_Expecter { - return &MockRMNRemote_Expecter{mock: &_m.Mock} -} - -// GetFSign provides a mock function with given fields: -func (_m *MockRMNRemote) GetFSign() uint64 { - ret := _m.Called() - - if len(ret) == 0 { - panic("no return value specified for GetFSign") - } - - var r0 uint64 - if rf, ok := ret.Get(0).(func() uint64); ok { - r0 = rf() - } else { - r0 = ret.Get(0).(uint64) - } - - return r0 -} - -// MockRMNRemote_GetFSign_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetFSign' -type MockRMNRemote_GetFSign_Call struct { - *mock.Call -} - -// GetFSign is a helper method to define mock.On call -func (_e *MockRMNRemote_Expecter) GetFSign() *MockRMNRemote_GetFSign_Call { - return &MockRMNRemote_GetFSign_Call{Call: _e.mock.On("GetFSign")} -} - -func (_c *MockRMNRemote_GetFSign_Call) Run(run func()) *MockRMNRemote_GetFSign_Call { - _c.Call.Run(func(args mock.Arguments) { - run() - }) - return _c -} - -func (_c *MockRMNRemote_GetFSign_Call) Return(_a0 uint64) *MockRMNRemote_GetFSign_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *MockRMNRemote_GetFSign_Call) RunAndReturn(run func() uint64) *MockRMNRemote_GetFSign_Call { - _c.Call.Return(run) - return _c -} - -// GetRmnHomeConfigDigest provides a mock function with given fields: -func (_m *MockRMNRemote) GetRmnHomeConfigDigest() ccipocr3.Bytes32 { - ret := _m.Called() - - if len(ret) == 0 { - panic("no return value specified for GetRmnHomeConfigDigest") - } - - var r0 ccipocr3.Bytes32 - if rf, ok := ret.Get(0).(func() ccipocr3.Bytes32); ok { - r0 = rf() - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(ccipocr3.Bytes32) - } - } - - return r0 -} - -// MockRMNRemote_GetRmnHomeConfigDigest_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetRmnHomeConfigDigest' -type MockRMNRemote_GetRmnHomeConfigDigest_Call struct { - *mock.Call -} - -// GetRmnHomeConfigDigest is a helper method to define mock.On call -func (_e *MockRMNRemote_Expecter) GetRmnHomeConfigDigest() *MockRMNRemote_GetRmnHomeConfigDigest_Call { - return &MockRMNRemote_GetRmnHomeConfigDigest_Call{Call: _e.mock.On("GetRmnHomeConfigDigest")} -} - -func (_c *MockRMNRemote_GetRmnHomeConfigDigest_Call) Run(run func()) *MockRMNRemote_GetRmnHomeConfigDigest_Call { - _c.Call.Run(func(args mock.Arguments) { - run() - }) - return _c -} - -func (_c *MockRMNRemote_GetRmnHomeConfigDigest_Call) Return(_a0 ccipocr3.Bytes32) *MockRMNRemote_GetRmnHomeConfigDigest_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *MockRMNRemote_GetRmnHomeConfigDigest_Call) RunAndReturn(run func() ccipocr3.Bytes32) *MockRMNRemote_GetRmnHomeConfigDigest_Call { - _c.Call.Return(run) - return _c -} - -// GetRmnRemoteContractAddress provides a mock function with given fields: -func (_m *MockRMNRemote) GetRmnRemoteContractAddress() string { - ret := _m.Called() - - if len(ret) == 0 { - panic("no return value specified for GetRmnRemoteContractAddress") - } - - var r0 string - if rf, ok := ret.Get(0).(func() string); ok { - r0 = rf() - } else { - r0 = ret.Get(0).(string) - } - - return r0 -} - -// MockRMNRemote_GetRmnRemoteContractAddress_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetRmnRemoteContractAddress' -type MockRMNRemote_GetRmnRemoteContractAddress_Call struct { - *mock.Call -} - -// GetRmnRemoteContractAddress is a helper method to define mock.On call -func (_e *MockRMNRemote_Expecter) GetRmnRemoteContractAddress() *MockRMNRemote_GetRmnRemoteContractAddress_Call { - return &MockRMNRemote_GetRmnRemoteContractAddress_Call{Call: _e.mock.On("GetRmnRemoteContractAddress")} -} - -func (_c *MockRMNRemote_GetRmnRemoteContractAddress_Call) Run(run func()) *MockRMNRemote_GetRmnRemoteContractAddress_Call { - _c.Call.Run(func(args mock.Arguments) { - run() - }) - return _c -} - -func (_c *MockRMNRemote_GetRmnRemoteContractAddress_Call) Return(_a0 string) *MockRMNRemote_GetRmnRemoteContractAddress_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *MockRMNRemote_GetRmnRemoteContractAddress_Call) RunAndReturn(run func() string) *MockRMNRemote_GetRmnRemoteContractAddress_Call { - _c.Call.Return(run) - return _c -} - -// GetRmnReportVersion provides a mock function with given fields: -func (_m *MockRMNRemote) GetRmnReportVersion() string { - ret := _m.Called() - - if len(ret) == 0 { - panic("no return value specified for GetRmnReportVersion") - } - - var r0 string - if rf, ok := ret.Get(0).(func() string); ok { - r0 = rf() - } else { - r0 = ret.Get(0).(string) - } - - return r0 -} - -// MockRMNRemote_GetRmnReportVersion_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetRmnReportVersion' -type MockRMNRemote_GetRmnReportVersion_Call struct { - *mock.Call -} - -// GetRmnReportVersion is a helper method to define mock.On call -func (_e *MockRMNRemote_Expecter) GetRmnReportVersion() *MockRMNRemote_GetRmnReportVersion_Call { - return &MockRMNRemote_GetRmnReportVersion_Call{Call: _e.mock.On("GetRmnReportVersion")} -} - -func (_c *MockRMNRemote_GetRmnReportVersion_Call) Run(run func()) *MockRMNRemote_GetRmnReportVersion_Call { - _c.Call.Run(func(args mock.Arguments) { - run() - }) - return _c -} - -func (_c *MockRMNRemote_GetRmnReportVersion_Call) Return(_a0 string) *MockRMNRemote_GetRmnReportVersion_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *MockRMNRemote_GetRmnReportVersion_Call) RunAndReturn(run func() string) *MockRMNRemote_GetRmnReportVersion_Call { - _c.Call.Return(run) - return _c -} - -// GetSignersInfo provides a mock function with given fields: -func (_m *MockRMNRemote) GetSignersInfo() []types.RemoteSignerInfo { - ret := _m.Called() - - if len(ret) == 0 { - panic("no return value specified for GetSignersInfo") - } - - var r0 []types.RemoteSignerInfo - if rf, ok := ret.Get(0).(func() []types.RemoteSignerInfo); ok { - r0 = rf() - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).([]types.RemoteSignerInfo) - } - } - - return r0 -} - -// MockRMNRemote_GetSignersInfo_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetSignersInfo' -type MockRMNRemote_GetSignersInfo_Call struct { - *mock.Call -} - -// GetSignersInfo is a helper method to define mock.On call -func (_e *MockRMNRemote_Expecter) GetSignersInfo() *MockRMNRemote_GetSignersInfo_Call { - return &MockRMNRemote_GetSignersInfo_Call{Call: _e.mock.On("GetSignersInfo")} -} - -func (_c *MockRMNRemote_GetSignersInfo_Call) Run(run func()) *MockRMNRemote_GetSignersInfo_Call { - _c.Call.Run(func(args mock.Arguments) { - run() - }) - return _c -} - -func (_c *MockRMNRemote_GetSignersInfo_Call) Return(_a0 []types.RemoteSignerInfo) *MockRMNRemote_GetSignersInfo_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *MockRMNRemote_GetSignersInfo_Call) RunAndReturn(run func() []types.RemoteSignerInfo) *MockRMNRemote_GetSignersInfo_Call { - _c.Call.Return(run) - return _c -} - -// NewMockRMNRemote creates a new instance of MockRMNRemote. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. -// The first argument is typically a *testing.T value. -func NewMockRMNRemote(t interface { - mock.TestingT - Cleanup(func()) -}) *MockRMNRemote { - mock := &MockRMNRemote{} - mock.Mock.Test(t) - - t.Cleanup(func() { mock.AssertExpectations(t) }) - - return mock -} From 28e3f177075f093edf1c87be49774a910d073059 Mon Sep 17 00:00:00 2001 From: dimkouv Date: Tue, 7 Jan 2025 15:14:51 +0200 Subject: [PATCH 2/7] H3 - rmn query logic, issue on one chain affects all chains --- commit/merkleroot/query.go | 3 +- commit/merkleroot/query_test.go | 112 ++++++++++++++++++++++++++++---- 2 files changed, 101 insertions(+), 14 deletions(-) diff --git a/commit/merkleroot/query.go b/commit/merkleroot/query.go index 2d6f43fd0..cfe61ebd1 100644 --- a/commit/merkleroot/query.go +++ b/commit/merkleroot/query.go @@ -41,7 +41,8 @@ func (p *Processor) Query(ctx context.Context, prevOutcome Outcome) (Query, erro for _, sourceChainRange := range prevOutcome.RangesSelectedForReport { onRampAddress, err := p.ccipReader.GetContractAddress(consts.ContractNameOnRamp, sourceChainRange.ChainSel) if err != nil { - return Query{}, fmt.Errorf("get onRamp address for chain %v: %w", sourceChainRange.ChainSel, err) + p.lggr.Errorf("skipping chain %d updates, onRamp address error: %s", sourceChainRange.ChainSel, err) + continue } reqUpdates = append(reqUpdates, &rmnpb.FixedDestLaneUpdateRequest{ diff --git a/commit/merkleroot/query_test.go b/commit/merkleroot/query_test.go index 8683b5428..720e713d9 100644 --- a/commit/merkleroot/query_test.go +++ b/commit/merkleroot/query_test.go @@ -34,6 +34,12 @@ func TestProcessor_Query(t *testing.T) { dstChain: {consts.ContractNameOffRamp: []byte("0x1234567890123456789012345678901234567892")}, } + contractAddrNoErrs := map[ccipocr3.ChainSelector]map[string]error{ + srcChain1: {consts.ContractNameOnRamp: nil}, + srcChain2: {consts.ContractNameOnRamp: nil}, + dstChain: {consts.ContractNameOffRamp: nil}, + } + expSigs1 := &rmn.ReportSignatures{ Signatures: []*rmnpb.EcdsaSignature{ {R: []byte("r1"), S: []byte("s1")}, @@ -58,14 +64,15 @@ func TestProcessor_Query(t *testing.T) { rmnRemoteCfg := testhelpers.CreateRMNRemoteCfg() testCases := []struct { - name string - prevOutcome Outcome - contractAddresses map[ccipocr3.ChainSelector]map[string][]byte - cfg pluginconfig.CommitOffchainConfig - destChain ccipocr3.ChainSelector - rmnClient func(t *testing.T) *rmnmocks.MockController - expQuery Query - expErr bool + name string + prevOutcome Outcome + contractAddresses map[ccipocr3.ChainSelector]map[string][]byte + contractAddrErrors map[ccipocr3.ChainSelector]map[string]error + cfg pluginconfig.CommitOffchainConfig + destChain ccipocr3.ChainSelector + rmnClient func(t *testing.T) *rmnmocks.MockController + expQuery Query + expErr bool }{ { name: "happy path", @@ -77,7 +84,8 @@ func TestProcessor_Query(t *testing.T) { }, RMNRemoteCfg: rmnRemoteCfg, }, - contractAddresses: contractAddrs, + contractAddresses: contractAddrs, + contractAddrErrors: contractAddrNoErrs, cfg: pluginconfig.CommitOffchainConfig{ RMNEnabled: true, RMNSignaturesTimeout: 5 * time.Second, @@ -120,7 +128,7 @@ func TestProcessor_Query(t *testing.T) { expErr: false, }, { - name: "rmn timeout", + name: "onRamp address resolution on one chain should not affect other chains", prevOutcome: Outcome{ OutcomeType: ReportIntervalsSelected, RangesSelectedForReport: []plugintypes.ChainRange{ @@ -130,6 +138,81 @@ func TestProcessor_Query(t *testing.T) { RMNRemoteCfg: rmnRemoteCfg, }, contractAddresses: contractAddrs, + contractAddrErrors: map[ccipocr3.ChainSelector]map[string]error{ + srcChain1: {consts.ContractNameOnRamp: nil}, + srcChain2: {consts.ContractNameOnRamp: fmt.Errorf("some error")}, + dstChain: {consts.ContractNameOffRamp: nil}, + }, + cfg: pluginconfig.CommitOffchainConfig{ + RMNEnabled: true, + RMNSignaturesTimeout: 5 * time.Second, + }, + destChain: dstChain, + rmnClient: func(t *testing.T) *rmnmocks.MockController { + cl := rmnmocks.NewMockController(t) + cl.EXPECT(). + ComputeReportSignatures( + mock.Anything, + &rmnpb.LaneDest{ + DestChainSelector: uint64(dstChain), + OfframpAddress: contractAddrs[dstChain][consts.ContractNameOffRamp], + }, + []*rmnpb.FixedDestLaneUpdateRequest{ + { + LaneSource: &rmnpb.LaneSource{ + SourceChainSelector: uint64(srcChain1), + OnrampAddress: contractAddrs[srcChain1][consts.ContractNameOnRamp], + }, + ClosedInterval: &rmnpb.ClosedInterval{MinMsgNr: 10, MaxMsgNr: 20}, + }, + }, + rmnRemoteCfg, + ). + Return(&rmn.ReportSignatures{ + Signatures: []*rmnpb.EcdsaSignature{ + {R: []byte("r1"), S: []byte("s1")}, + }, + LaneUpdates: []*rmnpb.FixedDestLaneUpdate{ + { + LaneSource: &rmnpb.LaneSource{ + SourceChainSelector: uint64(srcChain1), + OnrampAddress: contractAddrs[srcChain1][consts.ContractNameOnRamp], + }, + }, + }, + }, nil) + return cl + }, + expQuery: Query{ + RetryRMNSignatures: false, + RMNSignatures: &rmn.ReportSignatures{ + Signatures: []*rmnpb.EcdsaSignature{ + {R: []byte("r1"), S: []byte("s1")}, + }, + LaneUpdates: []*rmnpb.FixedDestLaneUpdate{ + { + LaneSource: &rmnpb.LaneSource{ + SourceChainSelector: uint64(srcChain1), + OnrampAddress: contractAddrs[srcChain1][consts.ContractNameOnRamp], + }, + }, + }, + }, + }, + expErr: false, + }, + { + name: "rmn timeout", + prevOutcome: Outcome{ + OutcomeType: ReportIntervalsSelected, + RangesSelectedForReport: []plugintypes.ChainRange{ + {ChainSel: srcChain1, SeqNumRange: ccipocr3.NewSeqNumRange(10, 20)}, + {ChainSel: srcChain2, SeqNumRange: ccipocr3.NewSeqNumRange(50, 51)}, + }, + RMNRemoteCfg: rmnRemoteCfg, + }, + contractAddresses: contractAddrs, + contractAddrErrors: contractAddrNoErrs, cfg: pluginconfig.CommitOffchainConfig{ RMNEnabled: true, RMNSignaturesTimeout: time.Second, @@ -158,7 +241,8 @@ func TestProcessor_Query(t *testing.T) { }, RMNRemoteCfg: rmnRemoteCfg, }, - contractAddresses: contractAddrs, + contractAddresses: contractAddrs, + contractAddrErrors: contractAddrNoErrs, cfg: pluginconfig.CommitOffchainConfig{ RMNEnabled: true, RMNSignaturesTimeout: time.Second, @@ -212,7 +296,8 @@ func TestProcessor_Query(t *testing.T) { {ChainSel: srcChain2, SeqNumRange: ccipocr3.NewSeqNumRange(50, 51)}, }, }, - contractAddresses: contractAddrs, + contractAddresses: contractAddrs, + contractAddrErrors: contractAddrNoErrs, cfg: pluginconfig.CommitOffchainConfig{ RMNEnabled: true, RMNSignaturesTimeout: time.Second, @@ -230,7 +315,8 @@ func TestProcessor_Query(t *testing.T) { if !tc.prevOutcome.RMNRemoteCfg.IsEmpty() { for chainSel, contracts := range tc.contractAddresses { for name, addr := range contracts { - ccipReader.EXPECT().GetContractAddress(name, chainSel).Return(addr, nil) + ccipReader.EXPECT().GetContractAddress(name, chainSel). + Return(addr, tc.contractAddrErrors[chainSel][name]) } } } From 1f783da11fbd0e42902af3627db724045c060dfc Mon Sep 17 00:00:00 2001 From: dimkouv Date: Tue, 7 Jan 2025 16:03:19 +0200 Subject: [PATCH 3/7] H3 - disabled chain issue, affects all chains processing --- commit/merkleroot/observation.go | 13 ++++------- commit/merkleroot/observation_test.go | 18 +++++++++++---- commit/merkleroot/validate_observation.go | 14 ++++------- .../merkleroot/validate_observation_test.go | 17 +++++++------- commit/plugin_e2e_test.go | 23 +++++++++++-------- internal/mocks/inmem/ccipreader_inmem.go | 2 +- mocks/pkg/reader/ccip_reader.go | 14 +++++------ pkg/reader/ccip.go | 16 ++++++++----- pkg/reader/ccip_interface.go | 3 ++- 9 files changed, 64 insertions(+), 56 deletions(-) diff --git a/commit/merkleroot/observation.go b/commit/merkleroot/observation.go index 63eb18abc..aae52186a 100644 --- a/commit/merkleroot/observation.go +++ b/commit/merkleroot/observation.go @@ -380,17 +380,12 @@ func (o observerImpl) ObserveOffRampNextSeqNums(ctx context.Context) []plugintyp return nil } - if len(offRampNextSeqNums) != len(sourceChains) { - o.lggr.Errorf("call to NextSeqNum returned unexpected number of seq nums, got %d, expected %d", - len(offRampNextSeqNums), len(sourceChains)) - return nil - } - - result := make([]plugintypes.SeqNumChain, len(sourceChains)) - for i := range sourceChains { - result[i] = plugintypes.SeqNumChain{ChainSel: sourceChains[i], SeqNum: offRampNextSeqNums[i]} + result := make([]plugintypes.SeqNumChain, 0, len(sourceChains)) + for chainSelector, seqNum := range offRampNextSeqNums { + result = append(result, plugintypes.NewSeqNumChain(chainSelector, seqNum)) } + sort.Slice(result, func(i, j int) bool { return result[i].ChainSel < result[j].ChainSel }) return result } diff --git a/commit/merkleroot/observation_test.go b/commit/merkleroot/observation_test.go index 5544abd96..b96be1633 100644 --- a/commit/merkleroot/observation_test.go +++ b/commit/merkleroot/observation_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "golang.org/x/exp/maps" "github.com/smartcontractkit/libocr/commontypes" "github.com/smartcontractkit/libocr/ragep2p/types" @@ -176,7 +177,7 @@ func TestObservation(t *testing.T) { func Test_ObserveOffRampNextSeqNums(t *testing.T) { const nodeID commontypes.OracleID = 1 knownSourceChains := []cciptypes.ChainSelector{4, 7, 19} - nextSeqNums := []cciptypes.SeqNum{345, 608, 7713} + nextSeqNums := map[cciptypes.ChainSelector]cciptypes.SeqNum{4: 345, 7: 608, 19: 7713} testCases := []struct { name string @@ -233,7 +234,7 @@ func Test_ObserveOffRampNextSeqNums(t *testing.T) { expResult: nil, }, { - name: "nil is returned when nextSeqNums returns incorrect number of seq nums", + name: "nextSeqNums returns incorrect number of seq nums, other chains should be processed correctly", getDeps: func(t *testing.T) (*common_mock.MockChainSupport, *reader_mock.MockCCIPReader) { chainSupport := common_mock.NewMockChainSupport(t) chainSupport.EXPECT().SupportsDestChain(nodeID).Return(true, nil) @@ -241,12 +242,19 @@ func Test_ObserveOffRampNextSeqNums(t *testing.T) { chainSupport.EXPECT().KnownSourceChainsSlice().Return(knownSourceChains, nil) ccipReader := reader_mock.NewMockCCIPReader(t) // return a smaller slice, should trigger validation condition - ccipReader.EXPECT().NextSeqNum(mock.Anything, knownSourceChains).Return(nextSeqNums[1:], nil) + + nextSeqNumsCp := maps.Clone(nextSeqNums) + delete(nextSeqNumsCp, cciptypes.ChainSelector(4)) + + ccipReader.EXPECT().NextSeqNum(mock.Anything, knownSourceChains).Return(nextSeqNumsCp, nil) ccipReader.EXPECT().GetRmnCurseInfo(mock.Anything, mock.Anything, mock.Anything). Return(&reader.CurseInfo{}, nil) return chainSupport, ccipReader }, - expResult: nil, + expResult: []plugintypes.SeqNumChain{ + plugintypes.NewSeqNumChain(7, 608), + plugintypes.NewSeqNumChain(19, 7713), + }, }, { name: "dest chain is cursed sequence numbers not observed", @@ -286,7 +294,7 @@ func Test_ObserveOffRampNextSeqNums(t *testing.T) { knownSourceChains := []cciptypes.ChainSelector{4, 7, 19} cursedSourceChains := map[cciptypes.ChainSelector]bool{7: true, 4: false} knownSourceChainsExcludingCursed := []cciptypes.ChainSelector{4, 19} - nextSeqNumsExcludingCursed := []cciptypes.SeqNum{345, 7713} + nextSeqNumsExcludingCursed := map[cciptypes.ChainSelector]cciptypes.SeqNum{4: 345, 19: 7713} chainSupport := common_mock.NewMockChainSupport(t) chainSupport.EXPECT().SupportsDestChain(nodeID).Return(true, nil) diff --git a/commit/merkleroot/validate_observation.go b/commit/merkleroot/validate_observation.go index 043b63415..1b9053d01 100644 --- a/commit/merkleroot/validate_observation.go +++ b/commit/merkleroot/validate_observation.go @@ -209,16 +209,12 @@ func ValidateMerkleRootsState( return fmt.Errorf("get next sequence numbers: %w", err) } - if len(offRampExpNextSeqNums) != len(chainSlice) { - return fmt.Errorf("critical reader error: seq nums length mismatch") - } - - for i, offRampExpNextSeqNum := range offRampExpNextSeqNums { - chain := chainSlice[i] - - newNextOnRampSeqNum, ok := newNextOnRampSeqNums[chain] + for chain, newNextOnRampSeqNum := range newNextOnRampSeqNums { + offRampExpNextSeqNum, ok := offRampExpNextSeqNums[chain] if !ok { - return fmt.Errorf("critical unexpected error: newOnRampSeqNum not found") + // Due to some chain being disabled while the sequence numbers were already observed, reported should + // not be considered valid in that case. + return fmt.Errorf("offRamp expected next sequence number for chain %d was not found", chain) } if newNextOnRampSeqNum != offRampExpNextSeqNum { diff --git a/commit/merkleroot/validate_observation_test.go b/commit/merkleroot/validate_observation_test.go index df5ba36e2..33f6dd1df 100644 --- a/commit/merkleroot/validate_observation_test.go +++ b/commit/merkleroot/validate_observation_test.go @@ -345,7 +345,7 @@ func Test_validateMerkleRootsState(t *testing.T) { testCases := []struct { name string onRampNextSeqNum []plugintypes.SeqNumChain - offRampExpNextSeqNum []cciptypes.SeqNum + offRampExpNextSeqNum map[cciptypes.ChainSelector]cciptypes.SeqNum readerErr error expErr bool }{ @@ -355,7 +355,7 @@ func Test_validateMerkleRootsState(t *testing.T) { plugintypes.NewSeqNumChain(10, 100), plugintypes.NewSeqNumChain(20, 200), }, - offRampExpNextSeqNum: []cciptypes.SeqNum{100, 200}, + offRampExpNextSeqNum: map[cciptypes.ChainSelector]cciptypes.SeqNum{10: 100, 20: 200}, expErr: false, }, { @@ -364,7 +364,8 @@ func Test_validateMerkleRootsState(t *testing.T) { plugintypes.NewSeqNumChain(10, 100), plugintypes.NewSeqNumChain(20, 200), }, - offRampExpNextSeqNum: []cciptypes.SeqNum{100, 201}, // <- 200 is already on chain + // <- 200 is already on chain + offRampExpNextSeqNum: map[cciptypes.ChainSelector]cciptypes.SeqNum{10: 100, 20: 201}, expErr: true, }, { @@ -373,17 +374,17 @@ func Test_validateMerkleRootsState(t *testing.T) { plugintypes.NewSeqNumChain(10, 101), // <- onchain 99 but we submit 101 instead of 100 plugintypes.NewSeqNumChain(20, 200), }, - offRampExpNextSeqNum: []cciptypes.SeqNum{100, 200}, + offRampExpNextSeqNum: map[cciptypes.ChainSelector]cciptypes.SeqNum{10: 100, 20: 200}, expErr: true, }, { - name: "reader returned wrong number of seq nums", + name: "reader returned wrong number of seq nums, should be ok", onRampNextSeqNum: []plugintypes.SeqNumChain{ plugintypes.NewSeqNumChain(10, 100), plugintypes.NewSeqNumChain(20, 200), }, - offRampExpNextSeqNum: []cciptypes.SeqNum{100, 200, 300}, - expErr: true, + offRampExpNextSeqNum: map[cciptypes.ChainSelector]cciptypes.SeqNum{10: 100, 20: 200, 30: 300}, + expErr: false, }, { name: "reader error", @@ -391,7 +392,7 @@ func Test_validateMerkleRootsState(t *testing.T) { plugintypes.NewSeqNumChain(10, 100), plugintypes.NewSeqNumChain(20, 200), }, - offRampExpNextSeqNum: []cciptypes.SeqNum{100, 200}, + offRampExpNextSeqNum: map[cciptypes.ChainSelector]cciptypes.SeqNum{10: 100, 20: 200}, readerErr: fmt.Errorf("reader error"), expErr: true, }, diff --git a/commit/plugin_e2e_test.go b/commit/plugin_e2e_test.go index eb53b1ce9..6453402d2 100644 --- a/commit/plugin_e2e_test.go +++ b/commit/plugin_e2e_test.go @@ -143,7 +143,7 @@ func TestPlugin_E2E_AllNodesAgree_MerkleRoots(t *testing.T) { expTransmittedReports []ccipocr3.CommitPluginReport offRampNextSeqNumDefaultOverrideKeys []ccipocr3.ChainSelector - offRampNextSeqNumDefaultOverrideValues []ccipocr3.SeqNum + offRampNextSeqNumDefaultOverrideValues map[ccipocr3.ChainSelector]ccipocr3.SeqNum enableDiscovery bool }{ @@ -201,10 +201,13 @@ func TestPlugin_E2E_AllNodesAgree_MerkleRoots(t *testing.T) { }, }, { - name: "report generated in previous outcome, transmitted with success", - prevOutcome: outcomeReportGenerated, - offRampNextSeqNumDefaultOverrideKeys: []ccipocr3.ChainSelector{sourceChain1, sourceChain2}, - offRampNextSeqNumDefaultOverrideValues: []ccipocr3.SeqNum{11, 20}, + name: "report generated in previous outcome, transmitted with success", + prevOutcome: outcomeReportGenerated, + offRampNextSeqNumDefaultOverrideKeys: []ccipocr3.ChainSelector{sourceChain1, sourceChain2}, + offRampNextSeqNumDefaultOverrideValues: map[ccipocr3.ChainSelector]ccipocr3.SeqNum{ + sourceChain1: 11, + sourceChain2: 20, + }, expOutcome: committypes.Outcome{ MerkleRootOutcome: merkleroot.Outcome{ OutcomeType: merkleroot.ReportTransmitted, @@ -690,7 +693,7 @@ func prepareCcipReaderMock( if mockEmptySeqNrs { ccipReader.EXPECT().NextSeqNum(ctx, mock.Anything).Unset() - ccipReader.EXPECT().NextSeqNum(ctx, mock.Anything).Return([]ccipocr3.SeqNum{}, nil). + ccipReader.EXPECT().NextSeqNum(ctx, mock.Anything).Return(map[ccipocr3.ChainSelector]ccipocr3.SeqNum{}, nil). Maybe() } @@ -787,12 +790,12 @@ func setupNode(params SetupNodeParams) nodeSetup { } sort.Slice(sourceChains, func(i, j int) bool { return sourceChains[i] < sourceChains[j] }) - offRampNextSeqNums := make([]ccipocr3.SeqNum, 0) + offRampNextSeqNums := make(map[ccipocr3.ChainSelector]ccipocr3.SeqNum, 0) chainsWithNewMsgs := make([]ccipocr3.ChainSelector, 0) for _, sourceChain := range sourceChains { offRampNextSeqNum, ok := params.offRampNextSeqNum[sourceChain] assert.True(params.t, ok) - offRampNextSeqNums = append(offRampNextSeqNums, offRampNextSeqNum) + offRampNextSeqNums[sourceChain] = offRampNextSeqNum newMsgs := make([]ccipocr3.Message, 0) numNewMsgs := (params.onRampLastSeqNum[sourceChain] - offRampNextSeqNum) + 1 @@ -815,9 +818,9 @@ func setupNode(params SetupNodeParams) nodeSetup { } } - seqNumsOfChainsWithNewMsgs := make([]ccipocr3.SeqNum, 0) + seqNumsOfChainsWithNewMsgs := map[ccipocr3.ChainSelector]ccipocr3.SeqNum{} for _, chainSel := range chainsWithNewMsgs { - seqNumsOfChainsWithNewMsgs = append(seqNumsOfChainsWithNewMsgs, params.offRampNextSeqNum[chainSel]) + seqNumsOfChainsWithNewMsgs[chainSel] = params.offRampNextSeqNum[chainSel] } if len(chainsWithNewMsgs) > 0 { ccipReader.EXPECT().NextSeqNum(params.ctx, chainsWithNewMsgs).Return(seqNumsOfChainsWithNewMsgs, nil).Maybe() diff --git a/internal/mocks/inmem/ccipreader_inmem.go b/internal/mocks/inmem/ccipreader_inmem.go index 7b82f4811..3051518fb 100644 --- a/internal/mocks/inmem/ccipreader_inmem.go +++ b/internal/mocks/inmem/ccipreader_inmem.go @@ -110,7 +110,7 @@ func (r InMemoryCCIPReader) MsgsBetweenSeqNums( func (r InMemoryCCIPReader) NextSeqNum( ctx context.Context, chains []cciptypes.ChainSelector, -) (seqNum []cciptypes.SeqNum, err error) { +) (seqNum map[cciptypes.ChainSelector]cciptypes.SeqNum, err error) { panic("implement me") } diff --git a/mocks/pkg/reader/ccip_reader.go b/mocks/pkg/reader/ccip_reader.go index 0d8ee5a76..460c18a1d 100644 --- a/mocks/pkg/reader/ccip_reader.go +++ b/mocks/pkg/reader/ccip_reader.go @@ -940,23 +940,23 @@ func (_c *MockCCIPReader_MsgsBetweenSeqNums_Call) RunAndReturn(run func(context. } // NextSeqNum provides a mock function with given fields: ctx, chains -func (_m *MockCCIPReader) NextSeqNum(ctx context.Context, chains []ccipocr3.ChainSelector) ([]ccipocr3.SeqNum, error) { +func (_m *MockCCIPReader) NextSeqNum(ctx context.Context, chains []ccipocr3.ChainSelector) (map[ccipocr3.ChainSelector]ccipocr3.SeqNum, error) { ret := _m.Called(ctx, chains) if len(ret) == 0 { panic("no return value specified for NextSeqNum") } - var r0 []ccipocr3.SeqNum + var r0 map[ccipocr3.ChainSelector]ccipocr3.SeqNum var r1 error - if rf, ok := ret.Get(0).(func(context.Context, []ccipocr3.ChainSelector) ([]ccipocr3.SeqNum, error)); ok { + if rf, ok := ret.Get(0).(func(context.Context, []ccipocr3.ChainSelector) (map[ccipocr3.ChainSelector]ccipocr3.SeqNum, error)); ok { return rf(ctx, chains) } - if rf, ok := ret.Get(0).(func(context.Context, []ccipocr3.ChainSelector) []ccipocr3.SeqNum); ok { + if rf, ok := ret.Get(0).(func(context.Context, []ccipocr3.ChainSelector) map[ccipocr3.ChainSelector]ccipocr3.SeqNum); ok { r0 = rf(ctx, chains) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).([]ccipocr3.SeqNum) + r0 = ret.Get(0).(map[ccipocr3.ChainSelector]ccipocr3.SeqNum) } } @@ -988,12 +988,12 @@ func (_c *MockCCIPReader_NextSeqNum_Call) Run(run func(ctx context.Context, chai return _c } -func (_c *MockCCIPReader_NextSeqNum_Call) Return(seqNum []ccipocr3.SeqNum, err error) *MockCCIPReader_NextSeqNum_Call { +func (_c *MockCCIPReader_NextSeqNum_Call) Return(seqNum map[ccipocr3.ChainSelector]ccipocr3.SeqNum, err error) *MockCCIPReader_NextSeqNum_Call { _c.Call.Return(seqNum, err) return _c } -func (_c *MockCCIPReader_NextSeqNum_Call) RunAndReturn(run func(context.Context, []ccipocr3.ChainSelector) ([]ccipocr3.SeqNum, error)) *MockCCIPReader_NextSeqNum_Call { +func (_c *MockCCIPReader_NextSeqNum_Call) RunAndReturn(run func(context.Context, []ccipocr3.ChainSelector) (map[ccipocr3.ChainSelector]ccipocr3.SeqNum, error)) *MockCCIPReader_NextSeqNum_Call { _c.Call.Return(run) return _c } diff --git a/pkg/reader/ccip.go b/pkg/reader/ccip.go index b0993a989..d1ba9f57c 100644 --- a/pkg/reader/ccip.go +++ b/pkg/reader/ccip.go @@ -399,22 +399,26 @@ func (r *ccipChainReader) GetExpectedNextSequenceNumber( func (r *ccipChainReader) NextSeqNum( ctx context.Context, chains []cciptypes.ChainSelector, -) ([]cciptypes.SeqNum, error) { +) (map[cciptypes.ChainSelector]cciptypes.SeqNum, error) { cfgs, err := r.getOffRampSourceChainsConfig(ctx, chains) if err != nil { return nil, fmt.Errorf("get source chains config: %w", err) } - res := make([]cciptypes.SeqNum, 0, len(chains)) + res := make(map[cciptypes.ChainSelector]cciptypes.SeqNum, len(chains)) for _, chain := range chains { cfg, exists := cfgs[chain] if !exists { - return nil, fmt.Errorf("source chain config not found for chain %d", chain) + r.lggr.Debugw("source chain config not found for chain %d, chain is skipped.", chain) + continue } + if cfg.MinSeqNr == 0 { - return nil, fmt.Errorf("minSeqNr not found for chain %d", chain) + r.lggr.Errorw("minSeqNr not found for chain %d or is set to 0, chain is skipped.", chain) + continue } - res = append(res, cciptypes.SeqNum(cfg.MinSeqNr)) + + res[chain] = cciptypes.SeqNum(cfg.MinSeqNr) } return res, err @@ -1067,7 +1071,7 @@ func (scc sourceChainConfig) check() (bool /* enabled */, error) { } // getOffRampSourceChainsConfig returns the offRamp contract's source chain configurations for each supported source -// chain. +// chain. If some chain is disabled it is not included in the response. func (r *ccipChainReader) getOffRampSourceChainsConfig( ctx context.Context, chains []cciptypes.ChainSelector) (map[cciptypes.ChainSelector]sourceChainConfig, error) { if err := validateExtendedReaderExistence(r.contractReaders, r.destChain); err != nil { diff --git a/pkg/reader/ccip_interface.go b/pkg/reader/ccip_interface.go index e652b6e60..46bd376e2 100644 --- a/pkg/reader/ccip_interface.go +++ b/pkg/reader/ccip_interface.go @@ -108,7 +108,8 @@ type CCIPReader interface { // NextSeqNum reads the destination chain. // Returns the next expected sequence number for each one of the provided chains. // TODO: if destination was a parameter, this could be a capability reused across plugin instances. - NextSeqNum(ctx context.Context, chains []cciptypes.ChainSelector) (seqNum []cciptypes.SeqNum, err error) + NextSeqNum(ctx context.Context, chains []cciptypes.ChainSelector) ( + seqNum map[cciptypes.ChainSelector]cciptypes.SeqNum, err error) // GetContractAddress returns the contract address that is registered for the provided contract name and chain. GetContractAddress(contractName string, chain cciptypes.ChainSelector) ([]byte, error) From 5ff50c5843c011a8191cc5d977d4166fdefe60a2 Mon Sep 17 00:00:00 2001 From: dimkouv Date: Tue, 7 Jan 2025 16:08:22 +0200 Subject: [PATCH 4/7] H3 - onRamp rpc call issue, affects all chains processing --- commit/merkleroot/observation.go | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/commit/merkleroot/observation.go b/commit/merkleroot/observation.go index aae52186a..798df7a0a 100644 --- a/commit/merkleroot/observation.go +++ b/commit/merkleroot/observation.go @@ -408,23 +408,30 @@ func (o observerImpl) ObserveLatestOnRampSeqNums( sourceChains := mapset.NewSet(allSourceChains...).Intersect(supportedChains).ToSlice() sort.Slice(sourceChains, func(i, j int) bool { return sourceChains[i] < sourceChains[j] }) - latestOnRampSeqNums := make([]plugintypes.SeqNumChain, len(sourceChains)) + mu := &sync.Mutex{} + latestOnRampSeqNums := make([]plugintypes.SeqNumChain, 0, len(sourceChains)) eg := &errgroup.Group{} - for i, sourceChain := range sourceChains { + for _, sourceChain := range sourceChains { eg.Go(func() error { nextOnRampSeqNum, err := o.ccipReader.GetExpectedNextSequenceNumber(ctx, sourceChain, destChain) if err != nil { - return fmt.Errorf("failed to get expected next sequence number for source chain %d: %w", sourceChain, err) + o.lggr.Errorf("failed to get expected next seq num for source chain %d: %s", sourceChain, err) + return nil } + if nextOnRampSeqNum == 0 { - return fmt.Errorf("expected next sequence number for source chain %d is 0", sourceChain) + o.lggr.Errorf("unexpected next seq num for source chain %d, it is 0", sourceChain) + return nil } - latestOnRampSeqNums[i] = plugintypes.SeqNumChain{ - ChainSel: sourceChain, - SeqNum: nextOnRampSeqNum - 1, // Latest is the next one minus one. - } + mu.Lock() + latestOnRampSeqNums = append( + latestOnRampSeqNums, + plugintypes.NewSeqNumChain(sourceChain, nextOnRampSeqNum-1), // Latest is the next one minus one. + ) + mu.Unlock() + return nil }) } @@ -434,6 +441,9 @@ func (o observerImpl) ObserveLatestOnRampSeqNums( return nil } + sort.Slice(latestOnRampSeqNums, func(i, j int) bool { + return latestOnRampSeqNums[i].ChainSel < latestOnRampSeqNums[j].ChainSel + }) return latestOnRampSeqNums } From bf54d10be36ea50a8d994e39c27dabec349c2e92 Mon Sep 17 00:00:00 2001 From: dimkouv Date: Wed, 8 Jan 2025 08:36:39 +0200 Subject: [PATCH 5/7] fix typo --- commit/merkleroot/validate_observation.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/commit/merkleroot/validate_observation.go b/commit/merkleroot/validate_observation.go index 1b9053d01..b5c86239b 100644 --- a/commit/merkleroot/validate_observation.go +++ b/commit/merkleroot/validate_observation.go @@ -212,8 +212,8 @@ func ValidateMerkleRootsState( for chain, newNextOnRampSeqNum := range newNextOnRampSeqNums { offRampExpNextSeqNum, ok := offRampExpNextSeqNums[chain] if !ok { - // Due to some chain being disabled while the sequence numbers were already observed, reported should - // not be considered valid in that case. + // Due to some chain being disabled while the sequence numbers were already observed. + // Report should not be considered valid in that case. return fmt.Errorf("offRamp expected next sequence number for chain %d was not found", chain) } From 7c93b2cfd4cf7e5742fe2a4f06bb23d5bc1e5f37 Mon Sep 17 00:00:00 2001 From: dimkouv Date: Wed, 8 Jan 2025 08:39:17 +0200 Subject: [PATCH 6/7] switch log level --- pkg/reader/ccip.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reader/ccip.go b/pkg/reader/ccip.go index d1ba9f57c..2905a0880 100644 --- a/pkg/reader/ccip.go +++ b/pkg/reader/ccip.go @@ -409,7 +409,7 @@ func (r *ccipChainReader) NextSeqNum( for _, chain := range chains { cfg, exists := cfgs[chain] if !exists { - r.lggr.Debugw("source chain config not found for chain %d, chain is skipped.", chain) + r.lggr.Warnw("source chain config not found for chain %d, chain is skipped.", chain) continue } From a9a1bb6961b9da3700a88d429cc1450a9e47d878 Mon Sep 17 00:00:00 2001 From: dimkouv Date: Wed, 8 Jan 2025 09:47:08 +0200 Subject: [PATCH 7/7] gofmt --- commit/merkleroot/query_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commit/merkleroot/query_test.go b/commit/merkleroot/query_test.go index 31f3d6486..c81b3a8a8 100644 --- a/commit/merkleroot/query_test.go +++ b/commit/merkleroot/query_test.go @@ -90,7 +90,7 @@ func TestProcessor_Query(t *testing.T) { }, RMNRemoteCfg: rmnRemoteCfg, }, - contractAddresses: contractAddrs, + contractAddresses: contractAddrs, cfg: pluginconfig.CommitOffchainConfig{ RMNEnabled: true, RMNSignaturesTimeout: 5 * time.Second,