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

Remove miner from erigon command #124

Merged
merged 4 commits into from
Feb 29, 2024
Merged

Remove miner from erigon command #124

merged 4 commits into from
Feb 29, 2024

Conversation

boyuan-chen
Copy link

Remove miner command

@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.21%. Comparing base (ee1a647) to head (81bd9a4).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop     #124       +/-   ##
============================================
- Coverage    41.71%   28.21%   -13.50%     
============================================
  Files           74      168       +94     
  Lines         4898     7294     +2396     
  Branches       765     1311      +546     
============================================
+ Hits          2043     2058       +15     
- Misses        2749     5130     +2381     
  Partials       106      106               
Flag Coverage Δ
cannon-go-tests 82.20% <ø> (ø)
chain-mon-tests 27.14% <ø> (ø)
common-ts-tests 26.72% <ø> (ø)
contracts-bedrock-tests 0.62% <ø> (?)
contracts-ts-tests 12.25% <ø> (ø)
core-utils-tests 44.03% <ø> (ø)
sdk-next-tests 38.59% <ø> (ø)
sdk-tests 38.59% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 94 files with indirect coverage changes

@@ -15,9 +14,6 @@ COMMON_FLAGS=" \

ERIGON_FLAGS=" \
${COMMON_FLAGS} \
--mine \

Choose a reason for hiding this comment

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

Removing the etherbase and the sigfile seems fine, but if --mine isn't enabled, then I don't believe erigon will build blocks and act as a sequencer.

I just tried removing it in op-e2e/extrernal_erigon/main.go and the tests simply hang for me. If we're removing it here, I'd think we should remove it there as well?

Copy link
Author

Choose a reason for hiding this comment

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

I have removed it in our sequencer node. It's still building new blocks.

Choose a reason for hiding this comment

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

Looks like this breaks the e2e in CI as well -- does it work on your system?

Copy link
Author

Choose a reason for hiding this comment

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

No, I am trying to figure out why. The integration tests work, but op-e2e tests don't.

Copy link
Author

@boyuan-chen boyuan-chen Feb 28, 2024

Choose a reason for hiding this comment

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

Ok, figured out. The Regeneration ended observation should be removed. Geth also has this --mine option and it is not enabled in the op-e2e tests. I think we should be good to remove it.
https://github.com/ethereum-optimism/op-geth/blob/0402d543c3d0cff3a3d344c0f4f83809edb44f10/cmd/utils/flags.go#L457-L461

@boyuan-chen boyuan-chen requested a review from jyellick February 29, 2024 00:29
@@ -160,10 +158,6 @@ func execute(binPath string, config external.Config) (*erigonSession, error) {
}
fmt.Fscanf(sess.Err, "%d", &enginePort)
fmt.Printf("================== op-erigon shim got engine port %d ==========================\n", enginePort)
gm.Eventually(sess.Err, time.Minute).Should(gbytes.Say("Regeneration ended"))

Choose a reason for hiding this comment

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

I think we should probably look for another message that indicates startup is done -- and we can hopefully eliminate the sleep below. But that's outside the scope of this PR, I'll take a look and submit separately.

@boyuan-chen boyuan-chen merged commit a9bcb6f into develop Feb 29, 2024
70 checks passed
@boyuan-chen boyuan-chen deleted the remove-miner branch February 29, 2024 18:07
boyuan-chen pushed a commit that referenced this pull request Nov 12, 2024
* feat: add IERC7802 (#123)

* feat: add IERC7802

* fix: token address fuzz error

* feat: remove ERC165 contract inheritance

* feat: add IERC20 interface support (#124)

* feat: add IERC20 interface support

* fix: interfaces tests
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.

4 participants