feat: accept multiple URLs for checkpoint sync#374
Conversation
Greptile SummaryThis PR makes
Confidence Score: 4/5Safe to merge with minor log message improvements recommended. The failover logic is correct and backward-compatible. Two non-blocking issues exist: a warning message that says "trying next URL" even when there is no next URL to try, and a startup log that prints full URL strings which could expose embedded credentials in environments that use auth-bearing URLs. bin/ethlambda/src/main.rs — the warning log wording and the startup info log that prints all URLs verbatim.
|
| Filename | Overview |
|---|---|
| bin/ethlambda/src/main.rs | Extends --checkpoint-sync-url to accept a Vec of URLs with comma-delimiter support; adds try_checkpoint_url helper for per-URL retry; fetch_initial_state now iterates the slice and fails over sequentially. Two minor issues: the "trying next URL" warning fires even when no next URL exists, and the startup info log prints all URLs verbatim which could expose embedded credentials. |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
bin/ethlambda/src/main.rs:639-658
**Misleading "trying next URL" on last/only failure**
The warning message `"Checkpoint sync failed for this peer; trying next URL"` is emitted unconditionally after every failed attempt, including the final URL in the list (and the only URL in the single-URL case). When startup is about to abort with no more peers to try, operators will see the log say "trying next URL" right before the process exits, which is confusing. Consider checking whether there are remaining URLs before deciding which message to emit — or drop the "trying next URL" suffix and log the total count of remaining peers instead.
### Issue 2 of 2
bin/ethlambda/src/main.rs:636
**Potential credential exposure in startup log**
`info!(urls = ?checkpoint_urls, ...)` prints all URLs via their `Debug` representation, which includes the full URL string. If any URL contains embedded authentication material (basic-auth credentials or token query parameters), they will appear in plaintext in the log stream. Consider logging only the count of URLs, or stripping auth components before logging.
Reviews (1): Last reviewed commit: "feat: accept multiple URLs for checkpoin..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let Err(err) = &result { | ||
| warn!( | ||
| url = %first_url, | ||
| %err, | ||
| "Checkpoint sync failed for this peer; trying next URL" | ||
| ); | ||
| } | ||
|
|
||
| let mut attempt = 1; | ||
| let (state, signed_block) = loop { | ||
| match checkpoint_sync::fetch_finalized_anchor( | ||
| checkpoint_url, | ||
| genesis.genesis_time, | ||
| &validators, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(pair) => break pair, | ||
| Err(checkpoint_sync::CheckpointSyncError::AnchorPairingMismatch) | ||
| if attempt < MAX_ANCHOR_FETCH_ATTEMPTS => | ||
| { | ||
| warn!( | ||
| attempt, | ||
| max = MAX_ANCHOR_FETCH_ATTEMPTS, | ||
| "Anchor state and block disagree (peer likely advanced finalization mid-fetch); retrying" | ||
| ); | ||
| tokio::time::sleep(ANCHOR_FETCH_RETRY_DELAY).await; | ||
| attempt += 1; | ||
| } | ||
| Err(err) => return Err(err), | ||
| for url in rest_urls { | ||
| if result.is_ok() { | ||
| break; | ||
| } | ||
| }; | ||
| result = try_checkpoint_url(url, genesis.genesis_time, &validators).await; | ||
| if let Err(err) = &result { | ||
| warn!( | ||
| %url, | ||
| %err, | ||
| "Checkpoint sync failed for this peer; trying next URL" | ||
| ); | ||
| } |
There was a problem hiding this comment.
The warning message "Checkpoint sync failed for this peer; trying next URL" is emitted unconditionally after every failed attempt, including the final URL in the list (and the only URL in the single-URL case). When startup is about to abort with no more peers to try, operators will see the log say "trying next URL" right before the process exits, which is confusing. Consider checking whether there are remaining URLs before deciding which message to emit — or drop the "trying next URL" suffix and log the total count of remaining peers instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/ethlambda/src/main.rs
Line: 639-658
Comment:
**Misleading "trying next URL" on last/only failure**
The warning message `"Checkpoint sync failed for this peer; trying next URL"` is emitted unconditionally after every failed attempt, including the final URL in the list (and the only URL in the single-URL case). When startup is about to abort with no more peers to try, operators will see the log say "trying next URL" right before the process exits, which is confusing. Consider checking whether there are remaining URLs before deciding which message to emit — or drop the "trying next URL" suffix and log the total count of remaining peers instead.
How can I resolve this? If you propose a fix, please make it concise.| // Checkpoint sync path | ||
| info!(%checkpoint_url, "Starting checkpoint sync"); | ||
| // Checkpoint sync path: try URLs in order, fail over to the next on error. | ||
| info!(urls = ?checkpoint_urls, "Starting checkpoint sync"); |
There was a problem hiding this comment.
info!(urls = ?checkpoint_urls, ...) prints all URLs via their Debug representation, which includes the full URL string. If any URL contains embedded authentication material (basic-auth credentials or token query parameters), they will appear in plaintext in the log stream. Consider logging only the count of URLs, or stripping auth components before logging.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/ethlambda/src/main.rs
Line: 636
Comment:
**Potential credential exposure in startup log**
`info!(urls = ?checkpoint_urls, ...)` prints all URLs via their `Debug` representation, which includes the full URL string. If any URL contains embedded authentication material (basic-auth credentials or token query parameters), they will appear in plaintext in the log stream. Consider logging only the count of URLs, or stripping auth components before logging.
How can I resolve this? If you propose a fix, please make it concise.1 participant
🗒️ Description / Motivation
Adds redundancy to checkpoint sync by letting operators supply more than one peer URL via
--checkpoint-sync-url. The flag now accepts either a single URL (existing behavior), a comma-separated list, or multiple repeated occurrences. URLs are tried sequentially: the first peer that successfully serves a valid anchor wins, and we only fail startup if every URL fails. It is backward compatible. PR closes #111What Changed
bin/ethlambda/src/main.rs--checkpoint-sync-urlis nowOption<Vec<String>>withvalue_delimiter = ',', sou1,u2and repeated--checkpoint-sync-urlflags both populate the list. A single URL still works unchanged (backwards-compatible).try_checkpoint_url, which wraps the existing per-URLAnchorPairingMismatchretry (3 attempts, 1s backoff). No behavior change for the single-URL case.fetch_initial_statenow takes&[String], iterates URLs in order, logs awarn!with the URL and underlying error on each failure, and moves on to the next. Returns the last error only if every URL fails.Correctness / Behavior Guarantees
try_checkpoint_url, not changed).checkpoint_sync.rs— every peer's anchor is still validated against the local genesis config and internal state/block consistency before it is accepted.Tests Added / Run
main.rsorchestration code that drives the already-testedfetch_finalized_anchor/verify_checkpoint_statepaths. Existingcheckpoint_synctests (verification + URL normalization) still pass.cargo clippy -p ethlambda --all-targets -- -D warningscargo test --workspace --releaseRelated Issues / PRs
✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleancargo test --workspace --release— all passing