CCIP-1078 Support for finality tags in LogPoller #10762

mateusz-sekara · 2023-09-22T13:39:47Z

LogPoller now accepts cfg.EVM().FinalityTagEnabled() and, when enabled, can also fetch information about finalized block during PollAndSave step. This allows us to use chain finality instead of the opinionated value of finality depth without introducing many changes to the LogPoller API and logic.

A high-level overview of the changes introduced:

every LogPollerBlock has an additional attribute representing the last number of the finalized block when LogPollerBlock was persisted. This gives us the ability to ask about finalized logs using the same nested query we did so far (with slight modification)
every LogPoller function accepting confs can now return finalized logs. A dedicated Confirmation type was introduced for this case

Additionally:

LogsUntilBlockHashDataWordGreaterThan is removed because it's no longer needed (this was used on the CCIP end to get finalized logs based on the last finalized block hash)

github-actions · 2023-09-22T13:40:11Z

I see that you haven't updated any README files. Would it make sense to do so?

core/chains/evm/logpoller/log_poller.go

dimkouv

Overall logic, looks nice. I added a few comments about the code. Let's address some of them before merging.

core/chains/evm/chain.go

core/chains/evm/client/simulated_backend_client.go

core/chains/evm/logpoller/log_poller.go

reductionista · 2023-10-06T21:31:19Z

core/chains/evm/logpoller/log_poller.go

 	// Current is where the mismatch starts.
 	// Check its parent to see if its the same as ours saved.
 	parent, err := lp.ec.HeadByHash(ctx, current.ParentHash)
 	if err != nil {
 		return nil, err
 	}
