Repository navigation
test: read the handle layout from LibMemoryKV, not literals - #192
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 47 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
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 |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> # Conflicts: # test/src/lib/LibMemoryKV.types.t.sol
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thedavidmeister
added a commit
that referenced
this pull request
Sep 18, 2026
This was referenced Sep 18, 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
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.
Closes #123
Changes
Test code reads the handle layout from
LibMemoryKV(LIST_COUNT,SLOT_BITS,POINTER_MASK,COUNT_BIT_OFFSET,NODE_BYTES) and the count bound fromCOUNT_MAXintest/lib/LibMemoryKVHandle.sol. No test declares its own copy of a layout value, and no comment states one as a literal. Values that are derived from the layout are written as expressions over those constants, so every fixture keeps the value it had.types.t.solSLOTSremoved. The empty-store loop readsLibMemoryKV.LIST_COUNTandheadOf.testLayoutConstantsTileTheWord. It assertsCOUNT_BIT_OFFSET == LIST_COUNT * SLOT_BITS,COUNT_BIT_OFFSET + SLOT_BITS == 256,POINTER_MASK == 2 ** SLOT_BITS - 1andCOUNT_MAX == POINTER_MASK.COUNT_MAX,headOfandlengthOfare imported on one line fromtest/lib/LibMemoryKVHandle.sol. The header now names the layout, and the empty-store NatSpec names constants instead of "top 16 bits", "240 bits" and "Slot 14".crossFunction.t.sol: the localLIST_COUNT,SLOT_BITSandNODE_SIZEare removed. The saturated handle's expected word is built fromLibMemoryKV.LIST_COUNT,COUNT_BIT_OFFSET,NODE_BYTESandSLOT_BITS. In NatSpec, "fifteen" and "sixteen fields" become constant names.gasClaims.t.sol:SLOTSis removed and the loops readLibMemoryKV.LIST_COUNT.SHALLOW_SLOT = LibMemoryKV.LIST_COUNT - 1. "Any of the 15" and "all 15 slots" become "Any list" and "every list".wordCountOverflow.t.sol: counts and payloads are written fromCOUNT_MAX(COUNT_MAX - 3,- 2,- 1,COUNT_MAX,COUNT_MAX + 1). Addresses areLibMemoryKV.POINTER_MASKandPOINTER_MASK + 1. NatSpec quotes the error docs assrcnow words them: "pastPOINTER_MASK" and "at a pointer abovePOINTER_MASK". In thetestWordCountBoundIsUnreachableFromAnEmptyStoreNatSpec, "carrying the count to0x10000needs 32768 inserts" becomes "a count pastCOUNT_MAXneeds(COUNT_MAX + 1) / 2inserts", which is the fill the test itself performs.exportWalk.t.sol:HEAD_POINTER = LibMemoryKV.POINTER_MASK + 1 - 0x40andCHAIN_POINTER = HEAD_POINTER - LibMemoryKV.NODE_BYTES, still0xFFC0and0xFF60. The assert thatHEAD_POINTER + 0x40 == POINTER_MASK + 1is removed because it now holds by construction; NatSpec states the relation instead. The header re-quotessrc.getset.t.sol:0xFFFF,0x10000,0x1001Fand0xFFC0becomePOINTER_MASK,POINTER_MASK + 1,POINTER_MASK + 0x20andPOINTER_MASK + 1 - 0x40. "sixteen bit" and "16 bit" becomeSLOT_BITS. Assertion messages no longer name a literal.setSlotMask.t.sol: lists 14 and 13 becomeLibMemoryKV.LIST_COUNT - 1and- 2, with messages "the highest list" and "the list below it".HIGH_BIT_POINTER = 2 ** (LibMemoryKV.SLOT_BITS - 1) + 0x100, still0x8100. "sixteen bits", "(0x8000)" and "bit 224" become constant expressions.bisect.t.sol:OCCUPANCY_HIGH_HALF = OCCUPANCY_FULL ^ OCCUPANCY_LOW_HALF, still0x7F00. Layout prose is rewritten with constant names: "16 bit length field", "Fifteen slots is 32768 combinations", "the 32768 cases", the "0xFFFFceiling", "15 bit mask", "bit 15", "16 bits", "All fifteen slots" and "fifteen key constants". The assertion messages say "POINTER_HIGH_BITset" and "every key in a slot of its own".setInsert.t.sol:0x10000in the overflow-payload NatSpec becomesLibMemoryKV.POINTER_MASK + 1.array.t.sol: "each of the 15 slots" becomes "each of theLibMemoryKV.LIST_COUNTslots".test/lib/LibMemoryKVHandle.t.sol:testHandleWithHeadsOnlyTheNamedListfuzzesuint256head and count bounded toPOINTER_MASKandCOUNT_MAX, instead ofuint16parameters that hard-code a 16 bit slot.The issue lists some sites. The same category was also found and fixed in
getset, the word-level prose incrossFunctionandbisect, the derivation ofsetSlotMask'sHIGH_BIT_POINTERand thetypesheader.Literals left in place:
bisect.t.solkeeps the slot numbers that are coordinates insrc's hand-unrolled bisect tree, whichsrcitself writes as literals:checkSoleSlot(14),testHighSlot14…, slot pairs 12/13 and 7/8,2 ** 12 | 2 ** 13 | 2 ** 14) andOCCUPANCY_LOW_HALF = 0x00FF;src's bisect code.The
slotKeysvalues are keys.ABI and fixture addresses, which are not layout:
0x60ingetScratchand in thegetWalkmemory-range keccak;frameLocal0xA0,0xC0and0xE0;LOW_FREE_POINTERand0x12345;0x20/0x40, whichsrcdoes not name;uint16 gapinLibFreeMemory.t.sol.setCapacity's682,0x10040,0xFFE0and "16 bit slot", its0xA0 + 681 * 0x60=0x10000scenario, and the "48 times earlier, at the 683rd, at0x10040" line and0x10040revert inwordCountOverflow: test: name the empty-frame capacity EMPTY_FRAME_PAIRS #178 (setCapacity writes the empty-frame capacity as literals in test names, bodies and the revert address #173) owns these.The
BISECT_SAVINGNatSpec quote ingasClaims("15 slots"): test: pin the export saving as an ordering across occupancy #181 ([A15] [LOW] toBytes32Array's "~1-1.5k gas every export" claim is wrong and its gas pin is one-sided #118) deletes it.Merge notes:
types.t.sol: test: read handle fields through the helpers and count exports exactly #190 importsheadOf, lengthOffrom the same file. The second PR to merge resolves the import line to the union.testEmptyStoreHasNoWordCountandtestEmptyStoreHasNoHeadPointerInAnySlot, and that deletion wins over this PR's edits to them.wordCountOverflow.t.sol: test: name the empty-frame capacity EMPTY_FRAME_PAIRS #178 rewrites the line directly below this PR's "a count pastCOUNT_MAX" line, so the two hunks meet. The resolution keeps both.QA
Discriminating test:
testLayoutConstantsTileTheWord. Every other change here is a refactor: each constant expression evaluates to the literal it replaces, and the whole suite passes unchanged (166 tests on the probe baseline).Mutations ran on
4ec96d3with suite--match-test testLayoutConstantsTileTheWord(baseline green, 1 passed). An earlier run on7e43957gave the same verdicts (head-report.txt). The head,dfaede3, adds only the merge of main's test: saturated builder refuses keys that reach past its base #187 ([A31] [INFO] buildSaturatedExternal rewinds to SATURATED_BASE without checking it clears the decoded keys #134). That merge touchescrossFunction.t.solalone: it addstestSaturatedBuilderRefusesKeysReachingPastItsBaseand changes NatSpec.srcLIST_COUNT = 14srcSLOT_BITS = 0x11srcPOINTER_MASK = 0x7FFFsrcCOUNT_BIT_OFFSET = 0xe0srcNODE_BYTES = 0x80test/libCOUNT_MAX = type(uint256).max >> (COUNT_BIT_OFFSET - 1)COUNT_MAX == POINTER_MASKassert can fail under it.NODE_BYTESis not part of the word's tiling. The whole suite kills it (split/123/head-4ec96d3-M05-suite-report.txt, baseline 166 passed). Killers includetestCrossFrameGetReadsTheReadingFramesMemory,testEightUnrelatedWordsCostThreePairs,testOneUnrelatedWordLeavesRoomFor681Pairs,testOneUnrelatedWordMakes682PairsOverflowandtestSet682PairsFitAnOtherwiseEmptyFrame; the probe lists at most five.Reports:
/home/gildlab/artifacts/memkv/audit-2026-09-18/split/123/head-4ec96d3-report.{txt,json}andhead-4ec96d3-M05-suite-report.{txt,json}, with the tomlshead.tomlandhead-M05-suite.tomlalongside. The7e43957runs arehead-report.*andhead-M05-suite-report.*.Oracle:
testLayoutConstantsTileTheWordasserts the relations theMemoryKVNatSpec states: the lists fill the word below the count, the count is the top slot, and the pointer bound and count bound are each one slot wide. It is written independently of the values.Category check: two greps over
test:grep -n -i -E '\b(15|14|16|0x0f|0x0e|0x60|0x10|0xf0|0xe0|0xffff|0x10000|0x8000|fifteen|sixteen|fourteen|32768|240|224)\b' -r testgrep -n -E '(>>|<<|&) *0x|uint16|2 \*\* *1[0-9]' -r testOn this head they match only the
forge-std-1.16.2import paths and the sites listed above as left in place.🤖 Generated with Claude Code