Skip to content
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

integration-tests/smoke: add batching test #15292

Merged
merged 25 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions deployment/ccip/changeset/test_assertions.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,9 +390,13 @@ func ConfirmExecWithSeqNrs(
startBlock *uint64,
expectedSeqNrs []uint64,
) (executionStates map[uint64]int, err error) {
timer := time.NewTimer(5 * time.Minute)
if len(expectedSeqNrs) == 0 {
return nil, fmt.Errorf("no expected sequence numbers provided")
}

timer := time.NewTimer(3 * time.Minute)
defer timer.Stop()
tick := time.NewTicker(5 * time.Second)
tick := time.NewTicker(3 * time.Second)
defer tick.Stop()
sink := make(chan *offramp.OffRampExecutionStateChanged)
subscription, err := offRamp.WatchExecutionStateChanged(&bind.WatchOpts{
Expand Down
96 changes: 92 additions & 4 deletions integration-tests/smoke/ccip_batching_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func Test_CCIPBatching(t *testing.T) {
require.NoError(t, changeset.AddLaneWithDefaultPrices(e.Env, state, sourceChain2, destChain))

const (
numMessages = 30
numMessages = 5
makramkd marked this conversation as resolved.
Show resolved Hide resolved
)
var (
startSeqNum = map[uint64]ccipocr3.SeqNum{
Expand Down Expand Up @@ -90,6 +90,20 @@ func Test_CCIPBatching(t *testing.T) {
)
require.NoErrorf(t, err, "failed to confirm commit from chain %d", sourceChain1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though the test is oriented around commit batching, I think it would be worth waiting for messages execution. We don't care about batch sizes for exec, but we want to make sure everything is successfully executed (e.g. this large commit batch is properly processed by the exec)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought about that as well. Exec doesn't have to batch like commit due to gas limits though


states, err := changeset.ConfirmExecWithSeqNrs(
t,
e.Env.Chains[sourceChain1],
e.Env.Chains[destChain],
state.Chains[destChain].OffRamp,
nil,
genSeqNrRange(startSeqNum[sourceChain1], endSeqNum[sourceChain1]),
)
require.NoError(t, err)
// assert that all states are successful
for _, state := range states {
require.Equal(t, changeset.EXECUTION_STATE_SUCCESS, state)
}

startSeqNum[sourceChain1] = endSeqNum[sourceChain1] + 1
endSeqNum[sourceChain1] = startSeqNum[sourceChain1] + ccipocr3.SeqNum(numMessages) - 1
})
Expand Down Expand Up @@ -177,6 +191,49 @@ func Test_CCIPBatching(t *testing.T) {
require.NotNil(t, reports[1], "commit report should not be nil")
require.Equal(t, reports[0], reports[1], "commit reports should be the same")

// confirm execution
execErrs := make(chan outputErr[map[uint64]int], len(sourceChains))
for _, srcChain := range sourceChains {
wg.Add(1)
go assertExecAsync(
t,
e,
state,
srcChain,
destChain,
genSeqNrRange(startSeqNum[srcChain], endSeqNum[srcChain]),
&wg,
execErrs,
)
}

t.Log("waiting for exec reports")
wg.Wait()

i = 0
var execStates []map[uint64]int
outer3:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for i < len(sourceChains) {
	select {
	case outputErr := <-execErrs:
		require.NoError(t, outputErr.err)
		execStates = append(execStates, outputErr.output)
		i++
	case <-ctx.Done():
		require.FailNow(t, "didn't get all exec reports before test context was done")
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that does work, just weirder :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it before writing it :D. I know it works but why is it weirder? I think it's much clearer that way.

for {
select {
case outputErr := <-execErrs:
require.NoError(t, outputErr.err)
execStates = append(execStates, outputErr.output)
i++
if i == len(sourceChains) {
break outer3
}
case <-ctx.Done():
require.FailNow(t, "didn't get all exec reports before test context was done")
}
}

// assert that all states are successful
for _, states := range execStates {
for _, state := range states {
require.Equal(t, changeset.EXECUTION_STATE_SUCCESS, state)
}
}

startSeqNum[sourceChain1] = endSeqNum[sourceChain1] + 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not used anywhere, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used anywhere but if we add test cases further down (which was the plan, wanted to add something where we manual exec a message that is part of a root with multiple msgs)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not gonna be used in this PR then better remove them to make the next PR easier to understand when seeing it added.

endSeqNum[sourceChain1] = startSeqNum[sourceChain1] + ccipocr3.SeqNum(numMessages) - 1
startSeqNum[sourceChain2] = endSeqNum[sourceChain2] + 1
Expand All @@ -189,6 +246,29 @@ type outputErr[T any] struct {
err error
}

func assertExecAsync(
t *testing.T,
e changeset.DeployedEnv,
state changeset.CCIPOnChainState,
sourceChainSelector,
destChainSelector uint64,
seqNums []uint64,
wg *sync.WaitGroup,
errs chan<- outputErr[map[uint64]int],
) {
defer wg.Done()
states, err := changeset.ConfirmExecWithSeqNrs(
t,
e.Env.Chains[sourceChainSelector],
e.Env.Chains[destChainSelector],
state.Chains[destChainSelector].OffRamp,
nil,
seqNums,
)

errs <- outputErr[map[uint64]int]{states, err}
}

func assertCommitReportsAsync(
t *testing.T,
e changeset.DeployedEnv,
Expand All @@ -201,8 +281,7 @@ func assertCommitReportsAsync(
errs chan<- outputErr[*offramp.OffRampCommitReportAccepted],
) {
defer wg.Done()
var err error
sourceChain1Report, err := changeset.ConfirmCommitWithExpectedSeqNumRange(
commitReport, err := changeset.ConfirmCommitWithExpectedSeqNumRange(
t,
e.Env.Chains[sourceChainSelector],
e.Env.Chains[destChainSelector],
Expand All @@ -211,7 +290,7 @@ func assertCommitReportsAsync(
ccipocr3.NewSeqNumRange(startSeqNum, endSeqNum),
)

errs <- outputErr[*offramp.OffRampCommitReportAccepted]{sourceChain1Report, err}
errs <- outputErr[*offramp.OffRampCommitReportAccepted]{commitReport, err}
}

func sendMessagesAsync(
Expand Down Expand Up @@ -338,3 +417,12 @@ func genMessages(

return calls, totalValue, nil
}

// creates an array of uint64 from start to end inclusive
func genSeqNrRange(start, end ccipocr3.SeqNum) []uint64 {
var seqNrs []uint64
for i := start; i <= end; i++ {
seqNrs = append(seqNrs, uint64(i))
}
return seqNrs
}
Loading