feat: add tx status in mempool #1287

ninabarbakadze · 2024-04-05T09:23:57Z

Description

Fixes #1281

Opens #1381

rootulp

LGTM. Suggestions were mostly optional comment improvements

mempool/cache.go

mempool/cat/pool.go

mempool/v0/clist_mempool.go

mempool/v1/mempool.go

rpc/core/tx_status.go

mempool/v1/mempool.go

mempool/cat/pool.go

mempool/mock/mempool.go

rpc/core/tx_status.go

Co-authored-by: Rootul P <[email protected]>

rootulp · 2024-06-13T16:35:26Z

mempool/cat/store.go

@@ -142,14 +142,17 @@ func (s *store) getTxsBelowPriority(priority int64) ([]*wrappedTx, int64) {

 // purgeExpiredTxs removes all transactions that are older than the given height
 // and time. Returns the amount of transactions that were removed
-func (s *store) purgeExpiredTxs(expirationHeight int64, expirationAge time.Time) int {
+func (s *store) purgeExpiredTxs(expirationHeight int64, expirationAge time.Time, evictedTxCache *LRUTxCache) int {


[no change needed] it seems like purge could be renamed to evict here. IMO it's easier to understand what's happening if we use consistent names for consistent things.

In other words, it isn't immediately obvious to me that purging a transaction should add it to the evicted tx cache. However, evicting a transaction seems like it should add the tx to the evicted tx cache.

mempool/mempool.go

rootulp · 2024-06-13T16:38:20Z

mempool/mempool.go

+	// Used in the RPC endpoint: TxStatus.
+	GetTxByKey(key types.TxKey) (types.Tx, bool)
+
+	// IsTxEvicted returns true if the tx is evicted from the mempool and exists in the


nit

Suggested change

// IsTxEvicted returns true if the tx is evicted from the mempool and exists in the

// IsTxEvicted returns true if the tx was evicted from the mempool and exists in the

[question] what happens if the tx gets evicted from the mempool and then the user re-submits the exact same tx so that it exists in the tx cache? Is it possible for one tx to exist in both the tx cache and the evicted tx cache at the same time?

yes, it's possible that a previously evicted tx exists in both caches. evicted tx only gets rid of its records once it reaches its max size(v0) or once it's reset alongside all other caches(v1). I think how we can avoid confusion between re-added txs is if we check for eviction last.

I think a more correct phrasing of the method is WasRecentlyEvicted

rootulp · 2024-06-13T16:42:03Z

mempool/v0/clist_mempool.go

@@ -183,6 +183,16 @@ func (mem *CListMempool) TxsFront() *clist.CElement {
 	return mem.txs.Front()
 }

+// GetTxByKey is not supported by the v0 mempool but it is required to satisfy the mempool interface.


[blocking] I don't think we have rough consensus on deprecating the v0 mempool. See #1382.

IMO if it's easy to add, then we should add support for GetTxByKey until we make a decision on deprecating the v0 mempool. If it's difficult to add support for GetTxByKey to the v0 mempool, that should be information that we use when making the decision to deprecate or not. If that's the case, making the decision should block merging this PR.

rootulp · 2024-06-13T16:42:48Z

mempool/v0/clist_mempool.go

+// IsTxEvicted is not supported by the v0 mempool but it is required to satisfy the mempool interface.
+func (mem *CListMempool) IsTxEvicted(key types.TxKey) bool {
+	return false
+}


[blocking] I don't think we have rough consensus on deprecating the v0 mempool. See #1382.

IMO if it's easy to add, then we should add support for IsTxEvicted until we make a decision on deprecating the v0 mempool. If it's difficult to add support for IsTxEvicted to the v0 mempool, that should be information that we use when making the decision to deprecate or not. If that's the case, making the decision should block merging this PR.

if i'm understanding it correctly v0 is currently not in use. I'm not sure why we'd want to support TxStatus in v0 if it's not enabled and won't be used. We could merge this pr as long as we satisfy the interface.

I have no problem implementing those methods in v0 I just don't really understand why we should

The point I think rootul is making is even although it's not the default, it is feasible that someone use the mempool v0, thus we do support it and until we stop supporting it, features like TxStatus will need to be implemented across all implementations

mempool/v1/mempool.go

Co-authored-by: Rootul P <[email protected]>

cmwaters

This is almost done, just a few remaining remarks

cmwaters · 2024-06-15T10:59:01Z

mempool/cat/pool.go

@@ -92,6 +94,7 @@ func NewTxPool(
 		proxyAppConn:     proxyAppConn,
 		metrics:          mempool.NopMetrics(),
 		rejectedTxCache:  NewLRUTxCache(cfg.CacheSize),
+		evictedTxCache:   NewLRUTxCache(cfg.CacheSize),


Can we perhaps make this 1/5 the size of the rejectedTxCache. The default in the config is currently 10k, so I think keeping the last 2k of evicted transactions is sufficient

cmwaters · 2024-06-15T11:05:48Z

mempool/cat/store.go

@@ -142,14 +142,17 @@ func (s *store) getTxsBelowPriority(priority int64) ([]*wrappedTx, int64) {

 // purgeExpiredTxs removes all transactions that are older than the given height
 // and time. Returns the amount of transactions that were removed
-func (s *store) purgeExpiredTxs(expirationHeight int64, expirationAge time.Time) int {
+func (s *store) purgeExpiredTxs(expirationHeight int64, expirationAge time.Time, evictedTxCache *LRUTxCache) int {


I don't like the cache been passed into the store. It feels leaky. Either the caches are built into the store or this method returns an array of evicted tx keys that the mempool can then plug into the cache. I think I would prefer the latter

I returns purged txs from the store but i'm afraid that's also api breaking?

cmwaters · 2024-06-15T11:08:13Z

mempool/v0/clist_mempool.go

+// IsTxEvicted is not supported by the v0 mempool but it is required to satisfy the mempool interface.
+func (mem *CListMempool) IsTxEvicted(key types.TxKey) bool {
+	return false
+}


The point I think rootul is making is even although it's not the default, it is feasible that someone use the mempool v0, thus we do support it and until we stop supporting it, features like TxStatus will need to be implemented across all implementations

…tedTxCache size

ninabarbakadze

Added txstatus in v0 @cmwaters @rootulp

mempool/cat/store.go

rootulp · 2024-06-17T19:49:25Z

mempool/v0/clist_mempool.go

 }

-// IsTxEvicted is not supported by the v0 mempool but it is required to satisfy the mempool interface.
-func (mem *CListMempool) IsTxEvicted(key types.TxKey) bool {
+// WasRecentlyEvicted returns false consistently as this implementation does not support transaction eviction.


[question] why does this implementation not support transaction eviction?

evicted txs are valid transactions that were evicted because of size/ttl reasons for example when mempool is full. afaik Clist mempool on the other hand directly rejects transactions if the cache is full rather than evicting them.

There is also no TTL on the v0 mempool

Co-authored-by: Rootul P <[email protected]>

cmwaters

LGTM - I have one minor comment which you can choose to do as a follow up PR if you wish

cmwaters · 2024-06-18T12:45:54Z

rpc/core/tx.go

+const (
+	txStatusUnknown   string = "UNKNOWN"
+	txStatusPending   string = "PENDING"
+	txStatusEvicted   string = "EVICTED"
+	txStatusCommitted string = "COMMITTED"
+)


Why are these private, shouldn't they be public.

For example the TxClient is going to have some logic like: wait until tx status == committed

I thought that we shall export it once we need it exported

cmwaters · 2024-06-18T12:47:19Z

rpc/core/tx.go

+	txInfo := env.BlockStore.LoadTxInfo(hash)
+	if txInfo != nil {
+		return &ctypes.ResultTxStatus{Height: txInfo.Height, Index: txInfo.Index, Status: txStatusCommitted}, nil
+	}


Q: Does TxInfo save the resulting code of the transaction as well? If not we should add that as a follow up issue

no it doesn't I think there should be an issue for this otherwise i'll create one

feat: add tx status in mempool

473937e

rootulp assigned ninabarbakadze Apr 5, 2024

ninabarbakadze added 2 commits April 18, 2024 18:55

refactor: LRUTxCache to take tx key instead of tx

33355a0

feat: add the same methods to other mempools to build (wip)

d4a86e3

cmwaters mentioned this pull request Apr 23, 2024

Signer should use TxStatus instead of Tx to confirm submission celestiaorg/celestia-app#3366

Closed

ninabarbakadze added 11 commits May 17, 2024 11:05

chore: fix go sum entry error

cd28593

(wip): remove rejected, evicted txs works

aff84b8

refactor: tests and functions to make it cleaner

4039a56

Merge branch 'main' into nina/current-tx-status

51f2e96

fix: build

6fb9af9

test: more tests

2f38c2b

style: lint

edbcdae

style: cleanup

652f814

style: lint

2cede15

style: gofumped

125ef6f

style: goimports

37c8cd1

ninabarbakadze marked this pull request as ready for review June 6, 2024 15:55

ninabarbakadze requested a review from a team as a code owner June 6, 2024 15:55

ninabarbakadze requested review from ramin, evan-forbes and cmwaters and removed request for a team and ramin June 6, 2024 15:55

rootulp previously approved these changes Jun 6, 2024

View reviewed changes

evan-forbes added the WS: BestTxs label Jun 6, 2024

cmwaters reviewed Jun 7, 2024

View reviewed changes

Update mempool/cache.go

90f482c

Co-authored-by: Rootul P <[email protected]>

ninabarbakadze dismissed rootulp’s stale review via 90f482c June 11, 2024 08:40

Update mempool/cache.go

5cdd6e0

Co-authored-by: Rootul P <[email protected]>

ninabarbakadze marked this pull request as draft June 11, 2024 08:41

refactor: revert accidentally removed comment

6ac1205

ninabarbakadze marked this pull request as ready for review June 13, 2024 15:00

ninabarbakadze requested review from cmwaters, rootulp and evan-forbes June 13, 2024 15:00

rootulp reviewed Jun 13, 2024

View reviewed changes

ninabarbakadze and others added 4 commits June 13, 2024 18:55

Update mempool/v1/mempool.go

4e09a6c

Co-authored-by: Rootul P <[email protected]>

Update mempool/mempool.go

cc8c178

Co-authored-by: Rootul P <[email protected]>

Update mempool/mempool.go

7437fd9

Co-authored-by: Rootul P <[email protected]>

test: update test

c6f3e9a

cmwaters reviewed Jun 15, 2024

View reviewed changes

ninabarbakadze marked this pull request as draft June 17, 2024 14:11

ninabarbakadze added 3 commits June 17, 2024 16:16

refactor: no longer pass the cache in purgeExpiredTxs and reduce evic…

bc74ef3

…tedTxCache size

feat: add txstatus in mempool and test it

64eb498

style: lint

bbcb66a

ninabarbakadze commented Jun 17, 2024

View reviewed changes

ninabarbakadze added 2 commits June 17, 2024 18:50

lint: remove lint error

a75c6d5

refactor: rename IsTxEvicted to WasRecentlyEvicted

14d09b0

ninabarbakadze marked this pull request as ready for review June 17, 2024 17:14

ninabarbakadze requested review from rootulp and cmwaters June 17, 2024 19:28

rootulp previously approved these changes Jun 17, 2024

View reviewed changes

Update mempool/cat/store.go

8e3c51d

Co-authored-by: Rootul P <[email protected]>

ninabarbakadze dismissed rootulp’s stale review via 8e3c51d June 18, 2024 06:50

cmwaters approved these changes Jun 18, 2024

View reviewed changes

evan-forbes approved these changes Jun 18, 2024

View reviewed changes

ninabarbakadze mentioned this pull request Jun 18, 2024

Expand TxInfo to save the resulting code of the transaction #1397

Closed

ninabarbakadze merged commit ff2bff4 into celestiaorg:main Jun 18, 2024
15 checks passed

ninabarbakadze deleted the nina/current-tx-status branch June 18, 2024 13:37

ninabarbakadze mentioned this pull request Jul 1, 2024

chore: backport tx status #1405

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add tx status in mempool #1287

feat: add tx status in mempool #1287

ninabarbakadze commented Apr 5, 2024 •

edited

Loading

rootulp left a comment

rootulp Jun 13, 2024

rootulp Jun 13, 2024

rootulp Jun 13, 2024

ninabarbakadze Jun 13, 2024

cmwaters Jun 15, 2024

rootulp Jun 13, 2024

rootulp Jun 13, 2024

ninabarbakadze Jun 13, 2024

cmwaters Jun 15, 2024

cmwaters left a comment

cmwaters Jun 15, 2024

cmwaters Jun 15, 2024

ninabarbakadze Jun 17, 2024

cmwaters Jun 15, 2024

ninabarbakadze left a comment

rootulp Jun 17, 2024

ninabarbakadze Jun 17, 2024

cmwaters Jun 18, 2024

cmwaters left a comment

cmwaters Jun 18, 2024

ninabarbakadze Jun 18, 2024

cmwaters Jun 18, 2024

ninabarbakadze Jun 18, 2024

	// IsTxEvicted returns true if the tx is evicted from the mempool and exists in the
	// IsTxEvicted returns true if the tx was evicted from the mempool and exists in the

feat: add tx status in mempool #1287

feat: add tx status in mempool #1287

Conversation

ninabarbakadze commented Apr 5, 2024 • edited Loading

Description

rootulp 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

cmwaters 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

ninabarbakadze 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

cmwaters 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

ninabarbakadze commented Apr 5, 2024 •

edited

Loading