Re-order check#153
Conversation
WalkthroughThe logic for validating whether an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
src/protocol/taiko_alethia/TaikoInbox.sol (1)
69-69: Optional: avoid both underflow and addition overflow with two comparisonsTo be resilient even if
maxAnchorBlockIdOffsetis misconfigured to an extreme value (preventing a revert on addition overflow), consider this equivalent formulation:-require(anchorBlockId + maxAnchorBlockIdOffset >= block.number, AnchorBlockTooOld()); +require( + anchorBlockId <= block.number + && block.number - anchorBlockId <= maxAnchorBlockIdOffset, + AnchorBlockTooOld() +);Pros: no subtraction underflow (guarded by the first clause), no addition overflow, clearer intent as “distance ≤ offset.” Cons: marginally more gas due to two comparisons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/protocol/taiko_alethia/TaikoInbox.sol(1 hunks)
🔇 Additional comments (1)
src/protocol/taiko_alethia/TaikoInbox.sol (1)
69-69: Reordered check eliminates subtraction underflow — LGTMSwitching to
anchorBlockId + maxAnchorBlockIdOffset >= block.numberavoids the potential revert fromblock.number - maxAnchorBlockIdOffsetunderflow in Solidity 0.8+. Intent is preserved while improving robustness on early chains.
|
|
||
| // Build the attribute for the anchor transaction inputs | ||
| require(anchorBlockId >= block.number - maxAnchorBlockIdOffset, AnchorBlockTooOld()); | ||
| require(anchorBlockId + maxAnchorBlockIdOffset >= block.number, AnchorBlockTooOld()); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add boundary tests to lock in semantics and prevent regressions
Please add tests covering:
- Line 69 boundary:
anchorBlockId == block.number - maxAnchorBlockIdOffset(passes) andanchorBlockId == block.number - maxAnchorBlockIdOffset - 1(revertsAnchorBlockTooOld()). - Early-chain scenario:
maxAnchorBlockIdOffset > block.number(should not revert due to underflow; still fails later withBlockhashUnavailable()if out of 256-block window). - Future/current block IDs:
anchorBlockId >= block.number(should revertBlockhashUnavailable()). - Very large
maxAnchorBlockIdOffset(if allowed by config), to ensure no unexpected addition overflow behavior.
You can quickly check for existing tests and anchor usages with:
I can draft Foundry/Hardhat tests for these cases if helpful.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for AnchorBlockTooOld usage/tests:"
rg -n --hidden --glob '!*node_modules*' 'AnchorBlockTooOld' -A 3 -B 3 || true
echo "Search for TaikoInbox.publish tests or calls:"
rg -n --hidden --glob '!*node_modules*' 'publish\(' -A 3 -B 3 || true
echo "Search for anchorBlockId boundary conditions in tests:"
rg -n --hidden --glob '!*node_modules*' 'anchorBlockId' -A 5 -B 5 || trueLength of output: 14681
Add comprehensive boundary tests for publish in TaikoInbox
We need to lock in the semantics of the require(anchorBlockId + maxAnchorBlockIdOffset >= block.number, AnchorBlockTooOld()) check and the subsequent BlockhashUnavailable() guard. No existing unit tests cover these edge cases, so please add tests (e.g. in test/InboxBoundary.t.sol) for:
-
Exact lower boundary:
• anchorBlockId == block.number − maxAnchorBlockIdOffset → should succeed
• anchorBlockId == block.number − maxAnchorBlockIdOffset − 1 → should revertAnchorBlockTooOld() -
Underflow “early‐chain” scenario (when
maxAnchorBlockIdOffset > block.number):
• Ensure the underflowed sum still passes the firstrequire, but then revertsBlockhashUnavailable()if the block is out of the 256‐block hash window. -
Future and current block IDs:
• anchorBlockId ≥ block.number → should revertBlockhashUnavailable()(becauseblockhash(anchorBlockId)returns 0) -
Large
maxAnchorBlockIdOffsetvalues:
• If your config allows very large offsets, verify no overflow inanchorBlockId + maxAnchorBlockIdOffsetand correct revert paths.
Pinpoint locations:
- File:
src/protocol/taiko_alethia/TaikoInbox.sol, line 69 (theAnchorBlockTooOld()check) - Immediately after: the
blockhashlookup and itsBlockhashUnavailable()require
Feel free to use Foundry’s vm.roll(...) to manipulate block.number and vm.expectRevert(...) for these scenarios.
🤖 Prompt for AI Agents
In src/protocol/taiko_alethia/TaikoInbox.sol at line 69 and immediately after,
add comprehensive unit tests in test/InboxBoundary.t.sol to cover boundary
conditions of the require check and blockhash validation in the publish
function. Specifically, write tests for the exact lower boundary cases where
anchorBlockId equals block.number minus maxAnchorBlockIdOffset (should succeed)
and one less than that (should revert AnchorBlockTooOld). Also, test the
underflow scenario when maxAnchorBlockIdOffset is greater than block.number to
ensure the require passes but blockhash lookup reverts with
BlockhashUnavailable. Include tests for anchorBlockId greater or equal to
block.number to confirm it reverts BlockhashUnavailable due to blockhash
returning zero. Finally, if large maxAnchorBlockIdOffset values are allowed,
verify no overflow occurs in the sum and that revert paths behave correctly. Use
Foundry’s vm.roll to manipulate block.number and vm.expectRevert to assert the
expected reverts.
2 participants
Less likely to run in underflow scenarios
Summary by CodeRabbit