Conversation
arnaubennassar
changed the title
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) | ||
| } |
|
|
Claude finished @arnaubennassar's task in 2m 9s —— View job Code Review Complete ✅Reviewed PR #1570: fix: backward-forward-let 🚀 What's NewThis PR extends the 🐛 Bug Fixes
📋 Config UpdatesNone. The
|
🔄 Changes Summary
backward-forward-letwith the staging-drill workflow validated on a real staging network:craft-certto build and sign malicious certificates for controlled BFL recovery drills,send-cert --no-dbso a crafted certificate 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--cert-exits-fileoverride data,NoDivergence, and fail fast when bridge-service indexing is still behind.--cert-exits-filefallback procedure,craft-cert,send-cert --no-db,--cert-exits-filereuse during crafting).📋 Config Updates
craft-certreuses the existingAggSender.AggsenderPrivateKeysigner config by default, so existing shared signer backends such as GCP KMS continue to work.✅ Testing
go test ./aggsender/statuschecker/...go test ./tools/backward_forward_let/...NoDivergence🐞 Issues
🔗 Related PRs
📝 Notes
craft-certremains explicitly gated behind--staging-onlyand is intended for staging drills, not production operation.