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 trustedNodeSync logic from default CLI command flow #6598

Closed

Conversation

chirag-parmar
Copy link

@chirag-parmar chirag-parmar commented Oct 5, 2024

Changelog

  • Removes the external-beacon-api-url from the default CLI command (noCommand)
  • marks related CLI options trusted-state-root and trusted-block-root as deprecated in the default CLI flow
  • tidies deprecated options to be processed at one place
  • updates descriptions for deprecated options

Rationale

  • View the discussion here
  • The logic is stale and doesn't adapt well with many centralized checkpoint providers

@chirag-parmar chirag-parmar force-pushed the remove-tns-default-cli branch 2 times, most recently from f1c42fb to 0fec41b Compare October 5, 2024 15:37
Copy link

github-actions bot commented Oct 5, 2024

Unit Test Results

0 files   -          9  0 suites   - 1 355   0s ⏱️ - 39m 50s
0 tests  -   5 152  0 ✔️  -   4 804  0 💤  - 348  0 ±0 
0 runs   - 21 531  0 ✔️  - 21 127  0 💤  - 404  0 ±0 

Results for commit 0fec41b. ± Comparison against base commit ac32170.

@chirag-parmar chirag-parmar force-pushed the remove-tns-default-cli branch from 0fec41b to b9fc8d6 Compare October 5, 2024 16:31
@etan-status
Copy link
Contributor

The logic is stale and doesn't adapt well with many centralized checkpoint providers

ethpandaops/checkpointz#143

checkpointz is GPL-3.0 licensed so I personally prefer not touching it (risk of tainting our own permissive code with GPL), but in principle someone could simply adjust it to provide the LC data, making this mechanism work fine with --trusted-block-root parameter (it already works with --trusted-state-root).

@chirag-parmar
Copy link
Author

chirag-parmar commented Oct 11, 2024

Closing this PR for the rationale below

There are two logical flows for a checkpoint sync in Nimbus.

  1. Using the trustedNodeSync subcommand, which by default grabs the finalized state from either a centralized checkpoint server or a manually configured trusted node (--trusted-node-url). The subcommand also accepts two other parameters --trusted-state-root, for specifying an explicit state that the user trusts in, OR, --trusted-block-root for specifying an explicit block root against which light client data is queried for the sync (explanation).
  2. Using --external-beacon-api-url which provides the same parameters(--trusted-state-root and --trusted-block-root) as above but does not provide a default behaviour. This mandates either a state root or a block root to be provided.

The rationale for removing --external-beacon-api-url(this PR) was to remove redundancy in code and to require an explicit interaction from the user for the "trusted" nature of the sync. However, since the second flow of sync requires explicit specification of a state root (or block root) it still maintains the explicit interactions while providing a 'one-shot' command to sync AND start the node.

This PR solves all our problems for the long term scenario

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.

2 participants