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

Deposit snapshot feature doesn't support some of the public endpoints #7806

Closed
zilm13 opened this issue Dec 8, 2023 · 0 comments · Fixed by #7808
Closed

Deposit snapshot feature doesn't support some of the public endpoints #7806

zilm13 opened this issue Dec 8, 2023 · 0 comments · Fixed by #7808
Assignees

Comments

@zilm13
Copy link
Contributor

zilm13 commented Dec 8, 2023

Description

A follow up to #7793
Deposit snapshot loading feature in Teku consists of 2 steps

  1. Loading. If it's local file it's just read, if it's url we try to download file with header requesting ssz by setting type application/octet-stream. Support of ssz type is a part of the spec, see: https://github.com/ethereum/beacon-APIs/blob/master/apis/beacon/deposit_snapshot.yaml
  2. Reading file. As we download and store it as ssz we expect to read only this type, so we read it as ssz.

Both steps are happen in DepositSnapshotFileLoader and few other classes it uses.

The issue is that since we start to use this feature along with --checkpoint-sync-url we are no longer run it only in Teku environment, most public endpoints use another CL-clients which doesn't support ssz output for DepositTreeSnapshot. I have checked following public endpoints:

All 3 endpoints do not support ssz application/octet-stream, only json type is supported.

So to make our feature work as intended, we should add a fallback to json type. It could be done in 2 ways:

  1. Simple. In catch block of DepositSnapshotFileLoader add call to something like loadJsonFromUrl, which should look like (trying to download as json, read as json):
  private DepositTreeSnapshot loadFromJsonUrl(final String path) throws IOException {
    final Bytes snapshotData =
        ResourceLoader.urlOrFile()
            .loadBytes(path)
            .orElseThrow(
                () -> new FileNotFoundException(String.format("File '%s' not found", path)));

    return JsonUtil.parse(new String(snapshotData.toArray(), StandardCharsets.UTF_8), DepositTreeSnapshot.getJsonTypeDefinition());
  }

Pros of this approach is that we are not only try to download it as json, we will try to read it as json even if local file is provided. Also another catch (IOException e) block should be added on failover retry.
2. Read the answer from url and if it has error code 415 {"code":415,"message":"unsupported content-type: application/octet-stream"}, try failover. It could be implemented a bit cleaner than 1 but we will get json support only for urls, not for local files

Also we may consider not exiting if deposit snapshot is not loaded, we have bundled snapshot, it's not a critical failure, but with failover to json implemented I think we are ok to exit in case of the failure, it should be very rare.

Steps to Reproduce (Bug)

$ ./teku [...skipped...] --checkpoint-sync-url=https://beaconstate.ethstaker.cc

[...skipped...]
17:03:02.245 INFO  - Loading deposit tree snapshot from https://beaconstate.ethstaker.cc/eth/v1/beacon/deposit_snapshot
17:03:02.249 INFO  - Syncing     *** Target slot: 7934713, Head slot: 7934592, Remaining slots: 121, Connected peers: 0
17:03:02.253 ERROR - Execution Client request failed. Make sure the Execution Client is online and can respond to requests.
Failed to load deposit tree snapshot from https://beaconstate.ethstaker.cc/eth/v1/beacon/deposit_snapshot: File 'https://beaconstate.ethstaker.cc/eth/v1/beacon/deposit_snapshot' not found

To display full help:
teku [COMMAND] --help

> Task :teku:Teku.main() FAILED
@zilm13 zilm13 assigned zilm13 and courtneyeh and unassigned zilm13 Dec 8, 2023
@zilm13 zilm13 changed the title Deposit snapshot feature doesn't support major public endpoints Deposit snapshot feature doesn't support some of public endpoints Dec 8, 2023
@zilm13 zilm13 changed the title Deposit snapshot feature doesn't support some of public endpoints Deposit snapshot feature doesn't support some of the public endpoints Dec 8, 2023
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 a pull request may close this issue.

3 participants