Refactor TxPool, improve its robustness #1802

andreibancioiu · 2020-05-26T07:06:32Z

Score computation logic extracted to allow for easier change / algorithm replacements
Some components decoupled from cacheConfig etc.
bucketSortedMap does not call computeScore anymore, but receives the score instead
txListForSender does not compute its score, it only computes the senderScoreParams.
3 and 4 allow us to reason more easily about critical sections within txListForSender.
Fixed a possible uint32 overflow in RemoveTxsBulk (it would have a benign overflow, the value only used to monitor / diagnose eviction)
Wrapped transaction selection, addition and removal in try / catch blocks.
Removed deprecated cache implementation
Add log with number of senders with failed selection (per selection)
Add log with number of transactions to remove when block is finalized (sharded tx pool)

…ical sections.

# Conflicts: # dataRetriever/txpool/shardedTxPool.go # storage/txcache/config.go # storage/txcache/disabledCache.go # storage/txcache/txByHashMap.go # storage/txcache/txListBySenderMap_test.go # storage/txcache/txListForSender.go # storage/txcache/txListForSenderAsSortedMapItem_test.go # storage/txcache/txListForSender_test.go # storage/txcache/wrappedTransaction.go

core/tryCatch.go

iulianpascalau · 2020-05-26T08:49:59Z

process/block/preprocess/sortedTransactionsProvider.go

@@ -0,0 +1,65 @@
+package preprocess


why not create a sub-package called factory?
Why not splitting this file? Contains the factory instance and a disabled component. HArd to figure this out just reading the filename.

As stated at 8), this PR removes the old, deprecated sorting operations of the old transactions cache.

preprocess/transactions handles the TxCache using the Cacher interface. However, when it selects transactions at proposal time, it requires the selection operation, which the Cacher interface does not provide. This is why we needed the adapter in the first place. The adapter factory was switching between the new sortedTxsProvider and the old one - the old one was used if the cast cache.(*TxCache) failed (never the case in real life). Now, since we've removed the old components, we had to use a disabled substitute. Of course, this is suboptimal design. We actually have to fix transactions.go to depend on a well-defined interface instead, and discard the adapters and the adapter factory altogether. Not doable in this PR though, I've added a TODO now. I will try to figure out another way in the next PR, pool++cross.

Furthermore, the factory and the adapters were kept in separate files before this PR. I grouped them together to increase the cohesion of smelly things that we should remove sometimes soon.

Is this reasonable (that is, doing this non-trivial removal / change incrementally to avoid larger changes in the code at once)?

It is reasonable as it is for now.

process/block/preprocess/sortedTransactionsProvider.go

storage/txcache/factory.go

storage/txcache/score.go

storage/txcache/txCacheFailsafe.go

AdoAdoAdo · 2020-05-27T10:06:13Z

process/block/preprocess/sortedTransactionsProvider.go

+)
+
+// createSortedTransactionsProvider is a "simple factory" for "SortedTransactionsProvider" objects
+func createSortedTransactionsProvider(transactionsPreprocessor *transactions, cache storage.Cacher, cacheKey string) SortedTransactionsProvider {


transactionsPreprocessor and cacheKey are not used.

Fixed, removed leftovers :)

AdoAdoAdo · 2020-05-27T10:14:44Z

storage/txcache/score.go

-	listForSender.scoreChunkMutex.Lock()
-	listForSender.scoreChunk = scoreChunk
-	listForSender.scoreChunkMutex.Unlock()
+func (computer *disabledScoreComputer) computeScore(scoreParams senderScoreParams) uint32 {


unused parameter, maybe use _

AdoAdoAdo · 2020-05-27T10:29:28Z

storage/txcache/txByHashMap.go


 	for _, txHash := range txHashes {
-		txMap.removeTx(string(txHash))
+		_, removed := txMap.removeTx(string(txHash))


the removeTx implementation seems a bit weak as for example if we concurrently try to remove the same txhash we might end up with a wrong count and wrong size (removal succeeds only once but we decrement the counter and the size twice)

Very nice catch!

tx, ok := txMap.getTx(txHash) could return ok == true for both the concurring routines and both will decrement, which, indeed, is not correct.

The issue can appear if transactions are removed during eviction (on high load) and also removed (at the same time) on the "commit / finalize block" event.

In monitoring.go, in function diagnose() we have:

numTxsEstimate := int(cache.CountTx()) // this comes from the possibly wrong counter numTxsInChunks := cache.txByHash.backingMap.Count()

If the estimate does not equal numTxsInChunks, a log.Debug() is emitted and we should see the difference. This diagnose() is unfortunately (currently) called only upon transaction selection (on my turn).

A possible solution to this issue: in txMap.backingMap.Remove, when the map chunk is locked (under mutex), do the "item exists" check and call a onItemRemovedCallback() to decrement the atomic counters. Would this be fine?

Another solution would be to apply corrections from time to time - that would work fine for the counter, but the corrections for the size counter would be quite inefficient.

I've added a TODO for this correction (it's an old condition).

I was thinking the same, add the check in the Remove under lock and have the function return a bool if the remove was done.

AdoAdoAdo · 2020-05-27T10:34:34Z

storage/txcache/txCache.go

@@ -9,6 +9,7 @@ import (
 )

 var _ storage.Cacher = (*TxCache)(nil)
+var _ txCache = (*TxCache)(nil)



Put and HasOrAdd have some unused parameters.

Fixed / _ (these methods of the Cacher interface are not implemented by the TxCache). For disabledCache I've left them as they were, since we plan to remove that struct in a future PR anyway (see comments above). Is this fine?

AdoAdoAdo · 2020-05-27T10:42:45Z

storage/txcache/txCacheFailsafe.go

+
+// Remove delegates to backing cache
+func (decorator *txCacheFailsafeDecorator) Remove(key []byte) {
+	decorator.RemoveTxByHash(key)


_= unhandled error

The failSafe component was removed in the fix-after-review.

AdoAdoAdo · 2020-05-27T10:49:52Z

storage/txcache/txListBySenderMap.go

+func (txMap *txListBySenderMap) notifyScoreChange(txList *txListForSender, scoreParams senderScoreParams) {
+	score := txMap.scoreComputer.computeScore(scoreParams)
+	txList.setLastComputedScore(score)
+	txMap.backingMap.NotifyScoreChange(txList, score)
 }


line 106 removeSender can act incorrectly if called concurrently for same sender, e.g removing the only tx for that sender and removing the sender due to low tx both will call removeSender. The removal is done only once but the counter is decremented twice.

Nice catch! As above, added a TODO. Is this ok?

iulianpascalau · 2020-05-27T15:53:35Z

storage/txcache/txListBySenderMap.go

@@ -108,6 +108,11 @@ func (txMap *txListBySenderMap) removeSender(sender string) bool {
 		return false
 	}

+	// TODO / bugfix: when we concurrently try to remove the same sender,
+	// we might end up decrementing the counter twice.
+	// Possible solution: use an "onItemRemoved" callback in the "backingMap"


To prevent double decreasing, might use a mutex or atomic function for that in order to make the decrementation concurrent safe.

LucianMincu

System tests passed.

Andrei Bancioiu and others added 30 commits May 14, 2020 20:00

Merge branch 'pool++limits' into pool++robust

da0dae6

Fix possible uint32 overflow.

2b67f8f

Refactor, to pass score instead of calling ComputeScore().

f8b9a28

Add interface assertion.

2ede48f

Rename file.

400963d

Move functions.

831f472

Sketch new struct, scoreComputer.

bfc5627

Score params computed within sender list. Easier to reason about crit…

e5ae797

…ical sections.

Fix test utility.

39cc3f2

Move tests.

824eb76

Move remaining tests.

38ab154

Move code to "wrappedTransaction.go".

6075a53

Cover function.

82c0289

Receive min gas price in score computer.

a90810f

Decouple from cache config; receive senderConstraints struct.

45f2bb4

Rename struct.

f419e2f

Receive scoreComputer in constructor (as interface).

7af51e1

Merge branch 'development' into pool++robust

b20c8c3

Add try/catch components.

ff587ed

Add benchmarks for try/catch.

8907b27

See TxCache as interface in block/preprocess.

4f034f6

Call CreateCache from sharded tx pool.

708d1dd

Wrap important txcache functions in try / catch, via a decorator.

0121a4f

Add function to interface.

f6b2081

Merge branch 'development' into pool++robust

3dfbed5

Remove old tx cache and its tests.

b18bd72

Refactor - rename file, move logic to single file (a bit more cohesive).

a2d19f4

Merge branch 'development' into pool++robust

aec22a2

Extra log with number of removed transactions (at block finality).

f00b4fc

Monitor selection in a more detailed manner.

312815d

andreibancioiu and others added 6 commits May 17, 2020 21:54

Add extra tests.

88d09cb

Add some tests.

aa0b112

Merge branch 'development' into pool++robust

d079c97

Merge branch 'pool++limits-fixed' into pool++robust

11be956

Merge branch 'development' into pool++robust

a08576d

andreibancioiu self-assigned this May 26, 2020

andreibancioiu requested review from AdoAdoAdo and iulianpascalau May 26, 2020 07:06

andreibancioiu added 2 commits May 26, 2020 10:39

Fix after self review.

8f3423a

Fix after self-review, add comment.

dafaecd

iulianpascalau requested changes May 26, 2020

View reviewed changes

LucianMincu added 3 commits May 26, 2020 18:00

Merge branch 'development' into pool++robust

ff59691

Merge branch 'development' into pool++robust

8b77693

Merge branch 'development' into pool++robust

8228346

AdoAdoAdo requested changes May 27, 2020

View reviewed changes

Andrei Bancioiu added 2 commits May 27, 2020 17:25

Fix after review.

5fd6b43

Fix after review.

7f3e4f2

andreibancioiu requested review from AdoAdoAdo and iulianpascalau May 27, 2020 15:36

iulianpascalau reviewed May 27, 2020

View reviewed changes

iulianpascalau approved these changes May 27, 2020

View reviewed changes

AdoAdoAdo approved these changes May 27, 2020

View reviewed changes

Merge branch 'development' into pool++robust

717b615

LucianMincu approved these changes May 28, 2020

View reviewed changes

iulianpascalau merged commit 1380a2f into development May 28, 2020

iulianpascalau deleted the pool++robust branch May 28, 2020 10:33

This was referenced Jun 1, 2020

[Minor bug / minor fix] TxPool - fix inconsistent counters (estimators) #1841

Closed

[Minor bug / minor fix] TxPool - fix inconsistent counters (estimators) #1842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor TxPool, improve its robustness #1802

Refactor TxPool, improve its robustness #1802

andreibancioiu commented May 26, 2020

iulianpascalau May 26, 2020

andreibancioiu May 27, 2020

iulianpascalau May 27, 2020

AdoAdoAdo May 27, 2020

andreibancioiu May 27, 2020

AdoAdoAdo May 27, 2020

andreibancioiu May 27, 2020

AdoAdoAdo May 27, 2020

andreibancioiu May 27, 2020 •

edited

Loading

AdoAdoAdo May 27, 2020 •

edited

Loading

AdoAdoAdo May 27, 2020

andreibancioiu May 27, 2020 •

edited

Loading

AdoAdoAdo May 27, 2020

andreibancioiu May 27, 2020

AdoAdoAdo May 27, 2020

andreibancioiu May 27, 2020 •

edited

Loading

iulianpascalau May 27, 2020

LucianMincu left a comment

Refactor TxPool, improve its robustness #1802

Refactor TxPool, improve its robustness #1802

Conversation

andreibancioiu commented May 26, 2020

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

andreibancioiu May 27, 2020 • edited Loading

Choose a reason for hiding this comment

AdoAdoAdo May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu May 27, 2020 • 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

andreibancioiu May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucianMincu left a comment

Choose a reason for hiding this comment

andreibancioiu May 27, 2020 •

edited

Loading

AdoAdoAdo May 27, 2020 •

edited

Loading

andreibancioiu May 27, 2020 •

edited

Loading

andreibancioiu May 27, 2020 •

edited

Loading