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

Add ephemery network config #7563

Merged
merged 40 commits into from
Oct 4, 2024

Conversation

gconnect
Copy link
Contributor

@gconnect gconnect commented Sep 3, 2024

PR description

Fixed Issue(s)

  • Add Ephemery genesis file to the resources
  • Add Ephemery network to NetworkName
  • Add test for Ephemery to BesuCommandTest
  • Add Ephemery to NetworkDeprecationTest file
  • Add Ephemery genesis function to automatically generate a new genesis file in line with the spec requirements for Ephermery Testnet
  • Add test for the genesis generation function

This PR is in response to this issue 7404

@non-fungible-nelson please review when you have some time.

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@gconnect
Copy link
Contributor Author

gconnect commented Sep 3, 2024

@macfarla you can go over the requested changes here.

@macfarla macfarla self-assigned this Sep 5, 2024
@gconnect
Copy link
Contributor Author

@macfarla all requested updates have been made.

Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

ok think it is getting close! a few nits and suggestions on naming

@gconnect
Copy link
Contributor Author

@macfarla made the requested changes thanks for reviewing.

@gconnect
Copy link
Contributor Author

@macfarla I am done with all the requested changes please review when you have the time. @Gabriel-Trintinalia please also assist with reviewing when you have the time thanks.

Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

couple of final comments on the test class, otherwise LGTM

changelog has a conflict

Copy link
Contributor

@jflo jflo left a comment

Choose a reason for hiding this comment

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

Fantastic contribution, thanks so much. The only thing I would add (doesn't have to be in this PR, would be fine in a new one) is maybe some Ephemery specific logging when the network rolls over. Right now, the user will get:

2024-10-01 08:09:00.429-04:00 | main | ERROR | Besu | Failed to start Besu
picocli.CommandLine$ExecutionException: Supplied genesis block does not match chain data stored in /Users/jflo/src/besu/build/data.
Please specify a different data directory with --data-path, specify the original genesis file with --genesis-file or supply a testnet/mainnet option with --network.

It might be worth adding a line explaining that if Ephemery rolled over, they need to reset their data, and this is to be expected.

@gconnect
Copy link
Contributor Author

gconnect commented Oct 1, 2024

Fantastic contribution, thanks so much. The only thing I would add (doesn't have to be in this PR, would be fine in a new one) is maybe some Ephemery specific logging when the network rolls over. Right now, the user will get:

2024-10-01 08:09:00.429-04:00 | main | ERROR | Besu | Failed to start Besu
picocli.CommandLine$ExecutionException: Supplied genesis block does not match chain data stored in /Users/jflo/src/besu/build/data.
Please specify a different data directory with --data-path, specify the original genesis file with --genesis-file or supply a testnet/mainnet option with --network.

It might be worth adding a line explaining that if Ephemery rolled over, they need to reset their data, and this is to be expected.

@jflo thanks for this. Once this PR is merged the automatic database reset feature is what I will be working on next. That should fix the above issue.

@gconnect
Copy link
Contributor Author

gconnect commented Oct 1, 2024

@macfarla I have made the requested changes.

@macfarla macfarla enabled auto-merge (squash) October 2, 2024 01:03
auto-merge was automatically disabled October 2, 2024 07:59

Head branch was pushed to by a user without write access

@macfarla macfarla enabled auto-merge (squash) October 3, 2024 21:32
@macfarla macfarla disabled auto-merge October 3, 2024 21:52
@macfarla
Copy link
Contributor

macfarla commented Oct 3, 2024

@gconnect there's a failing test

BesuCommandTest > ephemeryValuesAreUsed() FAILED
org.opentest4j.AssertionFailedError at BesuCommandTest.java:1877

Signed-off-by: gconnect <[email protected]>
@macfarla macfarla enabled auto-merge (squash) October 4, 2024 01:01
@macfarla macfarla merged commit 67d738c into hyperledger:main Oct 4, 2024
43 checks passed
@macfarla
Copy link
Contributor

macfarla commented Oct 4, 2024

Thanks for your contribution on this @gconnect 🎉

@gconnect
Copy link
Contributor Author

gconnect commented Oct 4, 2024

Thanks for your contribution on this @gconnect 🎉

Thanks for reviewing.

2fehee added a commit to 2fehee/besu that referenced this pull request Oct 4, 2024
* Add Ephemery genesis config file

Signed-off-by: gconnect <[email protected]>

---------

Signed-off-by: gconnect <[email protected]>
Signed-off-by: Glory Agatevure <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>Signed-off-by: Chulhee lee <[email protected]>
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