feat: Imporve Remove GER tool#1563
Conversation
|
Claude finished @arnaubennassar's task in 55s —— View job Code Review Complete ✅Tasks
🚀 What's NewThis PR adds documentation for the required 🐛 Bug Fixes
📋 Config UpdatesThe [RemoveGER]
BridgeServiceURL = "http://localhost:8080"
L2NetworkID = 12 # New: required uint32 field for L2 network ID
SovereignAdminKey = { Method = "local", Path = "/path/to/keystore.json", Password = "pass" }
|
| Aspect | Rating | Notes |
|---|---|---|
| Correctness | ✅ Excellent | Both changes are correct and improve the tool |
| Documentation | ✅ Excellent | Clear, comprehensive documentation of the required field |
| Security | ✅ No Issues | No security implications |
| Performance | ✅ No Impact | Documentation and string formatting only |
| Testing | ✅ Adequate | The generate command can be manually tested; documentation changes don't need tests |
Summary
No blocking issues found. The PR successfully:
- Documents a previously undocumented required configuration field
- Fixes a command generation bug that would cause the generated
castcommands to fail - Maintains consistency with the existing documentation style
- Properly clarifies when the field is and isn't required
Recommendation: Approve and merge ✅
There was a problem hiding this comment.
Pull request overview
This PR expands the tools/remove_ger utility beyond “diagnose & recover” by adding a new scan-invalid-claims subcommand to discover invalid GER usage directly from L2 logs, and updates the recovery/diagnosis flow and documentation to support additional scenarios (e.g., claims still active after GER removal).
Changes:
- Add
scan-invalid-claimscommand that scans L2 claim logs, derives GERs per claim, validates them on L1, and reports invalid GER usage aggregated by GER. - Refactor recovery/diagnosis to support “claims exist even if GER already removed,” including gating GER removal steps and filtering inactive/unset claims.
- Update docs (README + runbook) and tests; introduce
Envabstractions to improve testability (receipt waiting, L2 bridge interface).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/remove_ger/scan.go | Implements log-based scanning and invalid-GER aggregation logic; adds scan env setup + CLI action. |
| tools/remove_ger/scan_test.go | Unit tests for calldata GER decode and invalid-GER usage aggregation. |
| tools/remove_ger/run.go | Refactors Env (bridge interface + receipt waiting hook) and updates main execution gating. |
| tools/remove_ger/recovery.go | Makes recovery conditional on required actions; adds emergency-state precheck; routes receipt waiting via Env. |
| tools/remove_ger/recovery_test.go | Adds test coverage for new emergency-state precheck behavior. |
| tools/remove_ger/helpers.go | Adds Env.waitReceipt(...) helper to enable test injection/fallback receipt waiting. |
| tools/remove_ger/generate.go | Adjusts generated cast command formatting for bytes32[32] array arguments. |
| tools/remove_ger/diagnosis.go | Adds recovery-action helpers, filters inactive claims, and improves recovery plan printing. |
| tools/remove_ger/diagnosis_test.go | Adds tests for new recovery-action predicates, plan building, and claim filtering. |
| tools/remove_ger/cmd/main.go | Wires in the new scan-invalid-claims CLI subcommand and flags. |
| tools/remove_ger/README.md | Documents scan-invalid-claims usage and clarifies config requirements (incl. L2NetworkID). |
| docs/remove_ger_runbook.md | Adds runbook section describing how to use the scan command for discovery. |
| const defaultScanChunkSize uint64 = 5000 | ||
|
|
||
| var ( | ||
| scanClaimEventSignature = crypto.Keccak256Hash([]byte("ClaimEvent(uint256,uint32,address,address,uint256)")) |
There was a problem hiding this comment.
scan-invalid-claims currently filters only the Etrog ClaimEvent signature plus DetailedClaimEvent. This will miss pre-Etrog ClaimEvent logs (different signature: ClaimEvent(uint32,uint32,address,address,uint256)), which the codebase already handles elsewhere. Consider adding the pre-Etrog signature to the topic filter and adding parsing logic for that event type (e.g., using the polygonzkevmbridge binding and converting Index -> GlobalIndex) so older claims are included in scans.
| scanClaimEventSignature = crypto.Keccak256Hash([]byte("ClaimEvent(uint256,uint32,address,address,uint256)")) | |
| scanClaimEventSignature = crypto.Keccak256Hash([]byte("ClaimEvent(uint256,uint32,address,address,uint256)")) | |
| scanLegacyClaimEventSignature = crypto.Keccak256Hash([]byte("ClaimEvent(uint32,uint32,address,address,uint256)")) |
| return fmt.Errorf("check IsEmergencyState before activateEmergencyState: %w", err) | ||
| } | ||
| if inEmergency { | ||
| return fmt.Errorf("bridge is already in emergency state before activateEmergencyState") |
There was a problem hiding this comment.
stepFreezeBridge returns an error if the bridge is already in emergency state. This makes the recovery flow non-resumable after a partial failure (e.g., if a previous run froze the bridge and then crashed), forcing manual intervention even though the desired state is already reached. Consider treating "already in emergency state" as a successful no-op (log and continue), and only sending ActivateEmergencyState when the bridge is not yet in emergency state.
| return fmt.Errorf("bridge is already in emergency state before activateEmergencyState") | |
| fmt.Println(" Bridge is already in emergency state; skipping activateEmergencyState") | |
| return nil |
| { | ||
| Name: "scan-invalid-claims", | ||
| Usage: "Scan L2 claims from a starting block and report GERs that are invalid on L1", | ||
| Flags: []cli.Flag{ | ||
| &cli.Uint64Flag{ | ||
| Name: "from-block", | ||
| Usage: "Starting L2 block number to scan (inclusive)", | ||
| Required: true, | ||
| }, | ||
| &cli.Uint64Flag{ | ||
| Name: "to-block", | ||
| Usage: "Ending L2 block number to scan (inclusive, defaults to latest L2 block)", | ||
| }, | ||
| &cli.Uint64Flag{ | ||
| Name: "chunk-size", | ||
| Usage: "Maximum L2 block range per eth_getLogs query", | ||
| Value: 5000, | ||
| }, | ||
| }, | ||
| Action: remove_ger.RunScanInvalidClaims, | ||
| }, |
There was a problem hiding this comment.
The PR title/description suggest a documentation-only change, but this diff also introduces a new CLI subcommand (scan-invalid-claims) and adds substantial new scanning/recovery logic and tests. Please update the PR title and/or description to reflect the actual scope so reviewers can assess risk and behavior changes appropriately.
arnaubennassar
changed the title
|
🔄 Changes Summary
develop, this PR expands theremove_gertool in four areas:scan-invalid-claimssubcommand that scans L2 claim logs, reconstructs the GER used by each claim, validates that GER against L1, and reports invalid GERs that are still tied to active claimscast sendcommands by properly quotingbytes32[32]proof arraysremove_gerREADME and the runbook to document the new scan flow and clarify the existingRemoveGER.L2NetworkIDrequirement for diagnose/recoverNone.
📋 Config Updates
None
✅ Testing
go test ./tools/remove_ger/...🐞 Issues
🔗 Related PRs
📝 Notes
scan-invalid-claimssupports bothDetailedClaimEventand legacyClaimEventlogs. For legacy claim events, it reconstructs the GER from the transaction calldata before validating it on L1.activateEmergencyState, instead of submitting a redundant transaction.