[Draft] Add database reset on Ephemery Network #8626

gconnect · 2024-09-21T02:38:54Z

PR Description

Add deposit chainId to the generated network.yml file
Reset the db

Todo

Still need to ensure that the above only happens on Ephemery

Doing this tends not to work as expected

  final Eth2NetworkConfiguration networkConfig =
        Eth2NetworkConfiguration.builder(Eth2Network.EPHEMERY).build();

    if (networkConfig.getEth2Network().equals(Optional.of(Eth2Network.EPHEMERY))) {
      ....
    }

Still figuring how to handle the check.

Fixed Issue(s)

"fixes #8589"

Documentation

I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

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

gconnect · 2024-09-21T02:45:35Z

@rolfyone please take a look when you have the time.

rolfyone · 2024-09-22T20:10:35Z

Thanks for raising, will review shortly. One initial comment -
We'll only want 'partially addresses' in the 'fixes, because this would otherwise close the issue...

rolfyone · 2024-09-22T20:12:15Z

storage/src/main/java/tech/pegasys/teku/storage/server/network/DatabaseNetwork.java

    if (source.exists()) {
      final DatabaseNetwork databaseNetwork =
          objectMapper.readerFor(DatabaseNetwork.class).readValue(source);

+      // Fork version validation


we can remove these comments, they don't really add to the readability of the code... (basically all of the comments in this file at least)

Alright, I will remove it.

gconnect · 2024-09-22T20:18:43Z

Thanks for raising, will review shortly. One initial comment - We'll only want 'partially addresses' in the 'fixes, because this would otherwise close the issue...

Not sure what you meant here.

rolfyone

We could potentially treat this as a 'tracer' PR and just create a new branch with what we want in the first one... Alternatively we need to remove all of the functionality other than the network yaml change
Basically, I need a PR that just changes the network yaml and doesn't break anything when reading the old configuration file that doesn't have the chain id in it... This is really critical, because thats basically ensuring its not a breaking change.

rolfyone · 2024-09-22T20:15:28Z

storage/src/main/java/tech/pegasys/teku/storage/server/network/DatabaseNetwork.java

-      DatabaseNetwork databaseNetwork =
-          new DatabaseNetwork(forkVersionString, depositContractString);
+      // Handle Ephemery or other networks
+      String depositChainIdString = depositChainId.orElse(null);


rolfyone · 2024-09-22T20:16:08Z

storage/src/main/java/tech/pegasys/teku/storage/server/network/DatabaseNetwork.java

+      System.out.println(
+          "network " + NETWORK_CONFIG.getEth2Network().equals(Optional.of(Eth2Network.EPHEMERY)));


we don't want System.out in our code, we'd want to be using loggers... Is this just a test message?

we don't want System.out in our code, we'd want to be using loggers... Is this just a test message?

Yes it a test message, forgot to remove it before pushing

rolfyone · 2024-09-22T20:24:25Z

storage/src/main/java/tech/pegasys/teku/storage/server/network/DatabaseNetwork.java

+      if (NETWORK_CONFIG.getEth2Network().equals(Optional.of(Eth2Network.EPHEMERY))) {
+        databaseNetwork =
+            new DatabaseNetwork(forkVersionString, depositContractString, depositChainIdString);
+      } else {
+        databaseNetwork = new DatabaseNetwork(forkVersionString, depositContractString, null);
+      }


We can just always write deposit_chain_id to the file which makes this a lot more readable. The conditional check I'd be more worried about would be ensuring we don't crash if it's not there on startup once we're checking that.

rolfyone · 2024-09-22T20:25:01Z

storage/src/main/java/tech/pegasys/teku/storage/server/network/DatabaseNetwork.java

@@ -95,11 +140,12 @@ public boolean equals(final Object o) {
    }
    final DatabaseNetwork that = (DatabaseNetwork) o;
    return Objects.equals(forkVersion, that.forkVersion)
-        && Objects.equals(depositContract, that.depositContract);
+        && Objects.equals(depositContract, that.depositContract)
+        && Objects.equals(depositChainId, that.depositChainId); // Compare deposit_chain_id


remove comment

rolfyone · 2024-09-22T20:27:19Z

storage/src/main/java/tech/pegasys/teku/storage/server/network/DatabaseNetwork.java

    this.forkVersion = forkVersion;
    this.depositContract = depositContract;
+    this.depositChainId = depositChainId; // Can be null for non-Ephemery networks


remove comment - will also be null for any old databases that ARE ephemery that you open... we can't assume all ephemery will have this data.
Also though it's less important, people just have to manually reset their db (it'll literally be a dev - you or me basically)

