Replace string errors with custom errors in OnChainProposer contracts

[avilagaston9]

Motivation

The two OnChainProposer contracts use inconsistent error formats — the non-based contract uses opaque hex codes ("007", "00c") while the based contract uses full strings ("OnChainProposer: Invalid RISC0 proof failed proof verification"). This causes a real bug: the Rust l1_proof_sender detects invalid proofs by string-matching on message.contains("Invalid TDX proof"), which works for the based contract but fails silently for the non-based contract. Invalid proofs are never deleted.

Custom errors are cheaper (no string storage in bytecode), self-documenting (the error name IS the documentation), and can be decoded programmatically from the ABI.

Closes #4196, closes #6098

Description

Replace all require(condition, "string") / revert("string") in both OnChainProposer contracts with Solidity custom errors defined in the interfaces. Both contracts now use identical error names.

On the Rust side, add a data: Option<String> field to RpcRequestError::RPCError to capture the revert data from JSON-RPC error responses (custom errors encode their identity as a 4-byte selector in the data field, not in message). Update proof sender and verifier to match on these selectors instead of string-matching.

Solidity changes:

• Define custom errors in both IOnChainProposer interfaces
• Replace ~35 string errors in the non-based contract and ~25 in the based contract
• Eliminate the duplicate "012" code that was used for two different errors

Rust changes:

• Add data: Option<String> to RpcRequestError::RPCError
• Propagate error_response.error.data in all RPCError construction sites
• Define 4-byte selector constants for InvalidRisc0Proof, InvalidSp1Proof, InvalidTdxProof, and AlignedProofVerificationFailed
• Fix l1_proof_sender to match on selectors in data instead of strings in message
• Fix l1_proof_verifier to match on AlignedProofVerificationFailed selector instead of the stale "00m" code

Checklist

• Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync.

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR replaces string-based require() statements with custom Solidity errors across the OnChainProposer contracts and updates the Rust client code to handle these new error types. The changes improve gas efficiency and error handling.

Issues Found

1. Missing Custom Error Definitions

File: crates/l2/contracts/src/l1/OnChainProposer.sol
Lines: 1-50 (contract definition)

The contract uses custom errors like MissingRisc0Verifier(), InvalidRisc0Proof(), etc., but these error definitions are missing from the contract. They are only defined in the interface files (IOnChainProposer.sol). Solidity contracts must define their own custom errors or import them.

Fix: Add all the custom error definitions to the contract itself, not just the interface.

2. Inconsistent Error Naming

File: crates/l2/contracts/src/l1/OnChainProposer.sol
Lines: 146-155

The error MissingRisc0Verifier() should probably be MissingRisc0Verifier (without parentheses) to match Solidity convention, and similarly for all other custom errors.

3. Selector Mismatch Risk

File: crates/l2/sequencer/utils.rs
Lines: 25-30

The hardcoded selectors in Rust (INVALID_RISC0_PROOF_SELECTOR, etc.) are brittle. If the error signatures change in Solidity, these will break silently.

Fix: Consider generating these selectors programmatically from the ABI or at least add tests to verify they match the actual contract errors.

4. Potential Re-entrancy in verifyBatchAligned

File: crates/l2/contracts/src/l1/OnChainProposer.sol
Lines: 580-590

The external call to ALIGNEDPROOFAGGREGATOR could potentially be re-entered. While the contract uses whenNotPaused modifier, consider adding a re-entrancy guard for additional security.

5. Error Handling in Rust Could Be More Robust

File: crates/l2/sequencer/l1_proof_sender.rs
Lines: 438-460

The error matching logic only checks if the data starts with the selector, but doesn't fully parse the error. This could lead to false matches if the error data contains additional parameters.

Fix: Consider using a proper ABI decoder to parse the full error data.

Positive Aspects

1. Gas Optimization: Custom errors are significantly more gas-efficient than string-based require statements
2. Better Error Handling: The Rust client can now handle specific error types more precisely
3. Code Clarity: Error messages are more descriptive and consistent
4. Interface Consistency: Both L1 and based versions now have consistent error handling

Minor Suggestions

1. Consider adding NatSpec documentation for each custom error
2. Add tests to verify error selectors match between Solidity and Rust
3. Consider using OpenZeppelin's custom error libraries for standardization

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Summary
Nice cleanup moving to custom errors and using revert selectors; overall looks good and should reduce gas on revert paths. I have two correctness/robustness concerns.

Findings

1. Potential regression when RPC error data is missing
   You now only delete invalid proofs when error.data is Some and matches a selector. Some RPC providers omit error.data for reverts or strip it in certain modes; in those cases invalid proofs will no longer be deleted and the sender/verifier may keep retrying a bad proof. Consider retaining the old message fallback or logging + deleting when data is None but the message clearly indicates invalid proof.
   File refs: crates/l2/sequencer/l1_proof_sender.rs:438, crates/l2/sequencer/l1_proof_sender.rs:451, crates/l2/sequencer/l1_proof_verifier.rs:251, crates/l2/sequencer/l1_proof_verifier.rs:258.

2. Hard-coded selectors risk drift from Solidity errors
   The selector constants are hard-coded strings. If the Solidity error signature changes (renaming, adding params, etc.), detection will silently break. Consider computing selectors from ABI at build time or via a small helper using keccak256("ErrorName()") to keep them in sync, or add a unit test asserting these match the on-chain ABI.
   File refs: crates/l2/sequencer/utils.rs:28, crates/l2/sequencer/utils.rs:31, crates/l2/sequencer/l1_proof_sender.rs:451, crates/l2/sequencer/l1_proof_verifier.rs:258.

