Skip to content
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

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

Closed
wants to merge 52 commits into from

Conversation

tynes
Copy link
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 6 commits May 13, 2021 17:15
* 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

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

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
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
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
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