I do also think that we could have a constructor with just the old interface and default the new field to null, which would simplify testing and make it clearer...

rolfyone · 2024-09-22T20:28:23Z

storage/src/main/java/tech/pegasys/teku/storage/server/network/DatabaseNetwork.java

+      if (NETWORK_CONFIG.getEth2Network().equals(Optional.of(Eth2Network.EPHEMERY))) {
+
+        // Deposit chain ID validation
+        if (depositChainId.isPresent()
+            && databaseNetwork.depositChainId != null
+            && !databaseNetwork.depositChainId.equals(depositChainId.get())) {
+          throw DatabaseStorageException.unrecoverable(
+              formatMessage(
+                  "deposit chain ID",
+                  depositChainId.get(),
+                  String.valueOf(databaseNetwork.depositChainId)));
+        }
+      }


defer this check to another PR please. it'll make testing a lot easier to see.

rolfyone · 2024-09-22T20:31:52Z

storage/src/test/java/tech/pegasys/teku/storage/server/network/DatabaseNetworkTest.java

@@ -37,11 +37,12 @@ public void shouldCreateNetworkFile(@TempDir final File tempDir) throws IOExcept
    assertThat(networkFile).doesNotExist();
    final Bytes4 fork = dataStructureUtil.randomFork().getCurrentVersion();
    final Eth1Address eth1Address = dataStructureUtil.randomEth1Address();
-    assertThat(DatabaseNetwork.init(networkFile, fork, eth1Address))
+    assertThat(DatabaseNetwork.init(networkFile, fork, eth1Address, null))


i'd be surprised that any of these tests pass? we're pushing a null into an optional field which rarely ends well...
My suggestion above was to make the init not take an optional, so this will likely be a better answer after that...

rolfyone · 2024-09-22T20:32:50Z

storage/src/main/java/tech/pegasys/teku/storage/server/VersionedDatabaseFactory.java

+  // This implementation is required for Ephemery Network
+  private void resetDatabaseDirectories() {
+    File networkFile = getNetworkFile();
+
+    try {
+      if (dbDirectory.exists()) {
+        deleteDirectoryRecursively(dbDirectory.toPath());
+      }
+      if (v5ArchiveDirectory.exists()) {
+        deleteDirectoryRecursively(v5ArchiveDirectory.toPath());
+      }
+      if (networkFile.exists()) {
+        try {
+          LOG.info("Resetting network file at: {}", networkFile.getAbsolutePath());
+          Files.delete(networkFile.toPath());
+          System.out.println("Database directories reset successfully.");
+        } catch (IOException e) {
+          throw DatabaseStorageException.unrecoverable(
+              String.format("Failed to reset network file at %s", networkFile.getAbsolutePath()),
+              e);
+        }
+      }
+    } catch (IOException e) {
+      throw DatabaseStorageException.unrecoverable("Failed to reset database directories", e);
+    }
+  }
+
+  private void deleteDirectoryRecursively(final Path path) throws IOException {
+    if (Files.isDirectory(path)) {
+      try (var stream = Files.walk(path)) {
+        stream
+            .sorted((o1, o2) -> o2.compareTo(o1))
+            .forEach(
+                p -> {
+                  try {
+                    Files.delete(p);
+                  } catch (IOException e) {
+                    throw new RuntimeException("Failed to delete file: " + p, e);
+                  }
+                });
+      }
+    }
+  }


