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

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

Merged
merged 5 commits into from
Sep 23, 2024

Conversation

dhaidashenko
Copy link
Collaborator

LogPoller aborts fetching logs and silently aborts replay if the chain’s latest finalized block is 0.

This causes CCIP Load tests to fail, as Jobs, created before finality is reached, are unable to find configs.

@dhaidashenko dhaidashenko changed the title LP support chains that have not reached finality yet BCFR-888 LP support chains that have not reached finality yet Sep 6, 2024
@dhaidashenko dhaidashenko marked this pull request as ready for review September 6, 2024 17:13
@dhaidashenko dhaidashenko requested review from a team as code owners September 6, 2024 17:13
@dhaidashenko dhaidashenko requested review from ilija42 and removed request for a team September 6, 2024 17:13
DylanTinianov
DylanTinianov previously approved these changes Sep 6, 2024
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

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

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.

@dhaidashenko dhaidashenko requested a review from a team as a code owner September 11, 2024 18:26
@@ -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

@dimriou dimriou added this pull request to the merge queue Sep 23, 2024
Merged via the queue into develop with commit 27d5cbf Sep 23, 2024
152 of 153 checks passed
@dimriou dimriou deleted the feature/BCFR-888-logpoller-suport-yong-chains branch September 23, 2024 11:09
momentmaker added a commit that referenced this pull request Sep 24, 2024
…develop

* origin/develop: (233 commits)
  update Smoke Test TestAutomationBasic to load pre-deployed contracts if given (#14537)
  CCIP-2881  USDC Reader integration tests  (#14516)
  [TT-1624] link PR to solidity review issue (#14521)
  Fix skipped notification in E2E test workflow (#14538)
  Add regression testing for pruning bug (#14454)
  Bump owner dep (#14531)
  Fix state view (#14532)
  Deployment tooling (#14533)
  CCIP 3388 - add offramp generation (#14526)
  CCIP-3416 paginate token admin registry get all tokens call (#14514)
  Change Polygon zkEVM to use FeeHistory estimator (#14161)
  [TT-1747] fix core changeset (#14524)
  TT-1459 Use CTF actions from .github repo (#14522)
  [TT-1693] try more universal Solidity scripts (#14436)
  Remove unused workflow for e2e tests (#14520)
  BCI-3668 Optimise HeadTracker's memory usage (#14130)
  BCFR-888 LP support chains that have not reached finality yet (#14366)
  support new heads polling over http rpc client (#14373)
  Bump CTF (#14518)
  TT-1550 Migrate remaining E2E workflows to the reusable workflow (#14403)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants