Conversation
arnaubennassar
changed the title
|
|
Claude finished @joanestebanr's task in 2m 5s —— View job Code Review for PR #1570🚀 What's New
🐛 Bug FixesNone 📋 Config UpdatesNone
|
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
There was a problem hiding this comment.
Pull request overview
This PR hardens AggSender startup recovery against stale/conflicting local certificate state (treating AggLayer as authoritative), and extends the backward-forward-let tooling/runbooks to support staging drills (including crafting and sending certificates without persisting them locally).
Changes:
- AggSender startup recovery now deletes/replaces conflicting local cert entries instead of failing, syncing local state back to AggLayer.
backward-forward-letaddscraft-cert(staging-gated) and enhancessend-certwith--no-db; diagnosis/reporting logic is refined for partial results.- Documentation/runbooks are updated to reflect the operator workflow and fallback procedures when aggsender exit data is unavailable.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/backward_forward_let/types.go | Adds a helper for “complete + no divergence” diagnosis checks. |
| tools/backward_forward_let/send_cert.go | Adds --no-db path to send certs without persisting in aggsender DB. |
| tools/backward_forward_let/send_cert_test.go | Adds coverage for --no-db and CLI flag wiring. |
| tools/backward_forward_let/run.go | Uses the new “complete no divergence” helper to avoid false “nothing to do”. |
| tools/backward_forward_let/diagnosis.go | Adjusts printing flow and changes bridge-service NotFound handling to fail fast. |
| tools/backward_forward_let/diagnosis_test.go | Adds tests for partial results and updates bridge-service NotFound expectations. |
| tools/backward_forward_let/craft_cert.go | Introduces craft-cert command implementation and multi-source exit reconstruction logic. |
| tools/backward_forward_let/craft_cert_test.go | Adds comprehensive tests for crafted cert behavior and fallback reconstruction paths. |
| tools/backward_forward_let/config.go | Extends tool config to reuse AggSender.AggsenderPrivateKey for craft-cert signing. |
| tools/backward_forward_let/cmd/main.go | Registers craft-cert and adds --no-db to send-cert. |
| tools/backward_forward_let/RECOVERY_PROCEDURE.md | Documents admin API auth header option and operational staging notes. |
| tools/backward_forward_let/README.md | Adds full tool/command documentation including staging drill flows. |
| docs/backward_forward_let_runbook.md | Rewrites runbook to a tool-driven operator workflow and fallback steps. |
| aggsender/statuschecker/initial_state.go | Adds delete-local-cert recovery actions and makes reconciliation permissive. |
| aggsender/statuschecker/initial_state_test.go | Updates expectations to validate new recovery actions (including delete/insert sequencing). |
| aggsender/statuschecker/cert_status_checker.go | Executes the new delete-local-cert action and surfaces warnings. |
| aggsender/statuschecker/cert_status_checker_test.go | Adds coverage for executing delete-local-cert actions. |
| .gitignore | Ignores debug/ and bin/ directories. |
Comments suppressed due to low confidence (1)
tools/backward_forward_let/cmd/main.go:104
- The
send-certcommandUsagestring still says it records the cert in the aggsender DB, but this PR makes DB storage optional via--no-db. Update the help text to reflect the new behavior, and consider validating flag combinations (e.g., require exactly one of--db-pathor--no-db, and error if both are set) to avoid surprising no-op/ignored flags.
Name: "send-cert",
Usage: "Send a certificate to the agglayer and record it in the aggsender DB",
Flags: []cli.Flag{
&cli.StringFlag{
Name: "cert-json",
Usage: "Certificate JSON string (mutually exclusive with --cert-file)",
| return nil, fmt.Errorf("parse --amount %q as decimal big.Int", c.String("amount")) | ||
| } | ||
|
|
||
| originTokenAddr := common.HexToAddress(c.String("origin-token-address")) |
| func makeFakeBridgeExit(opts *craftCertOptions, exitIndex int) *agglayertypes.BridgeExit { | ||
| addrBytes := crypto.Keccak256(append(append([]byte(nil), opts.nonce...), byte(exitIndex))) | ||
| return &agglayertypes.BridgeExit{ |
| return nil, err | ||
| } | ||
| if cert == nil { | ||
| return nil, fmt.Errorf("certificate not found") |
| func callCraftCertRPCWithTimeout[T any](fn func() (T, error)) (T, error) { | ||
| type result struct { | ||
| value T | ||
| err error | ||
| } | ||
|
|
||
| resultCh := make(chan result, 1) | ||
| go func() { | ||
| value, err := fn() | ||
| resultCh <- result{value: value, err: err} | ||
| }() | ||
|
|
||
| select { | ||
| case result := <-resultCh: | ||
| return result.value, result.err | ||
| case <-time.After(craftCertRPCRequestTimeout): | ||
| var zero T | ||
| return zero, fmt.Errorf("aggsender RPC request timed out after %s", craftCertRPCRequestTimeout) | ||
| } |
🔄 Changes Summary
aggsenderstartup recovery so stale local certificates no longer block progress: when local state is ahead of or conflicts with AggLayer, recovery now treats AggLayer as the source of truth, deletes stale local certs, and reinserts the correct settled/latest cert state when needed.backward-forward-letwith staging-drill tooling:craft-certcommand to build and sign malicious certificates for controlled staging exercises,send-certto run with--no-dbso a cert can be sent to AggLayer without persisting it in the aggsender DB,craft-certfallback behavior so it can reconstruct historical leaves from aggsender RPC, aggsender DB, bridge service, and certificate-exit override data.None
📋 Config Updates
None
✅ Testing
go test ./aggsender/statuscheckergo test ./tools/backward_forward_letdevelop...feat/imporve-bw-fw-lettooldiff against the updated docs/runbook to ensure the documented recovery and staging-drillflows match the implemented CLI behavior.
🐞 Issues
🔗 Related PRs
📝 Notes
craft-certis explicitly gated behind--staging-onlyand is intended for staging drills, not production operation.