Adjust fee currency blacklisting by piersy · Pull Request #345 · celo-org/op-geth

piersy · 2025-03-27T15:02:38Z

No description provided.

miner/hash_blocklist.go

eth/api_admin.go

palango · 2025-03-27T16:13:18Z

miner/currency_blocklist.go

 	headerEvictionTimeoutSeconds uint64
 	oldestHeader                 *types.Header
+
+	enactFilterEnabled *atomic.Int32


Why not use atomic.Bool?

That's a very good question... somehow I thought it doesn't exist

miner/worker.go

We want to be verbose when the API handler fails, since the blocklist is assumed to not be nil

mcortesi · 2025-03-27T17:42:00Z

miner/tx_ringbuffer.go

+	}
+
+	// Clean up old transaction hash from map
+	oldHash := b.ring[b.index]


wonder what happens when the ring is not yet full... this returns zero and delete(zero) doesn't break?

It will be the empty hash, and delete the empty hash which will be a no-op.

mcortesi · 2025-03-27T17:42:55Z

miner/worker.go

 		common.CurrencyAllowlist(env.feeCurrencyContext.ExchangeRates),
 		header,
 	)
+	env.blockingAllowed = miner.feeCurrencyBlocklist.GetEnableFilterStatus()


is this done for every block?

Yep, env is constructed per block at line 331

Yes, every time a new payload is requested to be built by the engine-api a new environment is created

mcortesi · 2025-03-27T17:44:06Z

miner/worker.go

 			}
 		}
+		if miner.blockedTxRingbuffer.Has(ltx.Hash) {
+			log.Trace("Skipping tx execution, tx in blockedTxRingbuffer", "hash", ltx.Hash)


trace? maybe some level we had for skipping a tx due to fee currency lock

This would be logged on every block build attempt, so every 1 second

This is the same level we had for skipping a tx due to the fee currency lock, see line 597

mcortesi · 2025-03-27T17:45:40Z

miner/worker.go

 	// so that they are not allowlisted in the following blocks
 	// (only locally in the txpool, not consensus-critical)
-	miner.feeCurrencyBlocklist.Add(feeCurrency, *env.header)
+	miner.feeCurrencyBlocklist.Add(*tx.FeeCurrency(), *env.header)


we're sure that tx.FeeCurrency is never nil here?

Previously this function was being called with the de-referenced fee currency here (line 427, github seems to have an issue correctly scrolling to that point) so I'm as sure as we previously were that fee currency is not nil.

miner/miner.go

seolaoh · 2025-03-28T09:58:19Z

eth/api_admin.go

+	api.eth.Miner().UnblockTransaction(hash)
+}
+
+func (api *AdminAPI) UnblockFeeCurrency(address common.Address) {


Would it be helpful to have getter API for blocked fee currency list or tx list?

Why not. I think the fee-currency is not strictly necessary since we have the "block" and "evict" log messages.
The transactions would be helpful, because this is only logged with trace level.

Yeah I think so. TBH I want to go over this whole blocking feature again and ensure the design is clean. Yesterday we were somewhat in a hurry so didn't have the time to consider all the niceties that you normally would.

miner/tx_ringbuffer.go

Co-authored-by: Seola Oh <osa8361@gmail.com>

ezdac · 2025-04-03T13:19:48Z

I pulled out the fixes for deactivating the blocklist on l1 derivation / pending building to #363.
This PR is separate (without the changes here), but I have a local copy of the branch that keeps the changes in this branch here into account.

seolaoh

LGTM, just some nits

seolaoh · 2025-04-04T08:57:47Z

miner/currency_blocklist.go

+	return b.enactFilterEnabled.Load()
+}
+
+// This only activates / deactivates wether the


Suggested change

// This only activates / deactivates wether the

// SetEnableFilterStatus only activates / deactivates whether the

seolaoh · 2025-04-04T08:58:01Z

miner/worker.go

 	receipt, err := miner.applyTransaction(env, tx)
 	if err != nil {
 		if errors.Is(err, contracts.ErrFeeCurrencyEVMCall) {
+


Suggested change

piersy · 2025-04-04T09:06:44Z

LGTM, just some nits

Hey @seolaoh, thanks we actually decided to split this up, so there will be some smaller PRs arriving soon, first one here - #363

piersy · 2025-04-04T09:07:07Z

Closing in favour of 3 more targeted PRs

mcortesi reviewed Mar 27, 2025

View reviewed changes

miner/hash_blocklist.go Outdated Show resolved Hide resolved

palango reviewed Mar 27, 2025

View reviewed changes

eth/api_admin.go Outdated Show resolved Hide resolved

ezdac and others added 3 commits March 27, 2025 16:24

Allow to set filter status on address blocklist

9545dae

Add setter for currency blocklist filter status

16276fc

Add rpc call to enable/disable the fee currency blocklist

1143d87

ezdac force-pushed the piersy/adjust-fee-currency-blacklisting branch from 006e714 to 1143d87 Compare March 27, 2025 15:27

Fix camelcase in admin api method

c20186a

ezdac force-pushed the piersy/adjust-fee-currency-blacklisting branch from 1b82392 to c20186a Compare March 27, 2025 15:30

piersy and others added 2 commits March 27, 2025 15:31

Add tx ringbuffer

750cf72

Fix invalidating currency gas-pool when blocking disabled

8227088

palango reviewed Mar 27, 2025

View reviewed changes

ezdac added 2 commits March 27, 2025 17:23

Add RW mutex to tx-ringbuffer

08aa267

Fix adding tx to ringbuffer multiple times

732a539

seolaoh reviewed Mar 27, 2025

View reviewed changes

miner/worker.go Show resolved Hide resolved

piersy and others added 5 commits March 27, 2025 16:34

Connect transaction blocking

840824b

Add admin methods

5fc3967

Add unblock-fee currency method to miner

0f1f5dd

Add unblock transaction

ad22809

Remove nil check in fee-currency-blocklist setter

1fcc294

We want to be verbose when the API handler fails, since the blocklist is assumed to not be nil

piersy marked this pull request as ready for review March 27, 2025 16:45

Use atomic.Bool in address-blocklist

ed193d8

ezdac force-pushed the piersy/adjust-fee-currency-blacklisting branch from b7c39f9 to ed193d8 Compare March 27, 2025 16:47

piersy added 2 commits March 27, 2025 16:50

Fix ringbuffer remove locking

f2b577b

Return bool to indicate whether removal actions had any effect

30da047

mcortesi reviewed Mar 27, 2025

View reviewed changes

seolaoh reviewed Mar 28, 2025

View reviewed changes

Update miner/miner.go

431589c

Co-authored-by: Seola Oh <osa8361@gmail.com>

Add metrics-counter to blocklists

4e2425a

ezdac force-pushed the piersy/adjust-fee-currency-blacklisting branch 2 times, most recently from 304d0fc to 4e2425a Compare April 3, 2025 12:53

seolaoh approved these changes Apr 4, 2025

View reviewed changes

piersy closed this Apr 4, 2025

	// This only activates / deactivates wether the
	// SetEnableFilterStatus only activates / deactivates whether the

Conversation

piersy commented Mar 27, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piersy Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ezdac commented Apr 3, 2025

Uh oh!

seolaoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piersy commented Apr 4, 2025

Uh oh!

piersy commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

piersy Mar 28, 2025 •

edited

Loading

piersy commented Apr 4, 2025 •

edited

Loading