Patch txpool core/list for managing multi currencies by hbandura · Pull Request #43 · celo-org/op-geth

hbandura · 2024-01-19T13:45:48Z

Patches the txpool for multi currency. Validation, List (per user nonce heap), TxComparator(pricedHeap)

core/txpool/legacypool/celo_legacypool.go

core/txpool/celo_rates.go

palango

Nice work, to me this feels like a better approach that the celo_list. Do you agree?

Some general notes:

A short PR description is always helpful, with some hints on what might be surprising or some key design choices.
Some tests are failing, I didn't check what causes them.
There's still todos in the code, especially around error handling and the L1 cost. I didn't look into those.
Are there other thing you intent on working in this PR?

One question around the design: There's lot's of new objects, and I'm not sure why you don't use functions instead. For example, the whitelistcheck struct is only created in NewWhitelistChecker, which is only used in LegacyPool.filter. Its IsWhitelisted method just passes down the request to FeeCurrencyValidator. For me this seems a bit meritless and I'd either use the validator directly or even the free IsWhitelisted function with the exchange rate stored in the txpool. What do you think?

contracts/fee_currencies.go

core/txpool/celo_rates.go

core/txpool/legacypool/list.go

hbandura · 2024-01-30T16:11:20Z

Nice work, to me this feels like a better approach that the celo_list. Do you agree?

Yes, but the celo_list approach had not be done to look cleaner, but to avoid the most common issues I used to have when merging upstream. However it might make sense to compromise a bit here.

Some general notes:

A short PR description is always helpful, with some hints on what might be surprising or some key design choices.
You are right, sorry about that.

Some tests are failing, I didn't check what causes them.

I'm fixing them on the go, I didn't want to wait to start the review, I doubt they will need a big change to be fixed (I can always be surprised)

There's still todos in the code, especially around error handling and the L1 cost. I didn't look into those.

Everything regarding L1Cost was intentional, I'm not sure if we are clear on how we want to manage that in the pool.
The logging I want to see how we are logging elsewhere first.

Are there other thing you intent on working in this PR?

No, unless there's something spotted in this review that I missed.

One question around the design: There's lot's of new objects, and I'm not sure why you don't use functions instead. For example, the whitelistcheck struct is only created in NewWhitelistChecker, which is only used in LegacyPool.filter. Its IsWhitelisted method just passes down the request to FeeCurrencyValidator. For me this seems a bit meritless and I'd either use the validator directly or even the free IsWhitelisted function with the exchange rate stored in the txpool. What do you think?

Many of this choices were made step by step and when you reach the final product you start cutting back, at least I already removed some as soon as I created the PR.
The specific you mentioned, whitelistchecker, was because I didn't want the list to have the stateDb, so I fixed that in a different object and passed that.
Still, I have been thinking in remove the whole state hierarchy call and use an exchangerate fixed in the pool for mostly everything except balances. I will attempt to make some of those changes and see how it looks.

core/txpool/celo_rates_test.go

core/txpool/celo_rates.go

core/txpool/celo_validation.go

core/txpool/legacypool/list.go

core/celo_backend.go

core/celo_evm.go

core/txpool/celo_rates.go

core/txpool/blobpool/blobpool.go

core/txpool/celo_validation.go

core/txpool/validation.go

core/celo_backend.go

core/txpool/blobpool/celo_blobpool.go

core/txpool/blobpool/blobpool.go

core/txpool/legacypool/legacypool.go

core/txpool/legacypool/list.go

core/txpool/legacypool/celo_list.go

core/celo_backend.go

core/txpool/celo_rates.go

core/txpool/legacypool/legacypool.go

core/txpool/legacypool/list.go

@palango

CRU Move comment line Rename ToCurrencyValue to DenominateInCurrency list modification for celo changes list.Add and list.FilterBalance not working yet Cherry-pick of 704201f (@palango fee currency management) list.Add & some work on rates Add tests for list totalcost & whitelist filter Refactor FilterBalance for clarity & testiness Add test for list FilterBalance (gaslimit pruning) Add txcomparator Use ratesComparator for pricedList Add implementation of tx comparator Implement GetRatesFromState checking balance from txpool & lint fixes Ignore l1cost for the moment Add translate value impl Remove 'areequaladdresses' since golang pointer comparisons work Fix nil pointer Change functions signature type Change isCurrencyWhitelisted function to common Fix comment Add non-whitelisted test Fix some tests failing Changes list to use costCap in multicurrency fashion Remove translator Remove whitelist checker Remove unnecessary struct CRU Move getbalance to celobackend Add nonce replacement test to e2e tests Remove feeCurrencyValidator Move GetExchangeRates function to CeloBackend Add log Add current rates to legacypool Add more test cases for nonce replacement bump Code review updates remove txComparator as type, alias as function CRU

Fixes already made

Modifies the legacypool, list, and priceheap to allow for multi currency txs.

…m test (#43, #55, #293) Co-authored-by: Pastoh <hbandura@users.noreply.github.com> Co-authored-by: kourin <yoshiki.takabayashi@clabs.co> e2e(js): Loosen error check condition in e2e-test (#380) Due to possible expansion of the responses error string in potential proxy setups on a live RPC endpoint, the strict error check in the e2e viem test caused the test to fail, although the error condition was expected. e2e(js): Add test of sending CIP-64 tx with unregistered fee currency in viem e2e test e2e(js): Run e2e tests on alfajores (#212, #258, #268, #293) Adds a test of transactions that exceed the max intrinsic gas for a fee currency, this checks that the transaction fails and the fee currency is blocked. This entailed increasing gas limits for a number of transactions that interact with fee currencies, since the fee currencies on alfajores are different to our debug ones and also tests needed to be updated to not rely on hardcoded fee currency rates. Co-authored-by: Maximilian Langenfeld <15726643+ezdac@users.noreply.github.com> e2e(js): Get extra intrinsic gas for custom fee currency in e2e (#335) * Get IntrinsicGas for custom fee currency in e2e * Rename function name and add comments * Add missing await * Fix wrong method call * Fix codes based reviews

Modifies the legacypool, list, and priceheap to allow for multi currency txs. Validate debitGasFees execution in tx pool Txs must not fail during debitGasFees execution during the state transition, so we must remove problematic txs during the tx pool validation. We do this by executing debitGasFees inside the tx pool validation and discarding the resulting state changes. This is not overly efficient, but still better than the alternatives that have been considered so far.