Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove protocolBaseFee checks #8367

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Remove protocolBaseFee checks #8367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 19 additions & 46 deletions erigon-lib/txpool/pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,25 +96,22 @@ type Pool interface {

var _ Pool = (*TxPool)(nil) // compile-time interface check

// SubPoolMarker is an ordered bitset of six bits that's used to sort transactions into sub-pools. Bits meaning:
// 1. Minimum fee requirement. Set to 1 if feeCap of the transaction is no less than in-protocol parameter of minimal base fee. Set to 0 if feeCap is less than minimum base fee, which means this transaction will never be included into this particular chain.
// 2. Absence of nonce gaps. Set to 1 for transactions whose nonce is N, state nonce for the sender is M, and there are transactions for all nonces between M and N from the same sender. Set to 0 is the transaction's nonce is divided from the state nonce by one or more nonce gaps.
// 3. Sufficient balance for gas. Set to 1 if the balance of sender's account in the state is B, nonce of the sender in the state is M, nonce of the transaction is N, and the sum of feeCap x gasLimit + transferred_value of all transactions from this sender with nonces N+1 ... M is no more than B. Set to 0 otherwise. In other words, this bit is set if there is currently a guarantee that the transaction and all its required prior transactions will be able to pay for gas.
// 4. Not too much gas: Set to 1 if the transaction doesn't use too much gas
// 5. Dynamic fee requirement. Set to 1 if feeCap of the transaction is no less than baseFee of the currently pending block. Set to 0 otherwise.
// 6. Local transaction. Set to 1 if transaction is local.
// SubPoolMarker is an ordered bitset of five bits that's used to sort transactions into sub-pools. Bits meaning:
// 1. Absence of nonce gaps. Set to 1 for transactions whose nonce is N, state nonce for the sender is M, and there are transactions for all nonces between M and N from the same sender. Set to 0 is the transaction's nonce is divided from the state nonce by one or more nonce gaps.
// 2. Sufficient balance for gas. Set to 1 if the balance of sender's account in the state is B, nonce of the sender in the state is M, nonce of the transaction is N, and the sum of feeCap x gasLimit + transferred_value of all transactions from this sender with nonces N+1 ... M is no more than B. Set to 0 otherwise. In other words, this bit is set if there is currently a guarantee that the transaction and all its required prior transactions will be able to pay for gas.
// 3. Not too much gas: Set to 1 if the transaction doesn't use too much gas
// 4. Dynamic fee requirement. Set to 1 if feeCap of the transaction is no less than baseFee of the currently pending block. Set to 0 otherwise.
// 5. Local transaction. Set to 1 if transaction is local.
type SubPoolMarker uint8

const (
EnoughFeeCapProtocol = 0b100000
NoNonceGaps = 0b010000
EnoughBalance = 0b001000
NotTooMuchGas = 0b000100
EnoughFeeCapBlock = 0b000010
IsLocal = 0b000001

BaseFeePoolBits = EnoughFeeCapProtocol + NoNonceGaps + EnoughBalance + NotTooMuchGas
QueuedPoolBits = EnoughFeeCapProtocol
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my head: QueuedPoolBits=0 doesn’t mean that need remove it. We just reducing minimal requirements for queued sub-pool - it doesn’t make it useless - actually opposite - more txs will now go there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EnoughFeeCapProtocol was basically the first barrier to entry (because all txns first go to queued). The Subpool bits can never be less than 0. So, this barrier, QueuedPoolBits=0 is redundant.
The only requirement aside from txn validity was that the baseFee is at least greater than 7 wei for queued pool.
Now, any valid transaction could go to Queued pool, irrespective of their baseFee.

NoNonceGaps = 0b010000
EnoughBalance = 0b001000
NotTooMuchGas = 0b000100
EnoughFeeCapBlock = 0b000010
IsLocal = 0b000001

BaseFeePoolBits = NoNonceGaps + EnoughBalance + NotTooMuchGas
)

// metaTx holds transaction and some metadata
Expand Down Expand Up @@ -178,10 +175,6 @@ func SortByNonceLess(a, b *metaTx) bool {
return a.Tx.Nonce < b.Tx.Nonce
}

func calcProtocolBaseFee(baseFee uint64) uint64 {
return 7
}

