From 508c01e554e8634b615915f474a717c21fd50a5d Mon Sep 17 00:00:00 2001 From: Rembrandt Kuipers <50174308+RembrandtK@users.noreply.github.com> Date: Fri, 29 May 2026 14:06:31 +0000 Subject: [PATCH 1/3] fix: prevent over-allocation double-collection in presentPOI auto-downsize When an over-allocated indexer presents a POI, presentPOI takes rewards and updates the allocation in storage (snapshotRewards / clearPendingRewards), but passed the stale memory snapshot captured at the top of the function into _resizeAllocation. That snapshot recomputed the reward accumulator delta from the pre-snapshot value and re-credited accRewardsPending with the rewards just paid out, letting the indexer collect them again on the next POI. _resizeAllocation now reads the current allocation state from storage itself instead of accepting a caller-supplied copy, so neither caller has to keep a memory copy in sync with storage. This removes the stale-snapshot hazard for both the auto-downsize and explicit-resize paths. Includes failing-then-passing unit and integration regression tests. --- .../contracts/libraries/AllocationHandler.sol | 6 +- .../subgraph-service/indexer.test.ts | 58 ++++++++++- .../subgraph-service/permisionless.test.ts | 6 +- .../subgraphService/SubgraphService.t.sol | 6 +- .../collect/indexing/indexing.t.sol | 97 +++++++++++++++++++ packages/testing/foundry.toml | 4 + .../test/harness/FullStackHarness.t.sol | 24 ++++- .../test/harness/RealRewardsHarness.t.sol | 91 +++++++++++++++++ .../IndexingRewardsCollection.t.sol | 78 +++++++++++++++ 9 files changed, 357 insertions(+), 13 deletions(-) create mode 100644 packages/testing/test/harness/RealRewardsHarness.t.sol create mode 100644 packages/testing/test/integration/IndexingRewardsCollection.t.sol diff --git a/packages/subgraph-service/contracts/libraries/AllocationHandler.sol b/packages/subgraph-service/contracts/libraries/AllocationHandler.sol index d7552718f..bf5a18446 100644 --- a/packages/subgraph-service/contracts/libraries/AllocationHandler.sol +++ b/packages/subgraph-service/contracts/libraries/AllocationHandler.sol @@ -409,7 +409,6 @@ library AllocationHandler { params.graphStaking, params.graphRewardsManager, params._allocationId, - allocation, 0, params._delegationRatio, params.maxPOIStaleness @@ -502,7 +501,6 @@ library AllocationHandler { graphStaking, graphRewardsManager, _allocationId, - allocation, _tokens, _delegationRatio, _maxPOIStaleness @@ -518,7 +516,6 @@ library AllocationHandler { * @param graphStaking The staking contract * @param graphRewardsManager The rewards manager contract * @param _allocationId The allocation ID to resize - * @param allocation The current allocation state * @param _tokens The new token amount for the allocation * @param _delegationRatio The delegation ratio for provision tracking * @param _maxPOIStaleness The maximum POI staleness threshold @@ -530,11 +527,12 @@ library AllocationHandler { IHorizonStaking graphStaking, IRewardsManager graphRewardsManager, address _allocationId, - IAllocation.State memory allocation, uint256 _tokens, uint32 _delegationRatio, uint256 _maxPOIStaleness ) internal { + IAllocation.State memory allocation = _allocations.get(_allocationId); + // Update provision tracker uint256 oldTokens = allocation.tokens; if (_tokens > oldTokens) { diff --git a/packages/subgraph-service/test/integration/after-transition-period/subgraph-service/indexer.test.ts b/packages/subgraph-service/test/integration/after-transition-period/subgraph-service/indexer.test.ts index 4295a90e0..b4e60b5e0 100644 --- a/packages/subgraph-service/test/integration/after-transition-period/subgraph-service/indexer.test.ts +++ b/packages/subgraph-service/test/integration/after-transition-period/subgraph-service/indexer.test.ts @@ -499,9 +499,63 @@ describe('Indexer', () => { const afterProvisionTokens = (await staking.getProvision(indexer.address, subgraphService.target)).tokens expect(afterProvisionTokens).to.equal(beforeProvisionTokens + rewards, 'Rewards should be collected') - // Verify allocation was closed + // Over-allocated collect resizes the allocation to zero rather than closing it. const allocation = await subgraphService.getAllocation(allocationId) - expect(allocation.closedAt).to.not.equal(0) + expect(allocation.closedAt).to.equal(0n, 'Allocation should remain open after over-allocation resize') + expect(allocation.tokens).to.equal(0n, 'Allocation should be resized to zero') + }) + + // Invariant: each accumulator delta is collected exactly once across an + // over-allocation resize. When the first collect pays the accrual, clears + // `accRewardsPending`, and resizes the allocation to zero (but leaves it + // open), a later POI must pay nothing: `RewardsManager.takeRewards` returns + // `pending + tokens·delta/1e18`, which is zero once `pending` is cleared and + // `tokens == 0`. (The resize must therefore not re-credit `accRewardsPending` + // from a stale allocation copy, or the same delta could be paid twice.) + it('should not collect rewards again on a second POI after over-allocation resize', async () => { + // Mine blocks so the first collect has rewards to take. + for (let i = 0; i < 1000; i++) { + await ethers.provider.send('evm_mine', []) + } + + // First collect: takes rewards, snapshots, then resizes the allocation + // to zero because the indexer is over-allocated. + const poi1 = generatePOI() + const poiMetadata1 = encodePOIMetadata(0, poi1, 0, 0, 0) + const data1 = encodeCollectIndexingRewardsData(allocationId, poi1, poiMetadata1) + const firstRewards = await collect(indexer, [indexer.address, PaymentTypes.IndexingRewards, data1]) + expect(firstRewards).to.not.equal(0n, 'First collect should pay the accumulator delta') + + const allocationAfterFirst = await subgraphService.getAllocation(allocationId) + expect(allocationAfterFirst.tokens).to.equal(0n, 'Allocation should be resized to zero') + expect(allocationAfterFirst.closedAt).to.equal(0n, 'Allocation should remain open after resize') + + // Snapshot indexer provision balance and pending rewards before the second collect. + const beforeProvisionTokens = (await staking.getProvision(indexer.address, subgraphService.target)).tokens + + // Mine more blocks so the second presentPOI is not classified as too-young. + // With tokens=0, no new rewards should accrue between the two presentations. + for (let i = 0; i < 100; i++) { + await ethers.provider.send('evm_mine', []) + } + + // Second collect: pending was paid and cleared by the first collect and + // tokens=0, so the real RewardsManager mints zero. + const poi2 = generatePOI() + const poiMetadata2 = encodePOIMetadata(0, poi2, 0, 0, 0) + const data2 = encodeCollectIndexingRewardsData(allocationId, poi2, poiMetadata2) + const secondRewards = await collect(indexer, [indexer.address, PaymentTypes.IndexingRewards, data2]) + + expect(secondRewards).to.equal( + 0n, + 'Second collect on a zero-token allocation must not pay again — the accumulator delta was already collected', + ) + + const afterProvisionTokens = (await staking.getProvision(indexer.address, subgraphService.target)).tokens + expect(afterProvisionTokens).to.equal( + beforeProvisionTokens, + 'Indexer provision must not grow on a second collect against a zero-token allocation', + ) }) }) }) diff --git a/packages/subgraph-service/test/integration/after-transition-period/subgraph-service/permisionless.test.ts b/packages/subgraph-service/test/integration/after-transition-period/subgraph-service/permisionless.test.ts index 3652d9087..465c656b2 100644 --- a/packages/subgraph-service/test/integration/after-transition-period/subgraph-service/permisionless.test.ts +++ b/packages/subgraph-service/test/integration/after-transition-period/subgraph-service/permisionless.test.ts @@ -62,9 +62,11 @@ describe('Permissionless', () => { // Close allocation as anyone await subgraphService.connect(anyone).closeStaleAllocation(allocationId) - // Verify allocation is closed + // closeStaleAllocation resizes the allocation to zero rather than closing it, + // so any bound indexing agreement stays active. The allocation remains open. const afterAllocation = await subgraphService.getAllocation(allocationId) - expect(afterAllocation.closedAt).to.not.equal(0, 'Allocation should be closed') + expect(afterAllocation.closedAt).to.equal(0n, 'Allocation should remain open after resize-to-zero') + expect(afterAllocation.tokens).to.equal(0n, 'Allocation should be resized to zero') // Verify tokens are released const afterLockedTokens = await subgraphService.allocationProvisionTracker(indexer.address) diff --git a/packages/subgraph-service/test/unit/subgraphService/SubgraphService.t.sol b/packages/subgraph-service/test/unit/subgraphService/SubgraphService.t.sol index f24106880..02c5f3fc0 100644 --- a/packages/subgraph-service/test/unit/subgraphService/SubgraphService.t.sol +++ b/packages/subgraph-service/test/unit/subgraphService/SubgraphService.t.sol @@ -433,13 +433,13 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest { // For too-young allocations (created in current epoch), the contract returns early // without updating other allocation state or emitting IndexingRewardsCollected if (currentEpoch > allocation.createdAtEpoch) { - // Note: after resize (over-allocation), accRewardsPending is re-accumulated from - // the token delta and may be non-zero. This is expected — rewards from the resize - // delta are captured as pending for the next collection. + // Collecting clears pending rewards and snapshots the accumulator, so nothing remains + // collectable afterwards — including when an over-allocation downsize runs in the same call. uint256 accRewardsPerAllocatedToken = rewardsManager.onSubgraphAllocationUpdate( allocation.subgraphDeploymentId ); assertEq(allocation.accRewardsPerAllocatedToken, accRewardsPerAllocatedToken); + assertEq(allocation.accRewardsPending, 0, "collected accrual must leave no pending rewards"); } // Check indexer got paid the correct amount diff --git a/packages/subgraph-service/test/unit/subgraphService/collect/indexing/indexing.t.sol b/packages/subgraph-service/test/unit/subgraphService/collect/indexing/indexing.t.sol index 49c034e52..30ccae495 100644 --- a/packages/subgraph-service/test/unit/subgraphService/collect/indexing/indexing.t.sol +++ b/packages/subgraph-service/test/unit/subgraphService/collect/indexing/indexing.t.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.27; import { IGraphPayments } from "@graphprotocol/interfaces/contracts/horizon/IGraphPayments.sol"; import { AllocationHandler } from "../../../../../contracts/libraries/AllocationHandler.sol"; +import { IAllocation } from "@graphprotocol/interfaces/contracts/subgraph-service/internal/IAllocation.sol"; import { IRewardsManager } from "@graphprotocol/interfaces/contracts/contracts/rewards/IRewardsManager.sol"; import { ISubgraphService } from "@graphprotocol/interfaces/contracts/subgraph-service/ISubgraphService.sol"; @@ -155,6 +156,102 @@ contract SubgraphServiceCollectIndexingTest is SubgraphServiceTest { _collect(users.indexer, paymentType, collectData); } + // Collecting while over-allocated downsizes the allocation to zero tokens and must leave no + // pending rewards, so the accrual cannot be collected a second time. + function test_SubgraphService_Collect_Indexing_OverAllocated_DoesNotLeavePendingRewards( + uint256 tokens + ) public useIndexer { + tokens = bound(tokens, MINIMUM_PROVISION_TOKENS * 2, 10_000_000_000 ether); + + _createProvision(users.indexer, tokens, FISHERMAN_REWARD_PERCENTAGE, DISPUTE_PERIOD); + _register(users.indexer, abi.encode("url", "geoHash", address(0))); + bytes memory data = _createSubgraphAllocationData( + users.indexer, + subgraphDeployment, + allocationIdPrivateKey, + tokens + ); + _startService(users.indexer, data); + + // Thaw half the provision to make the indexer over-allocated. + staking.thaw(users.indexer, address(subgraphService), tokens / 2); + + // Advance so the allocation is old enough to collect rewards. + vm.roll(block.number + EPOCH_LENGTH); + + IGraphPayments.PaymentTypes paymentType = IGraphPayments.PaymentTypes.IndexingRewards; + // forge-lint: disable-next-line(unsafe-typecast) + bytes memory collectData = abi.encode(allocationId, bytes32("POI"), _getHardcodedPoiMetadata()); + subgraphService.collect(users.indexer, paymentType, collectData); + + IAllocation.State memory afterAlloc = subgraphService.getAllocation(allocationId); + + assertEq(afterAlloc.tokens, 0, "allocation should be resized to zero on over-allocation"); + assertEq(afterAlloc.accRewardsPending, 0, "collected accrual must leave no pending rewards"); + } + + // A second collection against the zero-token allocation must not pay the indexer again. + // MockRewardsManager.takeRewards ignores accRewardsPending, so it's mocked to the faithful + // `pending + delta` value — any pending left behind then surfaces here as a payout. + // (The real-RewardsManager integration test in packages/testing covers this without mocking.) + function test_SubgraphService_Collect_Indexing_OverAllocated_NoDoubleCollectionPayout( + uint256 tokens + ) public useIndexer { + tokens = bound(tokens, MINIMUM_PROVISION_TOKENS * 2, 10_000_000_000 ether); + + _createProvision(users.indexer, tokens, FISHERMAN_REWARD_PERCENTAGE, DISPUTE_PERIOD); + _register(users.indexer, abi.encode("url", "geoHash", address(0))); + bytes memory data = _createSubgraphAllocationData( + users.indexer, + subgraphDeployment, + allocationIdPrivateKey, + tokens + ); + _startService(users.indexer, data); + + // Make the indexer over-allocated. + staking.thaw(users.indexer, address(subgraphService), tokens / 2); + vm.roll(block.number + EPOCH_LENGTH); + + IGraphPayments.PaymentTypes paymentType = IGraphPayments.PaymentTypes.IndexingRewards; + // forge-lint: disable-next-line(unsafe-typecast) + bytes memory collectData = abi.encode(allocationId, bytes32("POI"), _getHardcodedPoiMetadata()); + + // First collect: takes rewards and resizes the over-allocated allocation to zero. + subgraphService.collect(users.indexer, paymentType, collectData); + + IAllocation.State memory afterFirst = subgraphService.getAllocation(allocationId); + assertEq(afterFirst.tokens, 0); + uint256 pendingAfterFirst = afterFirst.accRewardsPending; + + // Stage the second collect as if running against the production RewardsManager: + // rewards = accRewardsPending + tokens * (updatedAcc - accRewardsPerAllocatedToken) / 1e18 + // With tokens=0 and updatedAcc == accRewardsPerAllocatedToken, the delta term is 0, so a + // faithful takeRewards returns exactly the remaining accRewardsPending and mints that to + // the rewards issuer (the SubgraphService). When pending is 0 this mints nothing. + deal(address(token), address(subgraphService), pendingAfterFirst); + vm.mockCall( + address(rewardsManager), + abi.encodeWithSelector(IRewardsManager.takeRewards.selector, allocationId), + abi.encode(pendingAfterFirst) + ); + + uint256 indexerProvisionBefore = staking.getProviderTokensAvailable(users.indexer, address(subgraphService)); + + // Roll so the new POI presentation isn't classified as too-young. + vm.roll(block.number + EPOCH_LENGTH); + subgraphService.collect(users.indexer, paymentType, collectData); + + uint256 indexerProvisionAfter = staking.getProviderTokensAvailable(users.indexer, address(subgraphService)); + + // The accrual was paid by the first collect and tokens are now 0, so the second pays nothing. + assertEq( + indexerProvisionAfter - indexerProvisionBefore, + 0, + "indexer must not be paid again after over-allocation resize" + ); + } + function test_SubgraphService_Collect_Indexing_RevertWhen_IndexerIsNotAllocationOwner( uint256 tokens ) public useIndexer useAllocation(tokens) { diff --git a/packages/testing/foundry.toml b/packages/testing/foundry.toml index e81bd9bc3..be0a761b5 100644 --- a/packages/testing/foundry.toml +++ b/packages/testing/foundry.toml @@ -18,6 +18,10 @@ remappings = [ ] solc_version = '0.8.35' evm_version = 'cancun' +# Allow reading the hardhat-compiled artifact for the real (Solidity 0.7.6) RewardsManager, +# which forge cannot compile in this 0.8.x project and must load as precompiled bytecode. +# See test/harness/RealRewardsHarness.t.sol and test/integration/IndexingRewardsCollection.t.sol. +fs_permissions = [{ access = "read", path = "node_modules/@graphprotocol/contracts/artifacts" }] # Opt-in profile for production-matching bytecode (viaIR + optimizer). # Use FOUNDRY_PROFILE=prod for CI / pre-merge runs that should mirror what hardhat diff --git a/packages/testing/test/harness/FullStackHarness.t.sol b/packages/testing/test/harness/FullStackHarness.t.sol index efc2fef6a..b65d2e24b 100644 --- a/packages/testing/test/harness/FullStackHarness.t.sol +++ b/packages/testing/test/harness/FullStackHarness.t.sol @@ -105,6 +105,10 @@ abstract contract FullStackHarness is Test { MockCuration internal curation; MockEpochManager internal epochManager; MockRewardsManager internal rewardsManager; + /// @dev The RewardsManager address registered in the Controller. Equals `address(rewardsManager)` + /// in the default (mock) configuration; a subclass may override `_deployRewardsManager` to + /// register a different implementation (e.g. real bytecode — see RealRewardsHarness). + address internal rewardsManagerAddress; // -- Helpers -- RecurringCollectorHelper internal rcHelper; @@ -143,9 +147,14 @@ abstract contract FullStackHarness is Test { vm.startPrank(deployer); token = new MockGRTToken(); GraphProxy stakingProxy = new GraphProxy(address(0), address(proxyAdmin)); - rewardsManager = new MockRewardsManager(token, REWARDS_PER_SIGNAL, REWARDS_PER_SUBGRAPH_ALLOCATION_UPDATE); curation = new MockCuration(); epochManager = new MockEpochManager(); + vm.stopPrank(); + + // RewardsManager via pluggable factory (token must already exist). The factory + // self-manages pranks and leaves none active. No prank is needed for the predicted-address + // computation below; the registration block re-establishes the governor prank. + rewardsManagerAddress = _deployRewardsManager(); // Predict GraphPayments and PaymentsEscrow addresses using actual creation code. // We use type(...).creationCode instead of vm.getCode to get the exact bytecode @@ -175,7 +184,7 @@ abstract contract FullStackHarness is Test { vm.startPrank(governor); controller.setContractProxy(keccak256("GraphToken"), address(token)); controller.setContractProxy(keccak256("Staking"), address(stakingProxy)); - controller.setContractProxy(keccak256("RewardsManager"), address(rewardsManager)); + controller.setContractProxy(keccak256("RewardsManager"), rewardsManagerAddress); controller.setContractProxy(keccak256("GraphPayments"), predictedGP); controller.setContractProxy(keccak256("PaymentsEscrow"), predictedEscrow); controller.setContractProxy(keccak256("EpochManager"), address(epochManager)); @@ -262,6 +271,17 @@ abstract contract FullStackHarness is Test { rcHelper = new RecurringCollectorHelper(recurringCollector, recurringCollectorProxyAdmin); } + /// @notice Deploy the RewardsManager and return the address to register in the Controller. + /// @dev Override to swap the reward-accounting implementation (e.g. real contract bytecode) + /// without touching the rest of the stack. Implementations must self-manage pranks and + /// leave none active on return. Called after `token` exists and before SubgraphService is + /// constructed (GraphDirectory caches the RewardsManager address at construction). + function _deployRewardsManager() internal virtual returns (address) { + vm.prank(deployer); + rewardsManager = new MockRewardsManager(token, REWARDS_PER_SIGNAL, REWARDS_PER_SUBGRAPH_ALLOCATION_UPDATE); + return address(rewardsManager); + } + // ── RAM + IssuanceAllocator deployment ────────────────────────────── function _deployRAMStack() private { diff --git a/packages/testing/test/harness/RealRewardsHarness.t.sol b/packages/testing/test/harness/RealRewardsHarness.t.sol new file mode 100644 index 000000000..53226877d --- /dev/null +++ b/packages/testing/test/harness/RealRewardsHarness.t.sol @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +pragma solidity ^0.8.27; + +import { GraphProxy } from "@graphprotocol/contracts/contracts/upgrades/GraphProxy.sol"; +import { GraphUpgradeable } from "@graphprotocol/contracts/contracts/upgrades/GraphUpgradeable.sol"; +import { IRewardsManager } from "@graphprotocol/interfaces/contracts/contracts/rewards/IRewardsManager.sol"; + +import { FullStackHarness } from "./FullStackHarness.t.sol"; + +/// @dev Minimal handle for the real RewardsManager's governor-only admin surface. +/// The full ABI lives in the 0.7.6 artifact loaded as bytecode. +interface IRealRewardsManager { + function initialize(address controller) external; + function setIssuancePerBlock(uint256 issuancePerBlock) external; + function setSubgraphService(address subgraphService) external; +} + +/// @title RealRewardsHarness +/// @notice FullStackHarness variant that wires the **real production RewardsManager** +/// (Solidity 0.7.6) in place of `MockRewardsManager`, with zero other changes to the +/// stack. Any test that extends this instead of FullStackHarness exercises the genuine +/// reward-accounting math, rather than a mock whose simplified `takeRewards` cannot +/// surface over-allocation double-collection in the resize path. +/// +/// The RewardsManager cannot be compiled by this 0.8.x forge project, so it is loaded as +/// precompiled bytecode from the hardhat artifact (produced by building the contracts package). +/// If the artifact is absent the harness falls back to the mock and sets `realRmAvailable = false` +/// so dependent tests can `vm.skip` rather than fail. +abstract contract RealRewardsHarness is FullStackHarness { + string internal constant RM_ARTIFACT = + "node_modules/@graphprotocol/contracts/artifacts/contracts/rewards/RewardsManager.sol/RewardsManager.json"; + uint256 internal constant RM_ISSUANCE_PER_BLOCK = 100 ether; + uint256 internal constant RM_CURATION_SIGNAL = 1_000_000 ether; + + /// @notice True when the real RewardsManager was deployed; false when it fell back to the mock. + bool internal realRmAvailable; + /// @notice Typed handle to the real RewardsManager proxy (zero when unavailable). + IRewardsManager internal realRewardsManager; + + /// @dev Swap in the real bytecode-loaded RewardsManager. Self-manages pranks. + function _deployRewardsManager() internal override returns (address) { + if (!vm.exists(RM_ARTIFACT)) { + realRmAvailable = false; + return super._deployRewardsManager(); // mock fallback keeps the stack deployable + } + realRmAvailable = true; + + bytes memory rmBytecode = vm.parseJsonBytes(vm.readFile(RM_ARTIFACT), ".bytecode"); + address rmImpl; + // solhint-disable-next-line no-inline-assembly + assembly { + rmImpl := create(0, add(rmBytecode, 0x20), mload(rmBytecode)) + } + require(rmImpl != address(0), "real RM deploy failed"); + + vm.startPrank(governor); + GraphProxy rmProxy = new GraphProxy(address(0), address(proxyAdmin)); + proxyAdmin.upgrade(rmProxy, rmImpl); + proxyAdmin.acceptProxyAndCall( + GraphUpgradeable(rmImpl), + rmProxy, + abi.encodeCall(IRealRewardsManager.initialize, (address(controller))) + ); + vm.stopPrank(); + + realRewardsManager = IRewardsManager(address(rmProxy)); + return address(rmProxy); + } + + function setUp() public virtual override { + super.setUp(); + if (!realRmAvailable) return; + + // Real-RM config that the mock doesn't need: authorize SubgraphService as the rewards + // issuer, set issuance, and provide curation signal so rewards accrue (MockCuration has + // no getCurationPoolTokens). Signal magnitude only scales rewards; it does not gate any + // code path. Other config (unpause, maxPOIStaleness, thawing period) is done by + // FullStackHarness._configureProtocol. + vm.startPrank(governor); + IRealRewardsManager(address(realRewardsManager)).setSubgraphService(address(subgraphService)); + IRealRewardsManager(address(realRewardsManager)).setIssuancePerBlock(RM_ISSUANCE_PER_BLOCK); + vm.stopPrank(); + + token.mint(address(curation), RM_CURATION_SIGNAL); + vm.mockCall( + address(curation), + abi.encodeWithSignature("getCurationPoolTokens(bytes32)"), + abi.encode(RM_CURATION_SIGNAL) + ); + } +} diff --git a/packages/testing/test/integration/IndexingRewardsCollection.t.sol b/packages/testing/test/integration/IndexingRewardsCollection.t.sol new file mode 100644 index 000000000..8c4e50b46 --- /dev/null +++ b/packages/testing/test/integration/IndexingRewardsCollection.t.sol @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +pragma solidity ^0.8.27; + +import { IGraphPayments } from "@graphprotocol/interfaces/contracts/horizon/IGraphPayments.sol"; +import { IAllocation } from "@graphprotocol/interfaces/contracts/subgraph-service/internal/IAllocation.sol"; +import { Allocation } from "subgraph-service/libraries/Allocation.sol"; + +import { RealRewardsHarness } from "../harness/RealRewardsHarness.t.sol"; + +/// @title IndexingRewardsCollectionTest +/// @notice Integration tests for indexing-reward collection accounting, exercised end-to-end +/// against the **real production RewardsManager** (via RealRewardsHarness) so the reward +/// math is the genuine on-chain implementation rather than a mock. +/// +/// Core property under test: an indexer is paid each accrual period exactly once — including when +/// presenting a POI while over-allocated, which auto-downsizes the allocation to zero tokens while +/// keeping it open as a (now zero-token) rewards target. +contract IndexingRewardsCollectionTest is RealRewardsHarness { + using Allocation for IAllocation.State; + + bytes32 internal constant SUBGRAPH = keccak256("indexing-rewards-subgraph"); + + /// @notice Collecting while over-allocated pays the accrued reward once: the allocation is + /// downsized to zero tokens but stays open, no pending reward is left behind, and a + /// further collection on the zero-token allocation pays nothing. + function test_Collect_Indexing_OverAllocated_PaysEachAccrualOnce() public { + if (!realRmAvailable) { + emit log( + string.concat( + "SKIP: RewardsManager artifact not found at ", + RM_ARTIFACT, + " (build the contracts package first)" + ) + ); + vm.skip(true); + return; + } + + IndexerSetup memory ix = _setupIndexer("indexer", SUBGRAPH, MINIMUM_PROVISION_TOKENS * 100); + + // Thaw half the provision so the allocated tokens exceed the available provision: the + // indexer is now over-allocated and the next POI will trigger the auto-downsize path. + vm.prank(ix.addr); + staking.thaw(ix.addr, address(subgraphService), ix.provisionTokens / 2); + + vm.roll(block.number + 100); // let rewards accrue over 100 blocks + + bytes memory collectData = abi.encode(ix.allocationId, bytes32("POI"), _poiMetadata()); + + // Collect the accrued reward. Over-allocation downsizes the allocation to zero tokens. + uint256 supplyBefore = token.totalSupply(); + vm.prank(ix.addr); + subgraphService.collect(ix.addr, IGraphPayments.PaymentTypes.IndexingRewards, collectData); + uint256 firstReward = token.totalSupply() - supplyBefore; + assertGt(firstReward, 0, "accrued indexing reward is minted on collection"); + + // The downsized allocation stays open as a valid (zero-token) rewards target, and the + // reward just paid leaves nothing collectable behind. + IAllocation.State memory alloc = subgraphService.getAllocation(ix.allocationId); + assertEq(alloc.tokens, 0, "over-allocation downsizes the allocation to zero tokens"); + assertEq(alloc.closedAt, 0, "downsized allocation remains open"); + assertTrue(alloc.isOpen(), "downsized allocation is still a valid rewards target"); + assertFalse(alloc.isStale(MAX_POI_STALENESS), "collection took the active-rewards path, not stale-reclaim"); + assertEq(alloc.accRewardsPending, 0, "a collected accrual leaves no pending rewards"); + + // A second collection over the same (now zero-token) allocation, with no further accrual, + // pays nothing: each accrual is paid exactly once. + vm.prank(ix.addr); + subgraphService.collect(ix.addr, IGraphPayments.PaymentTypes.IndexingRewards, collectData); + uint256 secondReward = token.totalSupply() - supplyBefore - firstReward; + assertEq(secondReward, 0, "a zero-token allocation accrues and pays no further rewards"); + } + + function _poiMetadata() internal view returns (bytes memory) { + // forge-lint: disable-next-line(unsafe-typecast) + return abi.encode(block.number, bytes32("PUBLIC_POI"), uint8(0), uint8(0), uint256(0)); + } +} From e6c9d26180de18f605263144ae7fe950fc449bb9 Mon Sep 17 00:00:00 2001 From: Rembrandt Kuipers <50174308+RembrandtK@users.noreply.github.com> Date: Fri, 29 May 2026 14:07:30 +0000 Subject: [PATCH 2/3] refactor: narrow _distributeIndexingRewards to the indexer address _distributeIndexingRewards only needs the (immutable) indexer, not the full allocation struct. Pass allocation.indexer instead of the whole IAllocation.State, so a post-snapshot distribution path cannot hand a stale allocation copy to a callee. Preventative follow-up to the over-allocation double-collection fix; no behavioural change. --- .../contracts/libraries/AllocationHandler.sol | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/subgraph-service/contracts/libraries/AllocationHandler.sol b/packages/subgraph-service/contracts/libraries/AllocationHandler.sol index bf5a18446..2a4974838 100644 --- a/packages/subgraph-service/contracts/libraries/AllocationHandler.sol +++ b/packages/subgraph-service/contracts/libraries/AllocationHandler.sol @@ -374,7 +374,7 @@ library AllocationHandler { // Scoped for stack management { (uint256 tokensIndexerRewards, uint256 tokensDelegationRewards) = _distributeIndexingRewards( - allocation, + allocation.indexer, rewardsCollected, params ); @@ -654,14 +654,14 @@ library AllocationHandler { /** * @notice Distributes indexing rewards to delegators and indexer - * @param _allocation The allocation state + * @param _indexer The indexer that owns the allocation * @param _rewardsCollected Total rewards to distribute * @param _params The present params containing staking, token, and destination info * @return tokensIndexerRewards Amount sent to indexer * @return tokensDelegationRewards Amount sent to delegation pool */ function _distributeIndexingRewards( - IAllocation.State memory _allocation, + address _indexer, uint256 _rewardsCollected, PresentParams memory _params ) private returns (uint256 tokensIndexerRewards, uint256 tokensDelegationRewards) { @@ -669,18 +669,18 @@ library AllocationHandler { // Calculate and distribute delegator share uint256 delegatorCut = _params.graphStaking.getDelegationFeeCut( - _allocation.indexer, + _indexer, _params.dataService, IGraphPayments.PaymentTypes.IndexingRewards ); IHorizonStakingTypes.DelegationPool memory pool = _params.graphStaking.getDelegationPool( - _allocation.indexer, + _indexer, _params.dataService ); tokensDelegationRewards = pool.shares > 0 ? _rewardsCollected.mulPPM(delegatorCut) : 0; if (tokensDelegationRewards > 0) { _params.graphToken.approve(address(_params.graphStaking), tokensDelegationRewards); - _params.graphStaking.addToDelegationPool(_allocation.indexer, _params.dataService, tokensDelegationRewards); + _params.graphStaking.addToDelegationPool(_indexer, _params.dataService, tokensDelegationRewards); } // Distribute indexer share @@ -688,7 +688,7 @@ library AllocationHandler { if (tokensIndexerRewards > 0) { if (_params._paymentsDestination == address(0)) { _params.graphToken.approve(address(_params.graphStaking), tokensIndexerRewards); - _params.graphStaking.stakeToProvision(_allocation.indexer, _params.dataService, tokensIndexerRewards); + _params.graphStaking.stakeToProvision(_indexer, _params.dataService, tokensIndexerRewards); } else { _params.graphToken.pushTokens(_params._paymentsDestination, tokensIndexerRewards); } From f3b6a93481bd0c1eb688837d6784ee73e600bd99 Mon Sep 17 00:00:00 2001 From: Rembrandt Kuipers <50174308+RembrandtK@users.noreply.github.com> Date: Fri, 29 May 2026 14:07:45 +0000 Subject: [PATCH 3/3] test: describe expected behaviour instead of point-in-time bugs in regression tests Several regression tests narrated the bug they were born from ("THE BUG: with the original buggy code...", "Old code: A.sub(S).add(P) reverts...", "the old buggy ... approach") in comments and assertion messages. Once the underlying fix lands, that narration is stale and misleading: it describes code that no longer exists rather than the invariant the test guards. Reframe the comments and assertion messages to state the expected behaviour as a permanent property (snapshot-delta distribution regardless of same-block ordering; underflow-safe A.add(P).sub(S) under an inverted snapshot state; per-provider escrow deficit; pending-update escrow freed on cancel). Also rename two tests whose identifiers referenced the old bug: PendingUpdateEscrowNotFreed -> ...Freed (the test verifies escrow IS freed) and a snapshot test's "post-fix steady state" -> "synced snapshot steady state". Comments and message strings only; no test logic, values, or assertions changed. --- .../rewards-signal-allocation-update.test.ts | 43 ++++++++++--------- .../rewards-snapshot-inversion.test.ts | 36 +++++++++------- .../cancelWithPendingUpdate.t.sol | 22 +++++----- .../unit/agreement-manager/fundingModes.t.sol | 10 +++-- 4 files changed, 59 insertions(+), 52 deletions(-) diff --git a/packages/contracts-test/tests/unit/rewards/rewards-signal-allocation-update.test.ts b/packages/contracts-test/tests/unit/rewards/rewards-signal-allocation-update.test.ts index 62097acbb..930c2b21c 100644 --- a/packages/contracts-test/tests/unit/rewards/rewards-signal-allocation-update.test.ts +++ b/packages/contracts-test/tests/unit/rewards/rewards-signal-allocation-update.test.ts @@ -13,17 +13,16 @@ import { NetworkFixture } from '../lib/fixtures' const { HashZero } = constants /** - * Test for the signal/allocation update accounting bug fix. + * Invariant: signal/allocation update accounting. * - * The bug: When `onSubgraphSignalUpdate()` is called before `onSubgraphAllocationUpdate()` - * in the SAME BLOCK, the per-signal delta is zero but rewards tracked in `accRewardsForSubgraph` - * are never distributed to allocations. This causes rewards to be "bricked". + * When `onSubgraphSignalUpdate()` runs before `onSubgraphAllocationUpdate()` in the SAME BLOCK, + * the per-signal delta is zero. Rewards already tracked in `accRewardsForSubgraph` must still be + * distributed to allocations via the snapshot delta + * (`accRewardsForSubgraph - accRewardsForSubgraphSnapshot`), rather than relying on the per-signal + * delta alone. Distribution must never depend on the ordering of these two calls within a block. * - * The fix: Use the snapshot delta (accRewardsForSubgraph - accRewardsForSubgraphSnapshot) instead - * of only relying on the per-signal delta for calculating new rewards. - * - * IMPORTANT: These tests use evm_setAutomine to batch transactions into the same block, - * which is necessary to reproduce the bug condition where per-signal delta = 0. + * IMPORTANT: These tests use evm_setAutomine to batch transactions into one block so the + * per-signal delta is zero, exercising the snapshot-delta path. */ describe('Rewards: Signal and Allocation Update Accounting', () => { const graph = hre.graph() @@ -141,11 +140,11 @@ describe('Rewards: Signal and Allocation Update Accounting', () => { // Get final state const subgraphAfterAllocation = await rewardsManager.subgraphs(subgraphDeploymentID) - // THE FIX: accRewardsPerAllocatedToken should be updated even though per-signal delta was 0 - // With the bug, this would remain unchanged because newRewards=0 caused early return + // accRewardsPerAllocatedToken must advance via the snapshot delta even when the per-signal + // delta is zero, so accumulated rewards reach allocations regardless of update ordering. expect(subgraphAfterAllocation.accRewardsPerAllocatedToken).to.be.gt( accRewardsPerAllocatedTokenBefore, - 'accRewardsPerAllocatedToken should increase (BUG: was not updated when signal update preceded allocation update)', + 'accRewardsPerAllocatedToken should increase when a signal update precedes an allocation update in the same block', ) // Verify snapshot consistency @@ -196,12 +195,12 @@ describe('Rewards: Signal and Allocation Update Accounting', () => { // Get stored state const subgraph = await rewardsManager.subgraphs(subgraphDeploymentID) - // THE BUG: With the original buggy code, accRewardsPerAllocatedToken would remain at 0 - // because newRewards from per-signal delta is 0, causing early return. - // THE FIX: accRewardsPerAllocatedToken should be updated to reflect the accumulated rewards + // Even when the per-signal delta is zero, accRewardsPerAllocatedToken must reflect the + // rewards accumulated in accRewardsForSubgraph via the snapshot delta, so they are + // distributed to allocations rather than stranded. expect(subgraph.accRewardsPerAllocatedToken).to.be.gt( 0, - 'accRewardsPerAllocatedToken should be non-zero (BUG: rewards were bricked)', + 'accRewardsPerAllocatedToken should be non-zero so accumulated rewards are distributed, not stranded', ) // Verify view function and stored state are consistent @@ -297,7 +296,8 @@ describe('Rewards: Signal and Allocation Update Accounting', () => { 'accRewardsPerAllocatedToken should not increase when denied', ) - // THE FIX: accRewardsForSubgraphSnapshot should be updated to prevent re-reclaiming + // accRewardsForSubgraphSnapshot must advance in the reclaim path so the same rewards + // cannot be reclaimed again on a later update. expect(subgraphAfter.accRewardsForSubgraphSnapshot).to.be.gte( subgraphBefore.accRewardsForSubgraphSnapshot, 'accRewardsForSubgraphSnapshot should be updated in reclaim path', @@ -368,12 +368,12 @@ describe('Rewards: Signal and Allocation Update Accounting', () => { await helpers.mine(100) // Call onSubgraphSignalUpdate (simulates curator action) - // With Option B fix: rewards should be reclaimed immediately + // For a denied subgraph, rewards are reclaimed immediately rather than accumulated. const tx = await rewardsManager.connect(governor).onSubgraphSignalUpdate(subgraphDeploymentID) const receipt = await tx.wait() const afterSignalUpdate = await rewardsManager.subgraphs(subgraphDeploymentID) - // With Option B: accRewardsForSubgraph should NOT change for denied subgraphs + // For a denied subgraph, accRewardsForSubgraph must NOT change // (rewards are reclaimed directly, not stored) expect(afterSignalUpdate.accRewardsForSubgraph).to.equal( afterDenial.accRewardsForSubgraph, @@ -429,7 +429,8 @@ describe('Rewards: Signal and Allocation Update Accounting', () => { const rewardsDuringDenial = await rewardsManager.getAccRewardsForSubgraph(subgraphDeploymentID) expect(rewardsDuringDenial).to.equal(rewardsAtDenial, 'View should not increase during denial') - // Call signal update (with bug, this would NOT reclaim, causing view to jump on next allocation update) + // A signal update on a denied subgraph reclaims the accumulated rewards, so the view stays + // stable and does not jump on the next allocation update. // Configure reclaim address so rewards are reclaimed const SUBGRAPH_DENIED = hre.ethers.utils.id('SUBGRAPH_DENIED') await rewardsManager.connect(governor).setReclaimAddress(SUBGRAPH_DENIED, governor.address) @@ -513,7 +514,7 @@ describe('Rewards: Signal and Allocation Update Accounting', () => { it('should maintain accounting invariant across mixed updates (with same-block scenarios)', async function () { await setupSubgraphWithAllocation() - // Sequence of operations that could trigger the bug + // Sequence exercising the same-block signal/allocation ordering await helpers.mine(25) // First: signal update followed by allocation update in SAME BLOCK diff --git a/packages/contracts-test/tests/unit/rewards/rewards-snapshot-inversion.test.ts b/packages/contracts-test/tests/unit/rewards/rewards-snapshot-inversion.test.ts index af22ea210..da09d182e 100644 --- a/packages/contracts-test/tests/unit/rewards/rewards-snapshot-inversion.test.ts +++ b/packages/contracts-test/tests/unit/rewards/rewards-snapshot-inversion.test.ts @@ -20,17 +20,20 @@ const { HashZero } = constants * S = accRewardsForSubgraphSnapshot (stored snapshot, set at allocation updates) * P = rewardsSinceSignalSnapshot (pending rewards since last signal snapshot) * - * After a proxy upgrade, subgraphs whose last pre-upgrade interaction was - * `onSubgraphAllocationUpdate` have A < S. The old code set S from a view function - * (storage + pending) while leaving A at its stored value, so S leads and A lags. - * The original code's `A.sub(S).add(P)` reverts on the intermediate `A - S`. + * For affected subgraphs, on-chain storage can hold an inverted snapshot state + * where A < S: the snapshot S leads (it was written from a view value of + * storage + pending) while the accumulator A lags at its stored value. * - * The fix: Rearrange to `A.add(P).sub(S)` — add P first, then subtract S. - * Since P covers T1→now and the gap S - A covers T1→T2, and now >= T2, - * we have S - A <= P, so S <= A + P always holds. No clamping needed. + * Invariant the reward math must uphold: with an inverted state (A < S), the + * computation of A + P - S must never underflow. It does so by adding pending + * first and subtracting the snapshot last — `A.add(P).sub(S)` — so the + * intermediate `A + P` stays non-negative. Since P covers T1→now and the gap + * S - A covers T1→T2, and now >= T2, we have S - A <= P, so S <= A + P always + * holds; no clamping is needed. Subtracting the gap S - A discards rewards + * already distributed, preventing double-counting. * - * These tests use `hardhat_setStorageAt` to directly create the inverted storage state - * that exists on-chain for affected subgraphs. + * These tests use `hardhat_setStorageAt` to construct the inverted storage state + * directly so the invariant can be exercised for affected subgraphs. */ describe('Rewards: Snapshot Inversion', () => { const graph = hre.graph() @@ -202,8 +205,9 @@ describe('Rewards: Snapshot Inversion', () => { // Advance enough blocks so P > gap. At ~200 GRT/block, 50 blocks ≈ 10,000 GRT > 7,000. await helpers.mine(50) - // Old code: A.sub(S).add(P) reverts on intermediate A - S when A < S. - // Fix: A.add(P).sub(S) adds P first, so A + P >= S always holds. + // With an inverted state (A < S), the update must not revert: pending P is + // added before the snapshot S is subtracted (A.add(P).sub(S)), so the + // intermediate A + P stays non-negative and A + P >= S always holds. await expect(rewardsManager.connect(governor).onSubgraphSignalUpdate(subgraphDeploymentID)).to.not.be.reverted }) @@ -229,11 +233,11 @@ describe('Rewards: Snapshot Inversion', () => { // First call with inverted state await rewardsManager.connect(governor).onSubgraphSignalUpdate(subgraphDeploymentID) - // After the fix processes the inverted state, snapshots should be synced + // After processing the inverted state, snapshots should be synced const after = await rewardsManager.subgraphs(subgraphDeploymentID) expect(after.accRewardsForSubgraphSnapshot).to.equal( after.accRewardsForSubgraph, - 'snapshot should equal accumulated after fix processes inverted state', + 'snapshot should equal accumulated after processing inverted state', ) // Subsequent calls should work normally @@ -272,10 +276,10 @@ describe('Rewards: Snapshot Inversion', () => { expect(perAllocBefore).to.be.lt(after.accRewardsPerAllocatedToken, 'should distribute rewards: 0 < (A + P) - S') // The distributed amount should be less than total new rewards (P) - // because the gap represents already-distributed rewards from the old code + // because the gap represents already-distributed rewards // Undistributed = (A + P) - S = P - gap (since S = A + gap) // If P ≈ 2000 GRT and gap = 500 GRT, undistributed ≈ 1500 GRT - // Without the gap subtraction, it would have been P ≈ 2000 GRT (double-counting) + // Subtracting the gap is what keeps this from being P ≈ 2000 GRT (double-counting) // Verify snapshots are synced expect(after.accRewardsForSubgraphSnapshot).to.equal(after.accRewardsForSubgraph) @@ -350,7 +354,7 @@ describe('Rewards: Snapshot Inversion', () => { }) describe('normal operation (no inversion)', function () { - it('should produce identical results when A == S (post-fix steady state)', async function () { + it('should produce identical results when A == S (synced snapshot steady state)', async function () { await setupSubgraphWithAllocation() // Ensure snapshots are synced (normal state) diff --git a/packages/issuance/test/unit/agreement-manager/cancelWithPendingUpdate.t.sol b/packages/issuance/test/unit/agreement-manager/cancelWithPendingUpdate.t.sol index 09f4d2675..bff046947 100644 --- a/packages/issuance/test/unit/agreement-manager/cancelWithPendingUpdate.t.sol +++ b/packages/issuance/test/unit/agreement-manager/cancelWithPendingUpdate.t.sol @@ -14,12 +14,12 @@ import { RecurringAgreementManagerSharedTest } from "./shared.t.sol"; contract RecurringAgreementManagerCancelWithPendingUpdateTest is RecurringAgreementManagerSharedTest { /* solhint-disable graph/func-name-mixedcase */ - /// @notice Demonstrates the bug: when an accepted agreement with a pending (unapplied) - /// update is canceled, the pendingUpdateMaxNextClaim escrow is NOT freed during - /// cancelAgreement. The escrow remains locked until the agreement is fully drained - /// and deleted, even though the update can never be accepted (collector rejects - /// updates on non-Accepted agreements). - function test_CancelAgreement_PendingUpdateEscrowNotFreed() public { + /// @notice Verifies that a pending (unapplied) update on a canceled agreement does not + /// inflate sumMaxNextClaim. When an accepted agreement with a pending update is canceled, + /// the pendingUpdateMaxNextClaim escrow must be freed: the update can never be applied + /// (the collector rejects updates on non-Accepted agreements), so only the base claim + /// counts toward reserved escrow. + function test_CancelAgreement_PendingUpdateEscrowFreed() public { // 1. Offer and accept an agreement (IRecurringCollector.RecurringCollectionAgreement memory rca, ) = _makeRCAWithId( 100 ether, @@ -68,16 +68,16 @@ contract RecurringAgreementManagerCancelWithPendingUpdateTest is RecurringAgreem ); assertTrue(exists, "agreement should still exist (has remaining claims)"); - // 4. BUG: The pending update can never be accepted (collector rejects updates on - // canceled agreements), yet pendingUpdateMaxNextClaim is still reserved. + // 4. The pending update can never be applied — the collector rejects updates on + // canceled agreements — so its escrow must not count toward sumMaxNextClaim. uint256 sumAfterCancel = agreementManager.getSumMaxNextClaim(_collector(), indexer); - // The pending escrow should have been freed (zeroed) since the update is dead. - // sumMaxNextClaim should only include the base claim, not the dead pending update. + // The pending escrow must be freed (zeroed) since the update can no longer be applied. + // sumMaxNextClaim must include only the base claim, not the unappliable pending update. assertEq( sumAfterCancel, agreementManager.getAgreementMaxNextClaim(IAgreementCollector(address(recurringCollector)), agreementId), - "BUG: sumMaxNextClaim should only include the base claim, not the dead pending update" + "sumMaxNextClaim must include only the base claim, not the unappliable pending update" ); } diff --git a/packages/issuance/test/unit/agreement-manager/fundingModes.t.sol b/packages/issuance/test/unit/agreement-manager/fundingModes.t.sol index b2d3b80e7..525aad2e3 100644 --- a/packages/issuance/test/unit/agreement-manager/fundingModes.t.sol +++ b/packages/issuance/test/unit/agreement-manager/fundingModes.t.sol @@ -236,10 +236,12 @@ contract RecurringAgreementManagerFundingModesTest is RecurringAgreementManagerS uint256 maxClaim2 = 2 ether * 7200 + 200 ether; // indexer is fully deposited (undeposited = 0), indexer2 has full deficit (undeposited = maxClaim2) - // totalEscrowDeficit must be maxClaim2, NOT 0 (the old buggy sumMaxNextClaim - totalInEscrow approach - // would compute sumMaxNextClaim = maxClaim1 + maxClaim2, totalInEscrow = maxClaim1, - // deficit = maxClaim2 — which happens to be correct here, but would be wrong if indexer - // were over-deposited and the excess masked indexer2's deficit) + // totalEscrowDeficit must be computed per-provider: it sums each provider's own undeposited + // shortfall, so here it equals indexer2's full deficit (maxClaim2) and nothing from indexer. + // Per-provider computation is required as a permanent invariant: a naive + // sum(maxNextClaim) - totalInEscrow would let one provider's surplus offset another's + // shortfall, so if indexer were over-deposited its excess would mask indexer2's deficit and + // understate the total. The per-provider sum avoids that by never netting surplus against deficit. assertEq(agreementManager.getTotalEscrowDeficit(), maxClaim2, "Undeposited = indexer2's full deficit"); // Verify per-provider escrow state