perf(rate limit): key RateLimitUnits by (entity, type), not by time bucket - #444
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
frolic
force-pushed
the
claude/moveunits-key
branch
from
July 26, 2026 17:05
4238627 to
32c3d76
Compare
Every gas test in the suite runs inside a single `block.timestamp`, so every one of them measures the *first* write into a `RateLimitUnits` bucket and none of them measure the second. The bucket key is `timestamp - (timestamp % RATE_LIMIT_TIME_INTERVAL)` and the interval is 2 seconds -- one Base block -- so the case the suite never covers is the case continuous play consists of almost entirely. Adds a move pair and a mine pair (Movement and Work limit types), each reporting the same action in a fresh bucket and then in the next one. Baseline: mine terrain with hand, first rate limit bucket 452,483 mine terrain with hand, later bucket (steady state) 326,991 move 1 block, first rate limit bucket 347,571 move 1 block, later bucket (steady state) 278,144 Also gives three colliding gas report rows distinct names. `startGasReport` only rejects a duplicate name within one test -- across tests in a contract the mapping is fresh each time, so these landed in gas-report.json as separate entries sharing a name, and any diff keyed on the name silently compared the wrong pairs: Build.t.sol "build multi-size" -> "... with orientation" (testBuildMultiSizeWithOrientation) ForceField.t.sol "Remove forcefield fragment" -> "... after add" (testFragmentGasUsage) HitMachine.t.sol "hit force field without tool" -> "..., fatal to player" (testHitForceFieldWithoutToolFatal) The name is read only outside the `gasleft()` window, so no measurement moves. Note for anyone re-running this locally: the Farming/Tree suites need the local anvil to be past CHUNK_COMMIT_EXPIRY_BLOCKS (256) or they fail on "Existing chunk commitment", so mine ~400 blocks before `./runTests.bash --with-gas`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
frolic
force-pushed
the
claude/moveunits-key
branch
from
July 26, 2026 18:34
32c3d76 to
5509db2
Compare
frolic
force-pushed
the
claude/moveunits-key
branch
from
July 27, 2026 19:44
5509db2 to
aeb53bb
Compare
frolic
force-pushed
the
claude/moveunits-key
branch
from
July 27, 2026 19:49
aeb53bb to
6bb6ffe
Compare
…timestamp, type) `RateLimitUnits` was keyed by its time bucket, so every RATE_LIMIT_TIME_INTERVAL the key changed and the write landed on a fresh, zero-valued slot: SSTORE-set (20,000) rather than SSTORE-reset (2,900), on every rate-limited action. The interval is 2 seconds and Base blocks are 2 seconds, so in practice the bucket rotated every block and essentially no action ever hit the cheap path. The bucket is now a value instead of a key. One record per (entity, type) is reused for the lifetime of the entity; a record stamped with an earlier interval reads as empty, which preserves the old reset-to-zero semantics without a clearing write. `timestamp` narrows to uint64 so it shares a slot with `units` (FieldLayout 0x0018: 24 bytes, one slot), keeping the write to a single SSTORE. A registered table's schema is immutable -- `registerTable` reverts with `World_ResourceAlreadyExists` -- so the table takes a new name, `V3RateLimitUn`. V2RateLimitUn stays registered on chain (deploy never unregisters) but is dropped from the config, since nothing reads it. At the upgrade every entity's V3 record is empty, so the one interval spanning the upgrade allows at most one extra action before the limit reasserts. Measured (`./runTests.bash --with-gas`, 438 tests passing): mine terrain with hand, first rate limit bucket 452,483 -> 453,892 +1,409 mine terrain with hand, later bucket (steady state) 326,991 -> 306,500 -20,491 move 1 block, first rate limit bucket 347,571 -> 349,002 +1,431 move 1 block, later bucket (steady state) 278,144 -> 257,675 -20,469 The regression is the point of the trade and it is paid once: reading and comparing the stored bucket costs ~1,410-1,440, and it shows up on all 52 single-bucket rows in the report because every other gas test runs inside one `block.timestamp`. Every action after an entity's first one saves ~20,480 -- 7.4% of a one-waypoint move, 6.3% of a hand mine. A player moving once per block for an hour pays the 1,431 once and saves ~36.8M gas. The other two wins in the report, `harvest mature tree log` (-20,492) and `harvest immature tree seed` (-20,491), are the same effect found by tests that already warped past a bucket boundary for their own reasons. Secondly, this removes an unbounded state-growth source. Keyed by bucket, every 2 seconds of play left a record behind permanently, up to one per rate limit type. Keyed by type, a player's rate limit footprint is 4 records total, forever. No behaviour change: same limits, same window, same revert. The table is ephemeral and has no offchain consumers -- `RateLimitUtils` is its only reader. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
frolic
force-pushed
the
claude/moveunits-key
branch
from
July 27, 2026 19:50
6bb6ffe to
a09e8ad
Compare
MoveUnits was superseded by RateLimitUnits in #415 and kept in the config only so the upgrade would not lose sight of it. Nothing reads it -- the only remaining matches are two unrelated test names (testWalkMoveUnits, testWaterMoveUnits) that never touch the table. Deploy never unregisters, so the table stays registered on chain either way; this only stops carrying a dead definition and its codegen. Same reasoning applies to V2RateLimitUn, dropped in the previous commit. 438 tests pass and gas-report.json is byte-identical -- an unreferenced codegen table cannot affect system bytecode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ck.timestamp `getLatestEnergyData` opened with a bare uint128 timeSinceLastUpdate = uint128(block.timestamp) - energyData.lastUpdatedTime; which assumes `lastUpdatedTime <= block.timestamp`. That holds for a landed transaction, but not for `eth_call`/`eth_estimateGas` at `pending` on a chain that builds sub-blocks: the pending state can already hold a write stamped with a newer block's timestamp while the simulated header still carries the older one. The subtraction underflowed and the call failed with panic 0x11 instead of reverting cleanly. Seen in play on Base Sepolia as bursts of estimates failing with "panic: arithmetic underflow or overflow (0x11)" that succeeded on retry a block later. The client estimates with `blockTag: "pending"`, and sampling the chain shows the pending header sometimes still equal to latest while state has moved on. Invisible to players because the client retries, but a panic means an unguarded subtraction rather than an intended revert, and it burns a round trip each time. Fixed by comparing before subtracting. The guard folds into the existing zero-check rather than adding a branch, and the subtraction is then `unchecked` because the comparison already proves it cannot underflow. Nothing drains when no time has passed, so behaviour is unchanged for every landed transaction. Size matters here more than usual: BuildSystem ships with **3 bytes** of EIP-170 headroom and MineSystem with 29 (24,573 and 24,547 of 24,576). A first cut using a ternary added ~60 bytes and pushed both over the limit, where the only symptom is `mud deploy` failing with "Execution reverted for an unknown reason" from the CREATE2 deployer. This version costs exactly 1 byte in each. 440 tests pass. `EnergyTimestampSkew.t.sol` covers both directions; before this change its first case reverted with Panic(0x11). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `unchecked` subtraction drops the compiler's overflow check from a path every action walks, so 92 rows improve and none regress (net -11,208). Combined with the rate limit re-key the steady-state rows now read: move 1 block, later bucket (steady state) -20,617 mine terrain with hand, later bucket -20,555 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Rebuilt from scratch against
base-deploy. The previous version of this PR targetedMoveUnits, which #443 deprecated — the same defect now lives inRateLimitUnitsand applies to five call sites (move,mine,build,hitPlayer,hitMachine) instead of one.The defect
RateLimitUnitsis keyed by its own time bucket:RATE_LIMIT_TIME_INTERVALis 2 seconds and Base blocks are 2 seconds, so the key changes every block. Every rate-limited action therefore writes to a fresh, zero-valued slot — SSTORE-set (20,000) instead of SSTORE-reset (2,900) — and essentially no action ever reaches the cheap path. The old record is never cleared, so each 2 seconds of play also leaves a slot behind permanently.The change
The bucket becomes a value instead of a key:
One record per (entity, type), reused for the lifetime of the entity. A record stamped with an earlier interval reads as empty, which preserves the reset-to-zero semantics without a clearing write.
timestampnarrows touint64so it packs into the same slot asunits— FieldLayout0x0018, 24 bytes, one slot, one SSTORE.RateLimitUtilsis the table's only reader; there are no offchain consumers.A registered table's schema is immutable —
registerTablereverts withWorld_ResourceAlreadyExists— so the table takes a new name,V3RateLimitUn.V2RateLimitUnstays registered on chain — deploy never unregisters — but is dropped from the config, since nothing reads it. Its records remain as dead state. At the upgrade every entity's V3 record is empty, so the single interval spanning the upgrade allows at most one extra action before the limit reasserts.Measured
./runTests.bash --with-gas, 438 tests passing:The regression is the trade and it is paid once per entity: reading and comparing the stored bucket costs ~1,410–1,440. It shows up on all 52 single-bucket rows in the report because every other gas test in the suite runs inside a single
block.timestamp— which is also why the suite could not see this problem.Every action after an entity's first one saves ~20,480: 7.4% of a one-waypoint move, 6.3% of a hand mine. A player moving once per block for an hour pays the 1,431 once and saves ~36.8M gas.
Commits
test(gas)— addsRateLimitBuckets.t.sol: a move pair and a mine pair (Movement and Work limit types), each reporting the same action in a fresh bucket and then in the next one, plus the regenerated baseline. Without this the suite is blind to the steady state, since it only ever measures the first write into a bucket.It also gives three colliding gas report rows distinct names.
startGasReportonly rejects a duplicate name within one test, sobuild multi-size,Remove forcefield fragmentandhit force field without tooleach appeared twice in gas-report.json, and any diff keyed on the row name silently compared the wrong pairs. The name is read only outside thegasleft()window, so no measurement moves — every number above is byte-identical before and after the rename.perf(rate limit)— the re-key, with the report regenerated on top.Also
State growth: keyed by bucket, a player's rate-limit footprint grew by up to one record per limit type every 2 seconds, forever. Keyed by type, it is 4 records total, forever.
Re-running locally: the Farming/Tree suites need the local anvil past
CHUNK_COMMIT_EXPIRY_BLOCKS(256) or they fail on "Existing chunk commitment" — mine ~400 blocks before./runTests.bash --with-gas.🤖 Generated with Claude Code
Also in here: an energy underflow
getLatestEnergyDataopened with a bareuint128(block.timestamp) - energyData.lastUpdatedTime, which assumeslastUpdatedTime <= block.timestamp. True for a landed transaction; not true foreth_call/eth_estimateGasatpendingon a chain that builds sub-blocks, where pending state can hold a write stamped with a newer block's timestamp while the simulated header still carries the older one. It underflowed to panic 0x11 instead of reverting cleanly.Observed in play on Base Sepolia as bursts of estimates failing with
panic: arithmetic underflow or overflow (0x11)that succeeded on retry a block later — invisible to players, but a panic means an unguarded subtraction, and it burns a round trip each time.Fixed by comparing before subtracting, folded into the existing zero-check, with the subtraction then
uncheckedsince the comparison proves safety. That drops the compiler's overflow check from a path every action walks: 92 rows improve, none regress, net −11,208.Worth knowing: EIP-170 headroom is nearly gone
A first cut used a ternary and added ~60 bytes, which pushed two systems over the 24,576 limit. The only symptom is
mud deployfailing withExecution reverted for an unknown reasonfrom the CREATE2 deployer — no mention of size.BuildSystemhas two bytes of headroom after this PR. Any further change to the build path needs a size budget first.