We'll defer this to another PR please, lets get the new field in and not breaking anything.

rolfyone · 2024-09-22T20:42:26Z

storage/src/main/java/tech/pegasys/teku/storage/server/VersionedDatabaseFactory.java

+          getNetworkFile(),
+          spec.getGenesisSpecConfig().getGenesisForkVersion(),
+          eth1Address,
+          Optional.of(String.valueOf(spec.getGenesisSpecConfig().getDepositChainId())));


i feel like at this level we just want the depositChainId as a long going in...

rolfyone · 2024-09-22T23:55:00Z

storage/src/main/java/tech/pegasys/teku/storage/server/network/DatabaseNetwork.java

+  static final Eth2NetworkConfiguration NETWORK_CONFIG =
+      Eth2NetworkConfiguration.builder(Eth2Network.EPHEMERY).build();


Ideally I think we avoid something like this... at the very least we just want to add our deposit chain id in this pr.

gconnect · 2024-09-23T00:57:52Z

We could potentially treat this as a 'tracer' PR and just create a new branch with what we want in the first one... Alternatively we need to remove all of the functionality other than the network yaml change Basically, I need a PR that just changes the network yaml and doesn't break anything when reading the old configuration file that doesn't have the chain id in it... This is really critical, because thats basically ensuring its not a breaking change.

What changes will I apply to the network.yaml file if I am not adding the chainId?

gconnect · 2024-09-23T03:41:45Z

opened a new PR here #8631

gconnect added 2 commits September 21, 2024 03:36

Add deposit chainId to the network.yml file and database reset

243f927

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

Merge branch 'master' into add-automatic-reset-for-ephemery-network

8777c9c

gconnect changed the title ~~Add deposit chainId to the network.yml file and database reset~~ [Draft] Add deposit chainId to the network.yml file and database reset Sep 21, 2024

gconnect changed the title ~~[Draft] Add deposit chainId to the network.yml file and database reset~~ [Draft] Add database reset on Ephemery Network Sep 21, 2024

rolfyone reviewed Sep 22, 2024

View reviewed changes

rolfyone reviewed Sep 23, 2024

View reviewed changes

gconnect closed this Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] Add database reset on Ephemery Network #8626

[Draft] Add database reset on Ephemery Network #8626

gconnect commented Sep 21, 2024 •

edited

Loading

gconnect commented Sep 21, 2024

rolfyone commented Sep 22, 2024

rolfyone Sep 22, 2024 •

edited

Loading

gconnect Sep 22, 2024

gconnect commented Sep 22, 2024

rolfyone left a comment

rolfyone Sep 22, 2024

rolfyone Sep 22, 2024

gconnect Sep 23, 2024

rolfyone Sep 22, 2024

rolfyone Sep 22, 2024

rolfyone Sep 22, 2024

rolfyone Sep 22, 2024

rolfyone Sep 22, 2024

rolfyone Sep 22, 2024

rolfyone Sep 22, 2024

rolfyone Sep 22, 2024

gconnect commented Sep 23, 2024

gconnect commented Sep 23, 2024

		System.out.println(
		"network " + NETWORK_CONFIG.getEth2Network().equals(Optional.of(Eth2Network.EPHEMERY)));

		static final Eth2NetworkConfiguration NETWORK_CONFIG =
		Eth2NetworkConfiguration.builder(Eth2Network.EPHEMERY).build();

[Draft] Add database reset on Ephemery Network #8626

[Draft] Add database reset on Ephemery Network #8626

Conversation

gconnect commented Sep 21, 2024 • edited Loading

PR Description

Todo

Fixed Issue(s)

Documentation

Changelog

gconnect commented Sep 21, 2024

rolfyone commented Sep 22, 2024

rolfyone Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gconnect commented Sep 22, 2024

rolfyone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gconnect commented Sep 23, 2024

gconnect commented Sep 23, 2024

gconnect commented Sep 21, 2024 •

edited

Loading

rolfyone Sep 22, 2024 •

edited

Loading