Add LibInterpreterStateDataContract.bytecodeOf serialized-blob extractor - #531
Merged
Conversation
Exposes the bytecode portion of a serialized (constants, bytecode) blob (the IParserV2.parse2 output that evaluable bytecode holds) as a standalone, bounds-safe primitive, mirroring the constants-skip unsafeDeserialize already performs. Downstream consumers can introspect parse2 output (e.g. count an order's sources) by composing it with LibBytecode, without reimplementing the serialization layout in their own contract. Returns empty bytes for a blob too short to hold both length words, or one declaring a constants length that overruns it, so a consumer reading untrusted input gets a zero-source bytecode rather than reading out of bounds. Adds fuzz tests, including one asserting bytecodeOf returns byte-identical to the bytecode unsafeDeserialize references so the two constants-skips cannot drift. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR introduces ChangesBytecode Extraction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/state/LibInterpreterStateDataContract.sol (1)
3-3:⚠️ Potential issue | 🟠 MajorPin Solidity pragma to exact
=0.8.25insrc/lib/state/LibInterpreterStateDataContract.sol(lines 3)
The currentpragma solidity ^0.8.25;is floating; repo policy requires an exact compiler version.Suggested fix
-pragma solidity ^0.8.25; +pragma solidity =0.8.25;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/state/LibInterpreterStateDataContract.sol` at line 3, Replace the floating pragma at the top of LibInterpreterStateDataContract.sol by pinning the compiler to the exact version; change the pragma directive in the file (the `pragma solidity ^0.8.25;` line) to use an exact version specifier `=0.8.25` so the contract compiles only with that specific compiler version.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/state/LibInterpreterStateDataContract.sol`:
- Around line 156-169: In LibInterpreterStateDataContract.bytecodeOf, after
computing constantsLength (and before returning bc) load the embedded
bytecodeLength from serialized at bytecodeOffset = 0x40 + constantsLength * 0x20
(use assembly "memory-safe" mload(add(serialized, bytecodeOffset))) and validate
that bytecodeLength <= serialized.length - bytecodeOffset; if the check fails
return "" instead of bc. Ensure you compute bytecodeOffset the same way as when
deriving bc and reference symbols constantsLength, bytecodeOffset,
bytecodeLength, and bc in the new validation.
In `@test/src/lib/state/LibInterpreterStateDataContract.t.sol`:
- Around line 340-354: Add a new test that constructs a serialized blob where
the constants length word is in-bounds but the embedded bytecode length field
claims more bytes than remain (so bytecode overruns the payload) and assert
LibInterpreterStateDataContract.bytecodeOf(serialized).length == 0; build the
blob similar to existing tests in LibInterpreterStateDataContract.t.sol:
allocate a 0x40+N bytes buffer, write a valid small constantsLength at offset
0x20, then write a bytecodeLength at the offset where bytecode length is stored
that is larger than the actual remaining bytes, and call the same assertion as
in testBytecodeTooShortIsEmpty/testBytecodeOverrunConstantsIsEmpty to ensure an
overrun-by-bytecode case is covered.
- Around line 315-354: Add complete NatSpec tags for each new test function so
they meet audit policy: update the doc comments for
testBytecodeMatchesDeserialize, testBytecodeSingleSource,
testBytecodeTooShortIsEmpty, and testBytecodeOverrunConstantsIsEmpty to use full
NatSpec tags (e.g., `@notice` and `@dev`, and `@param/`@return where applicable)
instead of plain comments; ensure each tag describes purpose, behavior, and any
parameters (for functions with arguments like bytes32[] memory constants, bytes
memory serialized, and uint256 constantsLength) and that the comments precede
the corresponding function declarations.
---
Outside diff comments:
In `@src/lib/state/LibInterpreterStateDataContract.sol`:
- Line 3: Replace the floating pragma at the top of
LibInterpreterStateDataContract.sol by pinning the compiler to the exact
version; change the pragma directive in the file (the `pragma solidity ^0.8.25;`
line) to use an exact version specifier `=0.8.25` so the contract compiles only
with that specific compiler version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b5cbf1b-7ed9-4adf-a532-b782e3824a10
📒 Files selected for processing (2)
src/lib/state/LibInterpreterStateDataContract.soltest/src/lib/state/LibInterpreterStateDataContract.t.sol
Comment on lines
+156
to
+169
| function bytecodeOf(bytes memory serialized) internal pure returns (bytes memory bc) { | ||
| if (serialized.length < 0x40) { | ||
| return ""; | ||
| } | ||
| uint256 constantsLength; | ||
| assembly ("memory-safe") { | ||
| constantsLength := mload(add(serialized, 0x20)) | ||
| } | ||
| if (constantsLength > (serialized.length - 0x40) / 0x20) { | ||
| return ""; | ||
| } | ||
| assembly ("memory-safe") { | ||
| bc := add(add(serialized, 0x20), mul(0x20, add(constantsLength, 1))) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file
ls -la
fd -t f 'LibInterpreterStateDataContract\.sol' . || true
# Print the function and nearby code (bounded output)
FILE="src/lib/state/LibInterpreterStateDataContract.sol"
if [ -f "$FILE" ]; then
echo "---- ${FILE} (around bytecodeOf) ----"
rg -n "function bytecodeOf" "$FILE"
# show ~80 lines around the match
LINE=$(rg -n "function bytecodeOf" "$FILE" | head -n1 | cut -d: -f1)
START=$((LINE-60))
END=$((LINE+80))
sed -n "${START},${END}p" "$FILE" | nl -ba | sed -n "1,200p"
fi
# Find any other definitions/helpers that might validate bytecode length
if [ -f "$FILE" ]; then
echo "---- search for bytecodeLength / offsets in ${FILE} ----"
rg -n "bytecodeLength|bytecodeOffset|bytecodeOf\(" "$FILE"
fi
# Search for usages of bytecodeOf and unsafeDeserialize
echo "---- Usage search ----"
rg -n "bytecodeOf\(" . || true
rg -n "unsafeDeserialize|unsafeSerialize" . || true
# Search tests for malformed serialized blobs / bytecode length overrun
echo "---- Test search ----"
fd -t f -e sol test . || true
rg -n "serialized.*bytecode|bytecode.*length|constantsLength.*overrun|bytecode.*overrun|bytecodeOf" test . || trueRepository: rainlanguage/rainlang
Length of output: 2349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/lib/state/LibInterpreterStateDataContract.sol"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
echo "---- bytecodeOf implementation ----"
LINE=$(rg -n "function bytecodeOf" "$FILE" | head -n1 | cut -d: -f1)
START=$((LINE-20))
END=$((LINE+60))
sed -n "${START},${END}p" "$FILE" | nl -ba -w1 -s' ' || sed -n "${START},${END}p" "$FILE"
echo "---- Search within file for bytecode length checks ----"
rg -n "bytecodeLength|bytecodeOffset|mload\\(add\\(serialized,|serialized\\.length|constantsLength" "$FILE"
echo "---- usages of bytecodeOf and unsafeDeserialize ----"
rg -n "bytecodeOf\\(" src test
rg -n "unsafeDeserialize|unsafeSerialize" src test
echo "---- look for tests covering malformed bytecode length ----"
rg -n "bytecode.*length|overrun|malformed|serialized.*bytecode|constantsLength.*overrun|bytecodeOf\\(" test || true
echo "---- bytecodeOf call sites for untrusted input assumptions ----"
rg -n "bytecodeOf\\(" -S src || trueRepository: rainlanguage/rainlang
Length of output: 8183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/lib/state/LibInterpreterStateDataContract.sol"
TEST="test/src/lib/state/LibInterpreterStateDataContract.t.sol"
python3 - <<'PY'
import itertools,sys
def dump(path, start, end):
print(f"\n---- {path} lines {start}-{end} ----")
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:4d} {line.rstrip()}")
dump("src/lib/state/LibInterpreterStateDataContract.sol", 1, 220)
dump("src/lib/state/LibInterpreterStateDataContract.sol", 220, 420)
PY
echo "---- bytecodeOf: search for bytecodeLength guard in production ----"
rg -n "bytecodeLength|mload\\(.*bytecode|bytecode.*overrun|bytecodeOf\\(" "$FILE"
echo "---- tests around bytecodeOf malformed inputs ----"
python3 - <<'PY'
path="test/src/lib/state/LibInterpreterStateDataContract.t.sol"
start=280
end=420
print(f"\n---- {path} lines {start}-{end} ----")
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:4d} {line.rstrip()}")
PY
echo "---- look for any malformed serialized crafting for bytecode length ----"
rg -n "bytecode.*length|bytecodeLength|malformed|overrun|serialized\\[|mstore|assembly" "$TEST" || trueRepository: rainlanguage/rainlang
Length of output: 14401
Add a bytecode-length bounds check in LibInterpreterStateDataContract.bytecodeOf (lines 156-169).
bytecodeOf only bounds-checks constantsLength to ensure the bytecode length word is in-range, but it never validates the embedded bytecodeLength against the remaining bytes in serialized. A malformed blob can therefore return an in-place bytes slice with an oversized length, causing out-of-bounds reads/leaky ABI encoding when callers use the result.
Guard bytecodeLength before returning bc by loading it from serialized at bytecodeOffset = 0x40 + constantsLength * 0x20 and returning "" unless bytecodeLength <= serialized.length - bytecodeOffset.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/state/LibInterpreterStateDataContract.sol` around lines 156 - 169, In
LibInterpreterStateDataContract.bytecodeOf, after computing constantsLength (and
before returning bc) load the embedded bytecodeLength from serialized at
bytecodeOffset = 0x40 + constantsLength * 0x20 (use assembly "memory-safe"
mload(add(serialized, bytecodeOffset))) and validate that bytecodeLength <=
serialized.length - bytecodeOffset; if the check fails return "" instead of bc.
Ensure you compute bytecodeOffset the same way as when deriving bc and reference
symbols constantsLength, bytecodeOffset, bytecodeLength, and bc in the new
validation.
Comment on lines
+315
to
+354
| /// `bytecode` returns exactly the body `unsafeDeserialize` references, for | ||
| /// fuzzed constants — the constants-skip in both must agree. | ||
| function testBytecodeMatchesDeserialize(bytes32[] memory constants) external view { | ||
| bytes memory expected = buildTwoSourceBytecode(3, 5); | ||
| bytes memory serialized = serialize(expected, constants); | ||
|
|
||
| bytes memory got = LibInterpreterStateDataContract.bytecodeOf(serialized); | ||
| assertEq(got.length, expected.length); | ||
| assertEq(keccak256(got), keccak256(expected)); | ||
|
|
||
| InterpreterState memory state = iExtern.deserialize( | ||
| serialized, 0, FullyQualifiedNamespace.wrap(0), IInterpreterStoreV3(address(0)), new bytes32[][](0), "" | ||
| ); | ||
| assertEq(keccak256(got), keccak256(state.bytecode)); | ||
| } | ||
|
|
||
| /// `bytecode` round-trips a single-source body behind fuzzed constants. | ||
| function testBytecodeSingleSource(bytes32[] memory constants) external pure { | ||
| bytes memory expected = buildSingleSourceBytecode(1, 2, 0, 1); | ||
| bytes memory serialized = serialize(expected, constants); | ||
| bytes memory got = LibInterpreterStateDataContract.bytecodeOf(serialized); | ||
| assertEq(got.length, expected.length); | ||
| assertEq(keccak256(got), keccak256(expected)); | ||
| } | ||
|
|
||
| /// A blob too short to hold both length words yields empty bytecode. | ||
| function testBytecodeTooShortIsEmpty(bytes memory serialized) external view { | ||
| vm.assume(serialized.length < 0x40); | ||
| assertEq(LibInterpreterStateDataContract.bytecodeOf(serialized).length, 0); | ||
| } | ||
|
|
||
| /// A declared constants length that overruns the blob yields empty bytecode. | ||
| function testBytecodeOverrunConstantsIsEmpty(uint256 constantsLength) external view { | ||
| constantsLength = bound(constantsLength, 1, type(uint256).max); | ||
| bytes memory serialized = new bytes(0x40); | ||
| assembly ("memory-safe") { | ||
| mstore(add(serialized, 0x20), constantsLength) | ||
| } | ||
| assertEq(LibInterpreterStateDataContract.bytecodeOf(serialized).length, 0); | ||
| } |
Contributor
There was a problem hiding this comment.
Use complete NatSpec tags on the newly added tests to satisfy audit policy.
These new test doc comments are descriptive, but not fully tagged NatSpec entries as required by your test audit guideline.
As per coding guidelines, "test/**/*.{sol,rs}: All test files must meet audit requirements ... including ... complete NatSpec".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/src/lib/state/LibInterpreterStateDataContract.t.sol` around lines 315 -
354, Add complete NatSpec tags for each new test function so they meet audit
policy: update the doc comments for testBytecodeMatchesDeserialize,
testBytecodeSingleSource, testBytecodeTooShortIsEmpty, and
testBytecodeOverrunConstantsIsEmpty to use full NatSpec tags (e.g., `@notice` and
`@dev`, and `@param/`@return where applicable) instead of plain comments; ensure
each tag describes purpose, behavior, and any parameters (for functions with
arguments like bytes32[] memory constants, bytes memory serialized, and uint256
constantsLength) and that the comments precede the corresponding function
declarations.
Source: Coding guidelines
Comment on lines
+340
to
+354
| /// A blob too short to hold both length words yields empty bytecode. | ||
| function testBytecodeTooShortIsEmpty(bytes memory serialized) external view { | ||
| vm.assume(serialized.length < 0x40); | ||
| assertEq(LibInterpreterStateDataContract.bytecodeOf(serialized).length, 0); | ||
| } | ||
|
|
||
| /// A declared constants length that overruns the blob yields empty bytecode. | ||
| function testBytecodeOverrunConstantsIsEmpty(uint256 constantsLength) external view { | ||
| constantsLength = bound(constantsLength, 1, type(uint256).max); | ||
| bytes memory serialized = new bytes(0x40); | ||
| assembly ("memory-safe") { | ||
| mstore(add(serialized, 0x20), constantsLength) | ||
| } | ||
| assertEq(LibInterpreterStateDataContract.bytecodeOf(serialized).length, 0); | ||
| } |
Contributor
There was a problem hiding this comment.
Add a malformed bytecode-length overrun test case.
Current edge tests cover short blobs and constants overruns, but not a blob where constants are in-bounds and the embedded bytecode length overruns the payload.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/src/lib/state/LibInterpreterStateDataContract.t.sol` around lines 340 -
354, Add a new test that constructs a serialized blob where the constants length
word is in-bounds but the embedded bytecode length field claims more bytes than
remain (so bytecode overruns the payload) and assert
LibInterpreterStateDataContract.bytecodeOf(serialized).length == 0; build the
blob similar to existing tests in LibInterpreterStateDataContract.t.sol:
allocate a 0x40+N bytes buffer, write a valid small constantsLength at offset
0x20, then write a bytecodeLength at the offset where bytecode length is stored
that is larger than the actual remaining bytes, and call the same assertion as
in testBytecodeTooShortIsEmpty/testBytecodeOverrunConstantsIsEmpty to ensure an
overrun-by-bytecode case is covered.
Contributor
Author
|
Reviewed 79d246c: Additive LibInterpreterStateDataContract.bytecodeOf — a bounds-safe constants-skip mirroring unsafeDeserialize, with a 2048-run fuzz test asserting byte-identity to the deserialize path so the two skips can't drift. No deployed-contract bytecode change (unused by interpreter/parser/deployer); full suite green bar RPC-env-gated fork tests; CI green. |
Contributor
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. SIZE=M |
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
1 participant
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.
Adds
LibInterpreterStateDataContract.bytecodeOf(bytes)— returns the bytecode portion of a serialized(constants, bytecode)blob (anIParserV2.parse2output), referenced in place, mirroring the constants-skipunsafeDeserializealready performs.Why
Consumers that store
evaluable.bytecode(the serialized blob) and want to introspect it — e.g. raindex rejecting orders with no calculate / handle-IO source at add time — have no library-level way to reach the inner rain bytecode. The only existing path,unsafeDeserialize, allocates a fullInterpreterState. This exposes just the bytecode slice, so a consumer composesLibBytecode.sourceCount(LibInterpreterStateDataContract.bytecodeOf(serialized))instead of reimplementing the serialization layout in their own (money-moving) contract.Safety
Bounds-checked for untrusted input: returns empty bytes when the blob is too short to hold both length words, or declares a constants length that would overrun it — so a consumer reads a zero-source bytecode rather than out of bounds.
Tests
4 new fuzz tests (2048 runs each). The key one,
testBytecodeMatchesDeserialize, assertsbytecodeOfreturns byte-identical to the bytecodeunsafeDeserializereferences across fuzzed constants, so the two constants-skips cannot drift. Plus single-source round-trip, too-short → empty, overrun-constants → empty.No deployed-contract bytecode changes (the function is unused by the interpreter / parser / deployer), so no redeploy — just the soldeer package publish.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests