Skip to content

l2geth: remove the miner from the hot code path#875

Closed
tynes wants to merge 52 commits intodevelopfrom
l2geth/sync-spec-remove-miner-2
Closed

l2geth: remove the miner from the hot code path#875
tynes wants to merge 52 commits intodevelopfrom
l2geth/sync-spec-remove-miner-2

Conversation

@tynes
Copy link
Copy Markdown
Contributor

@tynes tynes commented May 14, 2021

Description
This is still WIP and builds on top of #552
Major changes

  • Removes the miner from the hot code path
  • Adds a new consensus.Engine specifically for the rollup

Some features do not appear to be working correctly yet, like logs are not being indexed. Will follow up and squash these commits

Additional context
This greatly reduces the diff and complexity of having the miner in the hot code path

tynes added 30 commits April 21, 2021 15:55
* l2geth: more in depth usage string

* l2geth: add standard client getters for index

* l2geth: refactor sync service into shared codepaths

* l2geth: clean up tests

* l2geth: better logging and error handling

* test: improve test coverage around timestamps

* l2geth: improve docstring

* l2geth: rename variable
tynes added 4 commits May 13, 2021 17:25
* dtl: update constant to 9 million gaslimit

* dtl: parse as rlp without top level type

* chore: add changeset

* test: delete dead code

* lint: fix

* deps: add etherproject transactions

* dtl: better handler for more number types

* tests: fix chainid
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented May 14, 2021

🦋 Changeset detected

Latest commit: f4fbf12

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@eth-optimism/data-transport-layer Patch
@eth-optimism/l2geth Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tynes tynes changed the base branch from develop to l2geth/sync-spec May 15, 2021 04:21
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #875 (3510a67) into l2geth/sync-spec (c38ec87) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##           l2geth/sync-spec     #875   +/-   ##
=================================================
  Coverage             82.21%   82.21%           
=================================================
  Files                    48       48           
  Lines                  1895     1895           
  Branches                303      303           
=================================================
  Hits                   1558     1558           
  Misses                  337      337           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c38ec87...3510a67. Read the comment docs.

Base automatically changed from l2geth/sync-spec to develop May 26, 2021 16:24
func (s *SyncService) transition() {
for {
select {
case tx := <-s.txCh:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is important that this channel is only big enough for 1 thing otherwise reorgs will be a pain since there will be txs buffered in the channel and we would have to empty the channel. If done like that then the lock is a bit redundant

@gakonst gakonst self-assigned this May 26, 2021
block, state := b.eth.miner.Pending()
return state, block.Header(), nil
if !vm.UsingOVM {
// Pending state is only known by the miner
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This probably should be hidden still

const (
PendingBlockNumber = BlockNumber(-2)
LatestBlockNumber = BlockNumber(-1)
EarliestBlockNumber = BlockNumber(0)

@gakonst gakonst mentioned this pull request Jun 13, 2021
3 tasks
@gakonst gakonst closed this Jun 13, 2021
@gakonst gakonst deleted the l2geth/sync-spec-remove-miner-2 branch June 13, 2021 14:26
OptimismBot pushed a commit that referenced this pull request Feb 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants