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.

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

core/txpool/legacypool: add support for SetCode transactions #31073

Merged
merged 15 commits into from
Feb 11, 2025
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
6 changes: 4 additions & 2 deletions core/txpool/blobpool/blobpool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ func (bc *testBlockChain) CurrentBlock() *types.Header {
GasLimit: gasLimit,
BaseFee: baseFee,
ExcessBlobGas: &excessBlobGas,
Difficulty: common.Big0,
}
}

Expand Down Expand Up @@ -1565,8 +1566,9 @@ func TestAdd(t *testing.T) {
if tt.block != nil {
// Fake a header for the new set of transactions
header := &types.Header{
Number: big.NewInt(int64(chain.CurrentBlock().Number.Uint64() + 1)),
BaseFee: chain.CurrentBlock().BaseFee, // invalid, but nothing checks it, yolo
Number: big.NewInt(int64(chain.CurrentBlock().Number.Uint64() + 1)),
Difficulty: common.Big0,
BaseFee: chain.CurrentBlock().BaseFee, // invalid, but nothing checks it, yolo
}
// Inject the fake block into the chain
txs := make([]*types.Transaction, len(tt.block))
Expand Down
9 changes: 9 additions & 0 deletions core/txpool/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,13 @@ var (
// input transaction of non-blob type when a blob transaction from this sender
// remains pending (and vice-versa).
ErrAlreadyReserved = errors.New("address already reserved")

// ErrAuthorityReserved is returned if a transaction has an authorization
// signed by an address which already has in-flight transactions known to the
// pool.
ErrAuthorityReserved = errors.New("authority already reserved")

// ErrAuthorityNonce is returned if a transaction has an authorization with
// a nonce that is not currently valid for the authority.
ErrAuthorityNonceTooLow = errors.New("authority nonce too low")
)
75 changes: 67 additions & 8 deletions core/txpool/legacypool/legacypool.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"math"
"math/big"
"slices"
"sort"
"sync"
"sync/atomic"
Expand Down Expand Up @@ -263,7 +264,7 @@ func New(config Config, chain BlockChain) *LegacyPool {
// pool, specifically, whether it is a Legacy, AccessList or Dynamic transaction.
func (pool *LegacyPool) Filter(tx *types.Transaction) bool {
switch tx.Type() {
case types.LegacyTxType, types.AccessListTxType, types.DynamicFeeTxType:
case types.LegacyTxType, types.AccessListTxType, types.DynamicFeeTxType, types.SetCodeTxType:
return true
default:
return false
Expand Down Expand Up @@ -540,7 +541,8 @@ func (pool *LegacyPool) validateTxBasics(tx *types.Transaction) error {
Accept: 0 |
1<<types.LegacyTxType |
1<<types.AccessListTxType |
1<<types.DynamicFeeTxType,
1<<types.DynamicFeeTxType |
1<<types.SetCodeTxType,
MaxSize: txMaxSize,
MinTip: pool.gasTip.Load().ToBig(),
}
Expand All @@ -565,6 +567,11 @@ func (pool *LegacyPool) validateTx(tx *types.Transaction) error {
if list := pool.queue[addr]; list != nil {
have += list.Len()
}
if pool.currentState.GetCodeHash(addr) != types.EmptyCodeHash || len(pool.all.auths[addr]) != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think len(pool.all.auths[addr]) != 0 could become an attack vector? Consider the following scenario:

  • An authorization w is created and included in transaction x.
  • An attacker copies the same authorization w and includes it in a new transaction y, submitting y to the network with a relatively lower gas price.
  • If a node receives transaction y first, it will reject transaction x.
  • Furthermore, if x contains multiple authorizations, constructing and submitting a conflicting transaction like y could effectively prevent the entire authorization list in x from being included.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, the len(pool.all.auths[addr]) != 0 is used to limit the number of txs an account can queue when they have the ability to sweep their balance from underneath the txpool w/o originating a tx to do so. Without this check, what is possible:

A submits a tx which sets code in B. During the tx, B sweeps its balance. Immediately after A submits tx1, B submits 16 txs to the pool. When A is mined first, it invalidates all txs from B. You can expand the A's tx to include many accounts, now A can invalidate many txs from many accounts.

By only allowing a single tx from accounts who have code / will potentially have code soon, we can reduce the potency of this attack.

Originally I blocked this attack, by not allowing a tx from an account with a pending delegation, but this leads to the attack Gary described. Now that we allow 1 tx from the account with a pending delegation, we can avoid that account getting deadlocked.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this example, X and Y are from different senders, say A an B respectively. Even if X is from A and includes the delegation for A, when B copies the auth and puts it in Y, the node will still accept X because although it has a pending delegation, it will still have 1 free slot for the account A. This does mean B can limit the number of txs A can queue.

The only scenario the tx will be rejected is as follows: suppose A submits an auth to bundler B. Say B submits the auth in tx Y. If A is able to queue a tx X before nodes start recieving Y then Y will be rejected.

Copy link
Member

Choose a reason for hiding this comment

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

A submits a tx which sets code in B. During the tx, B sweeps its balance. Immediately after A submits tx1, B submits 16 txs to the pool. When A is mined first, it invalidates all txs from B. You can expand the A's tx to include many accounts, now A can invalidate many txs from many accounts.

Will this have a serious impact? I think we have the similar issues nowadays in the legacy pool. Let's say account a has 16 transactions with continuous nonces. If the first transaction drains the balance of A and the following 15 transactions will be invalidated later.

In the scenario you mentioned, tx can set code for a list of accounts [B, C, D, ..., Z] and meantime these accounts can submit a tons of pending transactions. However, these accounts must sign an auth in which the balance will be drained by the code. Although the scale of this invalidation seems to be magnified, but it should be feasible to submit legacy pending transactions from multiple accounts and invalidate the remaining by using balance in the first tx?

Maybe I miss something

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I understand this. The quote you have here was me explaining an attack that we avoid by limiting the account to 1 pending tx? I don't think you can easily replace all the txs pending for an account because we check the total value of the stack?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can easily replace all the txs pending for an account because we check the total value of the stack?

Yeah right. There is no way to drain the balance of an EOA without the deployed code!

// Allow at most one in-flight tx for delegated accounts or those with
// a pending authorization.
return have, max(0, 1-have)
}
return have, math.MaxInt
},
ExistingExpenditure: func(addr common.Address) *big.Int {
Expand All @@ -581,6 +588,18 @@ func (pool *LegacyPool) validateTx(tx *types.Transaction) error {
}
return nil
},
KnownConflicts: func(from common.Address, auths []common.Address) []common.Address {
var conflicts []common.Address
// Authorities cannot conflict with any pending or queued transactions.
for _, addr := range auths {
if list := pool.pending[addr]; list != nil {
conflicts = append(conflicts, addr)
} else if list := pool.queue[addr]; list != nil {
conflicts = append(conflicts, addr)
}
}
return conflicts
},
}
if err := txpool.ValidateTransactionWithState(tx, pool.signer, opts); err != nil {
return err
Expand Down Expand Up @@ -1334,15 +1353,13 @@ func (pool *LegacyPool) promoteExecutables(accounts []common.Address) []*types.T
// Drop all transactions that are deemed too old (low nonce)
forwards := list.Forward(pool.currentState.GetNonce(addr))
for _, tx := range forwards {
hash := tx.Hash()
pool.all.Remove(hash)
pool.all.Remove(tx.Hash())
}
log.Trace("Removed old queued transactions", "count", len(forwards))
// Drop all transactions that are too costly (low balance or out of gas)
drops, _ := list.Filter(pool.currentState.GetBalance(addr), gasLimit)
for _, tx := range drops {
hash := tx.Hash()
pool.all.Remove(hash)
pool.all.Remove(tx.Hash())
}
log.Trace("Removed unpayable queued transactions", "count", len(drops))
queuedNofundsMeter.Mark(int64(len(drops)))
Expand Down Expand Up @@ -1531,8 +1548,8 @@ func (pool *LegacyPool) demoteUnexecutables() {
drops, invalids := list.Filter(pool.currentState.GetBalance(addr), gasLimit)
for _, tx := range drops {
hash := tx.Hash()
log.Trace("Removed unpayable pending transaction", "hash", hash)
pool.all.Remove(hash)
log.Trace("Removed unpayable pending transaction", "hash", hash)
}
pendingNofundsMeter.Mark(int64(len(drops)))

Expand Down Expand Up @@ -1641,12 +1658,15 @@ type lookup struct {
slots int
lock sync.RWMutex
txs map[common.Hash]*types.Transaction

auths map[common.Address][]common.Hash // All accounts with a pooled authorization
}

// newLookup returns a new lookup structure.
func newLookup() *lookup {
return &lookup{
txs: make(map[common.Hash]*types.Transaction),
txs: make(map[common.Hash]*types.Transaction),
auths: make(map[common.Address][]common.Hash),
}
}

Expand Down Expand Up @@ -1697,13 +1717,15 @@ func (t *lookup) Add(tx *types.Transaction) {
slotsGauge.Update(int64(t.slots))

t.txs[tx.Hash()] = tx
t.addAuthorities(tx)
}

// Remove removes a transaction from the lookup.
func (t *lookup) Remove(hash common.Hash) {
t.lock.Lock()
defer t.lock.Unlock()

t.removeAuthorities(hash)
tx, ok := t.txs[hash]
if !ok {
log.Error("No transaction found to be deleted", "hash", hash)
Expand All @@ -1727,6 +1749,43 @@ func (t *lookup) TxsBelowTip(threshold *big.Int) types.Transactions {
return found
}

// addAuthorities tracks the supplied tx in relation to each authority it
// specifies.
func (t *lookup) addAuthorities(tx *types.Transaction) {
for _, addr := range tx.SetCodeAuthorities() {
list, ok := t.auths[addr]
if !ok {
list = []common.Hash{}
}
if slices.Contains(list, tx.Hash()) {
// Don't add duplicates.
continue
}
list = append(list, tx.Hash())
t.auths[addr] = list
}
}

// removeAuthorities stops tracking the supplied tx in relation to its
// authorities.
func (t *lookup) removeAuthorities(hash common.Hash) {
for addr := range t.auths {
list := t.auths[addr]
// Remove tx from tracker.
if i := slices.Index(list, hash); i >= 0 {
list = append(list[:i], list[i+1:]...)
} else {
log.Error("Authority with untracked tx", "addr", addr, "hash", hash)
}
if len(list) == 0 {
// If list is newly empty, delete it entirely.
delete(t.auths, addr)
continue
}
t.auths[addr] = list
}
}

// numSlots calculates the number of slots needed for a single transaction.
func numSlots(tx *types.Transaction) int {
return int((tx.Size() + txSlotSize - 1) / txSlotSize)
Expand Down
Loading