Bump rain-lib-memkv to 0.2.0: fingerprint the stateKV contents, not the handle word - #586
Conversation
rain-lib-memkv 0.2.0 makes a non-empty MemoryKV handle the address of a fixed header, so the handle word no longer changes on an insert. The test fingerprint hashed abi.encode(state), which includes that word, so an insert into a non-empty stateKV left the fingerprint unchanged (updates were already invisible to it). The fingerprint now hashes every other field of the state as before and replaces the stateKV handle with the sorted keccak256 of each pair the store holds. Inserts and updates move it, and stores holding the same pairs fingerprint the same regardless of address or export order. Closes #585 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: rainlanguage/rainlang/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (31)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe project upgrades ChangesMemoryKV upgrade and fingerprint behavior
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant InterpreterState
participant LibInterpreterStateFingerprint
participant MemoryKV
participant Arrays
InterpreterState->>LibInterpreterStateFingerprint: fingerprint(state)
LibInterpreterStateFingerprint->>MemoryKV: toBytes32Array()
LibInterpreterStateFingerprint->>LibInterpreterStateFingerprint: hash key/value pairs
LibInterpreterStateFingerprint->>Arrays: sort pair hashes
LibInterpreterStateFingerprint->>LibInterpreterStateFingerprint: hash state and pair hashes
LibInterpreterStateFingerprint-->>InterpreterState: restore stateKV and return fingerprint
Merge Risk: ⚪ Minimal · up to The dependency upgrade is internally consistent and introduces no concrete merge-blocking risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue Full details: Out of Scope Changes checkExplanation The boolean-cst comment and lint-suppression changes in parser files and parser test doubles do not change
✨ 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 |
The rainix static job now runs `forge lint -D warnings`, which flags 21 `return (true|false, ...)` sites on unmodified main as boolean-cst. Each literal is the bool the function returns, not a condition operand, so each gets a scoped disable with that reason. Comments only: no code changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rals" This reverts commit 864cf10. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@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:
|
Closes #585
What
rain-lib-memkv0.2.0 (tagsol-v0.2.0on rainlanguage/rain.lib.memkv) infoundry.toml,soldeer.lockandremappings.txt, and moves the versioned import prefixrain-lib-memkv-0.1.5/torain-lib-memkv-0.2.0/in the 17 files that import it.test/lib/state/LibInterpreterStateFingerprint.sol: the fingerprint hashes the pairsstateKVholds instead of its handle word.Why
On 0.2.0 a non-empty
MemoryKVis the address of a fixed header, so the handle word stops changing after the first insert. The old fingerprint waskeccak256(abi.encode(state)), which includes that word, so an insert into a non-emptystateKVleft it unchanged. Updates were already invisible to it on 0.1.5.OpTest.opReferenceCheckActualandLibOpStack.t.soluse this fingerprint to assert an op leaves the state alone.How
fingerprintsetsstate.stateKVtoMEMORY_KV_EMPTY, hashesabi.encode(state, stateKVPairHashes(stateKV)), and restores the handle before returning. Every other field still goes throughabi.encode(state), so a field added toInterpreterStateis still covered.stateKVPairHashesis the keccak256 of each key/value pair, sorted with OpenZeppelinArrays.sort, becausetoBytes32Arraydocuments its pair order as unspecified. Inserts and updates move the fingerprint. Stores holding the same pairs fingerprint the same wherever they sit in memory.No
srclogic changes. Everysrcuse ofstateKVassigns the result ofsetback tostate.stateKV(LibOpSet, theLibOpGetmiss cache, theBaseRainlangInterpreter.eval4overlay,LibEval's export, the init inLibInterpreterStateDataContract). Nothing copies, branches or restores a handle, so 0.2.0's shared-store copies change nothing here.Gas figures (issue step 3): nothing to update in this repo. rainlang has no committed gas figures.
.gas-snapshotleft with the deploy half in 93f2825, and none of the tests assert a gas number.Out of scope (follow-ups, not in this PR)
BaseRainlangInterpreter,LibEval,LibOpGet,LibOpSetcompile against the newLibMemoryKV), so the release needs a new deploy through rainlang.deploy.rain-lib-memkvnext torainlangand need both bumped together.QA
test/src/lib/state/LibInterpreterState.fingerprint.t.sol, all fuzzed):testFingerprintChangesOnInsertIntoNonEmptyStateKVis the test the issue asks for. With 0.2.0 installed and main's helper (keccak256(abi.encode(state))) restored, it FAILS on the first fuzz input (runs: 0). On this branch it passes 2048 runs.testFingerprintChangesOnStateKVUpdateFAILS on main's helper and passes here.testFingerprintSameForSamePairsInSeparateStoresbuilds two stores from the same 32 pairs inserted in opposite orders. It first asserts that theirtoBytes32Arrayexports differ, so the sort is exercised, and then that their fingerprints are equal. It FAILS on main's helper (the handles differ) and passes here.testFingerprintDiffersForDifferentKeysandtestFingerprintLeavesStateKVHandletest the new mechanism: the key is part of each pair hash, and the handle is restored. The mutation pass below shows each one kills a mutant that nothing else kills.LibInterpreterStateFingerprint.sol, each applied withsed. After each one I ran the fingerprint suite, checked it ran 7 tests, restored the file and checked the tree was clean. 8 killed, 0 survived:SameForSamePairsInSeparateStoresLeavesStateKVHandleSameForSamePairsInSeparateStoresChangesOnStateKVUpdateDiffersForDifferentKeysChangesOnInsertIntoNonEmptyStateKVLibMemoryKVin rain.lib.memkv 0.2.0. It says a non-empty handle is a header address that every copy shares, and thattoBytes32Array's pair order "is unspecified and MUST NOT be relied upon; a caller that needs a canonical form MUST sort". The issue's own probe, the reference clone'stest/scratch/FingerprintProbe.t.sol, fails at "fingerprint must move on an insert into a non-empty stateKV" for the same reason the first test above does.rain-lib-memkv-0.1.5/remapping thatsoldeer updateleaves behind, so no 0.1.5 path can resolve.srcuse ofstateKV(git grep): each assignsset's result back tostate.stateKV, and nothing copies or restores a handle. The test-side handle comparisons (LibOpGet.t.sol,LibOpSet.t.sol,LibInterpreterStateDataContract.t.sol) compare against the empty handle0, which 0.2.0 keeps.github:rainlanguage/rainix/8657b83b68f41957ab85da91132c3f652c1f32c0#sol-shell, the shell CI uses:pre-commit run --all-filesandrainix-sol-single-contractboth exit 0.forge lint -D warningsexits 1 on the same 21boolean-cstfindings as unmodifiedmain. None of them is in a file this PR touches, and Scope boolean-cst lint disables on the tuple-return bool literals #588 fixes them./home/gildlab/artifacts/rainlang/issue-585/.🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Maintenance