Skip to content
This repository was archived by the owner on Aug 20, 2022. It is now read-only.

fix l1 fee cache, rpc, tracing and tx pool#31

Merged
protolambda merged 1 commit intooptimism-prototypefrom
l1-fee-fixes
Jul 11, 2022
Merged

fix l1 fee cache, rpc, tracing and tx pool#31
protolambda merged 1 commit intooptimism-prototypefrom
l1-fee-fixes

Conversation

@protolambda
Copy link
Copy Markdown
Contributor

@protolambda protolambda commented Jul 8, 2022 •
edited
Loading

This PR fixes:

  • Use chainConfig.OptimismConfig != nil instead of double hainConfig.OptimismConfig != nil && OptimismConfig.Enabled
  • L1 cost computation is now split between:
    • The L1 gas total, cached in Transaction for performance (repeatedly used in tx pool and state transition etc.)
    • The L1 fee parameters, cached in a closure, which is part of the BlockContext, just like other state related tx context
  • The ApplyTransaction now behaves the same as the block processing, fixing the caching bug. The cache is not filled when deposits are processed, so it'll use the latest l1 info data (introduced with a deposit at start of block)
  • Fix circular dependency issue with contract addresses.
  • Fix tx pool: transactions now get filtered based on L1 cost, both when adding, and when promoting/demoting txs (when fees are checked too).
  • Fix RPC: L1 cost is now applied everywhere, by initializing the BlockContext everywhere where AsMessage was being called.
    • Historical state production with tx replay
    • Tracing RPC

Works with op-e2e tests on monorepo develop branch

@protolambda protolambda force-pushed the l1-fee-fixes branch 3 times, most recently from f013e38 to bee7bc3 Compare July 8, 2022 19:29
@protolambda protolambda mentioned this pull request Jul 8, 2022
Merged
@protolambda protolambda marked this pull request as ready for review July 8, 2022 19:45
trianglesphere
trianglesphere reviewed Jul 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

I gave parts of this a light skim. The context functions seems like it makes sense. I bet I can rework the unmetering PR to not use the msg options interface.

Comment thread core/rollup_l1_cost.go Outdated
Comment thread core/rollup_l1_cost.go
tynes
tynes reviewed Jul 11, 2022
View reviewed changes
Comment thread core/types/transaction.go Outdated
tynes
tynes reviewed Jul 11, 2022
View reviewed changes
Comment thread params/config.go
tynes
tynes reviewed Jul 11, 2022
View reviewed changes
Comment thread core/rollup_l1_cost.go
tynes
tynes reviewed Jul 11, 2022
View reviewed changes
Comment thread core/rollup_l1_cost.go
tuxcanfly
tuxcanfly reviewed Jul 11, 2022
View reviewed changes
Comment thread core/rollup_l1_cost.go
@protolambda protolambda force-pushed the l1-fee-fixes branch 2 times, most recently from 734a9a9 to d9da8f7 Compare July 11, 2022 16:40
tynes
tynes reviewed Jul 11, 2022
View reviewed changes
Comment thread core/types/transaction.go
}

// RollupDataGas is the amount of gas it takes to confirm the tx on L1 as a rollup
func (tx *Transaction) RollupDataGas() uint64 {
Copy link
Copy Markdown

@tynes tynes Jul 11, 2022

Choose a reason for hiding this comment

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

This function will need to be hardforkable eventually

Copy link
Copy Markdown
Contributor Author

@protolambda protolambda Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Worst-case we deprecate the function, and call a different function. The l1 cost computation has access to both the chain config and current block number (in addition to the statedb), so there it's easy to hardfork:

reference-optimistic-geth/core/rollup_l1_cost.go

Line 49 in d9da8f7

rollupDataGas := msg.RollupDataGas() // Only fake txs for RPC view-calls are 0.

trianglesphere
trianglesphere approved these changes Jul 11, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

The only place I'm a little iffy on is the tx pool. I think it does work for now, but the semantics around the L1 Cost make it hard to integrate it into the tx pool.

Comment thread core/tx_pool.go
}
balance := pool.currentState.GetBalance(addr)
if !list.Empty() {
// Reduce the cost-cap by L1 rollup cost of the first tx if necessary. Other txs will get filtered out afterwards.
Copy link
Copy Markdown
Contributor

@trianglesphere trianglesphere Jul 11, 2022

Choose a reason for hiding this comment

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

I think this works, but it's a bit weird b/c there's no hard cap.

If there was a hard cap we could do something like enforce that the balance was enough to pay the cap & then place it into the tx priced list. Right now without the cap, it's not really clear when the tx would/could become valid again if it had low but present funds.

@protolambda
Copy link
Copy Markdown
Contributor Author

protolambda commented Jul 11, 2022

Seems like CI is flaky, TestFakedSyncProgress66Full runs fine for me locally and seems unrelated. Re-running it now

tuxcanfly
tuxcanfly approved these changes Jul 11, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@tuxcanfly tuxcanfly left a comment

Choose a reason for hiding this comment

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

OK.

@protolambda protolambda merged commit f579014 into optimism-prototype Jul 11, 2022
@protolambda protolambda deleted the l1-fee-fixes branch July 16, 2022 00:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@tynes tynes tynes left review comments

@tuxcanfly tuxcanfly tuxcanfly approved these changes

@trianglesphere trianglesphere trianglesphere approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@protolambda @tuxcanfly @tynes @trianglesphere