Initial work to add an in-memory layer on top of the TxStore #11190

poopoothegorilla · 2023-11-06T20:14:23Z

This PR is meant to start a discussion on a proposed design of the In-Memory Storage.

Some key points

InMemoryStore wraps the existing TxStore and is designed to use all the same method signatures as the existing TxStore.
The InMemoryStore's goal is to reduce the amount of reads to the TxStore for read only operations.
All methods that require Create, Update, and Delete actions will forward the call to the underlying TxStore and then apply the changes to the in memory state after success.
The test is a proposal of how we can ensure parity with the existing TxStore functionality.
The main goal of the InMemoryStore is to reduce entanglement with the postgres DB. It is part of a larger goal to make creating integrations faster and easier for developers.

github-actions · 2023-11-06T20:14:46Z

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

common/txmgr/inmemory_store.go

…age mechanics for address state

dhaidashenko

The main goal of the InMemoryStore is to reduce entanglement with the postgres DB. It is part of a larger goal to make creating integrations faster and easier for developers.

This seems to me more like performance optimisation, than simplification of integration process or reduction of entanglement with postgres. It definitely makes TxStore more complex. It would be great to measure if performance improvements are actually sufficient to pay this price.

If we are sure that potential of performance gain here is significant. We might want to considered changing the paradigm from using some form of key store that is shared between different runners (broadcaster, confirmer, etc) to using channels to transfer responsibility over the tx.
Example: confirmer instead of pooling storage for transactions to check, receives them from broadcaster via channel.
This requires more refactoring to TxM compared to in memory store, but makes it more modular and extandable.

dhaidashenko · 2023-11-17T15:56:45Z

common/txmgr/address_state.go

+	offset := 0
+	limit := 50
+	for {
+		txs, count, err := txStore.UnstartedTransactions(offset, limit, as.fromAddress, as.chainID)


Is there guarantee that two different instances of core won't run simultaneously on top of single DB? Example:(during maintenance or update of the node)

no i dont think there is. what scenarios are you thinking of?

In theory we might have a node that persists new tx after our node fetched them

dhaidashenko · 2023-11-17T16:00:17Z

common/txmgr/address_state.go

+		offset += limit
+	}
+
+	return nil


We also need to keep confirmed txs in memory to guarantee idempotent behaviour

that is true. i omitted it just to keep the PR from being too large

dhaidashenko · 2023-11-17T16:05:31Z

common/txmgr/address_state.go

