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

[BCI-3730] - Remove dependence on MinConfirmations in EVM TXM code #13973

Open
wants to merge 59 commits into
base: develop
Choose a base branch
from

Conversation

Farber98
Copy link
Contributor

Description

Parts of the EVM TXM code still relies on MinConfirmations regardless if the chain is enabled to use finality tags. Through this PR we are updating the code to utilize the head tracker’s latest finalized block instead of MinConfirmations.

Acceptance Criteria

  • All direct references to MinConfirmations should be removed to enable the use of finality tags.
  • Remove all instances of MinConfirmations from the EVM TXM code

Tickets:

Relates to:

Unblocks:

core/cmd/shell_local.go Outdated Show resolved Hide resolved
@@ -83,7 +83,7 @@ type TxRequest[ADDR types.Hashable, TX_HASH types.Hashable] struct {

// Pipeline variables - if you aren't calling this from chain tx task within
// the pipeline, you don't need these variables
MinConfirmations clnull.Uint32
MinConfirmations clnull.Uint32 // deprecated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just marking as deprecate until other teams stop using the field. See this eg. call

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a ticket or action item for another team to eventually remove this dependency? if there is none lets try to make sure one exists and maybe link it as well in the deprecated comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should not we respect the param if it was passed as part of task (job?) definition and only fallback to finality tags if was not specified?

Otherwise we are introducing changes that might affect UX (e.g. someone specified 1 as min number of confirmation, but we are waiting for 200 blocks)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@jmank88 jmank88 Aug 8, 2024

Choose a reason for hiding this comment

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

IIUC this format will be more widely recognized by tooling like linters, IDEs, etc.

Suggested change
MinConfirmations clnull.Uint32 // deprecated
// Deprecated
MinConfirmations clnull.Uint32

https://go.dev/wiki/Deprecated

@Farber98
Copy link
Contributor Author

Handing off WIP to @huangzhen1997

@huangzhen1997
Copy link
Contributor

Need to fix integration test TestIntegration_AsyncEthTx

jmank88
jmank88 previously approved these changes Aug 9, 2024
@@ -216,7 +217,6 @@ func (db *DbEthTx) FromTx(tx *Tx) {
db.Meta = tx.Meta
db.Subject = tx.Subject
db.PipelineTaskRunID = tx.PipelineTaskRunID
db.MinConfirmations = tx.MinConfirmations
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we either need to drop these fields if they are unused, or continue to set them from old data if they are still used. Having them around but unset will just be misleading.

…:smartcontractkit/chainlink into BCI-3730-remove-minconfirmations-evm-txm
@@ -531,7 +536,9 @@ observationSource = """
assert.Equal(t, []*string(nil), run.Errors)

testutils.WaitForLogMessage(t, o, "Sending transaction")
b.Commit() // Needs at least two confirmations
b.Commit() // Needs at least two confirmations
time.Sleep(10 * time.Millisecond) // wait for finalizer to process confirmed tx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is Sleep our only option here?

Copy link
Contributor

@huangzhen1997 huangzhen1997 Sep 3, 2024

Choose a reason for hiding this comment

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

I have to double check. I don't know what exactly cause the race condition, sleeping here somehow allow confirmer to execute processHead before finalizer

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate? Sleeping is not a fix for a true data race.

Copy link
Contributor

@huangzhen1997 huangzhen1997 Sep 3, 2024

Choose a reason for hiding this comment

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

I will double check why did sleep "fix" it.

The issue, from my observation, was a race condition, as finalizer and confirmer are both running in parallel processing new head from mailbox. If the confirmer gets runs first, everything is good. However, if finalizer process (processFinalizedHead) the new head before confirmer, the finalizer won't mark tx to be finalized successfully, since confirmer has not yet save the receipt. Later causing confirmer not able to pickup pending callback txs (needs to be finalized state) in ResumePendingTaskRuns

Comment on lines +3053 to +3056
txs, err3 := txStore.FindTxesByFromAddressAndNonce(tests.Context(t), fromAddress, int64(nonce))
assert.Nil(t, err3)
assert.Equal(t, 1, len(txs))
assert.Equal(t, true, txs[0].CallbackCompleted)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this changed?

Copy link
Contributor

@huangzhen1997 huangzhen1997 Sep 3, 2024

Choose a reason for hiding this comment

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

Due to the new logic in confirmer, we only fetch finalized tx for callback ,FindTxWithSequence doesn't work anymore because it returns only the state IN ('confirmed', 'confirmed_missing_receipt', 'unconfirmed')

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.

7 participants