Jtw/initial in memory work confirmer #11625

poopoothegorilla · 2023-12-19T17:17:26Z

WIP

…age mechanics for address state

amit-momin · 2024-02-10T00:39:11Z

common/txmgr/inmemory_store.go

+	if as.inprogress != nil {
+		return fmt.Errorf("find_next_unstarted_transaction_from_address: address %s is already busy with a transaction in progress", fromAddress)
+	}


Since this is just a Find function, I don't think we need validations like this in the memory store. I feel like the responsibility to check this should be with the Broadcaster

great point!

amit-momin · 2024-02-12T20:09:07Z

common/txmgr/inmemory_store.go

+	// sort by tx_id ASC, gas_price DESC, gas_tip_cap DESC
+	sort.SliceStable(attempts, func(i, j int) bool {
+		return attempts[i].TxID < attempts[j].TxID
+	})


Looks we only sort by tx ID here and don't include gas price and gas tip cap.

Yes, can we add them here for now.
Later on, we need to look for different design where the generic TXM doesn't sort these. Instead the chain specific logic should do any sorting by chain-specific params like gas price etc.

amit-momin · 2024-02-12T20:21:05Z

common/txmgr/inmemory_store.go

+		wg.Add(1)
+		go func(as *AddressState[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) {
+			for _, txID := range txIDs {
+				if err := as.MoveConfirmedMissingReceiptToUnconfirmed(txID); err != nil {


I think this method is assuming some logic from the method is function is called from. I noticed UpdateTxsUnconfirmed is currently only called in the context of moving txs from TxConfirmedMissingReceipt to TxUnconfirmed but the memory store function to mark tx as unconfirmed only cares about txIDs. To maintain existing behavior, think we should apply this change across all tx states as long as their IDs match.

Ahh, so the caller here just want to move from ConfirmedMissingReceipt --> Unconfirmed.
However the underlying method is generic unnecessarily.

I would prefer to just rename the TxStore.UpdateTxsUnconfirmed() method to TxStore. MoveConfirmedMissingReceiptToUnconfirmed().

Let the actual SQL query be untouched, but if nobody wants a generic enough implementation, lets not worry about adding it here. The code logic here seems sufficient to only look inside this state.

amit-momin · 2024-02-12T20:33:12Z

common/txmgr/inmemory_store.go

+		if tx.TxAttempts != nil && len(tx.TxAttempts) > 0 {
+			attempt := tx.TxAttempts[0]
+			return attempt.State != txmgrtypes.TxAttemptInsufficientFunds
+		}


Couldn't a tx have attempts with different states? In which case, I think we'd have to traverse the whole attempts list for at least one to match the TxAttemptInsufficientFunds state.

good catch will add 👍

amit-momin · 2024-02-12T20:50:15Z

common/txmgr/address_state.go

+		if filter(tx) {
+			txAttempts = append(txAttempts, tx.TxAttempts...)
+		}


Might need your context on all of the methods that use this. At least for FindTxAttemptsRequiringReceiptFetch, I believe the query would only return the specific attempts whose state does not equal insufficient_eth. Looks like we're returning all of the attempts for a tx here. Maybe the filter should be on TxAttempts instead of Txs for this method so we could narrow down specific attempts to return?

great point added an addition filter for filtering attempts as well

amit-momin · 2024-02-12T20:59:01Z

common/txmgr/inmemory_store.go

+
+		if tx.PipelineTaskRunID.Valid && tx.SignalCallback && !tx.CallbackCompleted &&
+			tx.TxAttempts[0].Receipts[0].GetBlockNumber() != nil &&
+			big.NewInt(blockNum-int64(tx.MinConfirmations.Uint32)).Cmp(tx.TxAttempts[0].Receipts[0].GetBlockNumber()) > 0 {


I think we'd also want to allow these to be equal

Suggested change

big.NewInt(blockNum-int64(tx.MinConfirmations.Uint32)).Cmp(tx.TxAttempts[0].Receipts[0].GetBlockNumber()) > 0 {

big.NewInt(blockNum-int64(tx.MinConfirmations.Uint32)).Cmp(tx.TxAttempts[0].Receipts[0].GetBlockNumber()) >= 0 {

amit-momin · 2024-02-12T21:23:37Z

common/txmgr/inmemory_store.go

+		if tx.TxAttempts == nil || len(tx.TxAttempts) == 0 {
+			return false
+		}
+		attempt := tx.TxAttempts[0]


Shouldn't we traverse all of the attempts to find the one that has receipts?

Yes, any attempt could have the receipt

great catch

amit-momin · 2024-02-12T21:34:29Z

common/txmgr/inmemory_store.go

+	filterFn := func(tx *txmgrtypes.Tx[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]) bool {
+		return true
+	}


Not sure how to improve this with the current setup but feel like we shouldn't have to do this. For future contributors, I feel like having to pass a filter even when it's not needed could cause some confusion.

amit-momin · 2024-02-12T21:41:51Z

common/txmgr/inmemory_store.go

+		if tx.TxAttempts == nil || len(tx.TxAttempts) == 0 {
+			return false
+		}
+		attempt := tx.TxAttempts[0]


Another place I think we should be traversing all attempts for the receipt

amit-momin · 2024-02-12T21:46:15Z

common/txmgr/inmemory_store.go

+			return false
+		}
+
+		return tx.TxAttempts[0].ID == attempt.ID


Just adding a note to track but think we should traverse attempts here too

amit-momin · 2024-02-12T21:50:02Z

common/txmgr/inmemory_store.go

+		if tx.TxAttempts == nil || len(tx.TxAttempts) == 0 {
+			return false
+		}
+		attempt := tx.TxAttempts[0]


Just marking another attempt filter

prashantkumar1982

This is great!
Added some comments. I might do another detailed pass of inmemory_store.go later, since that is the most sensitive part.

prashantkumar1982 · 2024-02-12T18:16:43Z

common/txmgr/txmgr.go

@@ -487,7 +486,7 @@ func (b *Txm[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) CreateTran
 	if txRequest.IdempotencyKey != nil {
 		var existingTx *txmgrtypes.Tx[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]
 		existingTx, err = b.txStore.FindTxWithIdempotencyKey(ctx, *txRequest.IdempotencyKey, b.chainID)
-		if err != nil && !errors.Is(err, sql.ErrNoRows) {
+		if err != nil && !errors.Is(err, ErrTxnNotFound) {


I don't see this error type changed in this PR. Was this a bug previously?
Do we have a unit-test to cover this case? If not, can you please add it, for coverage?

was trying to remove the sql dependency slowly, but will take it out of this PR and do it in a separate attempt

prashantkumar1982 · 2024-02-12T19:02:08Z

common/txmgr/tx_priority_queue.go

+)
+
+// TxPriorityQueue is a priority queue of transactions prioritized by creation time. The oldest transaction is at the front of the queue.
+type TxPriorityQueue[


There's many functions in this, to just satisfy the heap package's use.
Seems confusing for the caller to know which method to call.

Is it possible to create a different struct for callers, and let that contain an inner struct for our inner implementation for heap package, and hide it from callers?
That will just make things easier to read and understand for the caller.

Addressed in #12121

prashantkumar1982 · 2024-02-12T19:02:59Z

common/txmgr/broadcaster.go

@@ -709,7 +709,7 @@ func (eb *Broadcaster[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]) next
 	defer cancel()
 	etx := &txmgrtypes.Tx[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]{}
 	if err := eb.txStore.FindNextUnstartedTransactionFromAddress(ctx, etx, fromAddress, eb.chainID); err != nil {
-		if errors.Is(err, sql.ErrNoRows) {
+		if errors.Is(err, ErrTxnNotFound) {


Just to be sure, please ensure there's a unit-test for this case.

this is the unit test https://github.com/smartcontractkit/chainlink/pull/11625/files#diff-e740fd5bcf523cd12057fd17ccdcf9d65101d1c247d0b4b447bd4340e5f4e890L1266 for that method.

I am just going to break this into a separate PR which removes the PG dependancy for the txm

prashantkumar1982 · 2024-02-12T21:20:34Z

common/txmgr/address_state.go

+	return txAttempts
+}
+
+func (as *AddressState[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) fetchTxAttemptsFromStorage(


nit: Why not just rename to fetchTxAttempts?
The FromStorage seems confusing

weird thought i had done that before. thanks for reminding me

prashantkumar1982 · 2024-02-12T21:23:18Z

common/txmgr/address_state.go

+		delete(as.allTransactions, txID)
+		delete(as.unconfirmed, txID)
+		delete(as.confirmedMissingReceipt, txID)
+		delete(as.allTransactions, txID)


+1
This needs to be as. unstarted

prashantkumar1982 · 2024-02-13T00:55:49Z

common/txmgr/inmemory_store.go

+		wg.Add(1)
+		go func(as *AddressState[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) {
+			for _, txID := range txIDs {
+				if err := as.MoveConfirmedMissingReceiptToUnconfirmed(txID); err != nil {


Ahh, so the caller here just want to move from ConfirmedMissingReceipt --> Unconfirmed.
However the underlying method is generic unnecessarily.

I would prefer to just rename the TxStore.UpdateTxsUnconfirmed() method to TxStore. MoveConfirmedMissingReceiptToUnconfirmed().

Let the actual SQL query be untouched, but if nobody wants a generic enough implementation, lets not worry about adding it here. The code logic here seems sufficient to only look inside this state.

prashantkumar1982 · 2024-02-13T00:57:32Z

common/txmgr/inmemory_store.go

+	wg := sync.WaitGroup{}
+	for _, as := range ms.addressStates {
+		wg.Add(1)
+		go func(as *AddressState[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) {


nit: Up to you, but just pointing out that running a sequential loop over in-memory code would also be quick enough, so as to not require multiple threads.
It is ok to leave it as it is though.

prashantkumar1982 · 2024-02-13T01:02:04Z

common/txmgr/inmemory_store.go

+	for _, as := range ms.addressStates {
+		attempts = append(attempts, as.FetchTxAttempts(states, filterFn)...)
+	}
+	// sort by sequence ASC, gas_price DESC, gas_tip_cap DESC


This needs to also sort by gas price and gas tip cap

prashantkumar1982 · 2024-02-13T01:35:19Z

common/txmgr/inmemory_store.go

+		return err
+	}
+
+	// Update in memory store


Are these changes to in-memory store same as the persistent store?
In the EvmTxStore, I see many things being done for this.
We update fields in attempt, we change Tx state,

prashantkumar1982 · 2024-02-13T01:36:49Z

common/txmgr/inmemory_store.go

+		if tx.TxAttempts == nil || len(tx.TxAttempts) == 0 {
+			return false
+		}
+		attempt := tx.TxAttempts[0]


Yes, any attempt could have the receipt

cl-sonarqube-production · 2024-02-16T22:32:47Z

Quality Gate failed

Failed conditions
4.4% Duplication on New Code (required ≤ 3%)
7 New Major Issues (required ≤ 5)
C Security Rating on New Code (required ≥ A)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

poopoothegorilla · 2024-02-27T02:04:25Z

common/txmgr/inmemory_store.go

+}
+
+// SaveReplacementInProgressAttempt saves a replacement attempt for a transaction that is in_progress.
+func (ms *InMemoryStore[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) SaveReplacementInProgressAttempt(


I think alot of this modification logic might make sense in the address store... not in this component

github-actions · 2024-04-28T00:14:11Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions · 2024-08-30T00:14:46Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

poopoothegorilla added 30 commits October 31, 2023 16:38

initial commit for inmemory implementation

c53bb4c

some cleanup

b75d260

Merge branch 'develop' into jtw/initial-in-memory-work

41a572d

fix tests

3dde323

add some functions for the in memory store

201a2dd

Merge branch 'develop' into jtw/initial-in-memory-work

0c1d6cf

implement a few more methods for the in memory store

b565d45

some clean up

faf670f

add check for inprogress txn

178b0f0

make some changes to testing

fe25a26

reorganize tests

a0ac2a3

start initialization function; add priority queue system; change stor…

a28aa80

…age mechanics for address state

add initialization to address state

e2f3a87

change loops to not use range

91bea75

clean up idempotency key location

5314284

add find latest sequence

5a908be

remove FindLatestSequence from persistent interface

9872283

add UnconfirmedTransactions to TxSoreWebApi

df5e946

Merge branch 'develop' into jtw/initial-in-memory-work

fcb9021

address comments

5d78868

fix tests

a0a1a79

fix bug in tests

89d1056

run go generate

3663bd0

Merge branch 'develop' into jtw/initial-in-memory-work

17cd10d

address comment

54b7ecc

update inmemory store to implement txstore interface

f8e0175

cleanup tests

df1a11a

Merge branch 'develop' into jtw/initial-in-memory-work

5452d02

Merge branch 'develop' into jtw/initial-in-memory-work

5f0f6da

add deepcopy

f24eea7

amit-momin reviewed Feb 10, 2024

View reviewed changes

amit-momin reviewed Feb 12, 2024

View reviewed changes

prashantkumar1982 reviewed Feb 13, 2024

View reviewed changes

poopoothegorilla added 4 commits February 14, 2024 17:55

address initial comments

1c9ec0d

fix some txAttempt loops

fb8be7c

address some comments

526981d

update address_state

a6c68c5

poopoothegorilla temporarily deployed to sdlc February 16, 2024 22:10 — with GitHub Actions Inactive

poopoothegorilla added 2 commits February 16, 2024 17:12

add back sql ErrNoRows

aae832f

add back sql ErrNoRows to evm_tx_store

43e8211

poopoothegorilla temporarily deployed to sdlc February 16, 2024 22:21 — with GitHub Actions Inactive

poopoothegorilla commented Feb 27, 2024

View reviewed changes

github-actions bot added the Stale label Apr 28, 2024

github-actions bot closed this May 6, 2024

amit-momin reopened this Jun 26, 2024

amit-momin removed the Stale label Jun 26, 2024

github-actions bot added the Stale label Aug 30, 2024

github-actions bot closed this Sep 6, 2024

	big.NewInt(blockNum-int64(tx.MinConfirmations.Uint32)).Cmp(tx.TxAttempts[0].Receipts[0].GetBlockNumber()) > 0 {
	big.NewInt(blockNum-int64(tx.MinConfirmations.Uint32)).Cmp(tx.TxAttempts[0].Receipts[0].GetBlockNumber()) >= 0 {

Jtw/initial in memory work confirmer #11625

Jtw/initial in memory work confirmer #11625

Conversation

poopoothegorilla commented Dec 19, 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

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

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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Feb 16, 2024

Quality Gate failed

Choose a reason for hiding this comment

github-actions bot commented Apr 28, 2024

github-actions bot commented Aug 30, 2024