+
+	var maxSeq SEQ
+	if as.inprogress != nil && as.inprogress.Sequence != nil {
+		if (*as.inprogress.Sequence).Int64() > maxSeq.Int64() {


Probably out of score for this PR, but should we use comparable restriction for SEQ instead of relaying on evm specific property Int64.

I would say yes lets add that in

dhaidashenko · 2023-11-17T16:11:02Z

common/txmgr/address_state.go

+		return nil, fmt.Errorf("peek_in_progress_tx: %w (address: %s)", ErrTxnNotFound, as.fromAddress)
+	}
+
+	return tx, nil


Sharing pointer to tx feels extremely unsafe, should we implement marshal/unmarshal when receiving/returning instance.

I agree with this 💯 . We should think of ways to do this safely

dhaidashenko · 2023-11-17T16:22:55Z

common/txmgr/address_state.go

+		return fmt.Errorf("move_unstarted_to_in_progress: address %s already has a transaction in progress", as.fromAddress)
+	}
+
+	if tx != nil {


in which cases caller knowns better which tx to move?

could you reword this statement? I am not sure i understand

moveUnstartedToInProgress accepts tx as argument, but allows it to be nil. In that case we get tx from the priority queue. In which cases caller of the moveUnstartedToInProgress might want to use their own value of tx instead of one from the queue?

my reasoning for doing this was to minimize potential conflicts with the persistent store and the inmemory state. so in this case the caller gets the next unstarted tx from the db. I think we should move from this in the future though... it might make sense to exclude it until we are ready to use it though

dhaidashenko · 2023-11-17T16:33:35Z

common/txmgr/broadcaster.go

@@ -695,7 +695,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, sql.ErrNoRows) || errors.Is(err, ErrTxnNotFound) {


It feels weird to force users of txStore to check for two different error, that actually mean the same. Won't it be easier to return sql.ErrNoRows from in memory storage

I want to get rid of the reliance on sql.ErrNoRows so this is the first step. relying on it forces us to be dependent on the sql pkg... i think we should use our own error types for it so we can make this less dependent

dhaidashenko · 2023-11-17T16:36:10Z

common/txmgr/inmemory_store.go

+	maxUnstarted := 50
+	addresses, err := keyStore.EnabledAddressesForChain(chainID)
+	if err != nil {
+		return nil, fmt.Errorf("new_in_memory_store: %w", err)


IMHO wrapping should add context that is not know to caller. Should not we specify here that we failed to check if address is enabled?

sounds reasonable we can add more context to the errors

common/txmgr/address_state.go

dhaidashenko · 2023-11-17T16:43:31Z

common/txmgr/inmemory_store.go

+	if ms.chainID.String() != chainID.String() {
+		return tx, fmt.Errorf("create_transaction: %w", ErrInvalidChainID)
+	}
+	if _, ok := ms.addressStates[txRequest.FromAddress]; !ok {


are we sure that it's not possible to add new keys on the fly?

good find will add the missing locks

common/txmgr/inmemory_store.go

cl-sonarqube-production · 2024-01-05T14:46:06Z

SonarQube Quality Gate

Reliability Rating on New Code (is worse than A)
0.5% Coverage on New Code (is less than 75%)

See analysis details on SonarQube

Fix issues before they fail your Quality Gate with SonarLint in your IDE.

DylanTinianov · 2024-01-17T18:33:06Z

common/txmgr/address_state.go

+
+	// Load all unconfirmed transactions from persistent storage
+	offset = 0
+	limit = 50


Nit: Could make the load limit a const. Would be easier to change in a single place if needed.

DylanTinianov · 2024-01-17T19:02:13Z

common/txmgr/tx_priority_queue.go

+func (pq *TxPriorityQueue[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) Push(tx any) {
+	pq.txs = append(pq.txs, tx.(*txmgrtypes.Tx[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, SEQ, FEE]))
+}
+func (pq *TxPriorityQueue[CHAIN_ID, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) Pop() any {


Is this different from RemoveNextTx()? Can you add a comment explaining this, feels like we might not need both?

DylanTinianov

Great work :)

poopoothegorilla added 7 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

poopoothegorilla requested a review from prashantkumar1982 November 6, 2023 20:14

poopoothegorilla added 4 commits November 6, 2023 19:58

some clean up

faf670f

add check for inprogress txn

178b0f0

make some changes to testing

fe25a26

reorganize tests

a0ac2a3

poopoothegorilla commented Nov 7, 2023

View reviewed changes

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

poopoothegorilla added 2 commits November 8, 2023 18:56

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

a28aa80

…age mechanics for address state

add initialization to address state

e2f3a87

poopoothegorilla requested review from patrick-dowell and amit-momin November 14, 2023 05:37

poopoothegorilla added 5 commits November 14, 2023 00:43

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

dhaidashenko reviewed Nov 17, 2023

View reviewed changes

poopoothegorilla added 6 commits December 11, 2023 10:56

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

poopoothegorilla marked this pull request as ready for review December 12, 2023 03:48

poopoothegorilla requested review from samsondav and a team as code owners December 12, 2023 03:48

poopoothegorilla added 7 commits December 12, 2023 08:42

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

remove testing since this will be in future work

325fb12

DylanTinianov reviewed Jan 17, 2024

View reviewed changes

DylanTinianov approved these changes Jan 18, 2024

View reviewed changes

poopoothegorilla closed this Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial work to add an in-memory layer on top of the TxStore #11190

Initial work to add an in-memory layer on top of the TxStore #11190

poopoothegorilla commented Nov 6, 2023 •

edited

Loading

github-actions bot commented Nov 6, 2023

dhaidashenko left a comment

dhaidashenko Nov 17, 2023

poopoothegorilla Nov 17, 2023

dhaidashenko Nov 17, 2023

dhaidashenko Nov 17, 2023

poopoothegorilla Nov 17, 2023

dhaidashenko Nov 17, 2023

poopoothegorilla Nov 17, 2023 •

edited

Loading

dhaidashenko Nov 17, 2023

poopoothegorilla Nov 17, 2023

dhaidashenko Nov 17, 2023

poopoothegorilla Nov 17, 2023

dhaidashenko Nov 17, 2023

poopoothegorilla Nov 17, 2023

dhaidashenko Nov 17, 2023

poopoothegorilla Nov 17, 2023

dhaidashenko Nov 17, 2023

poopoothegorilla Nov 17, 2023

dhaidashenko Nov 17, 2023

poopoothegorilla Nov 17, 2023

cl-sonarqube-production bot commented Jan 5, 2024

DylanTinianov Jan 17, 2024 •

edited

Loading

DylanTinianov Jan 17, 2024

DylanTinianov left a comment

Initial work to add an in-memory layer on top of the TxStore #11190

Initial work to add an in-memory layer on top of the TxStore #11190

Conversation

poopoothegorilla commented Nov 6, 2023 • edited Loading

github-actions bot commented Nov 6, 2023

dhaidashenko 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

poopoothegorilla Nov 17, 2023 • edited Loading

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 Jan 5, 2024

DylanTinianov Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DylanTinianov left a comment

Choose a reason for hiding this comment

poopoothegorilla commented Nov 6, 2023 •

edited

Loading

poopoothegorilla Nov 17, 2023 •

edited

Loading

DylanTinianov Jan 17, 2024 •

edited

Loading