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.

Sign up for GitHub

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

BCFR-888 LP support chains that have not reached finality yet #14366

Merged
merged 5 commits into from
Sep 23, 2024
Merged

BCFR-888 LP support chains that have not reached finality yet #14366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
67d7f3a
LP support chains that have not reached finality yet
dhaidashenko Sep 6, 2024
491e0a3
changelog
dhaidashenko Sep 6, 2024
173f5c7
fix test
dhaidashenko Sep 9, 2024
c235e65
Merge branch 'develop' into feature/BCFR-888-logpoller-suport-yong-ch…
dhaidashenko Sep 10, 2024
ebe2bee
ensure Test_ContractTransmitter_TransmitWithoutSignatures do not run …
dhaidashenko Sep 11, 2024
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
5 changes: 5 additions & 0 deletions .changeset/famous-goats-hug.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

LogPoller polls logs even if chain have not reached finality #internal
1 change: 0 additions & 1 deletion core/capabilities/ccip/ocrimpls/contract_transmitter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ func Test_ContractTransmitter_TransmitWithoutSignatures(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
tc := tc
t.Parallel()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is required as now LP can fetch events even if the latest finalized block is genesis block. The test starts multiple instances of the LogPoller with the same chainID. Each LogPoller tries to insert observed blocks, but catches a deadlock on DB level as they are all connected to the same db and try to insert the same block. I was expecting that ON CONFLICT DO NOTHING will prevent the deadlock, but it does not seem to be the case

testTransmitter(t, tc.pluginType, tc.withSigs, tc.expectedSigsEnabled, tc.report)
})
}
Expand Down
21 changes: 11 additions & 10 deletions core/chains/evm/logpoller/log_poller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,18 +598,11 @@ 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.
latestBlock, latestFinalizedBlockNumber, err := lp.latestBlocks(ctx)
_, latestFinalizedBlockNumber, err := lp.latestBlocks(ctx)
if err != nil {
lp.lggr.Warnw("Unable to get latest for first poll", "err", err)
continue
}
// Do not support polling chains which don't even have finality depth worth of blocks.
// Could conceivably support this but not worth the effort.
// Need last finalized block number to be higher than 0
if latestFinalizedBlockNumber <= 0 {
lp.lggr.Warnw("Insufficient number of blocks on chain, waiting for finality depth", "err", err, "latest", latestBlock.Number)
continue
}
// Starting at the first finalized block. We do not backfill the first finalized block.
start = latestFinalizedBlockNumber
} else {
Expand Down Expand Up @@ -1023,8 +1016,16 @@ func (lp *logPoller) latestBlocks(ctx context.Context) (*evmtypes.Head, int64, e
return nil, 0, fmt.Errorf("failed to get latest and latest finalized block from HeadTracker: %w", err)
}

lp.lggr.Debugw("Latest blocks read from chain", "latest", latest.Number, "finalized", finalized.BlockNumber())
return latest, finalized.BlockNumber(), nil
finalizedBN := finalized.BlockNumber()
// This is a dirty trick that allows LogPoller to function properly in tests where chain needs significant time to
// reach finality depth. An alternative to this one-liner is a database migration that drops restriction
// LogPollerBlock.FinalizedBlockNumber > 0 (which we actually want to keep to spot cases when FinalizedBlockNumber was simply not populated)
// and refactoring of queries that assume that restriction still holds.
if finalizedBN == 0 {
finalizedBN = 1
}
lp.lggr.Debugw("Latest blocks read from chain", "latest", latest.Number, "finalized", finalizedBN)
return latest, finalizedBN, nil
Copy link
Contributor

@reductionista reductionista Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the past, we've just required all tests to make sure the chain has reached finality before running. Is there a test where this is infeasible?

If we're adding support for chains shorter than finality depth, then we should remove this comment saying it's not supported:
https://github.com/smartcontractkit/chainlink/pull/14366/files#diff-08d58e904d709d1cb1a2ba1e99cce7dfd594bbf7369f05d3b187e10316dc5babL606-L612

Also, are you sure there aren't other places where we make that assumption... does this change fix it everywhere?

Copy link
Collaborator Author

@dhaidashenko dhaidashenko Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the past, we've just required all tests to make sure the chain has reached finality before running. Is there a test where this is infeasible?

CCIP load tests do not follow this requirement. Also it's not feasible on chains with large finality depth as it might take hours for test to start generating load.

If we're adding support for chains shorter than finality depth, then we should remove this comment saying it's not supported:

Nice catch. Removed

Also, are you sure there aren't other places where we make that assumption... does this change fix it everywhere?

Yes, this seems to be the only place where LP makes assumptions on min chain length.

}

// Find the first place where our chain and their chain have the same block,
Expand Down
2 changes: 1 addition & 1 deletion core/chains/evm/logpoller/log_poller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1774,7 +1774,7 @@ func Test_PollAndSavePersistsFinalityInBlocks(t *testing.T) {
name: "setting last finalized block number to 0 if finality is too deep",
useFinalityTag: false,
finalityDepth: 20,
expectedFinalizedBlock: 0,
expectedFinalizedBlock: 1,
},
{
name: "using finality from chain",
Expand Down
Loading