-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
commit plugin - support batch sizes greater than one #131
Changes from 4 commits
f5894b2
3dd255c
9c9506b
4c4038a
44013ba
4df3a72
525193a
6df9088
4947a72
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -10,6 +10,8 @@ import ( | |||||||
"github.com/smartcontractkit/chainlink-common/pkg/logger" | ||||||||
cciptypes "github.com/smartcontractkit/chainlink-common/pkg/types/ccipocr3" | ||||||||
|
||||||||
"github.com/smartcontractkit/chainlink-ccip/internal/libs/cciptypeutil" | ||||||||
|
||||||||
"github.com/smartcontractkit/chainlink-ccip/commit/merkleroot/rmn" | ||||||||
"github.com/smartcontractkit/chainlink-ccip/internal/plugincommon" | ||||||||
"github.com/smartcontractkit/chainlink-ccip/internal/plugintypes" | ||||||||
|
@@ -46,7 +48,7 @@ func (w *Processor) getOutcome( | |||||||
|
||||||||
switch nextState { | ||||||||
case SelectingRangesForReport: | ||||||||
return reportRangesOutcome(q, consensusObservation), nextState | ||||||||
return reportRangesOutcome(q, w.lggr, consensusObservation, w.cfg.BatchLimits), nextState | ||||||||
case BuildingReport: | ||||||||
if q.RetryRMNSignatures { | ||||||||
// We want to retry getting the RMN signatures on the exact same outcome we had before. | ||||||||
|
@@ -64,10 +66,11 @@ func (w *Processor) getOutcome( | |||||||
} | ||||||||
|
||||||||
// reportRangesOutcome determines the sequence number ranges for each chain to build a report from in the next round | ||||||||
// TODO: ensure each range is below a limit | ||||||||
func reportRangesOutcome( | ||||||||
_ Query, | ||||||||
lggr logger.Logger, | ||||||||
consensusObservation ConsensusObservation, | ||||||||
rangeLimitsPerSourceChain map[cciptypes.ChainSelector]uint64, | ||||||||
) Outcome { | ||||||||
rangesToReport := make([]plugintypes.ChainRange, 0) | ||||||||
|
||||||||
|
@@ -82,11 +85,24 @@ func reportRangesOutcome( | |||||||
} | ||||||||
|
||||||||
if offRampNextSeqNum <= onRampMaxSeqNum { | ||||||||
rngLimit, ok := rangeLimitsPerSourceChain[chainSel] | ||||||||
if !ok { | ||||||||
rngLimit = DefaultSeqNumsBatchLimit | ||||||||
} | ||||||||
|
||||||||
rng := cciptypes.NewSeqNumRange(offRampNextSeqNum, onRampMaxSeqNum) | ||||||||
|
||||||||
chainRange := plugintypes.ChainRange{ | ||||||||
ChainSel: chainSel, | ||||||||
SeqNumRange: [2]cciptypes.SeqNum{offRampNextSeqNum, onRampMaxSeqNum}, | ||||||||
SeqNumRange: cciptypeutil.SeqNumRangeLimit(rng, rngLimit), | ||||||||
} | ||||||||
rangesToReport = append(rangesToReport, chainRange) | ||||||||
|
||||||||
if wasTruncated := rng.End() != chainRange.SeqNumRange.End(); wasTruncated { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
lggr.Infof("Range for chain %d: %s (before truncate: %v)", chainSel, chainRange.SeqNumRange, rng) | ||||||||
} else { | ||||||||
lggr.Infof("Range for chain %d: %s", chainSel, chainRange.SeqNumRange) | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
offRampNextSeqNums = append(offRampNextSeqNums, plugintypes.SeqNumChain{ | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7,6 +7,8 @@ | |||||
"github.com/stretchr/testify/require" | ||||||
|
||||||
cciptypes "github.com/smartcontractkit/chainlink-common/pkg/types/ccipocr3" | ||||||
|
||||||
"github.com/smartcontractkit/chainlink-ccip/plugintypes" | ||||||
) | ||||||
|
||||||
func Test_buildReport(t *testing.T) { | ||||||
|
@@ -37,3 +39,82 @@ | |||||
} | ||||||
}) | ||||||
} | ||||||
|
||||||
func Test_reportRangesOutcome(t *testing.T) { | ||||||
lggr := logger.Test(t) | ||||||
|
||||||
testCases := []struct { | ||||||
name string | ||||||
consensusObservation ConsensusObservation | ||||||
rangeLimitsPerSourceChain map[cciptypes.ChainSelector]uint64 | ||||||
expectedOutcome Outcome | ||||||
}{ | ||||||
{ | ||||||
name: "base empty outcome", | ||||||
expectedOutcome: Outcome{ | ||||||
OutcomeType: ReportIntervalsSelected, | ||||||
RangesSelectedForReport: []plugintypes.ChainRange{}, | ||||||
OffRampNextSeqNums: []plugintypes.SeqNumChain{}, | ||||||
}, | ||||||
}, | ||||||
{ | ||||||
name: "simple scenario with one chain", | ||||||
consensusObservation: ConsensusObservation{ | ||||||
OnRampMaxSeqNums: map[cciptypes.ChainSelector]cciptypes.SeqNum{ | ||||||
1: 20, | ||||||
}, | ||||||
OffRampNextSeqNums: map[cciptypes.ChainSelector]cciptypes.SeqNum{ | ||||||
1: 18, // off ramp next is 18, on ramp max is 20 so new msgs are: [18, 19, 20] | ||||||
}, | ||||||
}, | ||||||
rangeLimitsPerSourceChain: map[cciptypes.ChainSelector]uint64{}, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
expectedOutcome: Outcome{ | ||||||
OutcomeType: ReportIntervalsSelected, | ||||||
RangesSelectedForReport: []plugintypes.ChainRange{ | ||||||
{ChainSel: 1, SeqNumRange: cciptypes.NewSeqNumRange(18, 20)}, | ||||||
}, | ||||||
OffRampNextSeqNums: []plugintypes.SeqNumChain{ | ||||||
{ChainSel: 1, SeqNum: 18}, | ||||||
}, | ||||||
}, | ||||||
}, | ||||||
{ | ||||||
name: "simple scenario with one chain", | ||||||
consensusObservation: ConsensusObservation{ | ||||||
OnRampMaxSeqNums: map[cciptypes.ChainSelector]cciptypes.SeqNum{ | ||||||
1: 20, | ||||||
2: 1000, | ||||||
3: 10000, | ||||||
}, | ||||||
OffRampNextSeqNums: map[cciptypes.ChainSelector]cciptypes.SeqNum{ | ||||||
1: 18, // off ramp next is 18, on ramp max is 20 so new msgs are: [18, 19, 20] | ||||||
2: 995, // off ramp next is 995, on ramp max is 1000 so new msgs are: [995, 996, 997, 998, 999, 1000] | ||||||
3: 500, // off ramp next is 500, we have new messages up to 10000 (default limit applied) | ||||||
}, | ||||||
}, | ||||||
rangeLimitsPerSourceChain: map[cciptypes.ChainSelector]uint64{ | ||||||
2: 5, // limit to 5 messages, [1000] should be excluded | ||||||
}, | ||||||
expectedOutcome: Outcome{ | ||||||
OutcomeType: ReportIntervalsSelected, | ||||||
RangesSelectedForReport: []plugintypes.ChainRange{ | ||||||
{ChainSel: 1, SeqNumRange: cciptypes.NewSeqNumRange(18, 20)}, | ||||||
{ChainSel: 2, SeqNumRange: cciptypes.NewSeqNumRange(995, 999)}, | ||||||
{ChainSel: 3, SeqNumRange: cciptypes.NewSeqNumRange(500, 755)}, | ||||||
}, | ||||||
OffRampNextSeqNums: []plugintypes.SeqNumChain{ | ||||||
{ChainSel: 1, SeqNum: 18}, | ||||||
{ChainSel: 2, SeqNum: 995}, | ||||||
{ChainSel: 3, SeqNum: 500}, | ||||||
}, | ||||||
}, | ||||||
}, | ||||||
} | ||||||
|
||||||
for _, tc := range testCases { | ||||||
t.Run(tc.name, func(t *testing.T) { | ||||||
outc := reportRangesOutcome(Query{}, lggr, tc.consensusObservation, tc.rangeLimitsPerSourceChain) | ||||||
require.Equal(t, tc.expectedOutcome, outc) | ||||||
}) | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -14,6 +14,10 @@ import ( | |||||
"github.com/smartcontractkit/chainlink-ccip/pluginconfig" | ||||||
) | ||||||
|
||||||
// DefaultSeqNumsBatchLimit is the default number of max new messages to scan, we use this value when | ||||||
// the config is not set for a specific chain. | ||||||
const DefaultSeqNumsBatchLimit = 256 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not be here, this should be in offchain config There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And make sure to prefix it with EVM/have EVM somewhere in the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this the default unless there's an override? Or you're saying there should be no default and the system needs to crash if the config is missing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally the latter (IMO defaults are often more trouble than they're worth in this scenario, because people would just use the default even though it might be wrong) but to start with we could fall back to the default and enforce that its provided later if we decide to do so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added EVM prefix and moved to pluginconfig package |
||||||
|
||||||
// Processor is the processor responsible for | ||||||
// reading next messages and building merkle roots for them, | ||||||
// It's setup to use RMN to query which messages to include in the merkle root and ensures | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package cciptypeutil | ||
|
||
import "github.com/smartcontractkit/chainlink-common/pkg/types/ccipocr3" | ||
|
||
// SeqNumRangeLimit limits the range to n elements by truncating the end if necessary. | ||
func SeqNumRangeLimit(rng ccipocr3.SeqNumRange, n uint64) ccipocr3.SeqNumRange { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: could add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
numElems := rng.End() - rng.Start() + 1 | ||
if numElems <= 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
return rng | ||
} | ||
|
||
if uint64(numElems) > n { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this cast necessary? Isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess its type is |
||
newEnd := rng.Start() + ccipocr3.SeqNum(n) - 1 | ||
if newEnd > rng.End() { // overflow - do nothing | ||
return rng | ||
} | ||
rng.SetEnd(newEnd) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a fan of mutating func inputs, why not just return a brand new SeqNumRange? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pushed to chainlink-common: https://github.com/smartcontractkit/chainlink-common/pull/781/commits |
||
} | ||
|
||
return rng | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
package cciptypeutil | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/smartcontractkit/chainlink-common/pkg/types/ccipocr3" | ||
) | ||
|
||
func TestSeqNumRangeLimit(t *testing.T) { | ||
testCases := []struct { | ||
name string | ||
rng ccipocr3.SeqNumRange | ||
n uint64 | ||
want ccipocr3.SeqNumRange | ||
}{ | ||
{ | ||
name: "no truncation", | ||
rng: ccipocr3.NewSeqNumRange(0, 10), | ||
n: 11, | ||
want: ccipocr3.NewSeqNumRange(0, 10), | ||
}, | ||
{ | ||
name: "no truncation 2", | ||
rng: ccipocr3.NewSeqNumRange(100, 110), | ||
n: 11, | ||
want: ccipocr3.NewSeqNumRange(100, 110), | ||
}, | ||
{ | ||
name: "truncation", | ||
rng: ccipocr3.NewSeqNumRange(0, 10), | ||
n: 10, | ||
want: ccipocr3.NewSeqNumRange(0, 9), | ||
}, | ||
{ | ||
name: "truncation 2", | ||
rng: ccipocr3.NewSeqNumRange(100, 110), | ||
n: 10, | ||
want: ccipocr3.NewSeqNumRange(100, 109), | ||
}, | ||
{ | ||
name: "empty", | ||
rng: ccipocr3.NewSeqNumRange(0, 0), | ||
n: 0, | ||
want: ccipocr3.NewSeqNumRange(0, 0), | ||
}, | ||
{ | ||
name: "wrong range", | ||
rng: ccipocr3.NewSeqNumRange(20, 15), | ||
n: 3, | ||
want: ccipocr3.NewSeqNumRange(20, 15), | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
t.Run(tc.name, func(t *testing.T) { | ||
got := SeqNumRangeLimit(tc.rng, tc.n) | ||
if got != tc.want { | ||
t.Errorf("SeqNumRangeLimit(%v, %v) = %v; want %v", tc.rng, tc.n, got, tc.want) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latestOnRampSeqNum
would be set to 0 for a new chain. Is that OK?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, in that case there won't be any range reported, since (onRampMax < offRampNext).