From 9c4294790327372aaa9995c73e476863130bcdb6 Mon Sep 17 00:00:00 2001 From: openhands Date: Thu, 12 Mar 2026 12:31:11 +0000 Subject: [PATCH] fix: address review findings in FitnessEvaluator (#604) - Wrap upgradeTo() in try/catch: malformed candidate bytecode no longer aborts the entire batch; emit {"fitness":0,"error":"upgrade_failed"} and continue to the next candidate - Bootstrap recenter: require() after 5 retry attempts so silent failure (all scores identically equal to free WETH only) is surfaced as a hard test failure rather than silently producing meaningless results - mint_lp: capture the NPM tokenId returned by mint() and push it to _mintedNpmTokenIds; burn_lp now uses a 1-based index into that array (same pattern as stake/unstake), making attack files fork-block-independent - Remove dead atkBaseSnap variable and its compiler-warning suppression - Remove orphaned vm.snapshot() after vm.revertTo() in the attack loop - Fix misleading comment on delete _stakedPositionIds Co-Authored-By: Claude Sonnet 4.6 --- onchain/test/FitnessEvaluator.t.sol | 70 ++++++++++++++++++----------- 1 file changed, 45 insertions(+), 25 deletions(-) diff --git a/onchain/test/FitnessEvaluator.t.sol b/onchain/test/FitnessEvaluator.t.sol index 379bf68..5eb2699 100644 --- a/onchain/test/FitnessEvaluator.t.sol +++ b/onchain/test/FitnessEvaluator.t.sol @@ -169,6 +169,11 @@ contract FitnessEvaluator is Test { /// vm.snapshot/revertTo reverts this array's storage between attacks. uint256[] internal _stakedPositionIds; + /// @dev NPM tokenIds returned by mint_lp ops (in insertion order). + /// burn_lp references positions by 1-based index into this array so that + /// attack files are fork-block-independent (tokenIds vary by fork tip). + uint256[] internal _mintedNpmTokenIds; + // ─── Entry point ───────────────────────────────────────────────────────── /** @@ -217,17 +222,24 @@ contract FitnessEvaluator is Test { baseSnap = vm.snapshot(); // Etch candidate optimizer bytecode and upgrade proxy. + // Wrapped in try/catch: a malformed candidate (compiler bug, bad transpiler output) + // would otherwise abort the entire batch. On failure, emit fitness=0 and continue; + // vm.revertTo(baseSnap) at the top of the next iteration cleans up state. bytes memory candidateBytecode = vm.parseBytes(bytecodeHex); vm.etch(IMPL_SLOT, candidateBytecode); - // Test contract is the UUPS admin (set during initialize in _deploy). - UUPSUpgradeable(optProxy).upgradeTo(IMPL_SLOT); + bool upgradeOk = true; + try UUPSUpgradeable(optProxy).upgradeTo(IMPL_SLOT) { } + catch { + upgradeOk = false; + } + if (!upgradeOk) { + console.log(string.concat('{"candidate_id":"', candidateId, '","fitness":0,"error":"upgrade_failed"}')); + continue; + } // Bootstrap: fund LM, set recenterAccess, initial recenter. _bootstrap(); - // Post-bootstrap snapshot used to reset state between attacks. - uint256 atkBaseSnap = vm.snapshot(); - // Score: sum lm_eth_total across all attack sequences. uint256 totalFitness = 0; Vm.DirEntry[] memory entries = vm.readDir(attacksDir); @@ -238,15 +250,10 @@ contract FitnessEvaluator is Test { uint256 score = _runAttack(entries[i].path); totalFitness += score; vm.revertTo(atkSnap); - atkSnap = vm.snapshot(); // refresh for next attack } // Emit score as a JSON line (parsed by batch-eval.sh). console.log(string.concat('{"candidate_id":"', candidateId, '","fitness":', _uint2str(totalFitness), "}")); - - // Restore atkBaseSnap snapshot variable (already consumed above) — not needed - // since we always revert to baseSnap at the top of the loop. - (atkBaseSnap); // suppress unused variable warning } // Close manifest files. @@ -340,18 +347,18 @@ contract FitnessEvaluator is Test { // e. Initial recenter: no ANCHOR position exists yet so amplitude check is skipped; // recenterAccess is set so TWAP stability check is also skipped. - vm.prank(recenterAddr); - try ILM(lmAddr).recenter() { } - catch { - // If recenter fails on first attempt (e.g. pool not ready), mine some blocks and retry. - for (uint256 _attempt = 0; _attempt < 4; _attempt++) { - vm.roll(block.number + 50); - vm.prank(recenterAddr); - try ILM(lmAddr).recenter() returns (bool) { - break; - } catch { } - } + // If all retries fail, revert with a clear message — silent failure would make every + // candidate score identically (all lm_eth_total = free WETH only, no positions). + bool recentered = false; + for (uint256 _attempt = 0; _attempt < 5; _attempt++) { + if (_attempt > 0) vm.roll(block.number + 50); + vm.prank(recenterAddr); + try ILM(lmAddr).recenter() returns (bool) { + recentered = true; + break; + } catch { } } + require(recentered, "FitnessEvaluator: bootstrap recenter failed after 5 attempts"); } // ─── Attack execution ───────────────────────────────────────────────────── @@ -363,9 +370,11 @@ contract FitnessEvaluator is Test { function _runAttack(string memory attackFile) internal returns (uint256) { // Reset file read position so each call to _runAttack starts from line 1. vm.closeFile(attackFile); - // Clear any staked positions from a prior attack (snapshot revert handles on-chain state, - // but the dynamic array in test storage also needs resetting). + // vm.revertTo() reverts all EVM state including test contract storage, so these + // arrays are already empty after revert. Explicit delete is a defensive reset + // for the first attack (no preceding revert) and any future call-path changes. delete _stakedPositionIds; + delete _mintedNpmTokenIds; string memory line = vm.readLine(attackFile); while (bytes(line).length > 0) { @@ -439,7 +448,9 @@ contract FitnessEvaluator is Test { uint256 amount1 = vm.parseUint(vm.parseJsonString(line, ".amount1")); (address t0, address t1) = token0isWeth ? (WETH_ADDR, krkAddr) : (krkAddr, WETH_ADDR); vm.prank(advAddr); - INonfungiblePositionManager(NPM_ADDR).mint( + // Track the returned tokenId so burn_lp can reference it by 1-based index, + // making attack files fork-block-independent (NPM tokenIds depend on fork tip). + (uint256 mintedTokenId,,,) = INonfungiblePositionManager(NPM_ADDR).mint( INonfungiblePositionManager.MintParams({ token0: t0, token1: t1, @@ -454,8 +465,17 @@ contract FitnessEvaluator is Test { deadline: block.timestamp + 3600 }) ); + _mintedNpmTokenIds.push(mintedTokenId); } else if (_eq(op, "burn_lp")) { - uint256 tokenId = vm.parseJsonUint(line, ".tokenId"); + // .tokenId in the attack file is a 1-based index into _mintedNpmTokenIds + // (positions created by mint_lp ops in this run), not a raw NPM tokenId. + // This mirrors the stake/unstake index pattern and avoids fork-block sensitivity. + uint256 tokenIndex = vm.parseJsonUint(line, ".tokenId"); + require( + tokenIndex >= 1 && tokenIndex <= _mintedNpmTokenIds.length, + "FitnessEvaluator: burn_lp tokenId out of range (must be 1-based index of a prior mint_lp op)" + ); + uint256 tokenId = _mintedNpmTokenIds[tokenIndex - 1]; (,,,,,, , uint128 liquidity,,,,) = INonfungiblePositionManager(NPM_ADDR).positions(tokenId); if (liquidity == 0) return; vm.startPrank(advAddr);