Repository navigation
test: exclude word sets a parse meta cannot represent - #145
Conversation
`buildParseMetaV2` reverts `DuplicateFingerprint` when two words share an expansion bit and a 3 byte fingerprint under a seed it picks, because the 4 byte meta item cannot tell them apart. That is the documented outcome of a 1 byte bitmap plus 3 byte fingerprint, which the mask's own comment rates at solidity selector resistance; a real word set that hits it is fixed by renaming a word. A fuzzer drawing uniform `bytes32` words hits the ~2^-32 per pair coincidence often enough to flake CI. The counterexample from run 36937526403 is 114 words with exactly one colliding pair, at indices 30 and 66, which agree on expansion bit 199 and fingerprint 0x9540FF under seed 59 and on nothing under seed 0. `LibParseMetaFingerprint.findsDupes` scans every seed in [0, 255] and chains words on the low fingerprint byte, so a false result guarantees `buildParseMetaV2` cannot revert for that set. Scanning every seed is the cheap direction: predicting the seeds `findBestExpander` picks costs a full expander chain. The six fuzz tests that build a parse meta from a fuzzed word set assume on it, after the existing bloom. No production code changes and no assertion is weakened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: rainlanguage/rainlang.interface/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes add a helper that detects word pairs with matching masked fingerprints and expansion bitmaps across 256 seeds. Tests cover direct detection, a pinned metadata-generation counterexample, and fuzz-test exclusions. ChangesParse-meta fingerprint duplicate handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Other Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is identified; the change is ready to merge after normal checks. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 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:
|
1 participant
main's test job fails on some fuzz seeds withDuplicateFingerprint()intestRoundMetaExpanderDeeper, which blocks autopublish.It is a test bound, not a defect, and that was proven rather than assumed. The CI counterexample (run 36937526403) is 114 words. A brute-force scan of all 256 seeds against all 6441 pairs found exactly one colliding pair — indices 30 and 66, agreeing on expansion bit 199 and fingerprint
0x9540FFunder seed 59, and on nothing under seed 0. The collision is legitimate:wordBitmappedtakes the bitmap from the high byte ofkeccak(word‖seed)and the fingerprint from the low three, so 32 independent bits must agree, whichFINGERPRINT_MASK's own comment rates at solidity selector resistance. Two such words cannot both live in a 4 byte meta item, andDuplicateFingerprintexists to say so at codegen time. A real word set that hits it is fixed by renaming a word.LibParseMetaFingerprint.findsDupesscans every seed in[0, 255]and chains words on the low fingerprint byte, comparing the full fingerprint and the full bitmap, so afalseresult guaranteesbuildParseMetaV2cannot revert withDuplicateFingerprint. Scanning every seed is the cheap direction: predicting the seedsfindBestExpanderpicks costs a full expander chain. The six fuzz tests that build a parse meta from a fuzzed word set now assume on it, after the existing bloom.Cost, measured rather than estimated: the two heavy parse-meta suites take 7m38 with the guard. An assembly version of the guard was drafted and is not shipped — it has never compiled, and trusting it would need differential fuzz tests holding its inlined hashing to
wordBitmapped. The gas is Solidity per-iteration overhead, not hashing: the guard already hashes each word once per seed (29k keccaks at n=114), not once per pair.Two findings filed separately rather than folded in: #143 (
buildParseMetaV2can write one item past the items region) and #144 (checkNoOOBPointersfuzz tests getTruncatedSourceon some seeds — also red onmain, unrelated to this suite).QA
testCiCounterexampleBuildRevertspins that the 114-word set does revert;testCiCounterexampleIsGuardedthat the guard flags it;testCiCounterexampleBuildsWithoutCollidingWordthat dropping index 66 leaves 113 words that pass the guard, build and round-trip — the discriminator separating "guard works" from "guard rejects everything".testFindsDupesCollisionOnlyAtNonZeroSeedpins that the pair is clean under seed 0, so a seed-0-only scan is not enough.revert DuplicateFingerprint()→ assignment (testCiCounterexampleBuildReverts). Survived: ignore the bitmap compare — equivalent, it only over-rejects and the guard's contract is one-directional. Compare the chain head only initially survived, which was a real gap:testFindsDupesCollisionBuriedInSetburies the pair behind sequential fillers, so whether any filler shares the colliding bucket is a property of their hashes rather than of the test. AddedtestFindsDupesCollisionBehindAChainHead, which constructs a filler that does share the bucket; it kills that mutant.wordBitmapped's own documented split of the hash into bitmap and fingerprint bytes. The guard is exact, not bloomed, so itsfalseis a guarantee rather than a probability.findExpander.t.solleft alone becausefindBestExpanderhas noDuplicateFingerprintpath. No assertion weakened, no production code changed, notry/catcharound the build.forge fmt --check,forge lint -D warnings,slither(61 contracts, 0 results) andreuse lintall clean. 141 of 143 tests pass; the 2 failures are #144 onmain.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests