Update TxStore to use parent context created at initialization #10735

amit-momin · 2023-09-20T17:21:58Z

BCI-1585

Updated TxStore methods to use both the parent context created at initialization and (if available) the context passed in as a parameter
Removed pg.Opts from TxStore methods
Updated relevant methods to use a dedicated context parameter where one was previously passed in through pg.Opts
Updated upstream callers of these methods including services external to the TXM

github-actions · 2023-09-20T17:22:24Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

github-actions · 2023-09-20T17:22:25Z

I see that you haven't updated any README files. Would it make sense to do so?

samsondav

Why are we removing this?

amit-momin · 2023-09-20T19:02:01Z

Why are we removing this?

We want TXM to solely be in charge of the TxStore lifecycle so we wanted to move all of the methods to use a parent context created at TxStore initialization. This is also part of the work to untangle the TXM from external components so we didn't want to expose pg.Opts anymore on TxStore methods.

jmank88 · 2023-09-20T21:48:17Z

Why are we removing this?

We want TXM to solely be in charge of the TxStore lifecycle so we wanted to move all of the methods to use a parent context created at TxStore initialization. This is also part of the work to untangle the TXM from external components so we didn't want to expose pg.Opts anymore on TxStore methods.

Removing pg.Opts from the exported API makes sense, but why remove context.Context rather than use it in the traditional way? I'm concerned about upstream callers depending on the context behavior in subtle ways - switching from non-blocking to blocking is a major change.

prashantkumar1982 · 2023-09-22T14:10:02Z

Why are we removing this?

We want TXM to solely be in charge of the TxStore lifecycle so we wanted to move all of the methods to use a parent context created at TxStore initialization. This is also part of the work to untangle the TXM from external components so we didn't want to expose pg.Opts anymore on TxStore methods.

Removing pg.Opts from the exported API makes sense, but why remove context.Context rather than use it in the traditional way? I'm concerned about upstream callers depending on the context behavior in subtle ways - switching from non-blocking to blocking is a major change.

Yes, we should actually use Context in all places that have origin from an external caller.
For example, every downstream dependency of CreateTransaction() should use the Context that's coming from the caller.
Similarly, the ec.ProcessHead() is already passing us a Context, and we se should use it everywhere that we end up calling.

Only those functions which originate from within the TXM's poller threads can ignore passing in Context. But they still need to use the Context stored in the TxStore.

The idea is that if an external caller's call is stuck in a DB operation, and the external caller wants to cancel it, we should be able to cancel that call.
For other calls that are within TXM's internal lifecycle, they should use TxStore's member Context, so if TxStore is closing, all these calls can quickly return.

prashantkumar1982 · 2023-09-22T14:00:34Z

common/txmgr/txmgr.go

 	if err != nil {
 		return tx, errors.Wrap(err, "Txm#CreateTransaction")
 	}

-	tx, err = b.txStore.CreateTransaction(txRequest, b.chainID, qs...)
+	tx, err = b.txStore.CreateTransaction(txRequest, b.chainID)


Since this call is ultimately coming from an external caller, we should take a param ctx from external caller, and pass the ctx all the way to the evmTxStore.CreateTx(), and in to the DB call.
Thus, if the caller wants to abandon their call, they should be able to.

prashantkumar1982 · 2023-09-22T14:03:32Z

common/txmgr/types/tx_store.go

@@ -96,7 +95,7 @@ type TxHistoryReaper[CHAIN_ID types.ID] interface {
 }

 type UnstartedTxQueuePruner interface {
-	PruneUnstartedTxQueue(queueSize uint32, subject uuid.UUID, qopts ...pg.QOpt) (n int64, err error)
+	PruneUnstartedTxQueue(queueSize uint32, subject uuid.UUID, queryTimeout time.Duration) (n int64, err error)


This should also take ctx, which is coming from the external caller.

prashantkumar1982 · 2023-09-22T14:05:07Z

core/chains/evm/txmgr/evm_tx_store.go

+}
+
+// Only to be used directly for atomic transactions internal to the tx store
+func (o *evmTxStore) PreloadTxesAtomic(attempts []TxAttempt, qopts ...pg.QOpt) error {


If this is internal, please rename to preloadTxesAtomic. No need to export this function

amit-momin · 2023-09-22T15:39:35Z

Why are we removing this?

We want TXM to solely be in charge of the TxStore lifecycle so we wanted to move all of the methods to use a parent context created at TxStore initialization. This is also part of the work to untangle the TXM from external components so we didn't want to expose pg.Opts anymore on TxStore methods.

Removing pg.Opts from the exported API makes sense, but why remove context.Context rather than use it in the traditional way? I'm concerned about upstream callers depending on the context behavior in subtle ways - switching from non-blocking to blocking is a major change.

Yes, we should actually use Context in all places that have origin from an external caller. For example, every downstream dependency of CreateTransaction() should use the Context that's coming from the caller. Similarly, the ec.ProcessHead() is already passing us a Context, and we se should use it everywhere that we end up calling.

Only those functions which originate from within the TXM's poller threads can ignore passing in Context. But they still need to use the Context stored in the TxStore.

The idea is that if an external caller's call is stuck in a DB operation, and the external caller wants to cancel it, we should be able to cancel that call. For other calls that are within TXM's internal lifecycle, they should use TxStore's member Context, so if TxStore is closing, all these calls can quickly return.

I see so I should be using both the context that's passed and the one created during TxStore's initialization. @jmank88 would you have any examples where we have the design pattern to chain multiple contexts?

jmank88 · 2023-09-22T16:31:36Z

Why are we removing this?

We want TXM to solely be in charge of the TxStore lifecycle so we wanted to move all of the methods to use a parent context created at TxStore initialization. This is also part of the work to untangle the TXM from external components so we didn't want to expose pg.Opts anymore on TxStore methods.

Removing pg.Opts from the exported API makes sense, but why remove context.Context rather than use it in the traditional way? I'm concerned about upstream callers depending on the context behavior in subtle ways - switching from non-blocking to blocking is a major change.

Yes, we should actually use Context in all places that have origin from an external caller. For example, every downstream dependency of CreateTransaction() should use the Context that's coming from the caller. Similarly, the ec.ProcessHead() is already passing us a Context, and we se should use it everywhere that we end up calling.
Only those functions which originate from within the TXM's poller threads can ignore passing in Context. But they still need to use the Context stored in the TxStore.
The idea is that if an external caller's call is stuck in a DB operation, and the external caller wants to cancel it, we should be able to cancel that call. For other calls that are within TXM's internal lifecycle, they should use TxStore's member Context, so if TxStore is closing, all these calls can quickly return.

I see so I should be using both the context that's passed and the one created during TxStore's initialization. @jmank88 would you have any examples where we have the design pattern to chain multiple contexts?

Normally contexts are not meant to be stored, and they should only branch, never merge. However, if we are stuck with legacy code, then we can use a new 1.21 feature context.AfterFunc to have one cancel the other.

core/services/vrf/v2/listener_v2.go

makramkd · 2023-09-23T13:48:46Z

core/services/vrf/v2/listener_v2.go

@@ -917,7 +917,7 @@ func (lsn *listenerV2) enqueueForceFulfillment(
 		requestID := common.BytesToHash(p.req.req.RequestID().Bytes())
 		subID := p.req.req.SubID()
 		requestTxHash := p.req.req.Raw().TxHash
-		etx, err = lsn.txm.CreateTransaction(txmgr.TxRequest{
+		etx, err = lsn.txm.CreateTransaction(context.TODO(), txmgr.TxRequest{


This doesn't seem quite right. If this context is actually used we should probably pass in a non-test object.

common/txmgr/strategies.go

common/txmgr/txmgr.go

core/services/vrf/v2/listener_v2.go

core/services/vrf/v2/listener_v2_types.go

prashantkumar1982 · 2023-09-26T06:07:22Z

common/txmgr/types/tx_store.go

 	SaveConfirmedMissingReceiptAttempt(ctx context.Context, timeout time.Duration, attempt *TxAttempt[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE], broadcastAt time.Time) error
 	SaveInProgressAttempt(attempt *TxAttempt[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]) error
 	SaveInsufficientFundsAttempt(timeout time.Duration, attempt *TxAttempt[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE], broadcastAt time.Time) error
-	SaveReplacementInProgressAttempt(oldAttempt TxAttempt[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE], replacementAttempt *TxAttempt[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE], qopts ...pg.QOpt) error
+	SaveReplacementInProgressAttempt(oldAttempt TxAttempt[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE], replacementAttempt *TxAttempt[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]) error
 	SaveSentAttempt(timeout time.Duration, attempt *TxAttempt[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE], broadcastAt time.Time) error
 	SetBroadcastBeforeBlockNum(blockNum int64, chainID CHAIN_ID) error


This one too originates from ec.ProcessHead(), where we get an external context.
I guess there are more such functions from the above.

I ended up leaving the methods that weren't passed a context upstream through pg.Opts alone but I can wire them up as well

I've added a context parameter to the rest of the TxStore methods and wired them up to contexts upstream except for UpdateTxAttemptInProgressToBroadcast which will be handled in this PR

prashantkumar1982

Looks good to me, except the few places in TxStore interface that still are missing ctx, and are called from upstream callers.

…and TxStore contexts

makramkd

VRF+BHS changes LGTM

cl-sonarqube-production · 2023-10-04T07:38:20Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

99.6% Coverage
0.4% Duplication

amit-momin marked this pull request as ready for review September 20, 2023 17:50

amit-momin requested review from jkongie, samsondav, prashantkumar1982 and a team as code owners September 20, 2023 17:50

samsondav reviewed Sep 20, 2023

View reviewed changes

prashantkumar1982 reviewed Sep 22, 2023

View reviewed changes

amit-momin force-pushed the txm/BCI-1585 branch from 551032d to e609551 Compare September 22, 2023 21:59

amit-momin requested review from a team, connorwstein, PiotrTrzpil and jmank88 as code owners September 22, 2023 21:59

amit-momin force-pushed the txm/BCI-1585 branch from e609551 to 550c21c Compare September 22, 2023 22:04

jmank88 reviewed Sep 22, 2023

View reviewed changes

core/services/vrf/v2/listener_v2.go Outdated Show resolved Hide resolved

makramkd reviewed Sep 23, 2023

View reviewed changes

amit-momin force-pushed the txm/BCI-1585 branch from e1151fa to 5dc32ed Compare September 25, 2023 15:12

jmank88 reviewed Sep 25, 2023

View reviewed changes

common/txmgr/strategies.go Outdated Show resolved Hide resolved

jmank88 reviewed Sep 25, 2023

View reviewed changes

common/txmgr/txmgr.go Outdated Show resolved Hide resolved

jmank88 reviewed Sep 25, 2023

View reviewed changes

core/services/vrf/v2/listener_v2.go Outdated Show resolved Hide resolved

jmank88 reviewed Sep 25, 2023

View reviewed changes

core/services/vrf/v2/listener_v2_types.go Outdated Show resolved Hide resolved

amit-momin force-pushed the txm/BCI-1585 branch 2 times, most recently from 29c5398 to fe3f8c6 Compare September 25, 2023 17:53

prashantkumar1982 reviewed Sep 26, 2023

View reviewed changes

amit-momin added 4 commits September 26, 2023 12:43

Removed pg.Opts and updated TxStore methods to use both the provided …

855a34e

…and TxStore contexts

Updated upstream contexts

61e6bc3

Updated TxStore to maintain one context in scope at a time

510d34d

Updated Fluxmonitor context usage

cd7bde3

amit-momin force-pushed the txm/BCI-1585 branch 2 times, most recently from 32c5e45 to 96e95c8 Compare September 26, 2023 19:09

Wired remaining TxStore methods to upstream contexts

cb164f6

amit-momin force-pushed the txm/BCI-1585 branch from 96e95c8 to cb164f6 Compare September 26, 2023 19:43

Merge branch 'develop' into txm/BCI-1585

7e7d086

amit-momin requested a review from prashantkumar1982 September 28, 2023 15:41

makramkd approved these changes Oct 2, 2023

View reviewed changes

prashantkumar1982 approved these changes Oct 2, 2023

View reviewed changes

Merge branch 'develop' into txm/BCI-1585

c2919ae

amit-momin mentioned this pull request Oct 2, 2023

Move next nonce logic from Keystore to Broadcaster #10108

Merged

amit-momin added 2 commits October 3, 2023 09:31

Merge branch 'develop' into txm/BCI-1585

97ca72e

Merge branch 'develop' into txm/BCI-1585

8d9fa20

prashantkumar1982 added this pull request to the merge queue Oct 4, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 4, 2023

prashantkumar1982 added this pull request to the merge queue Oct 4, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 4, 2023

Merge branch 'develop' into txm/BCI-1585

07fb236

prashantkumar1982 enabled auto-merge October 4, 2023 06:58

prashantkumar1982 added this pull request to the merge queue Oct 4, 2023

Merged via the queue into develop with commit 61ccf8b Oct 4, 2023
83 checks passed

prashantkumar1982 deleted the txm/BCI-1585 branch October 4, 2023 07:56

snehaagni mentioned this pull request Nov 14, 2023

release/2.7.0 -> master #11290

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update TxStore to use parent context created at initialization #10735

Update TxStore to use parent context created at initialization #10735

amit-momin commented Sep 20, 2023 •

edited by jira bot

Loading

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

samsondav left a comment

amit-momin commented Sep 20, 2023

jmank88 commented Sep 20, 2023

prashantkumar1982 commented Sep 22, 2023

prashantkumar1982 Sep 22, 2023

prashantkumar1982 Sep 22, 2023

prashantkumar1982 Sep 22, 2023

amit-momin commented Sep 22, 2023

jmank88 commented Sep 22, 2023

makramkd Sep 23, 2023

prashantkumar1982 Sep 26, 2023

amit-momin Sep 26, 2023

amit-momin Sep 26, 2023

prashantkumar1982 left a comment

makramkd left a comment

cl-sonarqube-production bot commented Oct 4, 2023

Update TxStore to use parent context created at initialization #10735

Update TxStore to use parent context created at initialization #10735

Conversation

amit-momin commented Sep 20, 2023 • edited by jira bot Loading

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

samsondav left a comment

Choose a reason for hiding this comment

amit-momin commented Sep 20, 2023

jmank88 commented Sep 20, 2023

prashantkumar1982 commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amit-momin commented Sep 22, 2023

jmank88 commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashantkumar1982 left a comment

Choose a reason for hiding this comment

makramkd left a comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Oct 4, 2023

amit-momin commented Sep 20, 2023 •

edited by jira bot

Loading