// TxPool - holds all pool-related data structures and lock-based tiny methods
// most of logic implemented by pure tests-friendly functions
//
Expand Down Expand Up @@ -1087,7 +1080,6 @@ func addTxs(blockNum uint64, cacheView kvcache.CacheView, senders *sendersBatch,
pending *PendingPool, baseFee, queued *SubPool,
byNonce *BySenderAndNonce, byHash map[string]*metaTx, add func(*metaTx, *types.Announcements) txpoolcfg.DiscardReason, discard func(*metaTx, txpoolcfg.DiscardReason), collect bool,
logger log.Logger) (types.Announcements, []txpoolcfg.DiscardReason, error) {
protocolBaseFee := calcProtocolBaseFee(pendingBaseFee)
if assert.Enable {
for _, txn := range newTxs.Txs {
if txn.SenderID == 0 {
Expand Down Expand Up @@ -1134,7 +1126,7 @@ func addTxs(blockNum uint64, cacheView kvcache.CacheView, senders *sendersBatch,
return announcements, discardReasons, err
}
onSenderStateChange(senderID, nonce, balance, byNonce,
protocolBaseFee, blockGasLimit, pending, baseFee, queued, discard, logger)
blockGasLimit, pending, baseFee, queued, discard, logger)
}

promote(pending, baseFee, queued, pendingBaseFee, pendingBlobFee, discard, &announcements, logger)
Expand All @@ -1149,7 +1141,6 @@ func addTxsOnNewBlock(blockNum uint64, cacheView kvcache.CacheView, stateChanges
pending *PendingPool, baseFee, queued *SubPool,
byNonce *BySenderAndNonce, byHash map[string]*metaTx, add func(*metaTx, *types.Announcements) txpoolcfg.DiscardReason, discard func(*metaTx, txpoolcfg.DiscardReason),
logger log.Logger) (types.Announcements, error) {
protocolBaseFee := calcProtocolBaseFee(pendingBaseFee)
if assert.Enable {
for _, txn := range newTxs.Txs {
if txn.SenderID == 0 {
Expand Down Expand Up @@ -1203,7 +1194,7 @@ func addTxsOnNewBlock(blockNum uint64, cacheView kvcache.CacheView, stateChanges
return announcements, err
}
onSenderStateChange(senderID, nonce, balance, byNonce,
protocolBaseFee, blockGasLimit, pending, baseFee, queued, discard, logger)
blockGasLimit, pending, baseFee, queued, discard, logger)
}

return announcements, nil
Expand Down Expand Up @@ -1427,7 +1418,7 @@ func removeMined(byNonce *BySenderAndNonce, minedTxs []*types.TxSlot, pending *P
// nonces, and also affect other transactions from the same sender with higher nonce, it loops through all transactions
// for a given senderID
func onSenderStateChange(senderID uint64, senderNonce uint64, senderBalance uint256.Int, byNonce *BySenderAndNonce,
protocolBaseFee, blockGasLimit uint64, pending *PendingPool, baseFee, queued *SubPool, discard func(*metaTx, txpoolcfg.DiscardReason), logger log.Logger) {
blockGasLimit uint64, pending *PendingPool, baseFee, queued *SubPool, discard func(*metaTx, txpoolcfg.DiscardReason), logger log.Logger) {
noGapsNonce := senderNonce
cumulativeRequiredBalance := uint256.NewInt(0)
minFeeCap := uint256.NewInt(0).SetAllOne()
Expand Down Expand Up @@ -1477,16 +1468,6 @@ func onSenderStateChange(senderID uint64, senderNonce uint64, senderBalance uint
}

needBalance := requiredBalance(mt.Tx)
// 1. Minimum fee requirement. Set to 1 if feeCap of the transaction is no less than in-protocol
// parameter of minimal base fee. Set to 0 if feeCap is less than minimum base fee, which means
// this transaction will never be included into this particular chain.
mt.subPool &^= EnoughFeeCapProtocol
if mt.minFeeCap.Cmp(uint256.NewInt(protocolBaseFee)) >= 0 {
mt.subPool |= EnoughFeeCapProtocol
} else {
mt.subPool = 0 // TODO: we immediately drop all transactions if they have no first bit - then maybe we don't need this bit at all? And don't add such transactions to queue?
return true
}

// 2. Absence of nonce gaps. Set to 1 for transactions whose nonce is N, state nonce for
// the sender is M, and there are transactions for all nonces between M and N from the same
Expand Down Expand Up @@ -1551,10 +1532,8 @@ func promote(pending *PendingPool, baseFee, queued *SubPool, pendingBaseFee uint
tx := pending.PopWorst()
announcements.Append(tx.Tx.Type, tx.Tx.Size, tx.Tx.IDHash[:])
baseFee.Add(tx, logger)
} else if worst.subPool >= QueuedPoolBits {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don’t understand why this line removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Popping off from the worst of Pending could potentially either get injected to baseFee subpool or queued subpool. The only check for queued subpool was that the txn is not impossible to include at a future time.
At this stage, this check was with >= QueuedPoolBits (= EnoughFeeCapProtocol), which has now been removed. All other bits could change in the future making txn eligible for inclusion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure that QueuedPoolBits has only 1 bit?

Even if it’s true - still “bad txs need move to queued sub-pool” because maybe they will improve in future (sender may increase it’s balance, etc…)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes QueuedPoolBits only has 1 bit.

Moving worst txns down from pending and baseFee to Queued does happen still. Just that, discarding of txns from queued based on EnoughFeeCapProtocol won't be there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, make sense for me now

queued.Add(pending.PopWorst(), logger)
} else {
discard(pending.PopWorst(), txpoolcfg.FeeTooLow)
queued.Add(pending.PopWorst(), logger)
}
}

Expand All @@ -1567,11 +1546,7 @@ func promote(pending *PendingPool, baseFee, queued *SubPool, pendingBaseFee uint

// Demote worst transactions that do not qualify for base fee pool anymore, to queued sub pool, or discard
for worst := baseFee.Worst(); baseFee.Len() > 0 && worst.subPool < BaseFeePoolBits; worst = baseFee.Worst() {
if worst.subPool >= QueuedPoolBits {
somnathb1 marked this conversation as resolved.
Show resolved Hide resolved
queued.Add(baseFee.PopWorst(), logger)
} else {
discard(baseFee.PopWorst(), txpoolcfg.FeeTooLow)
}
queued.Add(baseFee.PopWorst(), logger)
}

// Promote best transactions from the queued pool to either pending or base fee pool, while they qualify
Expand All @@ -1586,9 +1561,7 @@ func promote(pending *PendingPool, baseFee, queued *SubPool, pendingBaseFee uint
}

// Discard worst transactions from the queued sub pool if they do not qualify
for worst := queued.Worst(); queued.Len() > 0 && worst.subPool < QueuedPoolBits; worst = queued.Worst() {
somnathb1 marked this conversation as resolved.
Show resolved Hide resolved
discard(queued.PopWorst(), txpoolcfg.FeeTooLow)
}
// <FUNCTIONALITY REMOVED>

// Discard worst transactions from pending pool until it is within capacity limit
for pending.Len() > pending.limit {
Expand Down
9 changes: 0 additions & 9 deletions erigon-lib/txpool/pool_fuzz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,6 @@ func FuzzOnNewBlocks(f *testing.F) {
if tx.subPool&NoNonceGaps > 0 {
assert.GreaterOrEqual(i.Nonce, senders[i.SenderID].nonce, msg, i.SenderID)
}
if tx.subPool&EnoughFeeCapProtocol > 0 {
assert.LessOrEqual(calcProtocolBaseFee(pendingBaseFee), tx.Tx.FeeCap, msg)
}
if tx.subPool&EnoughFeeCapBlock > 0 {
assert.LessOrEqual(pendingBaseFee, tx.Tx.FeeCap, msg)
}
Expand Down Expand Up @@ -370,9 +367,6 @@ func FuzzOnNewBlocks(f *testing.F) {
if tx.subPool&NoNonceGaps > 0 {
assert.GreaterOrEqual(i.Nonce, senders[i.SenderID].nonce, msg)
}
if tx.subPool&EnoughFeeCapProtocol > 0 {
assert.LessOrEqual(calcProtocolBaseFee(pendingBaseFee), tx.Tx.FeeCap, msg)
}
if tx.subPool&EnoughFeeCapBlock > 0 {
assert.LessOrEqual(pendingBaseFee, tx.Tx.FeeCap, msg)
}
Expand All @@ -391,9 +385,6 @@ func FuzzOnNewBlocks(f *testing.F) {
if tx.subPool&NoNonceGaps > 0 {
assert.GreaterOrEqual(i.Nonce, senders[i.SenderID].nonce, msg, i.SenderID, senders[i.SenderID].nonce)
}
if tx.subPool&EnoughFeeCapProtocol > 0 {
assert.LessOrEqual(calcProtocolBaseFee(pendingBaseFee), tx.Tx.FeeCap, msg)
}
if tx.subPool&EnoughFeeCapBlock > 0 {
assert.LessOrEqual(pendingBaseFee, tx.Tx.FeeCap, msg)
}
Expand Down
Loading