Skip to content

Commit

Permalink
[ccip-2496] The explicit check of onchain sequential number validity … (
Browse files Browse the repository at this point in the history
#1083)

## Motivation
We had an issue where commits with the incorrect minimum interval value
were continuously transmitted by the commit DON (and thus constantly
reverting)

in
[calculateMinMaxSequenceNumbers](https://github.com/smartcontractkit/ccip/blob/8394f24cc336892ac84fd06f1c3c62f8f9d2423c/core/services/ocr2/plugins/ccip/ccipcommit/ocr2.go#L143-L155)
we fetch the onchain next seq num and do a log poller query from
(onchain next seq num, onchain next seq num + 256)

we have a
[check](https://github.com/smartcontractkit/ccip/blob/8394f24cc336892ac84fd06f1c3c62f8f9d2423c/core/services/ocr2/plugins/ccip/ccipcommit/ocr2.go#L164-L170)
that checks that the minimum sequence number retrieved from the log
poller query is not equal to onchain sequence number.

if this is the case the log poller query sequence number must be >
onchain min sequence number cuz less than onchain sequence number is
outside of the range
([query](https://github.com/smartcontractkit/ccip/blob/8394f24cc336892ac84fd06f1c3c62f8f9d2423c/core/services/ocr2/plugins/ccip/internal/ccipdata/v1_2_0/onramp.go#L147-L158))

therefore the interval that was built by a quorum must have been
[min=onchain seq nr + 1, max=...]

## Solution
The fix for this is an explicit check in the isMerkleRootStale func that
`reportInterval.Min == nextSeqNum`
  • Loading branch information
valerii-kabisov-cll authored Jun 25, 2024
1 parent e135d77 commit a47849b
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 10 deletions.
11 changes: 7 additions & 4 deletions core/services/ocr2/plugins/ccip/ccipcommit/ocr2.go
Original file line number Diff line number Diff line change
Expand Up @@ -640,12 +640,15 @@ func (r *CommitReportingPlugin) isStaleMerkleRoot(ctx context.Context, lggr logg
return true
}

if nextSeqNum > reportInterval.Min {
// If the next min is already greater than this reports min, this report is stale.
lggr.Infow("Report is stale because of root", "onchain min", nextSeqNum, "report min", reportInterval.Min)
// The report is not stale and correct only if nextSeqNum == reportInterval.Min.
// Mark it stale if the condition isn't met.
if nextSeqNum != reportInterval.Min {
lggr.Infow("The report is stale because of sequence number mismatch with the commit store interval min value",
"nextSeqNum", nextSeqNum, "reportIntervalMin", reportInterval.Min)
return true
}
lggr.Infow("Report root is not stale", "onchain min", nextSeqNum, "report min", reportInterval.Min)

lggr.Infow("Report root is not stale", "nextSeqNum", nextSeqNum, "reportIntervalMin", reportInterval.Min)

// If a report has root and valid sequence number, the report should be submitted, regardless of price staleness
return false
Expand Down
35 changes: 29 additions & 6 deletions core/services/ocr2/plugins/ccip/ccipcommit/ocr2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1197,13 +1197,36 @@ func TestCommitReportingPlugin_isStaleReport(t *testing.T) {
commitStoreReader: commitStoreReader,
}

assert.False(t, r.isStaleReport(ctx, lggr, cciptypes.CommitStoreReport{
MerkleRoot: merkleRoot1,
Interval: cciptypes.CommitStoreInterval{Min: expNextSeqNum + 1, Max: expNextSeqNum + 10},
}, types.ReportTimestamp{}))
testCases := map[string]struct {
interval cciptypes.CommitStoreInterval
result bool
}{
"The nextSeqNumber is equal to the commit store interval Min value": {
interval: cciptypes.CommitStoreInterval{Min: expNextSeqNum, Max: expNextSeqNum + 10},
result: false,
},
"The nextSeqNumber is less than the commit store interval Min value": {
interval: cciptypes.CommitStoreInterval{Min: expNextSeqNum + 1, Max: expNextSeqNum + 10},
result: true,
},
"The nextSeqNumber is greater than the commit store interval Min value": {
interval: cciptypes.CommitStoreInterval{Min: expNextSeqNum - 1, Max: expNextSeqNum + 10},
result: true,
},
"Empty interval": {
interval: cciptypes.CommitStoreInterval{},
result: true,
},
}

assert.True(t, r.isStaleReport(ctx, lggr, cciptypes.CommitStoreReport{
MerkleRoot: merkleRoot1}, types.ReportTimestamp{}))
for tcName, tc := range testCases {
t.Run(tcName, func(t *testing.T) {
assert.Equal(t, tc.result, r.isStaleReport(ctx, lggr, cciptypes.CommitStoreReport{
MerkleRoot: merkleRoot1,
Interval: tc.interval,
}, types.ReportTimestamp{}))
})
}
})
}

Expand Down

0 comments on commit a47849b

Please sign in to comment.