commit plugin - support batch sizes greater than one #131

dimkouv · 2024-09-18T10:26:00Z

So far commit plugin did not support batch sizes greater than 1, meaning that the reported sequence number ranges were always of size 1 e.g. chain1: [123, 123].

In this PR I use the ccipReader to observe the onRamp sequence numbers to increase the batch sizes.

Since every onRamp exists on a different chain we cannot use batch calls, goroutines are spawned instead for each source chain using errgroup.

If for a specific source chain we cannot get the onRamp sequence number, onRamp sequence numbers observation stops.

Commit plugin should limit the amount of data it can process in a round. In a scenario when we have too many pending messages, e.g. [123, 10000] we limit this ranges up to a specific configured batch size which is set to a default of 256.

Limits can be configured on plugin initialization.

winder · 2024-09-18T12:52:31Z

commit/merkleroot/observation.go

+
+			latestOnRampSeqNums[i] = plugintypes.SeqNumChain{
+				ChainSel: sourceChain,
+				SeqNum:   nextOnRampSeqNum - 1, // Latest is the next one minus one.


latestOnRampSeqNum would be set to 0 for a new chain. Is that OK?

yeah, in that case there won't be any range reported, since (onRampMax < offRampNext).

winder · 2024-09-18T12:56:44Z

internal/libs/cciptypeutil/seqnumrange.go

+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 {


nit: could add a .Limit function to the SeqNumRange type in chainlink-common.

smartcontractkit/chainlink-common#781

winder · 2024-09-18T13:00:44Z

commit/merkleroot/outcome.go

 			}
 			rangesToReport = append(rangesToReport, chainRange)
+
+			if wasTruncated := rng.End() != chainRange.SeqNumRange.End(); wasTruncated {


Suggested change

if wasTruncated := rng.End() != chainRange.SeqNumRange.End(); wasTruncated {

// check if the range was truncated.

if rng.End() != chainRange.SeqNumRange.End() {

winder · 2024-09-18T13:06:14Z

commit/merkleroot/processor.go

@@ -15,6 +15,10 @@ import (
 	"github.com/smartcontractkit/chainlink-ccip/shared"
 )

+// 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


MaxNumberTreeLeaves

Suggested change

const DefaultSeqNumsBatchLimit = 256

const DefaultSeqNumsBatchLimit = merklemulti.MaxNumberTreeLeaves

This should not be here, this should be in offchain config

And make sure to prefix it with EVM/have EVM somewhere in the name

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?

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

Added EVM prefix and moved to pluginconfig package

winder · 2024-09-18T13:07:07Z

commit/merkleroot/outcome_test.go

+					1: 18, // off ramp next is 18, on ramp max is 20 so new msgs are: [18, 19, 20]
+				},
+			},
+			rangeLimitsPerSourceChain: map[cciptypes.ChainSelector]uint64{},


Suggested change

rangeLimitsPerSourceChain: map[cciptypes.ChainSelector]uint64{},

rangeLimitsPerSourceChain: map[cciptypes.ChainSelector]uint64{}, // default limits

winder

LGTM, just some minor suggestions to consider

makramkd · 2024-09-18T13:23:57Z

commit/merkleroot/processor.go

@@ -15,6 +15,10 @@ import (
 	"github.com/smartcontractkit/chainlink-ccip/shared"
 )

+// 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


This should not be here, this should be in offchain config

makramkd · 2024-09-18T13:24:24Z

commit/merkleroot/processor.go

@@ -15,6 +15,10 @@ import (
 	"github.com/smartcontractkit/chainlink-ccip/shared"
 )

+// 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


And make sure to prefix it with EVM/have EVM somewhere in the name

makramkd · 2024-09-18T13:26:08Z

internal/libs/cciptypeutil/seqnumrange.go

+// SeqNumRangeLimit limits the range to n elements by truncating the end if necessary.
+func SeqNumRangeLimit(rng ccipocr3.SeqNumRange, n uint64) ccipocr3.SeqNumRange {
+	numElems := rng.End() - rng.Start() + 1
+	if numElems <= 0 {


< 0 is impossible for unsigned types since they'd just overflow, maybe just check == 0 here or have a End() >= Start() check at the top of the func to completely avoid the overflow scenario?

< 0 is impossible but checking <= 0 instead of == 0 should be harmless. There's an overflow check below.

makramkd · 2024-09-18T13:30:15Z

internal/libs/cciptypeutil/seqnumrange.go

+		return rng
+	}
+
+	if uint64(numElems) > n {


Is this cast necessary? Isn't numElems already a uint64 by virtue of both End() and Start() being uint64?

I guess its type is cciptypes.SeqNum?

makramkd · 2024-09-18T13:31:48Z

internal/libs/cciptypeutil/seqnumrange.go

+		if newEnd > rng.End() { // overflow - do nothing
+			return rng
+		}
+		rng.SetEnd(newEnd)


Not a fan of mutating func inputs, why not just return a brand new SeqNumRange?

Pushed to chainlink-common: https://github.com/smartcontractkit/chainlink-common/pull/781/commits
works like this

makramkd · 2024-09-18T13:34:40Z

pluginconfig/commit.go

+	// BatchLimits is the maximum number of messages to include in a single report for a target chain.
+	// If for example in the next round we have 1000 pending messages and a batch limit of 256, only 256 seq nums
+	// will be in the report. If a value is not set we fallback to merkleroot.DefaultSeqNumsBatchLimit.
+	BatchLimits map[cciptypes.ChainSelector]uint64 `json:"batchLimits"`


Batch limit is only dest dependent and doesn't have to be for all chains in the system. I also recommend we call this "max tree size" to be consistent across the system and not change jargon too much.

Suggested change

// BatchLimits is the maximum number of messages to include in a single report for a target chain.

// If for example in the next round we have 1000 pending messages and a batch limit of 256, only 256 seq nums

// will be in the report. If a value is not set we fallback to merkleroot.DefaultSeqNumsBatchLimit.

BatchLimits map[cciptypes.ChainSelector]uint64 `json:"batchLimits"`

// MaxTreeSize is the maximum size of a merkle tree to create prior to calculating the merkle root.

// If for example in the next round we have 1000 pending messages and a max tree size of 256, only 256 seq nums

// will be in the report. If a value is not set we fallback to merklemulti. MaxNumberTreeLeaves.

MaxTreeSize int `json:"maxTreeSize"`

Made it like this, one value per dest / plugin instance

Added a warn log when value is not set

github-actions · 2024-09-19T09:28:34Z

Test Coverage

Branch	Coverage
dk/commit-batch-size	69.4%
main	69.3%

commit batch sizes > 1

f5894b2

dimkouv marked this pull request as ready for review September 18, 2024 10:40

dimkouv requested a review from a team as a code owner September 18, 2024 10:40

dimkouv requested review from makramkd, asoliman92 and winder September 18, 2024 10:41

query only node supported source chains

3dd255c

dimkouv requested a review from 0xnogo September 18, 2024 12:36

winder reviewed Sep 18, 2024

View reviewed changes

winder approved these changes Sep 18, 2024

View reviewed changes

makramkd requested changes Sep 18, 2024

View reviewed changes

dimkouv added 7 commits September 18, 2024 16:51

Merge branch 'main' into dk/commit-batch-size

9c9506b

Merge branch 'main' into dk/commit-batch-size

4c4038a

code reviews: makramkd, winder

44013ba

merge main

4df3a72

cfg override earlier

525193a

fix import

6df9088

upgrade cl-common

4947a72

makramkd approved these changes Sep 19, 2024

View reviewed changes

dimkouv merged commit 373d64f into main Sep 19, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

commit plugin - support batch sizes greater than one #131

commit plugin - support batch sizes greater than one #131

dimkouv commented Sep 18, 2024 •

edited

Loading

winder Sep 18, 2024

dimkouv Sep 19, 2024

winder Sep 18, 2024

dimkouv Sep 19, 2024

dimkouv Sep 19, 2024

winder Sep 18, 2024

winder Sep 18, 2024

makramkd Sep 18, 2024

makramkd Sep 18, 2024

winder Sep 18, 2024

makramkd Sep 18, 2024

dimkouv Sep 19, 2024

winder Sep 18, 2024

winder left a comment

makramkd Sep 18, 2024

makramkd Sep 18, 2024

makramkd Sep 18, 2024

dimkouv Sep 19, 2024

makramkd Sep 18, 2024

makramkd Sep 18, 2024

makramkd Sep 18, 2024

dimkouv Sep 19, 2024

makramkd Sep 18, 2024

dimkouv Sep 19, 2024

dimkouv Sep 19, 2024

github-actions bot commented Sep 19, 2024

	if wasTruncated := rng.End() != chainRange.SeqNumRange.End(); wasTruncated {
	// check if the range was truncated.
	if rng.End() != chainRange.SeqNumRange.End() {

	const DefaultSeqNumsBatchLimit = 256
	const DefaultSeqNumsBatchLimit = merklemulti.MaxNumberTreeLeaves

	rangeLimitsPerSourceChain: map[cciptypes.ChainSelector]uint64{},
	rangeLimitsPerSourceChain: map[cciptypes.ChainSelector]uint64{}, // default limits

commit plugin - support batch sizes greater than one #131

commit plugin - support batch sizes greater than one #131

Conversation

dimkouv commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2024

Test Coverage

dimkouv commented Sep 18, 2024 •

edited

Loading