core/tx_pool: count tx size in slots, bump max size ot 4x32 Kb #20352
Conversation
MichaelRiabzev-StarkWare
requested review from
holiman,
karalabe and
rjl493456442
as code owners
core/tx_pool.go
Outdated
| @@ -583,7 +595,7 @@ func (pool *TxPool) add(tx *types.Transaction, local bool) (replaced bool, err e | |||
| return false, ErrUnderpriced | |||
| } | |||
| // New transaction is better than our worse ones, make room for it | |||
| drop := pool.priced.Discard(pool.all.Count()-int(pool.config.GlobalSlots+pool.config.GlobalQueue-1), pool.locals) | |||
| drop := pool.priced.Discard(pool.all.SlotsUsed()-int(pool.config.GlobalSlots+pool.config.GlobalQueue)+NumSlots(tx), pool.locals) | |||
There was a problem hiding this comment.
If I'm reading this right, it means that one 4-slot tx will push out 4 1-slot transactions, even though it's only marginally better.
That may be fine, but please update the PR description with more information about the expected behaviours, such as this.
These types of choices that is made internally deep within the tx pool determines whether the macro behaviour of the network can be attacked or not.
It might be good to introduce some randomness to how the tx pool behaves in this regard.
There was a problem hiding this comment.
I've updated the PR description accordingly. Do you have concrete suggestions for the tx pool behavior?
core/tx_pool.go
Outdated
| @@ -270,6 +279,9 @@ func NewTxPool(config TxPoolConfig, chainconfig *params.ChainConfig, chain block | |||
| reorgDoneCh: make(chan chan struct{}), | |||
| reorgShutdownCh: make(chan struct{}), | |||
| gasPrice: new(big.Int).SetUint64(config.PriceLimit), | |||
|
|
|||
| // Heuristic limit, reject transactions requiring over 4 slots to prevent DOS attacks | |||
| maxTxSize: 4 * slotSize, | |||
There was a problem hiding this comment.
This is a 4x bump. I'm not sure we should bump that immediately, or do something a bit less dramatic, e.g. set it randomly so 50% retain 1x slotsize, 25% 2x and 25% 4x. Up for discussion (and please add it to PR description)
There was a problem hiding this comment.
I thought this was a solution you felt comfortable with, notice it does not enlarge resource consumption.
holiman
changed the title
karalabe
force-pushed
the
michael/slots
branch
from
fd57904 to
5c3fa5b
Compare
* tests for tx size * alow multiple slots transactions * tests for tx size limit (32 KB) * change tx size tests to use addRemoteSync instead of validateTx (requested in pool request). * core: minor tx slotting polishes, add slot tracking metric Co-authored-by: Michael Riabzev <RiabzevMichael@gmail.com> Co-authored-by: Péter Szilágyi <peterke@gmail.com>
This change allows transactions of bigger than 32KB. To prevent DoS over the tx pool (the original reason to limit the transaction size), this change should not introduce any additional memory consumption over the original implementation. Thus instead of the straightforward solution of enlarging the hard-coded limit, it introduces slots in the tx pool. The size of the tx pool is limited by the number of slots. Each slot is of size up to 32KB, and each transaction can now take up to 4 slots. The eviction of underpriced transactions stays the same as before, in particular, a 4-slot transaction can evict 4 1-slot transactions even in the case it is slightly more profitable.
This change is