+	// If finalityTag is not used, we need to compute lastFinalizedBlockNumber based on the configured finalityDepth and the parent block
+	if !lp.useFinalityTag {
+		lastFinalizedBlockNumber = parent.Number - lp.finalityDepth


I think this should be:
lastFinalizedBlockNumber = current.Number - lp.finalityDepth

and then below we want:

for parent.Number >= lastFinalizedBlockNumber - 1 {

At least, that's what would keep present logic the same... although it doesn't seem consistent with this comment:

// If the parent block number becomes < the first finalized block our reorg is too deep.

which would indicate to me either the comment was wrong or the current logic is 1 block safer than we need?

IIUC, what we are trying to do here is to find the first block (after the last finalized block) that matches our state in the database. Worst case scenario, we should return the last finalized block, so the loop should terminate when parent.Number < lastFinalizedBlockNumber, which matches current implementation

I think this should be:
lastFinalizedBlockNumber = current.Number - lp.finalityDepth
and then below we want:
for parent.Number >= lastFinalizedBlockNumber - 1 {

I think that parent.Number - lp.finalityDepth is actually current.Number - lp.finalityDepth - 1 (because parent is one block behind), so it kind of matches current logic, no?

Although, I'm not sure why we did compute max reorg depth based on the parent block, not the current in the previous implementation

for parent.Number >= (reorgStart - lp.finalityDepth) // reorgStart = parent.Number

Was that accidental?

Was that accidental

yeah looks to me like its one block extra of safety for no reason

Removed extra block, it required increasing finality depth in tests

Alright, makes sense now!

reductionista

I think all the outstanding issues have been resolved, LGTM!

connorwstein · 2023-10-10T22:08:45Z

core/chains/evm/logpoller/log_poller.go

@@ -497,21 +498,20 @@ func (lp *logPoller) run() {
 				}
 				// Otherwise this is the first poll _ever_ on a new chain.
 				// Only safe thing to do is to start at the first finalized block.
-				latest, err := lp.ec.HeadByNumber(lp.ctx, nil)
+				latestBlock, lastFinalizedBlockNumber, err := lp.latestBlocks(lp.ctx)


nit: prefer latestFinalizedBlockNumber to lastFinalizedBlockNumber, more clear and aligned with latestBlock

How about the database field? Do you have any suggestions? Should we keep it last_finalized_block_number or change to latest, or maybe only finalized_block_number? This is the last time we can easily change it ;)

yeah I like just finalized_block_number

core/chains/evm/logpoller/log_poller.go

connorwstein · 2023-10-10T22:29:38Z

core/chains/evm/logpoller/log_poller.go

@@ -558,22 +564,16 @@ func (lp *logPoller) BackupPollAndSaveLogs(ctx context.Context, backupPollerBloc
 			}
 			return
 		}
-
+		finalityDepth := latestBlock.Number - lastFinalizedBlockNumber


I think this is incorrect. Say we crash with finality depth 10, then reboot and latest finality depth is 5. Ignoring backup delay, we'll set backup poll to 5 when it should be 10. So we'll miss backup polling 5 blocks. Simple solution : use lastProcessed.LastFinalizedBlock number instead. I think whole thing can be simplified:

lp.backupPollerNextBlock = mathutil.Min(lastProcessed.LastFinalizedBlock + 1, lastProcessed.Number - backupPollerBlockDelay)

Can move the latest query back to where it was that way as well.

Wow, good catch!

It looks like the existing logic is equivalent to mathutil.Min(lastProcessed.LastFinalizedBlock - 1, lastProcessed.Number - backupPollerBlockDelay) though. Was it off by 2 blocks? Instead of starting at the block before last finalized, now we'd be using the block after the last finalized.

I think that sounds right, just wanted to confirm that this is an intentional fix of another case where the production code was "more safe than necessary", rather than keeping functionality the same and just refactoring.

Agree, you are right @connorwstein. Btw, we should have more tests that verify edge cases like these, I think the problem is also present in the current develop, as we subtract finalityDepth from the current tip of the blockchain

you are right @reductionista. Let's keep logic as is with starting from finalized - 1. The benefit is just consistency: everywhere we backfill, we backfill up to finalized - 1 (we need the finalized block itself to be able to reorg check on finalized+1).

connorwstein · 2023-10-10T22:31:06Z

core/chains/evm/logpoller/log_poller.go

@@ -735,7 +735,7 @@ func (lp *logPoller) getCurrentBlockMaybeHandleReorg(ctx context.Context, curren
 		// There can be another reorg while we're finding the LCA.
 		// That is ok, since we'll detect it on the next iteration.
 		// Since we go currentBlock by currentBlock for unfinalized logs, the mismatch starts at currentBlockNumber - 1.
-		blockAfterLCA, err2 := lp.findBlockAfterLCA(ctx, currentBlock)
+		blockAfterLCA, err2 := lp.findBlockAfterLCA(ctx, currentBlock, expectedParent.LastFinalizedBlockNumber)


cl-sonarqube-production · 2023-10-12T16:16:47Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
26 Code Smells

92.4% Coverage
0.0% Duplication

connorwstein · 2023-10-16T13:42:21Z

core/store/migrate/migrations/0202_default_values_for_last_finalized_block.sql.sql

+        evm_chain_id,
+        CASE
+            WHEN evm_chain_id = 43113 then 1 -- Avax Fuji
+            WHEN evm_chain_id = 43114 then 1 -- Avax Mainnet


might be worth including https://github.com/smartcontractkit/ccip/blob/ccip-develop/core/chains/evm/config/toml/defaults/Avalanche_ANZ_testnet.toml#L2 as well, though its just a testnet so not a big deal.

connorwstein · 2023-10-16T13:43:38Z

core/store/migrate/migrate_test.go

+
+	b.ResetTimer()
+
+	// 1. Measure time of migration 200


connorwstein · 2023-10-16T13:45:29Z

core/chains/evm/logpoller/log_poller_test.go

+		{
+			name:          "chain finality in use",
+			finalityDepth: 0,
+			finalityTag:   true,


connorwstein · 2023-10-16T13:47:33Z

core/chains/evm/logpoller/log_poller_test.go

-	require.Equal(t, uint64(10), b.Time())
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			th := SetupTH(t, tt.finalityTag, tt.finalityDepth, 3, 2)


nit: could pull this into its own function to reduce nesting, ditto for poll and save logs test

connorwstein · 2023-10-16T13:48:50Z

core/chains/evm/logpoller/log_poller.go

@@ -122,21 +127,22 @@ type logPoller struct {
 // How fast that can be done depends largely on network speed and DB, but even for the fastest
 // support chain, polygon, which has 2s block times, we need RPCs roughly with <= 500ms latency
 func NewLogPoller(orm ORM, ec Client, lggr logger.Logger, pollPeriod time.Duration,
-	finalityDepth int64, backfillBatchSize int64, rpcBatchSize int64, keepBlocksDepth int64) *logPoller {
+	useFinalityTag bool, finalityDepth int64, backfillBatchSize int64, rpcBatchSize int64, keepFinalizedBlocksDepth int64) *logPoller {


should comment somewhere that useFinalityTag=true overrides the finalityDepth parameter

mateusz-sekara force-pushed the lp/finality-support branch 2 times, most recently from c52292c to 19545b8 Compare September 25, 2023 12:45

mateusz-sekara commented Sep 25, 2023

View reviewed changes

core/chains/evm/logpoller/log_poller.go Outdated Show resolved Hide resolved

mateusz-sekara force-pushed the lp/finality-support branch 4 times, most recently from 5bf960e to 0aa4094 Compare September 26, 2023 10:01

mateusz-sekara marked this pull request as ready for review September 26, 2023 10:08

mateusz-sekara requested review from bolekk, justinkaseman, KuphJr, a team, samsondav, sdrug, martin-cll, reductionista, connorwstein and prashantkumar1982 as code owners September 26, 2023 10:08

connorwstein requested changes Sep 26, 2023

View reviewed changes

connorwstein mentioned this pull request Sep 27, 2023

Version abstraction smartcontractkit/ccip#151

Merged

mateusz-sekara requested a review from a team as a code owner September 28, 2023 10:18

mateusz-sekara force-pushed the lp/finality-support branch from 38cf585 to 1d71390 Compare September 28, 2023 10:21

dimkouv requested changes Sep 28, 2023

View reviewed changes

dimkouv previously approved these changes Sep 28, 2023

View reviewed changes

mateusz-sekara force-pushed the lp/finality-support branch from df4b28d to 4363421 Compare October 2, 2023 06:23

mateusz-sekara dismissed dimkouv’s stale review via 36d562b October 2, 2023 06:29

mateusz-sekara force-pushed the lp/finality-support branch from 4363421 to 36d562b Compare October 2, 2023 06:29

mateusz-sekara requested a review from jmank88 as a code owner October 2, 2023 07:58

mateusz-sekara force-pushed the lp/finality-support branch from b50059b to 36d01f5 Compare October 2, 2023 08:02

reductionista reviewed Oct 6, 2023

View reviewed changes

mateusz-sekara requested a review from reductionista October 9, 2023 10:13

mateusz-sekara force-pushed the lp/finality-support branch from 4bb0671 to 4f172a0 Compare October 9, 2023 12:06

reductionista previously approved these changes Oct 9, 2023

View reviewed changes

mateusz-sekara dismissed reductionista’s stale review via 85f9fb1 October 10, 2023 08:44

mateusz-sekara force-pushed the lp/finality-support branch 3 times, most recently from 06e62a5 to 7de8bd4 Compare October 10, 2023 09:02

mateusz-sekara requested a review from reductionista October 10, 2023 15:37

connorwstein previously approved these changes Oct 10, 2023

View reviewed changes

mateusz-sekara dismissed connorwstein’s stale review via d0bed42 October 11, 2023 12:19

mateusz-sekara requested a review from connorwstein October 11, 2023 15:48

mateusz-sekara force-pushed the lp/finality-support branch 2 times, most recently from 31b399f to 51467fc Compare October 12, 2023 15:36

mateusz-sekara added 2 commits October 12, 2023 17:36

Support for finality in the LogPoller

4af9dce

Post review fixes

755ff8d

mateusz-sekara force-pushed the lp/finality-support branch 3 times, most recently from 7dc5cf7 to 3f6373b Compare October 12, 2023 15:53

Post rebase fixes

a2cd824

mateusz-sekara force-pushed the lp/finality-support branch from 3f6373b to a2cd824 Compare October 12, 2023 16:01

connorwstein approved these changes Oct 16, 2023

View reviewed changes

mateusz-sekara added this pull request to the merge queue Oct 16, 2023

Merged via the queue into develop with commit 4519370 Oct 16, 2023
82 checks passed

mateusz-sekara deleted the lp/finality-support branch October 16, 2023 14:07

mateusz-sekara changed the title ~~Support for finality tags in LogPoller~~ CCIP-1078 Support for finality tags in LogPoller Oct 23, 2023

mateusz-sekara mentioned this pull request Oct 27, 2023

CCIP-1230 Exposing entire LogPollerBlock from LatestBlock in LogPoller #11105

Merged

snehaagni mentioned this pull request Nov 14, 2023

release/2.7.0 -> master #11290

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCIP-1078 Support for finality tags in LogPoller #10762

CCIP-1078 Support for finality tags in LogPoller #10762

mateusz-sekara commented Sep 22, 2023 •

edited

Loading

github-actions bot commented Sep 22, 2023

dimkouv left a comment

reductionista Oct 6, 2023

reductionista Oct 6, 2023

mateusz-sekara Oct 9, 2023

mateusz-sekara Oct 9, 2023 •

edited

Loading

connorwstein Oct 9, 2023

mateusz-sekara Oct 9, 2023

reductionista Oct 9, 2023

reductionista left a comment

connorwstein Oct 10, 2023

mateusz-sekara Oct 11, 2023

connorwstein Oct 11, 2023

connorwstein Oct 10, 2023

reductionista Oct 11, 2023

mateusz-sekara Oct 11, 2023

connorwstein Oct 11, 2023

connorwstein Oct 10, 2023

cl-sonarqube-production bot commented Oct 12, 2023

connorwstein Oct 16, 2023

connorwstein Oct 16, 2023

connorwstein Oct 16, 2023

connorwstein Oct 16, 2023

connorwstein Oct 16, 2023

CCIP-1078 Support for finality tags in LogPoller #10762

CCIP-1078 Support for finality tags in LogPoller #10762

Conversation

mateusz-sekara commented Sep 22, 2023 • edited Loading

github-actions bot commented Sep 22, 2023

dimkouv 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

mateusz-sekara Oct 9, 2023 • 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

reductionista 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

cl-sonarqube-production bot commented Oct 12, 2023

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

mateusz-sekara commented Sep 22, 2023 •

edited

Loading

mateusz-sekara Oct 9, 2023 •

edited

Loading