-
Notifications
You must be signed in to change notification settings - Fork 19
Adjust fee currency blacklisting #345
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
Changes from all commits
9545dae
16276fc
1143d87
c20186a
750cf72
8227088
08aa267
732a539
840824b
5fc3967
0f1f5dd
ad22809
1fcc294
ed193d8
f2b577b
30da047
431589c
4e2425a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| package miner | ||
|
|
||
| import ( | ||
| "sync" | ||
|
|
||
| "github.com/ethereum/go-ethereum/common" | ||
| ) | ||
|
|
||
| // TxRingBuffer is a ring buffer of transaction hashes, it has one method to add | ||
| // transaction hashes and another method to check if a transaction hash is in the buffer. | ||
| type TxRingBuffer struct { | ||
| ring []common.Hash | ||
| m map[common.Hash]struct{} | ||
| index int | ||
| mux *sync.RWMutex | ||
| } | ||
|
|
||
| // NewTxRingBuffer creates a new transaction ring buffer with the given size. | ||
| func NewTxRingBuffer(size int) *TxRingBuffer { | ||
| return &TxRingBuffer{ | ||
| ring: make([]common.Hash, size), | ||
| m: make(map[common.Hash]struct{}), | ||
| mux: &sync.RWMutex{}, | ||
| } | ||
| } | ||
|
|
||
| // Add adds a new transaction hash to the ring buffer, if the buffer is | ||
| // at capacity, the oldest transaction hash is overwritten. | ||
| func (b *TxRingBuffer) Add(tx common.Hash) bool { | ||
| b.mux.Lock() | ||
| defer b.mux.Unlock() | ||
|
|
||
| if _, exists := b.m[tx]; exists { | ||
| return false | ||
| } | ||
|
|
||
| // Clean up old transaction hash from map | ||
| oldHash := b.ring[b.index] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wonder what happens when the ring is not yet full... this returns zero and delete(zero) doesn't break?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will be the empty hash, and delete the empty hash which will be a no-op. |
||
| delete(b.m, oldHash) | ||
|
|
||
| // Add new transaction hash to ring and map | ||
| b.ring[b.index] = tx | ||
| b.m[tx] = struct{}{} | ||
|
|
||
| // Increment the index | ||
| b.index++ | ||
| if b.index == len(b.ring) { | ||
| b.index = 0 | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| func (b *TxRingBuffer) Has(tx common.Hash) bool { | ||
| b.mux.RLock() | ||
| defer b.mux.RUnlock() | ||
|
|
||
| _, exists := b.m[tx] | ||
| return exists | ||
| } | ||
|
|
||
| func (b *TxRingBuffer) Remove(tx common.Hash) bool { | ||
| b.mux.Lock() | ||
| defer b.mux.Unlock() | ||
|
|
||
| if _, exists := b.m[tx]; !exists { | ||
| return false | ||
| } | ||
| delete(b.m, tx) | ||
|
seolaoh marked this conversation as resolved.
|
||
| return true | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -61,6 +61,9 @@ var ( | |||
| txConditionalMinedTimer = metrics.NewRegisteredTimer("miner/transactionConditional/elapsedtime", nil) | ||||
|
|
||||
| txInteropRejectedCounter = metrics.NewRegisteredCounter("miner/transactionInterop/rejected", nil) | ||||
|
|
||||
| feeCurrenciesInBlocklistCounter = metrics.NewRegisteredCounter("miner/blocklist/feeCurrency/blocked", nil) | ||||
| transactionsInBlocklistCounter = metrics.NewRegisteredCounter("miner/blocklist/transactions/blocked", nil) | ||||
| ) | ||||
|
|
||||
| // environment is the worker's current environment and holds all | ||||
|
|
@@ -87,6 +90,7 @@ type environment struct { | |||
| // Celo specific | ||||
| multiGasPool *core.MultiGasPool // available per-fee-currency gas used to pack transactions | ||||
| feeCurrencyAllowlist common.AddressSet | ||||
| blockingAllowed bool | ||||
| feeCurrencyContext *common.FeeCurrencyContext | ||||
| } | ||||
|
|
||||
|
|
@@ -340,11 +344,13 @@ func (miner *Miner) prepareWork(genParams *generateParams, witness bool) (*envir | |||
| "evicted-fee-currencies", evicted, | ||||
| "eviction-timeout-seconds", EvictionTimeoutSeconds, | ||||
| ) | ||||
| feeCurrenciesInBlocklistCounter.Dec(int64(len(evicted))) | ||||
| } | ||||
| env.feeCurrencyAllowlist = miner.feeCurrencyBlocklist.FilterAllowlist( | ||||
| common.CurrencyAllowlist(env.feeCurrencyContext.ExchangeRates), | ||||
| header, | ||||
| ) | ||||
| env.blockingAllowed = miner.feeCurrencyBlocklist.GetEnableFilterStatus() | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this done for every block?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, env is constructed per block at line 331 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, every time a new payload is requested to be built by the engine-api a new |
||||
|
|
||||
| if header.ParentBeaconRoot != nil { | ||||
| core.ProcessBeaconBlockRoot(*header.ParentBeaconRoot, env.evm) | ||||
|
|
@@ -418,13 +424,14 @@ func (miner *Miner) commitTransaction(env *environment, tx *types.Transaction) e | |||
| receipt, err := miner.applyTransaction(env, tx) | ||||
| if err != nil { | ||||
| if errors.Is(err, contracts.ErrFeeCurrencyEVMCall) { | ||||
|
|
||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
| log.Warn( | ||||
| "fee-currency EVM execution error, temporarily blocking fee-currency in local txpools", | ||||
| "tx-hash", tx.Hash(), | ||||
| "fee-currency", tx.FeeCurrency(), | ||||
| "error", err.Error(), | ||||
| ) | ||||
| miner.blockFeeCurrency(env, *tx.FeeCurrency(), err) | ||||
| miner.registerFeeCurrencyTxFailure(env, tx, err) | ||||
| } | ||||
| return err | ||||
| } | ||||
|
|
@@ -596,6 +603,12 @@ func (miner *Miner) commitTransactions(env *environment, plainTxs, blobTxs *tran | |||
| continue | ||||
| } | ||||
| } | ||||
| if miner.blockedTxRingbuffer.Has(ltx.Hash) { | ||||
| log.Trace("Skipping tx execution, tx in blockedTxRingbuffer", "hash", ltx.Hash) | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. trace? maybe some level we had for skipping a tx due to fee currency lock There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be logged on every block build attempt, so every 1 second
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same level we had for skipping a tx due to the fee currency lock, see line 597 |
||||
| txs.Pop() | ||||
| continue | ||||
| } | ||||
|
|
||||
| // If we don't have enough space for the next transaction, skip the account. | ||||
| if env.gasPool.Gas() < ltx.Gas { | ||||
| log.Trace("Not enough gas left for transaction", "hash", ltx.Hash, "left", env.gasPool.Gas(), "needed", ltx.Gas) | ||||
|
|
@@ -834,14 +847,23 @@ func (miner *Miner) validateParams(genParams *generateParams) (time.Duration, er | |||
| return time.Duration(blockTime) * time.Second, nil | ||||
| } | ||||
|
|
||||
| func (miner *Miner) blockFeeCurrency(env *environment, feeCurrency common.Address, err error) { | ||||
| func (miner *Miner) registerFeeCurrencyTxFailure(env *environment, tx *types.Transaction, err error) { | ||||
| // the fee-currency is still in the allowlist of this environment, | ||||
| // so set the fee-currency block gas limit to 0 to prevent other | ||||
| // transactions. | ||||
| pool := env.multiGasPool.PoolFor(&feeCurrency) | ||||
| pool.SetGas(0) | ||||
| if env.blockingAllowed { | ||||
|
seolaoh marked this conversation as resolved.
|
||||
| pool := env.multiGasPool.PoolFor(tx.FeeCurrency()) | ||||
| pool.SetGas(0) | ||||
| } | ||||
| // also add the fee-currency to a worker-wide blocklist, | ||||
| // so that they are not allowlisted in the following blocks | ||||
| // (only locally in the txpool, not consensus-critical) | ||||
| miner.feeCurrencyBlocklist.Add(feeCurrency, *env.header) | ||||
| if miner.feeCurrencyBlocklist.Add(*tx.FeeCurrency(), *env.header) { | ||||
| feeCurrenciesInBlocklistCounter.Inc(1) | ||||
| } | ||||
|
|
||||
| // Add tx to block tx ringbuffer | ||||
| if miner.blockedTxRingbuffer.Add(tx.Hash()) { | ||||
| transactionsInBlocklistCounter.Inc(1) | ||||
| } | ||||
| } | ||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.