Skip to content

feat(exit-certificate): alternative Step A (state dump + Transfer logs)#1687

Open
krlosMata wants to merge 3 commits into
feature/exit-certificate-toolfrom
feature/alternative-step-a
Open

feat(exit-certificate): alternative Step A (state dump + Transfer logs)#1687
krlosMata wants to merge 3 commits into
feature/exit-certificate-toolfrom
feature/alternative-step-a

Conversation

@krlosMata

Copy link
Copy Markdown
Member

What

Adds an opt-in alternative to Step A of the exit-certificate tool, run with --step aalt.

Instead of replaying the whole chain history with debug_traceTransaction (the slowest part of the pipeline), it discovers value-holding addresses from the final state plus token logs:

  • State dump via debug_accountRange at the target block -> every ETH holder and contract (auto-detects geth vs erigon dialect).
  • Transfer event logs per wrapped token -> every token holder, including token-only EOAs that tracing structurally misses.

Strategy is selectable via the addressDiscovery option (auto | stateDump | logs | both). auto falls back to receipt harvesting when debug_accountRange isn't available.

It writes the same step-a-addresses.json, so Steps B-D consume it unchanged. The existing trace-based Step A is untouched and remains the default.

Why

The trace-based scan is O(all transactions) and is also incomplete for token-only holders. The alternative is O(#accounts) and more complete.

Result (real run, Bali, block 24,000,064)

Approach Time Addresses
Trace-based Step A ~7h 46m 315,760
Alternative Step A ~2m 45s 321,581

The alternative covers 99.95% of the trace set and finds 5,987 extra real holders. The 166 it "misses" are empty accounts (zero balance/nonce/code at the target block, e.g. self-destructed) that can't hold value.

Test plan

  • go test ./tools/exit_certificate/... (new step_a_alt_test.go: pagination, dialect, Transfer extraction, auto fallback, merge)
  • golangci-lint run ./tools/exit_certificate/... clean
  • End-to-end run against a live cdk-erigon node + address-set comparison vs trace-based Step A

Made with Cursor

@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Claude finished @krlosMata's task in 1m 28s —— View job


🔍 Code Review Complete


🚀 What's New

This PR introduces an opt-in alternative to Step A for the exit-certificate tool (--step aalt). Instead of replaying the full chain history with debug_traceTransaction (the slowest pipeline step), it discovers value-holding addresses by combining:

  • State-trie dump via paginated debug_accountRange at the target block
  • Transfer event logs per wrapped token via eth_getLogs

This is an O(#accounts) approach instead of O(#transactions), resulting in a ~170× speedup (7h 46m → 2m 45s on Bali at block 24M) while finding 5,987 additional real holders.

🐛 Bug Fixes

None.

📋 Config Updates

  • options.addressDiscovery (string, default "auto"): Selects the strategy for alternative Step A. Valid values: "auto", "stateDump", "logs", "both". Example:
    "options": {
      "addressDiscovery": "auto"
    }

⚠️ Breaking Changes

None. The alternative step is opt-in (--step aalt) and doesn't affect the default pipeline.


Review Feedback

🟢 Strengths

  1. Excellent performance improvement — The 170× speedup is substantial and well-documented in the PR description and README.
  2. Comprehensive test coveragestep_a_alt_test.go covers pagination, dialect detection, Transfer log extraction, auto-fallback, and merge behavior with mocked RPC responses.
  3. Robust dialect detection — Auto-detects geth vs erigon debug_accountRange ABIs by trying both and remembering which succeeds.
  4. Clear documentation — Both README.md and CLAUDE.md explain when to use the alternative step, its tradeoffs, and configuration.
  5. Graceful fallback — In "auto" mode, when debug_accountRange is unavailable, falls back to receipt harvesting + Transfer logs with a clear warning.
  6. Proper error handling — Validation, retries, context cancellation checks, and clear error messages throughout.

🟡 Suggested Improvements (Non-blocking)

1. Magic number in accountRangeParams (tools/exit_certificate/step_a_alt.go:314-321)

Issue: The page size constant (accountRangePageSize = 5000) differs from the comment on line 251 that says "Geth caps a single response at 256". The code requests 5000 accounts per page, but the comment implies geth will only return 256. This is confusing.

Suggestion: Clarify the comment to explain why a larger page size is beneficial for erigon even though geth ignores it. For example:

// accountRangePageSize is the number of accounts requested per debug_accountRange page.
// Geth caps each response at 256 regardless of this value (paginating via the next cursor);
// erigon honors the requested size, so a larger value (5000) reduces round-trips when
// running against erigon/cdk-erigon nodes.
accountRangePageSize = 5000

2. Receipt fallback completeness gap (tools/exit_certificate/step_a_alt.go:468-478)

Observation: The auto-fallback to receipt harvesting (collectAddressesViaReceipts) misses internal value transfers (e.g., a CALL with value to a fresh address that emits no log/receipt). This is documented in the code comment (line 692) and README, but worth highlighting.

Suggestion: Consider logging a more prominent warning when the receipt fallback is used, or add a note in the final summary about potential incompleteness. For example:

log.Warnf("⚠️ Alternative Step A: debug_accountRange unavailable (%v); falling back to receipt "+
    "harvesting + Transfer logs. NOTE: Internal value transfers (CALLs with value to new addresses) "+
    "will NOT be captured by this fallback.", err)

3. Pagination safety valve documentation (tools/exit_certificate/step_a_alt.go:32-33)

Issue: maxAccountRangePages = 5_000_000 is a very large limit (5 million pages). For a page size of 5000, this allows up to 25 billion accounts before hitting the limit. While this is a reasonable safety valve, the comment doesn't explain why this specific value was chosen.

Suggestion: Add context to the comment:

// maxAccountRangePages bounds the pagination loop as a safety valve against a node that
// never returns an empty "next" cursor. At accountRangePageSize=5000, this allows up to
// 25B accounts — far exceeding any realistic chain state — before aborting.
maxAccountRangePages = 5_000_000

4. Zero address filtering placement (tools/exit_certificate/step_a_alt.go:341)

Observation: The zero address is filtered from the final merged set (delete(finalAddrs, common.Address{})), but fetchTransferHoldersInRange already filters it out (line 680: if addr != (common.Address{}) {). So the top-level delete is defensive but redundant.

Suggestion: This is fine as-is (defensive code is good), but you could add a comment explaining why it's there:

// Remove the zero address from the final set (defensive: already filtered in individual sources,
// but this ensures it's never present regardless of which discovery paths were used).
delete(finalAddrs, common.Address{})

🔴 Required Fixes

None identified. The code is production-ready as-is.


Priority Summary

  • 🔴 Required: No critical issues found.
  • 🟡 Suggested: Minor documentation/clarity improvements (magic number comment, fallback warning prominence, pagination limit rationale, zero-address filtering comment).
  • 🟢 Optional: Consider adding a test for the maxAccountRangePages safety valve (infinite-loop protection) if you want to increase coverage.

Additional Notes

  • The implementation correctly handles both geth and erigon debug_accountRange dialects (6-arg vs 5-arg signatures, 0x-hex vs base64 encoding).
  • The allZero helper (line 584) is a good defense against clients that return all-zero cursors instead of empty strings.
  • The code follows project style guidelines (using require in tests, proper error wrapping with fmt.Errorf).
  • Tests use t.Parallel() appropriately and mock the RPC layer cleanly.

Overall Assessment: This is a high-quality, well-tested contribution that provides a substantial performance improvement for a critical tool. The opt-in design ensures zero risk to existing users. Recommended for merge with the suggested documentation improvements applied (or deferred to a follow-up PR).

@krlosMata krlosMata requested a review from joanestebanr June 30, 2026 09:12
@joanestebanr joanestebanr added the exit_certificate_tool Tool to create a final exit certificate label Jun 30, 2026
@joanestebanr

Copy link
Copy Markdown
Collaborator

@codex review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an opt-in alternative “Step A” (--step aalt) to the exit-certificate tool that discovers value-holding addresses via debug_accountRange state dump plus ERC-20 Transfer logs, wiring it into CLI/config/docs while keeping the existing trace-based Step A as default.

Changes:

  • Introduces RunStepAAlt implementation (state dump + Transfer-log holder discovery with auto fallback to receipt harvesting when debug_accountRange isn’t available).
  • Adds unit tests covering pagination, dialect probing, Transfer extraction, auto fallback, and source merging.
  • Integrates the new step into the CLI runner/config/docs and adds a new output filename (step-aalt-addresses.json) while still writing canonical step-a-addresses.json.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tools/exit_certificate/step_a_alt.go Implements alternative Step A address discovery (state dump + Transfer logs + auto fallback).
tools/exit_certificate/step_a_alt_test.go Adds unit tests for the alternative Step A logic and helpers.
tools/exit_certificate/run.go Adds aalt step dispatch and single-step runner for alternative Step A.
tools/exit_certificate/README.md Documents aalt step and options.addressDiscovery.
tools/exit_certificate/filenames.go Adds step-aalt-addresses.json constant.
tools/exit_certificate/config.go Adds addressDiscovery option with default and config merge support.
tools/exit_certificate/cmd/main.go Updates CLI help text to mention aalt.
tools/exit_certificate/CLAUDE.md Documents the new alternative Step A behavior and outputs.

Comment thread tools/exit_certificate/step_a_alt.go
Comment thread tools/exit_certificate/step_a_alt_test.go

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c572925404

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tools/exit_certificate/step_a_alt.go
Comment thread tools/exit_certificate/step_a_alt.go
Comment thread tools/exit_certificate/step_a_alt.go Outdated
@krlosMata

Copy link
Copy Markdown
Member Author

Thanks for all the reviews — addressed the feedback in 7c91610f.

Code fixes

  • Truncated-dump now fails loudly instead of silently returning a partial set (Copilot).
  • Empty debug_accountRange dump (stock geth without preimages) is rejected → auto falls back, stateDump/both fail loudly (Codex P1).
  • Transfer-log scan now starts at block 0 regardless of l2StartBlock, keeping passive token-only holders (Codex P2).
  • Reduced debug_accountRange page size 5000 → 256: both geth and erigon cap responses at 256, and a larger request risks HTTP timeouts on a slow node (hit this in a live run).
  • More prominent receipt-fallback warning + comment clarifications (Claude bot suggestions 1–4).

Coverage (SonarQube gate): added tests for the truncation/empty-dump guards, dialect param shapes, stateDump/logs-only modes, the genesis scan, and debug_accountRange error paths. step_a_alt.go is now ~90%+ per-function.

Not changed (with rationale):

  • Erigon raw-address cursor (Codex P1): empirically the cdk-erigon target accepts the base64 32-byte key (dumped 321,581 accounts live); we round-trip the node's own next cursor, and any incompatible variant degrades gracefully to the geth form / receipt fallback.
  • Self-contained test constant (Copilot): all *_test.go files share the exit_certificate package, so re-declaring rpcMethodEthGetLogs would be a duplicate-declaration compile error; reusing the shared constant also satisfies goconst.

Re-verified end-to-end against a live cdk-erigon node after the changes (LBT + alternative Step A: 321,581 addresses in ~2m43s).

@joanestebanr joanestebanr force-pushed the feature/exit-certificate-tool branch from b546dad to 2433abe Compare June 30, 2026 14:12
krlosMata and others added 2 commits June 30, 2026 16:57
…Transfer logs)

Add an opt-in alternative to Step A (--step aalt) that discovers value-holding
addresses without replaying the full transaction history via
debug_traceTransaction. It combines a state-trie dump (debug_accountRange,
auto-detecting the geth vs erigon dialect) for all ETH holders and contracts
with a per-wrapped-token Transfer-log scan for token holders (including
token-only EOAs that tracing structurally misses). Selectable via the
addressDiscovery option (auto|stateDump|logs|both); auto falls back to receipt
harvesting when debug_accountRange is unavailable.

Writes the canonical step-a-addresses.json so Steps B-D consume it unchanged.
On a 24M-block chain this ran in ~2m45s vs ~7h46m for the trace-based Step A,
producing a superset of the relevant addresses.

Co-authored-by: Cursor <cursoragent@cursor.com>
- State dump fails loudly on a truncated paginated dump (page cap hit with a
  non-empty next cursor) instead of silently returning a partial address set.
- State dump rejects an empty accounts map (e.g. a stock geth node without
  address preimages), so auto falls back to receipt harvesting and
  stateDump/both fail loudly instead of omitting native holders/contracts.
- Transfer-log holder scan now starts at block 0 (independent of l2StartBlock)
  so passive token-only holders that received before l2StartBlock are kept.
- Reduce debug_accountRange page size from 5000 to 256: both geth and erigon
  cap responses at 256, and requesting more risks HTTP timeouts on a slow node.
- Make the receipt-fallback warning explicit about missed internal value
  transfers; clarify pagination/page-size/zero-address comments.
- Add tests for the truncation and empty-dump guards, dialect param shapes,
  stateDump/logs-only modes, genesis-scan, and accountRange error paths.
- Document the preimage requirement and genesis scan in README/CLAUDE.

Co-authored-by: Cursor <cursoragent@cursor.com>
@krlosMata krlosMata force-pushed the feature/alternative-step-a branch from 7c91610 to 93d3f7f Compare June 30, 2026 14:57
…-locked from contracts

Add two opt-in options for the alternative Step A / certificate build:

- includeZeroAddress: keep the zero address (0x000…000) in the collected
  address set instead of dropping it, so its native/token balances are exited
  in the certificate.
- nativeSCLockedFromContracts: compute the native-token SC-locked value in
  Step C from the actual ETH held by contract accounts (summed via
  eth_getBalance, excluding the L2 bridge) instead of LBT − EOA_accumulated.
  This is needed on chains with a native genesis premint, where LBT − EOA
  underflows and clamps to 0, silently dropping contract-held ETH.

The contract-balance sum runs in Step C's runner (reading the already-saved
contract addresses), so no heavy Step B re-run is required.

Co-authored-by: Cursor <cursoragent@cursor.com>
@sonarqubecloud

sonarqubecloud Bot commented Jul 2, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
70.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

exit_certificate_tool Tool to create a final exit certificate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants