-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
base: develop
Are you sure you want to change the base?
Conversation
common/txmgr/types/tx.go
Outdated
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@poopoothegorilla Amit created one https://smartcontract-it.atlassian.net/browse/BCI-3962
There was a problem hiding this comment.
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.
MinConfirmations clnull.Uint32 // deprecated | |
// Deprecated | |
MinConfirmations clnull.Uint32 |
Handing off WIP to @huangzhen1997 |
Need to fix integration test |
@@ -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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this changed?
There was a problem hiding this comment.
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')
…BCI-3730-remove-minconfirmations-evm-txm
Quality Gate passedIssues Measures |
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 ofMinConfirmations
.Acceptance Criteria
MinConfirmations
should be removed to enable the use of finality tags.MinConfirmations
from the EVM TXM codeTickets:
Relates to:
Unblocks: