New higher capacity Txpool #2660

emailtovamos · 2024-08-20T11:44:06Z

Description

This PR works to expand the legacy transaction pool's functionality by adding two new concepts:

pool2 -> only broadcasts to static peers
pool3 -> local buffer pool which waits for pool2 to get space and when it can it sends the transactions to pool2.

The current pool as it is now is simply referred to pool1.

This will help accommodating more number of transactions especially in high traffic when somewhat underpriced and otherwise valid transactions get rejected.

Rationale

tell us why we need these changes...

Example

add an example CLI or API response...

Changes

Notable changes:

add each change in a bullet point here
...

WTDuck2255

Thx so much

MatusKysel · 2024-09-02T09:18:41Z

core/events.go

+	// Static bool is Whether to send to only Static peer or not.
+	// This is because at high traffic we still want to broadcast transactions to at least some peers so that we
+	// minimize the transaction lost.
+	Static bool


I would call it differently, maybe BroadcastToStatic or something more meaningful

Suggestion-1:
For pool-2: it has its own peers to broadcast the Txs, the peers of pool-2 could be consisted by:
1.StaticNodes
2.A subset of the other peers, sqrt()?
Because, many node may not contain any StaticNodes.

The sqrt() of peers is already what happens in pool-1. So we might do cube root or something?

The sqrt() of peers is already what happens in pool-1
// ==
do you mean the sqrt() logic in BroadcastTransactions? https://github.com/bnb-chain/bsc/blob/master/eth/handler.go#L865?

What I mean here is that: suppose we have 400 connected peers, 10 StaticNodes, then:
1.For pool-1: it has 400 peers, directly send full tx content to 20(sqrt(400) peers, and send announcement to the left 380 peers
2.For pool-2: it has sqrt(400) + 10(size of static nodes), discard the overlapped one, it could have 20 -> 30 peers. Suppose it is 25 peets, base on it, send full tx content to 5(sqrt(25)) peers, and send announcement to the left 20 peers.

In the above example, there is a chance that all 5 peers are non-static. Is that fine? Or we should have at least 1 static peer (if it exists) to send full tx always.

MatusKysel · 2024-09-02T09:24:52Z

core/txpool/legacypool/buffer.go

+	txSlots := numSlots(tx)
+
+	// Remove elements until there is enough capacity
+	for lru.size+txSlots > lru.capacity && lru.buffer.Len() > 0 {


why lru.buffer.Len() > 0 ?

MatusKysel · 2024-09-02T09:44:16Z

core/txpool/legacypool/legacypool.go

+		pool.all.Add(tx, pool2)
+		pool.priced.Put(tx, pool2)
+		pool.journalTx(from, tx)
+		pool.queueTxEvent(tx, false)
+		_, err := pool.enqueueTx(tx.Hash(), tx, isLocal, true, false) // At this point pool1 can incorporate this. So no need for pool2 or pool3
+		if err != nil {
+			return false, err
+		}
+		log.Trace("Pooled new executable transaction", "hash", tx.Hash(), "from", from, "to", tx.To())
+
+		// Successful promotion, bump the heartbeat
+		pool.beats[from] = time.Now()
+
+		return true, nil


this code repeats maybe it could be separate function

MatusKysel

looks good, some minor comments

Aces42020

JumpTasker#4900

cmd/geth/main.go

zzzckck · 2024-09-05T06:59:53Z

core/txpool/errors.go

@@ -30,6 +30,10 @@ var (
 	// configured for the transaction pool.
 	ErrUnderpriced = errors.New("transaction underpriced")

+	// ErrUnderpriced is returned if a transaction's gas price is below the minimum
+	// configured for the transaction pool.
+	ErrUnderpricedTransferredtoAnotherPool = errors.New("transaction underpriced, so it is either in pool2 or pool3")


why need this error? I think underpriced transaction can be simply discarded

zzzckck · 2024-09-05T07:16:17Z

core/txpool/legacypool/buffer.go

+	"github.com/ethereum/go-ethereum/core/types"
+)
+
+type LRUBuffer struct {


Suggestion-2:
maybe we can just use the fastcache instead? it is a RingBuffer, not LRU, RingBuffer has some advantages, you may refer: https://github.com/bnb-chain/bsc/blob/master/core/state/snapshot/disklayer.go#L36

Fastcache may not work directly, as it does not support iteration. We can add a list of TxHash to support the iteration.

I also suggest here to use well tested lib, like github.com/ethereum/go-ethereum/common/lru, github.com/VictoriaMetrics/fastcache, etc. They have less bug and better performance.

zzzckck · 2024-09-05T07:28:28Z

core/txpool/legacypool/legacypool.go

-	ReannounceTime time.Duration // Duration for announcing local pending transactions again
+	Lifetime              time.Duration // Maximum amount of time non-executable transaction are queued
+	ReannounceTime        time.Duration // Duration for announcing local pending transactions again
+	InterPoolTransferTime time.Duration // Attempt to transfer from pool3 to pool2 every this much time


why need InterPoolTransferTime?

zzzckck · 2024-09-05T07:39:47Z

core/txpool/legacypool/legacypool.go

+	txPoolSizeAfterCurrentTx := uint64(pool.all.Slots() + numSlots(tx))
+	var includePool1, includePool2, includePool3 bool
+	if txPoolSizeAfterCurrentTx <= maxPool1Size {
+		includePool1 = true


Suggestion-3:
transaction would be discarded in pool-1, even when pool-1 is not full.
like this case:

txpool.pending.length > pool.config.GlobalSlots

the sender's pending transaction > pool.config.AccountSlots

I think we can just move transactions into pool-2 if pool-1 reject it, so does pool-3, it is a prioritised pool:

1.try put it into pool-1, ok=> done

2.try put it into pool-2, ok=> done

3.try put it into pool-3, ok=> done
It could be much clearer

The above two scenarios check happen anyway during runReorg(). So even if such a transaction got added it will be removed in the later call of runReorg() just like what happens currently unless I am missing something?

zzzckck · 2024-09-05T07:43:42Z

core/txpool/legacypool/legacypool.go

@@ -2038,3 +2164,91 @@ func (t *lookup) RemotesBelowTip(threshold *big.Int) types.Transactions {
 func numSlots(tx *types.Transaction) int {
 	return int((tx.Size() + txSlotSize - 1) / txSlotSize)
 }
+
+func (pool *LegacyPool) startPeriodicTransfer(t time.Duration) {


Suggestion-4:
transfer should happen on new blocked inserted, rather than a timer.

Suggestion-4: transfer should happen on new blocked inserted, rather than a timer.

a timer will get better performance?
if tansfer happen on new blocked inserted, then will wait the transfer before mining new blocks

zzzckck · 2024-09-05T08:00:53Z

core/txpool/legacypool/legacypool.go

+	}()
+}
+
+// transferTransactions mainly moves from pool 3 to pool 2


Suggestion-5:
I think pool-3 does not need to transfer to pool-2 specifically. the transactions in pool-3 can just seen as new arrived transaction, there is the workflow:

1.check if it is still valid or not, remove if it is no longer valid

2.try to put it into pool-1, ok => remove it from pool-3

3.try to put it into pool-2, ok => remove it from pool-3

4.if failed to transfer, due to pool-1 and pool-2 are full, stop the transfer procedure.

galaio · 2024-09-09T07:26:44Z

core/txpool/legacypool/buffer_test.go

+func TestNewLRUBuffer(t *testing.T) {
+	capacity := 10
+	lru := NewLRUBuffer(capacity)
+	if lru.capacity != capacity {


It's better to use github.com/stretchr/testify/require to assert:

require.Equal(t, capacity, lru.capacity, "capacity wrong")

galaio · 2024-09-09T07:35:39Z

core/txpool/legacypool/legacypool.go

 				txs := make([]*types.Transaction, 0)
+				statics := make([]bool, 0)


This statics var can be removed because it drops when returned from func.

It is one of the function returns. How can I remove it? Or did I misunderstand?

galaio · 2024-09-10T06:47:10Z

core/txpool/legacypool/buffer.go

+	"github.com/ethereum/go-ethereum/core/types"
+)
+
+type LRUBuffer struct {


I also suggest here to use well tested lib, like github.com/ethereum/go-ethereum/common/lru, github.com/VictoriaMetrics/fastcache, etc. They have less bug and better performance.

galaio · 2024-09-10T06:58:18Z

core/txpool/legacypool/legacypool.go

+
+		// calculate total number of slots in drop. Accordingly add them to pool3 (if there is space)
+		// all members of drop will be dropped from pool1/2 regardless of whether they get added to pool3 or not
+		availableSlotsPool3 := pool.availableSlotsPool3()


Can the call here be simplified? Let the complex logic be coupled in the pool3 object? For example, for pool3, which is an LRU, just add it directly and let the LRU eliminate it.

pool.addToPool3(drop, isLocal)

buddh0 · 2024-09-20T09:07:13Z

core/txpool/legacypool/fastcache.go

+// Move a transaction to the front of the LRU order
+func (lru *LRUBufferFastCache) moveToFront(hash common.Hash) {
+	for i, h := range lru.order {
+		if h == hash {


iterate all？cost much

buddh0 · 2024-09-20T09:07:54Z

core/txpool/legacypool/fastcache.go

+	for i, h := range lru.order {
+		if h == hash {
+			// Remove the hash from its current position
+			lru.order = append(lru.order[:i], lru.order[i+1:]...)


huge copy？

buddh0 · 2024-09-20T09:08:03Z

core/txpool/legacypool/fastcache.go

+		}
+	}
+	// Add it to the front
+	lru.order = append(lru.order, hash)


huge copy？

buddh0 · 2024-09-20T09:44:05Z

core/txpool/legacypool/legacypool.go

 		reorgDoneCh:     make(chan chan struct{}),
 		reorgShutdownCh: make(chan struct{}),
 		initDoneCh:      make(chan struct{}),
+		localBufferPool: NewLRUBufferFastCache(int(config.Pool3Slots)),


100k means 100k*1024*0124, 100G?

buddh0 · 2024-09-20T10:03:27Z

core/txpool/legacypool/legacypool.go

+}
+
+// addToPool12OrPool3 adds a transaction to pool1 or pool2 or pool3 depending on which one is asked for
+func (pool *LegacyPool) addToPool12OrPool3(tx *types.Transaction, from common.Address, isLocal bool, pool1, pool2, pool3 bool) (bool, error) {


no scene call addToPool12OrPool3 with pool1 or pool2 ?

buddh0 · 2024-09-20T10:08:40Z

core/txpool/legacypool/fastcache.go

+		}
+	}
+	// Add it to the front
+	lru.order = append(lru.order, hash)


call move item to the back as moveToFront?

buddh0 · 2024-09-20T10:20:44Z

during the benchmark in QA env, I use the follwing config

GlobalSlots = 10000
GlobalQueue = 5000

then I find the txpool is not the bottleneck of performance, validator always have enough txs to handle

based on this, I think we don't need pool2

buddh0 · 2024-09-20T10:24:33Z

no replacement logic in pool3, so if txs put into pool3, the replacement may fail, does it matter?

emailtovamos added 26 commits July 16, 2024 05:49

txpool: incomplete pool2 and pool3

dfb046e

pool: optimise sending in reorg

e30248b

pool: add static info and simplify transfer

f80ac01

pool: remove comments, use queue not pool23

8655d30

pool: remove unused function

4538e92

pool: refactor and bugfix

4369e3d

pool: buffer test and size logic

d0d6a27

pool: add discarded ones to pool3 by default.

5faf413

pool: minor refactor

5bb78b3

pool: make slots config

6b4e16b

pool: initialise pool3 slots

d03f7e5

pool: add underpriced to pool2 or 3

94a60a9

pool: enqueue in pool2 & drop properly

ed2d1d7

pool: bugfix:always drop drop and pool2 size

6daecfb

pool: TestDualHeapEviction passing partly

9d7298f

pool: TestDualHeapEviction fully pass

a1a25e9

pool: some cleanups

253d9a5

pool: fix the TestTransactionFutureAttack test

0692a99

pool: cleanup debug logs

40dcfcd

pool: fix TestUnderpricingDynamicFee based on new pool

6673f3e

pool: fix all old tests

ebd8f59

pool: lint

bdb4cc2

pool: include static in flatten

e45e7eb

pool: proper use of AsyncSendPooledTransactionHashes

069eaf2

pool: flags for pool2 and 3 capacity

70ece93

pool: fix test as now by default pool2 and pool3 aren't empty

e7d0a16

WTDuck2255 approved these changes Aug 30, 2024

View reviewed changes

emailtovamos marked this pull request as ready for review September 2, 2024 06:18

MatusKysel reviewed Sep 2, 2024

View reviewed changes

Merge remote-tracking branch 'origin/develop' into txpool-new

16a2a53

Aces42020 reviewed Sep 2, 2024

View reviewed changes

cmd/geth/main.go Show resolved Hide resolved

cmd/geth/main.go Show resolved Hide resolved

emailtovamos added 7 commits September 3, 2024 12:41

pool: test for transfer

0f8a1b5

pool: set transfer time in config

76d157d

pool: remove unused criticalpathpool

aeec0c7

buffer: make private

53042e1

pool: bug fix and test fix

8e6833c

pool: pool2 can have 0 size

5f398db

pool: lint fix

706a24e

zzzckck reviewed Sep 5, 2024

View reviewed changes

emailtovamos added 2 commits September 6, 2024 12:20

test: requestPromoteExecutables after every enqueue for testing

0e61543

pool: queued goes to 0 locally after this change

0a5dbef

galaio reviewed Sep 10, 2024

View reviewed changes

emailtovamos added 6 commits September 11, 2024 15:57

pool: fastcache, interface, metrics modify

248bb6b

eth: send to some peers of pool2, not just static

cf10c5c

pool: transfer on block import and simplify it

b818cb7

pool: truly discard underpriced, Transfer after lock is over

774e314

pool: else ifs instead of ifs

629af6d

pool: address minor issues

3e3c56b

WillAR37 mentioned this pull request Sep 20, 2024

memory leak #2686

Open

buddh0 reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New higher capacity Txpool #2660

New higher capacity Txpool #2660

emailtovamos commented Aug 20, 2024 •

edited

Loading

WTDuck2255 left a comment

MatusKysel Sep 2, 2024

zzzckck Sep 5, 2024

emailtovamos Sep 9, 2024

zzzckck Sep 12, 2024 •

edited

Loading

emailtovamos Sep 19, 2024

MatusKysel Sep 2, 2024

MatusKysel Sep 2, 2024

MatusKysel left a comment

Aces42020 left a comment

zzzckck Sep 5, 2024

zzzckck Sep 5, 2024 •

edited

Loading

galaio Sep 10, 2024

zzzckck Sep 5, 2024

zzzckck Sep 5, 2024

emailtovamos Sep 9, 2024

zzzckck Sep 5, 2024

buddh0 Sep 20, 2024

zzzckck Sep 5, 2024

galaio Sep 9, 2024

galaio Sep 9, 2024

emailtovamos Sep 18, 2024

galaio Sep 10, 2024

galaio Sep 10, 2024

buddh0 Sep 20, 2024

buddh0 Sep 20, 2024

buddh0 Sep 20, 2024 •

edited

Loading

buddh0 Sep 20, 2024 •

edited

Loading

buddh0 Sep 20, 2024

buddh0 Sep 20, 2024

buddh0 commented Sep 20, 2024

buddh0 commented Sep 20, 2024 •

edited

Loading

		txs := make([]*types.Transaction, 0)
		statics := make([]bool, 0)

New higher capacity Txpool #2660

Are you sure you want to change the base?

New higher capacity Txpool #2660

Conversation

emailtovamos commented Aug 20, 2024 • edited Loading

Description

Rationale

Example

Changes

WTDuck2255 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

zzzckck Sep 12, 2024 • 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

MatusKysel left a comment

Choose a reason for hiding this comment

Aces42020 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzzckck Sep 5, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buddh0 Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

buddh0 Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buddh0 commented Sep 20, 2024

buddh0 commented Sep 20, 2024 • edited Loading

emailtovamos commented Aug 20, 2024 •

edited

Loading

zzzckck Sep 12, 2024 •

edited

Loading

zzzckck Sep 5, 2024 •

edited

Loading

buddh0 Sep 20, 2024 •

edited

Loading

buddh0 Sep 20, 2024 •

edited

Loading

buddh0 commented Sep 20, 2024 •

edited

Loading