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

Jump to bottom

Change Workflow Registry Syncer to use less block confirmations #16243

Closed
wants to merge 3 commits into from
Closed

Change Workflow Registry Syncer to use less block confirmations #16243

wants to merge 3 commits into from

Conversation

justinkaseman
Copy link
Contributor

@justinkaseman justinkaseman commented Feb 5, 2025 •
edited
Loading

Description

Speeds up the time it takes for a workflow to land on the nodes by watching for Unconfirmed transactions instead of Finalized. lowering the block confirmations to 3.

@justinkaseman justinkaseman requested a review from a team February 5, 2025 19:47
@justinkaseman justinkaseman requested a review from a team as a code owner February 5, 2025 19:47
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 5, 2025

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

  • #added For any new functionality added.
  • #breaking_change For any functionality that requires manual action for the node to boot.
  • #bugfix For bug fixes.
  • #changed For any change to the existing functionality.
  • #db_update For any feature that introduces updates to database schema.
  • #deprecation_notice For any upcoming deprecation functionality.
  • #internal For changesets that need to be excluded from the final changelog.
  • #nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
  • #removed For any functionality/config that is removed.
  • #updated For any functionality that is updated.
  • #wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

🎖️ No JIRA issue number found in: PR title, commit message, or branch name. Please include the issue ID in one of these.

@justinkaseman justinkaseman force-pushed the jk/syncer-confidence-level branch from 46f49bc to d401f78 Compare February 5, 2025 19:52
@justinkaseman justinkaseman requested review from cedric-cordenier and a team February 5, 2025 19:52
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 5, 2025 •
edited
Loading

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@justinkaseman justinkaseman changed the title Change Workflow Registry Syncer to watch for Unconfirmed transactions Change Workflow Registry Syncer to use less block confirmations Feb 6, 2025
@justinkaseman justinkaseman force-pushed the jk/syncer-confidence-level branch from 3dee282 to 1c638ad Compare February 6, 2025 23:48
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

reductionista
reductionista reviewed Feb 7, 2025
View reviewed changes
core/services/workflows/syncer/workflow_registry.go
},
string(WorkflowUpdatedEvent): {
ChainSpecificName: string(WorkflowUpdatedEvent),
ReadType: evmtypes.Event,
ConfidenceConfirmations: map[string]int{
"1.0": 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, you'd set a more realistic confidence level for the chain, like "0.9": 3 and pass that instead of Finalized to GetLatestValue , QueryKey, etc. and reserve 1.0 for meaning actually finalized. But if you're sure that nothing else that shares this config will ever need a higher confidence level, this works.

Copy link
Contributor

Choose a reason for hiding this comment

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

@EasterTheBunny would know better than I though whether there is any problem with doing it this way

@justinkaseman
Copy link
Contributor Author

Handled via node config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reductionista reductionista reductionista approved these changes

@cedric-cordenier cedric-cordenier Awaiting requested review from cedric-cordenier

@EasterTheBunny EasterTheBunny Awaiting requested review from EasterTheBunny

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@justinkaseman @reductionista @cedric-cordenier