[Merged by Bors] - feat: prune ineffectual mempool txs

[acud]

Motivation

Adds support for eviction of ineffectual transactions from the mempool.

Description

Currently, the mempool will not consider pending transactions with lower nonces than already executed for a given principal address. It filters out the relevance of transactions via a SQL query that fetches out the pending transactions for a principal address from a given nonce.

The problem is, that old, pending transactions may still live as zombie transactions in the database that will never be executed and will never be inserted into the mempool simply because they no longer satisfy the query constraints that fetches pending transactions.

Furthermore, a lookup for an old, low nonce transaction that lives in the database but not in the mempool, will result in the transaction showing up as pending in the mempool (from the perspective of the API), which is not correct. This is due to a disparity of state between DB-Mempool (db never cleans up old txs but they still have a "pending" state forever).

This PR amends this by cleaning up old irrelevant transactions into an intermediate table evicted_mempool, where they dwell for a certain timeout (12 hours currently) before finally getting erased. The intermediate table serves as some sort of a way to represent the transition of the transaction without having to add a new discrete transaction state (and therefore having to maintain a new transaction state for every transaction which might be problematic and complex under certain cases). This also allows the API to see if the transaction is indeed in this evicted state, and alert the user accordingly.

The relevant new transaction states were added to the API here: spacemeshos/api#386

The original issue was highlighted in #5317 but wasn't originally addressed.

Note: for transactions that were indeed evicted and eventually pruned out of the database but have somehow possible again for execution due to a reorg, the users are expected to resubmit the transaction to the network.

Test Plan

• Manual testing
• Tests passing

TODO

• Explain motivation or link existing issue(s)
• Test changes and document test plan
• Update documentation as needed
• Update changelog as needed

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 45 lines in your changes missing coverage. Please review.

Project coverage is 79.9%. Comparing base (f711c61) to head (2fe5ca4).
Report is 14 commits behind head on develop.

Files with missing lines	Patch %	Lines
sql/transactions/transactions.go	71.1%	10 Missing and 5 partials ⚠️
txs/cache.go	34.7%	10 Missing and 5 partials ⚠️
api/grpcserver/transaction_service.go	0.0%	9 Missing ⚠️
genvm/vm.go	0.0%	4 Missing ⚠️
txs/conservative_state.go	0.0%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6443     +/-   ##
=========================================
- Coverage     79.9%   79.9%   -0.1%     
=========================================
  Files          352     353      +1     
  Lines        46096   46396    +300     
=========================================
+ Hits         36865   37100    +235     
- Misses        7142    7192     +50     
- Partials      2089    2104     +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[acud]

bors try

[spacemesh-bors]

try

Build succeeded:

• ci-status
• systest-status

[fasmat]

I'm a bit confused by the implementation, why is the extra DB table needed? A types.MeshTransaction already has a State that can be PENDING, MEMPOOL and APPLIED. Would it make sense to add a EVICTED state there and store the TX in the transactions table instead of in a new table?

[fasmat]

After a discussion with @acud I understand the reasoning for the implementation. I have two more suggestion for a possible change in the code:

Right now transactions.EvictPendingNonce is a bit complex and would also require a transaction to be passed to the function so all queries are either executed or rolled back together. Would it make sense to split that function into 3 individual queries that are then called from a EvictPendingNonce in the accountCache? Something like:

func (ac *accountCache) EvictPendingNonce(db sql.StateDatabase) error {
	return db.WithTx(context.Background(), func(tx sql.Transaction) error {
		txIds, err := transactions.GetAcctPendingToNonce(tx, ac.addr, ac.startNonce)
		if err != nil {
			return fmt.Errorf("get pending to nonce: %w", err)
		}
		for _, tid := range txIds {
			if err := transactions.SetEvicted(tx, tid); err != nil {
				return fmt.Errorf("set evicted for %s: %w", tid, err)
			}
			if err := transactions.Delete(tx, tid); err != nil {
				return fmt.Errorf("delete tx %s: %w", tid, err)
			}
		}
		return nil
	})
}

The other change I would suggest is to make time an INT NOT NULL for and insert time.Now().UnixNano() when adding a transaction to evicted_mempool. When pruning those entries with transactions.PruneEvicted the time from which on the transactions should be pruned can then be passed as a parameter to the function:

func PruneEvicted(db sql.Executor, before time.Time) error {
	if _, err := db.Exec("delete from evicted_mempool where time < ?1;",
		func(stmt *sql.Statement) {
			stmt.BindInt64(1, before.UnixNano())
		}, nil); err != nil {
		return fmt.Errorf("prune evicted %w", err)
	}
	return nil
}

for now its OK to just pass a hard coded time as parameter when calling it:

transactions.PruneEvicted(db, time.Now().Add(-12*time.Hour))

but this will make it easier to make this parameter configurable in the future 🙂

[acud]

bors try

[spacemesh-bors]

try

Build failed:

• systest-status

[spacemesh-bors]

Build failed:

• systest-status

[acud]

bors merge

[spacemesh-bors]

Build failed:

• systest-status

[acud]

bors merge

[spacemesh-bors]

Build failed:

• systest-status

[acud]

bors merge

[spacemesh-bors]

Build failed:

• systest-status

[acud]

bors merge

[spacemesh-bors]

Canceled.

[acud]

bors try

[spacemesh-bors]

try

Build failed:

• ci-status

[acud]

bors merge

[spacemesh-bors]

Pull request successfully merged into develop.

Build succeeded:

• ci-status
• systest-status