Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe ChildToParentProver contract is refactored to replace block header-based verification with Sparse Merkle Tree (SMT) proof validation, introducing comprehensive account and storage proof verification flows with new error cases and updated method signatures. Changes
Sequence DiagramsequenceDiagram
actor Test as Test / Caller
participant Prover as ChildToParentProver
participant SMT as SparseMerkleProof Library
Test->>Prover: verifyTargetStateCommitment(homeBlockHash, input)
activate Prover
Note over Prover: Decode input:<br/>targetBlockNumber, accountLeafIndex,<br/>accountProof, accountValue, etc.
Prover->>SMT: Validate account proof
activate SMT
SMT-->>Prover: Account proof valid
deactivate SMT
Prover->>Prover: Verify account key matches
Prover->>Prover: Verify account value matches
Prover->>SMT: Validate storage proof
activate SMT
SMT-->>Prover: Storage proof valid
deactivate SMT
Prover->>Prover: Verify storage key matches
Prover->>Prover: Verify claimed storage value<br/>is non-zero
Prover-->>Test: Return targetStateCommitment
deactivate Prover
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/provers/linea/ChildToParentProver.t.sol (1)
125-326: Consider sourcing proof data from the JSON payload for maintainability. The inline proof arrays are large; loading fromtest/payloads/linea/buffer-proof.json(similar to other tests) would reduce noise and make updates easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/provers/linea/ChildToParentProver.t.sol` around lines 125 - 326, Replace the large inline proof data in test_verifyTargetStateCommitment_buffer by loading and parsing test/payloads/linea/buffer-proof.json at runtime: read the file (vm.readFile), parse the JSON fields (accountProof, storageProof, accountValue, claimedStorageValue, targetBlockNumber, accountLeafIndex, storageLeafIndex) into the same types used in the test, and use those variables when constructing input and calling ChildToParentProver.verifyTargetStateCommitment; update the test to remove the hardcoded hex arrays and ensure parsing yields bytes[]/bytes32/uint256 exactly as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contracts/provers/linea/ChildToParentProver.sol`:
- Around line 72-103: The code indexes the last element of accountProof and
storageProof (in ChildToParentProver.sol) without checking length, which can
cause a panic on empty arrays; before calling
SparseMerkleProof.getLeaf(accountProof[accountProof.length - 1]) and
getLeaf(storageProof[storageProof.length - 1]) add explicit guards like
require(accountProof.length > 0, ""); or if/ revert to ensure
accountProof.length > 0 and storageProof.length > 0 and revert with
InvalidAccountProof and InvalidStorageProof respectively (place these checks
immediately before the getLeaf/indexing and/or before any verifyProof usage so
the custom errors are thrown instead of a panic).
---
Nitpick comments:
In `@test/provers/linea/ChildToParentProver.t.sol`:
- Around line 125-326: Replace the large inline proof data in
test_verifyTargetStateCommitment_buffer by loading and parsing
test/payloads/linea/buffer-proof.json at runtime: read the file (vm.readFile),
parse the JSON fields (accountProof, storageProof, accountValue,
claimedStorageValue, targetBlockNumber, accountLeafIndex, storageLeafIndex) into
the same types used in the test, and use those variables when constructing input
and calling ChildToParentProver.verifyTargetStateCommitment; update the test to
remove the hardcoded hex arrays and ensure parsing yields
bytes[]/bytes32/uint256 exactly as before.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/contracts/provers/linea/ChildToParentProver.soltest/payloads/linea/buffer-proof.jsontest/provers/linea/ChildToParentProver.t.sol
Comment on lines
+72
to
+103
| bool accountValid = SparseMerkleProof.verifyProof(accountProof, accountLeafIndex, homeBlockHash); | ||
| if (!accountValid) { | ||
| revert InvalidAccountProof(); | ||
| } | ||
|
|
||
| SparseMerkleProof.Leaf memory accountLeaf = SparseMerkleProof.getLeaf(accountProof[accountProof.length - 1]); | ||
| bytes32 expectedAccountHKey = SparseMerkleProof.hashAccountKey(blockHashBuffer); | ||
| if (accountLeaf.hKey != expectedAccountHKey) { | ||
| revert AccountKeyMismatch(); | ||
| } | ||
|
|
||
| bytes32 expectedAccountHValue = SparseMerkleProof.hashAccountValue(accountValue); | ||
| if (accountLeaf.hValue != expectedAccountHValue) { | ||
| revert AccountValueMismatch(); | ||
| } | ||
|
|
||
| SparseMerkleProof.Account memory accountData = SparseMerkleProof.getAccount(accountValue); | ||
|
|
||
| bool storageValid = SparseMerkleProof.verifyProof(storageProof, storageLeafIndex, accountData.storageRoot); | ||
| if (!storageValid) { | ||
| revert InvalidStorageProof(); | ||
| } | ||
|
|
||
| SparseMerkleProof.Leaf memory storageLeaf = SparseMerkleProof.getLeaf(storageProof[storageProof.length - 1]); | ||
| bytes32 expectedStorageHKey = SparseMerkleProof.hashStorageKey(bytes32(slot)); | ||
| if (storageLeaf.hKey != expectedStorageHKey) { | ||
| revert StorageKeyMismatch(); | ||
| } | ||
|
|
||
| bytes32 expectedHValue = SparseMerkleProof.hashStorageValue(claimedStorageValue); | ||
| if (storageLeaf.hValue != expectedHValue) { | ||
| revert StorageValueMismatch(); |
There was a problem hiding this comment.
Guard against empty proof arrays before indexing. Line 77 and Line 95 index the last element of accountProof/storageProof. If either array is empty, this will revert with a panic instead of your custom errors. Add explicit length checks and revert with InvalidAccountProof/InvalidStorageProof.
🔧 Suggested fix
- bool accountValid = SparseMerkleProof.verifyProof(accountProof, accountLeafIndex, homeBlockHash);
+ if (accountProof.length == 0) {
+ revert InvalidAccountProof();
+ }
+ bool accountValid = SparseMerkleProof.verifyProof(accountProof, accountLeafIndex, homeBlockHash);
...
- bool storageValid = SparseMerkleProof.verifyProof(storageProof, storageLeafIndex, accountData.storageRoot);
+ if (storageProof.length == 0) {
+ revert InvalidStorageProof();
+ }
+ bool storageValid = SparseMerkleProof.verifyProof(storageProof, storageLeafIndex, accountData.storageRoot);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool accountValid = SparseMerkleProof.verifyProof(accountProof, accountLeafIndex, homeBlockHash); | |
| if (!accountValid) { | |
| revert InvalidAccountProof(); | |
| } | |
| SparseMerkleProof.Leaf memory accountLeaf = SparseMerkleProof.getLeaf(accountProof[accountProof.length - 1]); | |
| bytes32 expectedAccountHKey = SparseMerkleProof.hashAccountKey(blockHashBuffer); | |
| if (accountLeaf.hKey != expectedAccountHKey) { | |
| revert AccountKeyMismatch(); | |
| } | |
| bytes32 expectedAccountHValue = SparseMerkleProof.hashAccountValue(accountValue); | |
| if (accountLeaf.hValue != expectedAccountHValue) { | |
| revert AccountValueMismatch(); | |
| } | |
| SparseMerkleProof.Account memory accountData = SparseMerkleProof.getAccount(accountValue); | |
| bool storageValid = SparseMerkleProof.verifyProof(storageProof, storageLeafIndex, accountData.storageRoot); | |
| if (!storageValid) { | |
| revert InvalidStorageProof(); | |
| } | |
| SparseMerkleProof.Leaf memory storageLeaf = SparseMerkleProof.getLeaf(storageProof[storageProof.length - 1]); | |
| bytes32 expectedStorageHKey = SparseMerkleProof.hashStorageKey(bytes32(slot)); | |
| if (storageLeaf.hKey != expectedStorageHKey) { | |
| revert StorageKeyMismatch(); | |
| } | |
| bytes32 expectedHValue = SparseMerkleProof.hashStorageValue(claimedStorageValue); | |
| if (storageLeaf.hValue != expectedHValue) { | |
| revert StorageValueMismatch(); | |
| if (accountProof.length == 0) { | |
| revert InvalidAccountProof(); | |
| } | |
| bool accountValid = SparseMerkleProof.verifyProof(accountProof, accountLeafIndex, homeBlockHash); | |
| if (!accountValid) { | |
| revert InvalidAccountProof(); | |
| } | |
| SparseMerkleProof.Leaf memory accountLeaf = SparseMerkleProof.getLeaf(accountProof[accountProof.length - 1]); | |
| bytes32 expectedAccountHKey = SparseMerkleProof.hashAccountKey(blockHashBuffer); | |
| if (accountLeaf.hKey != expectedAccountHKey) { | |
| revert AccountKeyMismatch(); | |
| } | |
| bytes32 expectedAccountHValue = SparseMerkleProof.hashAccountValue(accountValue); | |
| if (accountLeaf.hValue != expectedAccountHValue) { | |
| revert AccountValueMismatch(); | |
| } | |
| SparseMerkleProof.Account memory accountData = SparseMerkleProof.getAccount(accountValue); | |
| if (storageProof.length == 0) { | |
| revert InvalidStorageProof(); | |
| } | |
| bool storageValid = SparseMerkleProof.verifyProof(storageProof, storageLeafIndex, accountData.storageRoot); | |
| if (!storageValid) { | |
| revert InvalidStorageProof(); | |
| } | |
| SparseMerkleProof.Leaf memory storageLeaf = SparseMerkleProof.getLeaf(storageProof[storageProof.length - 1]); | |
| bytes32 expectedStorageHKey = SparseMerkleProof.hashStorageKey(bytes32(slot)); | |
| if (storageLeaf.hKey != expectedStorageHKey) { | |
| revert StorageKeyMismatch(); | |
| } | |
| bytes32 expectedHValue = SparseMerkleProof.hashStorageValue(claimedStorageValue); | |
| if (storageLeaf.hValue != expectedHValue) { | |
| revert StorageValueMismatch(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contracts/provers/linea/ChildToParentProver.sol` around lines 72 - 103,
The code indexes the last element of accountProof and storageProof (in
ChildToParentProver.sol) without checking length, which can cause a panic on
empty arrays; before calling
SparseMerkleProof.getLeaf(accountProof[accountProof.length - 1]) and
getLeaf(storageProof[storageProof.length - 1]) add explicit guards like
require(accountProof.length > 0, ""); or if/ revert to ensure
accountProof.length > 0 and storageProof.length > 0 and revert with
InvalidAccountProof and InvalidStorageProof respectively (place these checks
immediately before the getLeaf/indexing and/or before any verifyProof usage so
the custom errors are thrown instead of a panic).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Collaborator
Author
There was a problem hiding this comment.
If the proofs are empty, they will revert with the WrongProofLength error, which is the expected behavior.
@pepebndc
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/contracts/provers/linea/ChildToParentProver.sol (1)
73-96: Empty-proof guard still unaddressed before array indexing (Lines 78 and 96).If
SparseMerkleProof.verifyProofinternally reverts (rather than returningfalse) when given an empty proof array, theif (!accountValid) revert InvalidAccountProof()guard on Lines 74-76 is never reached and the caller gets a non-descriptive revert instead of the custom error. The same applies tostorageProofat Line 96.Run this to check whether
verifyProofguards against empty arrays, or whether it can returntrue/panic:#!/bin/bash # Verify SparseMerkleProof.verifyProof implementation for empty-array behavior # and confirm all function signatures match the usage in ChildToParentProver.sol fd -e sol SparseMerkleProof --exec cat {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/linea/ChildToParentProver.sol` around lines 73 - 96, The code indexes accountProof[accountProof.length - 1] and storageProof[storageProof.length - 1] without guarding against empty arrays; add explicit empty-proof checks before calling SparseMerkleProof.getLeaf and before calling verifyProof (e.g., check accountProof.length == 0 and revert InvalidAccountProof(), and check storageProof.length == 0 and revert InvalidStorageProof()) so you never pass an empty array into SparseMerkleProof.verifyProof or index into it; update the logic around verifyProof, accountLeaf = SparseMerkleProof.getLeaf(...), and storageLeaf = SparseMerkleProof.getLeaf(...) accordingly to ensure the custom errors InvalidAccountProof and InvalidStorageProof are thrown for empty proofs.
🧹 Nitpick comments (1)
src/contracts/provers/linea/ChildToParentProver.sol (1)
107-108:requirestyle inconsistent with theif/revertpattern used throughout the function.Every other validation in this function uses
if (...) revert CustomError(). Line 108 switches torequire(..., CustomError()). Both are functionally equivalent in Solidity 0.8.30, but mixing the two styles in the same function reduces readability.♻️ Proposed consistency fix
- targetStateCommitment = claimedStorageValue; - require(targetStateCommitment != bytes32(0), InvalidTargetStateCommitment()); + if (claimedStorageValue == bytes32(0)) { + revert InvalidTargetStateCommitment(); + } + targetStateCommitment = claimedStorageValue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/linea/ChildToParentProver.sol` around lines 107 - 108, The assignment to targetStateCommitment from claimedStorageValue uses a require(...) that is inconsistent with the rest of the function's if (...) revert Pattern; change the require(targetStateCommitment != bytes32(0), InvalidTargetStateCommitment()) into the same style as the other checks by replacing it with an if (targetStateCommitment == bytes32(0)) revert InvalidTargetStateCommitment(); so the logic around targetStateCommitment, claimedStorageValue and the InvalidTargetStateCommitment() error matches the function's style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/contracts/provers/linea/ChildToParentProver.sol`:
- Around line 73-96: The code indexes accountProof[accountProof.length - 1] and
storageProof[storageProof.length - 1] without guarding against empty arrays; add
explicit empty-proof checks before calling SparseMerkleProof.getLeaf and before
calling verifyProof (e.g., check accountProof.length == 0 and revert
InvalidAccountProof(), and check storageProof.length == 0 and revert
InvalidStorageProof()) so you never pass an empty array into
SparseMerkleProof.verifyProof or index into it; update the logic around
verifyProof, accountLeaf = SparseMerkleProof.getLeaf(...), and storageLeaf =
SparseMerkleProof.getLeaf(...) accordingly to ensure the custom errors
InvalidAccountProof and InvalidStorageProof are thrown for empty proofs.
---
Nitpick comments:
In `@src/contracts/provers/linea/ChildToParentProver.sol`:
- Around line 107-108: The assignment to targetStateCommitment from
claimedStorageValue uses a require(...) that is inconsistent with the rest of
the function's if (...) revert Pattern; change the require(targetStateCommitment
!= bytes32(0), InvalidTargetStateCommitment()) into the same style as the other
checks by replacing it with an if (targetStateCommitment == bytes32(0)) revert
InvalidTargetStateCommitment(); so the logic around targetStateCommitment,
claimedStorageValue and the InvalidTargetStateCommitment() error matches the
function's style.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/contracts/provers/linea/ChildToParentProver.sol (2)
91-96:⚠️ Potential issue | 🟡 MinorEmpty
storageProofstill not guarded before indexing (past review).Same pattern as above:
storageProof[storageProof.length - 1]on line 96 panics instead of reverting withInvalidStorageProofon an empty array.🛡️ Proposed fix
+ if (storageProof.length == 0) { + revert InvalidStorageProof(); + } bool storageValid = SparseMerkleProof.verifyProof(storageProof, storageLeafIndex, accountData.storageRoot);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/linea/ChildToParentProver.sol` around lines 91 - 96, The code can panic when indexing storageProof[storageProof.length - 1] if storageProof is empty; before calling SparseMerkleProof.getLeaf (and after or before calling SparseMerkleProof.verifyProof), add an explicit guard that reverts with InvalidStorageProof() when storageProof.length == 0 so you never index an empty array — update the block around storageValid / SparseMerkleProof.getLeaf to check storageProof.length > 0 and then call SparseMerkleProof.getLeaf(storageProof[storageProof.length - 1]).
73-78:⚠️ Potential issue | 🟡 MinorEmpty
accountProofstill not guarded before indexing (past review).The previous review flagged that
accountProof[accountProof.length - 1]on line 78 will panic rather than revert withInvalidAccountProofif the array is empty. TheverifyProofcall on line 73 provides some protection only if the underlying Linea library guarantees it returnsfalse(nottrueor panics) for an empty slice — which can't be assumed without seeing that library's code. The explicit length guards recommended in the past review are still missing.🛡️ Proposed fix
+ if (accountProof.length == 0) { + revert InvalidAccountProof(); + } bool accountValid = SparseMerkleProof.verifyProof(accountProof, accountLeafIndex, homeBlockHash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/linea/ChildToParentProver.sol` around lines 73 - 78, The code indexes accountProof[accountProof.length - 1] without guarding for an empty array which can panic; before calling SparseMerkleProof.getLeaf or indexing, add an explicit check that accountProof.length > 0 and revert with InvalidAccountProof() if not, then proceed to call SparseMerkleProof.verifyProof(accountProof, accountLeafIndex, homeBlockHash) and only after both checks use SparseMerkleProof.getLeaf(accountProof[accountProof.length - 1]); ensure the fix references the same symbols (accountProof, verifyProof, getLeaf, InvalidAccountProof) so the empty-array case is handled safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/contracts/provers/linea/ChildToParentProver.sol`:
- Around line 91-96: The code can panic when indexing
storageProof[storageProof.length - 1] if storageProof is empty; before calling
SparseMerkleProof.getLeaf (and after or before calling
SparseMerkleProof.verifyProof), add an explicit guard that reverts with
InvalidStorageProof() when storageProof.length == 0 so you never index an empty
array — update the block around storageValid / SparseMerkleProof.getLeaf to
check storageProof.length > 0 and then call
SparseMerkleProof.getLeaf(storageProof[storageProof.length - 1]).
- Around line 73-78: The code indexes accountProof[accountProof.length - 1]
without guarding for an empty array which can panic; before calling
SparseMerkleProof.getLeaf or indexing, add an explicit check that
accountProof.length > 0 and revert with InvalidAccountProof() if not, then
proceed to call SparseMerkleProof.verifyProof(accountProof, accountLeafIndex,
homeBlockHash) and only after both checks use
SparseMerkleProof.getLeaf(accountProof[accountProof.length - 1]); ensure the fix
references the same symbols (accountProof, verifyProof, getLeaf,
InvalidAccountProof) so the empty-array case is handled safely.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/contracts/provers/zksync/ParentToChildProver.sol (1)
269-278:internal view→internal pure: no state is read.
_l2MessageToLogonly accesses aconstant(L1_MESSENGER) and the function argument. It reads no storage, sopureis the correct and more idiomatic qualifier.♻️ Proposed change
- function _l2MessageToLog(L2Message memory _message) internal view returns (L2Log memory) { + function _l2MessageToLog(L2Message memory _message) internal pure returns (L2Log memory) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/zksync/ParentToChildProver.sol` around lines 269 - 278, The function _l2MessageToLog is marked internal view but does not read contract state (only uses the L1_MESSENGER constant and its argument), so change its visibility specifier from view to pure; update the function signature for _l2MessageToLog to be internal pure and ensure any callers still compile against the new spec (no other logic change necessary).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contracts/provers/optimism/ParentToChildProver.sol`:
- Around line 27-29: The ANCHOR_GAME_SLOT constant is set to 3 but the
AnchorStateRegistry storage layout places anchorGame in slot 4, so update the
ANCHOR_GAME_SLOT constant from 3 to 4 to ensure verifyTargetStateCommitment
reads the correct storage slot; locate the ANCHOR_GAME_SLOT declaration and
change its value to 4 (and optionally update the inline comment/link if needed)
so downstream reads return the actual anchorGame address instead of the portal
address.
---
Nitpick comments:
In `@src/contracts/provers/zksync/ParentToChildProver.sol`:
- Around line 269-278: The function _l2MessageToLog is marked internal view but
does not read contract state (only uses the L1_MESSENGER constant and its
argument), so change its visibility specifier from view to pure; update the
function signature for _l2MessageToLog to be internal pure and ensure any
callers still compile against the new spec (no other logic change necessary).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/contracts/provers/arbitrum/ChildToParentProver.solsrc/contracts/provers/arbitrum/ParentToChildProver.solsrc/contracts/provers/linea/ChildToParentProver.solsrc/contracts/provers/linea/ParentToChildProver.solsrc/contracts/provers/optimism/ChildToParentProver.solsrc/contracts/provers/optimism/ParentToChildProver.solsrc/contracts/provers/scroll/ChildToParentProver.solsrc/contracts/provers/scroll/ParentToChildProver.solsrc/contracts/provers/taiko/ChildToParentProver.solsrc/contracts/provers/taiko/ParentToChildProver.solsrc/contracts/provers/zksync/ChildToParentProver.solsrc/contracts/provers/zksync/ParentToChildProver.sol
💤 Files with no reviewable changes (10)
- src/contracts/provers/arbitrum/ChildToParentProver.sol
- src/contracts/provers/zksync/ChildToParentProver.sol
- src/contracts/provers/linea/ChildToParentProver.sol
- src/contracts/provers/linea/ParentToChildProver.sol
- src/contracts/provers/optimism/ChildToParentProver.sol
- src/contracts/provers/scroll/ParentToChildProver.sol
- src/contracts/provers/arbitrum/ParentToChildProver.sol
- src/contracts/provers/taiko/ParentToChildProver.sol
- src/contracts/provers/taiko/ChildToParentProver.sol
- src/contracts/provers/scroll/ChildToParentProver.sol
luiz-lvj
force-pushed
the
fix/provers-H02
branch
from
38bc3ef to
26254eb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/provers/linea/ChildToParentProver.t.sol (2)
125-326: Consider loading proof data frombuffer-proof.jsonto reduce duplication.The test correctly exercises the full SMT verification path and all hardcoded values are consistent with
test/payloads/linea/buffer-proof.json. However, the ~200 lines of hardcoded proof nodes duplicate the JSON payload. You could usevm.readFile+stdJson(already imported and used in the existing tests above) to load the proof data, similar to howtest_verifyStorageSlot_broadcasterloads its payload. This would make maintenance easier if the proof data ever needs updating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/provers/linea/ChildToParentProver.t.sol` around lines 125 - 326, The test test_verifyTargetStateCommitment_buffer currently inlines ~200 lines of proof data; change it to read test/payloads/linea/buffer-proof.json via vm.readFile and parse with stdJson (as done in test_verifyStorageSlot_broadcaster) and populate accountProof, storageProof, accountValue, claimedStorageValue, targetBlockNumber, accountLeafIndex, and storageLeafIndex from the parsed JSON before calling ChildToParentProver.verifyTargetStateCommitment; replace the hardcoded byte[]/bytes/uint256 values with values extracted from the JSON and keep the same call to new ChildToParentProver(buffer, childChainId) and assertEq(targetStateCommitment, claimedStorageValue).
125-126: Consider adding negative tests for the new error paths.The happy-path coverage is good. The contract now has 6 new revert conditions (
InvalidAccountProof,AccountKeyMismatch,AccountValueMismatch,InvalidStorageProof,StorageKeyMismatch,StorageValueMismatch). Adding at least a few failure-case tests (e.g., tampered proof node, wrongclaimedStorageValue, wrong buffer address) would strengthen confidence in the validation logic.Want me to open an issue to track adding negative test cases for the new error paths?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/provers/linea/ChildToParentProver.t.sol` around lines 125 - 126, Add negative tests to the test suite (e.g., in ChildToParentProver.t.sol alongside test_verifyTargetStateCommitment_buffer) that assert the newly added revert conditions: InvalidAccountProof, AccountKeyMismatch, AccountValueMismatch, InvalidStorageProof, StorageKeyMismatch, and StorageValueMismatch. For each error path create a failing scenario such as tampering a proof node to trigger InvalidAccountProof/InvalidStorageProof, supplying a wrong buffer address or wrong account key/value to trigger AccountKeyMismatch/AccountValueMismatch, and using an incorrect claimedStorageValue or storage key to trigger StorageKeyMismatch/StorageValueMismatch; use vm.expectRevert(...) with the appropriate error selector or revert string and then call verifyTargetStateCommitment (or the relevant method in ChildToParentProver) to confirm each revert fires.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/provers/linea/ChildToParentProver.t.sol`:
- Around line 125-326: The test test_verifyTargetStateCommitment_buffer
currently inlines ~200 lines of proof data; change it to read
test/payloads/linea/buffer-proof.json via vm.readFile and parse with stdJson (as
done in test_verifyStorageSlot_broadcaster) and populate accountProof,
storageProof, accountValue, claimedStorageValue, targetBlockNumber,
accountLeafIndex, and storageLeafIndex from the parsed JSON before calling
ChildToParentProver.verifyTargetStateCommitment; replace the hardcoded
byte[]/bytes/uint256 values with values extracted from the JSON and keep the
same call to new ChildToParentProver(buffer, childChainId) and
assertEq(targetStateCommitment, claimedStorageValue).
- Around line 125-126: Add negative tests to the test suite (e.g., in
ChildToParentProver.t.sol alongside test_verifyTargetStateCommitment_buffer)
that assert the newly added revert conditions: InvalidAccountProof,
AccountKeyMismatch, AccountValueMismatch, InvalidStorageProof,
StorageKeyMismatch, and StorageValueMismatch. For each error path create a
failing scenario such as tampering a proof node to trigger
InvalidAccountProof/InvalidStorageProof, supplying a wrong buffer address or
wrong account key/value to trigger AccountKeyMismatch/AccountValueMismatch, and
using an incorrect claimedStorageValue or storage key to trigger
StorageKeyMismatch/StorageValueMismatch; use vm.expectRevert(...) with the
appropriate error selector or revert string and then call
verifyTargetStateCommitment (or the relevant method in ChildToParentProver) to
confirm each revert fires.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/contracts/provers/linea/ChildToParentProver.soltest/payloads/linea/buffer-proof.jsontest/provers/linea/ChildToParentProver.t.sol
frangio
reviewed
Feb 24, 2026
| /// @param homeBlockHash The state root of the home chain (Linea SMT state root). | ||
| /// @param input ABI encoded (uint256 targetBlockNumber, uint256 accountLeafIndex, bytes[] accountProof, | ||
| /// bytes accountValue, uint256 storageLeafIndex, bytes[] storageProof, bytes32 claimedStorageValue) | ||
| function verifyTargetStateCommitment(bytes32 homeBlockHash, bytes calldata input) |
Collaborator
There was a problem hiding this comment.
This variable is not a block hash. Suggested name: homeState.
frangio
approved these changes
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The verifyTargetStateCommitment function of the Linea ChildToParentProver contract uses ProverUtils.getSlotFromBlockHeader to verify the account and storage proofs. The ProverUtils function uses Merkle Patricia Tree proofs and keccak256 hashes. However, Linea uses Sparse Merkle Trees and the MiMC hash function and therefore the verifyTargetStateCommitment function is unable to verify real Linea state and any multi-hop route that requires proving Linea buffer storage will revert.
This PR updates Linea ChildToParentProver's
verifyStateCommitmentfunction to use Sparse Merkle Trees and the MiMC hash function.Summary by CodeRabbit
Release Notes
New Features
Tests