If you address those, the rest looks solid.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Based on my thorough review of the PR changes, here is my code review:

---

Code Review: PR 6206 - Replace string errors with custom errors in OnChainProposer contracts

Summary

This PR successfully addresses a critical bug where invalid proofs weren't being detected and deleted by the Rust side. The changes improve error handling consistency across both OnChainProposer contracts and make the error detection mechanism more robust. Overall, the implementation is solid with just a few minor concerns.

Critical Issues

None found - The core functionality appears correct.

High Priority

1. Verify error selector constants (crates/l2/sequencer/utils.rs:28-31)

The error selector constants need verification. These are derived from keccak256("ErrorName()")[:4]. While the implementation pattern is correct, the actual values should be verified against the compiled contract ABIs.

Recommended verification:

cast sig "InvalidRisc0Proof()"
cast sig "InvalidSp1Proof()"
cast sig "InvalidTdxProof()"
cast sig "AlignedProofVerificationFailed()"

If these don't match the constants defined in the code, the error detection will silently fail (the original bug won't be fixed).

2. Logic change in based contract (crates/l2/contracts/src/l1/based/OnChainProposer.sol:520-525)

The boolean logic was refactored from a complex nested condition to a simpler OR condition:

Before:

require(
    (!REQUIRE_SP1_PROOF || verificationKeys[commitHash][SP1_VERIFIER_ID] != bytes32(0)) &&
    (!REQUIRE_RISC0_PROOF || verificationKeys[commitHash][RISC0_VERIFIER_ID] != bytes32(0)),
    "013"
);

After:

if (
    (REQUIRE_SP1_PROOF && verificationKeys[commitHash][SP1_VERIFIER_ID] == bytes32(0)) ||
    (REQUIRE_RISC0_PROOF && verificationKeys[commitHash][RISC0_VERIFIER_ID] == bytes32(0))
) revert MissingVerificationKeyForCommit();

The new logic correctly applies De Morgan's laws and is equivalent. However, this differs from the non-based contract which uses separate if statements (lines 185-197 of OnChainProposer.sol). For consistency and clarity, consider using the same pattern in both contracts.

Medium Priority

3. Missing access control check on verifyBatch in based contract (crates/l2/contracts/src/l1/based/OnChainProposer.sol:301)

The verifyBatch function in the based contract has no access control modifier, while the non-based version has onlyOwner whenNotPaused. This appears intentional (based contracts allow sequencers to verify), but worth confirming this is the desired behavior. If anyone can call verify, there may be gas griefing or other implications.

4. Removed batch number progression check in based contract (crates/l2/contracts/src/l1/based/OnChainProposer.sol:301-560)

The non-based contract's verifyBatch checks:

if (batchNumber != lastVerifiedBatch + 1) revert BatchAlreadyVerified();

This check is missing in the based contract's verifyBatch function. This could allow batches to be verified out of order or the same batch to be verified multiple times. Review whether this is intentional or an oversight.

Low Priority / Style

5. Inconsistent error naming (crates/l2/contracts/src/l1/interfaces/IOnChainProposer.sol:767)

The non-based interface includes error BatchAlreadyVerified() but the based interface doesn't. This is because the based contract doesn't have that check (see item 4), but if that check should exist, the error definition should be added to both interfaces.

6. Gas optimization opportunity (Multiple locations)

The refactored code uses multi-line if statements without braces for single-statement bodies. While valid Solidity, consider wrapping all if bodies in braces for consistency with Solidity style guides and to prevent future bugs:

// Current
if (batchNumber != lastCommittedBatch + 1)
    revert BatchNumberNotSuccessor();

// Suggested
if (batchNumber != lastCommittedBatch + 1) {
    revert BatchNumberNotSuccessor();
}

Positive Observations

1. Excellent bug fix: The core issue (string matching failing for hex error codes) is properly fixed by using error selectors.

2. Comprehensive Rust changes: All RPC error construction sites properly propagate the data field.

3. Proper error categorization: The interface organizes errors into logical groups (initialization, commit, verify, etc.).

4. Gas savings: Custom errors are significantly cheaper than string errors.

5. Better developer experience: Named errors are self-documenting and easier to debug.

6. Consistent error names: Both contracts now use identical error names for the same conditions.

Testing Recommendations

1. Verify error selectors match compiled contract ABIs
2. Test invalid proof detection end-to-end to confirm the bug is fixed
3. Test the based contract's verifyBatch behavior without batch number checks
4. Verify RPC error data parsing with actual RPC responses containing custom error selectors

Rust Code Quality

The Rust changes are well-structured:

• Proper error propagation with the new data field
• Clear constant naming for error selectors
• Consistent pattern matching on Option<String> data
• Uses starts_with for selector matching (correct, as data may include additional parameters)

---

Recommendation: Approve with comments - Address items 1, 2, and 4 before merging.

---

Automated review by Claude (Anthropic) · custom prompt

[github-actions]

Lines of code report

Total lines added: 24
Total lines removed: 0
Total lines changed: 24

Detailed view

+----------------------------------------------------+-------+------+
| File                                               | Lines | Diff |
+----------------------------------------------------+-------+------+
| ethrex/crates/l2/networking/rpc/clients.rs         | 145   | +2   |
+----------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/l1_proof_sender.rs      | 469   | +5   |
+----------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/l1_proof_verifier.rs    | 386   | +2   |
+----------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/utils.rs                | 173   | +4   |
+----------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/clients/eth/errors.rs | 70    | +4   |
+----------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/clients/eth/mod.rs    | 574   | +7   |
+----------------------------------------------------+-------+------+