From 7a59051787e461aeda932c66ab2fd0f7b729c1b5 Mon Sep 17 00:00:00 2001 From: Domino Valdano <2644901+reductionista@users.noreply.github.com> Date: Wed, 10 Apr 2024 10:41:30 -0700 Subject: [PATCH] Fix bugs in TestLogPoller_BackupPollAndSaveLogsSkippingLogsThatAreTooOld This fixes 2 bugs on develop branch in this test, and removes some unused commented code. First Bug ========= The first bug was causing a false positive PASS on develop branch, which was obscuring a (very minor) bug in BackupPoller that's been fixed in this PR. The comment here about what the test was intended to test is still correct: // Only the 2nd batch + 1 log from a previous batch should be backfilled, because we perform backfill starting // from one block behind the latest finalized block Contrary to the comment, the code was returning 2 logs from the 1st batch (Data=9 & Data=10), plus 9 of 10 logs from the 2nd batch. This was incorrect behavior, but the test was also checking for the same incorrect behavior (looking for 11 logs with first one being Data=9) instead of what's described in the comment. The bug in the production code was that it starts the Backup Poller at Finalized - 1 instead of Finalized. This is a harmless "bug", just unnecessarily starting a block too early, since there's no reason for backup logpoller to re-request the same finalized logs that's already been processed. Now, the code returns the last log from the 1st batch + all but one logs from the 2nd batch, which is correct. (It can't return the last log because that goes beyond the last safe block.) So the test checks that there are 10 logs with first one being Data=10 (last log from the first batch.) Second Bug ========== The second bug was passing firstBatchBlock and secondBatchBlock directly to markBlockAsFinalized() instead of passing firstBatchBlock - 1 and secondBatchBlock - 1. This was only working because of a bug in the version of geth we're currently using: when you request the pending block from simulated geth, it gives you back the current block (1 block prior) instead of the current block. (For example, in the first case, even though we wanted block 11, the latest current block, we request block 12 and get back block 11.) This has been fixed in the latest version of geth... so presumably if we don't fix this here the test would have started failing as soon as we upgrade to the latest version of geth. It doesn't change any behavior of the test for the present version of geth, just makes it more clear that we want block 11 not 12. --- core/chains/evm/logpoller/log_poller_test.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/core/chains/evm/logpoller/log_poller_test.go b/core/chains/evm/logpoller/log_poller_test.go index c02112d6333..4e02f1806a2 100644 --- a/core/chains/evm/logpoller/log_poller_test.go +++ b/core/chains/evm/logpoller/log_poller_test.go @@ -545,8 +545,6 @@ func TestLogPoller_BackupPollAndSaveLogsSkippingLogsThatAreTooOld(t *testing.T) BackupPollerBlockDelay: 1, } th := SetupTH(t, lpOpts) - //header, err := th.Client.HeaderByNumber(ctx, nil) - //require.NoError(t, err) // Emit some logs in blocks for i := 1; i <= logsBatch; i++ { @@ -559,7 +557,7 @@ func TestLogPoller_BackupPollAndSaveLogsSkippingLogsThatAreTooOld(t *testing.T) // 1 -> 2 -> ... -> firstBatchBlock firstBatchBlock := th.PollAndSaveLogs(ctx, 1) // Mark current tip of the chain as finalized (after emitting 10 logs) - markBlockAsFinalized(t, th, firstBatchBlock) + markBlockAsFinalized(t, th, firstBatchBlock-1) // Emit 2nd batch of block for i := 1; i <= logsBatch; i++ { @@ -571,7 +569,7 @@ func TestLogPoller_BackupPollAndSaveLogsSkippingLogsThatAreTooOld(t *testing.T) // 1 -> 2 -> ... -> firstBatchBlock (finalized) -> .. -> firstBatchBlock + emitted logs secondBatchBlock := th.PollAndSaveLogs(ctx, firstBatchBlock) // Mark current tip of the block as finalized (after emitting 20 logs) - markBlockAsFinalized(t, th, secondBatchBlock) + markBlockAsFinalized(t, th, secondBatchBlock-1) // Register filter err := th.LogPoller.RegisterFilter(ctx, logpoller.Filter{ @@ -595,8 +593,8 @@ func TestLogPoller_BackupPollAndSaveLogsSkippingLogsThatAreTooOld(t *testing.T) th.EmitterAddress1, ) require.NoError(t, err) - require.Len(t, logs, logsBatch+1) - require.Equal(t, hexutil.MustDecode(`0x0000000000000000000000000000000000000000000000000000000000000009`), logs[0].Data) + require.Len(t, logs, logsBatch) + require.Equal(t, hexutil.MustDecode(`0x000000000000000000000000000000000000000000000000000000000000000a`), logs[0].Data) } func TestLogPoller_BlockTimestamps(t *testing.T) {