test: Add invariant tests for SumTree structure in PDPVerifier #166

Hany-Almnaem · 2025-05-26T17:10:47Z

Summary

This PR introduces a suite of tests to validate the correctness and stability of the sumTreeCounts structure in the PDPVerifier smart contract.

The tests ensure that each node in the SumTree maintains the invariant:

sumTreeCounts[setId][index] == sum of rootLeafCounts[setId][j] for the correct range derived from the node’s height.

This directly addresses and resolves issue: #45

Summary of Changes

Core Tests (Basic Coverage)

testSumTreeInvariantAfterAddsAndRemovals()
- Adds a controlled set of roots, removes a few, and asserts the sumtree structure.
assertSumTreeInvariant(uint256 setId)
- Shared invariant validation used across tests.
testAdvancedSumTreeInvariant()
- Simulates multiple add/remove rounds using dynamic values.
testFuzzedSumTreeInvariant(uint256 seed)
- Uses seeded randomness to test with pseudo-random leaf counts and root IDs.

Advanced Suite (Tagged `[ADVANCED]`)

Implemented in a separate SumTreeEnhancedTest contract:

testSumTreeStressTest()
- Applies add/remove operations over several rounds with increasing load.
testSumTreeEdgeCases()
- Verifies correctness under very small, very large, and alternating root sizes.
testSumTreePerformance()
- Executes 100 add/remove operations and logs gas usage.
testSumTreeRandomized(uint256 seed)
- Interleaves randomized add/remove logic with proving period progression.
Shared internal assertSumTreeInvariant() with more detailed error messaging for diagnostics.

Benefits

Validates the structural integrity of the SumTree under normal and edge-case usage.
Improves test coverage for internal tree logic, including depth-aware summation.
Reduces regression risk from future updates to root addition/removal logic.
Keeps invariant assertions reusable and centralized for clarity and reliability.

Notes

Tests pass locally using forge test
No contract logic has been modified—only test coverage has been added.
Code has been linted and follows existing style conventions.
Advanced tests are separated to keep the default suite performant and focused.

Please let me know if you prefer these broken into separate PRs or test files.

wjmelements · 2025-10-25T00:41:43Z

test/PDPVerifier.t.sol

+    }
+}
+
+function testSumTreeInvariantAfterAddsAndRemovals() public {


merge with latest master
run forge fmt

rvagg · 2025-12-04T12:29:54Z

@Hany-Almnaem please see the above comment - this needs a merge from master (or rebase) plus a fmt. Otherwise this seems pretty good I think.

- Resolve conflict in test/PDPVerifier.t.sol - Fix failing SumTreeEnhancedTest tests - Apply forge fmt formatting

Hany-Almnaem · 2025-12-04T15:03:01Z

@rvagg
Done! Merged upstream/main, resolved conflicts, and ran forge fmt.
All tests passing. Ready for review.

rvagg · 2025-12-05T03:59:37Z

Thanks @Hany-Almnaem . I'm going to have to personally put off reviewing this for now because it's a large-enough context switch for me to do it and I have a lot on my plate currently. Maybe @ZenGround0 gets inspired to come have a look, his head is more full of the details here than mine.

ZenGround0

I think the core of this PR is good once we stop swallowing underflow errors. It just needs to be cleaned up and simplified significantly.

ZenGround0 · 2026-01-13T19:16:50Z

test/PDPVerifier.t.sol

+        );
+    }
+
+    function assertSumTreeInvariant(uint256 setId) internal view {


Let's not duplicate this entire function from here. Please refactor this somehow, probably with a shared inherited contract, and remove duplication.

After looking at this further I suggest removing SumTreeEnhancedTest and most of its tests entirely for a smaller test set directly in SumTreeAddTest

ZenGround0 · 2026-01-13T19:19:07Z

test/PDPVerifier.t.sol

+/// @title SumTreeEnhancedTest
+/// @notice Advanced tests for SumTree data structure focusing on stress testing, performance, and edge cases
+/// @dev Tagged with [ADVANCED] to distinguish from basic tests
+contract SumTreeEnhancedTest is MockFVMTest, PieceHelper {


Why use a separate contract here with a duplicated setup? Why not add these tests to SumTreeAddTest contract? That would solve the duplicated assertSumTreeInvariant problem too.

ZenGround0 · 2026-01-13T19:24:38Z

test/PDPVerifier.t.sol

+            uint256 height = pdpVerifier.getTestHeightFromIndex(index);
+            uint256 range = 1 << height;
+
+            if (index + 1 < range) {


We should definitely not be swallowing these errors! If something is this wrong with either the sumtree or the test then we need to fail and fix what is broken.

ZenGround0 · 2026-01-13T19:34:18Z

test/PDPVerifier.t.sol

+            }
+
+            uint256 expectedSum = 0;
+            for (uint256 j = index + 1 - range; j <= index; j++) {


This looks correct ✅

ZenGround0 · 2026-01-13T19:41:44Z

test/PDPVerifier.t.sol

+        assertSumTreeInvariant(testSetId);
+    }
+
+    function testAdvancedSumTreeInvariant() public {


nit: testSumTreeInvariantAddsAndRemovals2

ZenGround0 · 2026-01-13T19:44:25Z

test/PDPVerifier.t.sol

+        uint256 numRounds = 5;
+        uint256 piecesPerRound = 8;
+
+        for (uint256 round = 0; round < numRounds; round++) {


Since this is so similar to this test I would suggest just replacing the single round version there with this multi round version. I don't see a lot of value in having both tests.

ZenGround0 · 2026-01-13T19:45:37Z

test/PDPVerifier.t.sol

+    }
+
+    /// @notice [ADVANCED] Tests SumTree behavior with edge case sizes and patterns
+    function testSumTreeEdgeCases() public {


I don't understand why this is "edge cases". Again this seems like something we could remove without losing any conceptual coverage at all.

ZenGround0 · 2026-01-13T19:47:32Z

test/PDPVerifier.t.sol

+        }
+    }
+
+    function testSumTreeWithValues(uint256[3] memory values) internal {


This and createFreshDataSet are not used consistently throughout testing. I suggest sticking to one way of setting things up, either directly as you do here or refactoring things to make use of helper functions like this and createFreshDataSet. Probably it will be clearer if you forego these helper functions but I leave that up to you.

ZenGround0 · 2026-01-13T19:49:52Z

test/PDPVerifier.t.sol

+    }
+
+    /// @notice [ADVANCED] Tests SumTree performance with large numbers of operations
+    function testSumTreePerformance() public {


This is not a large number of operations. Also unfortunately gas measured via solidity tests is ~ meaningless in the filecoin context as our gas model is quite different. Please delete this.

ZenGround0 · 2026-01-13T19:50:31Z

test/PDPVerifier.t.sol

+    }
+
+    /// @notice [ADVANCED] Tests SumTree with alternating add/remove pattern
+    function testSumTreeAlternatingPattern() public {


This test is different enough that it can stay.

BigLep · 2026-01-22T02:08:02Z

@Hany-Almnaem : thank you for your work here, and apologies for the delay for having maintainers to engage. Do you think you'll be able to incorporate comments so we can get this merged soon while the context is fresh?

test: Add invariant tests for SumTree (basic and advanced coverage)

5f32f4b

github-project-automation bot added this to PDP May 26, 2025

aarshkshah1992 requested review from ZenGround0 and aarshkshah1992 May 28, 2025 15:20

rjan90 moved this to 🔎 Awaiting review in PDP Jun 10, 2025

rjan90 added this to FOC Jul 15, 2025

github-project-automation bot moved this to 📌 Triage in FOC Jul 15, 2025

rjan90 moved this from 📌 Triage to 🔎 Awaiting review in FOC Jul 15, 2025

rjan90 linked an issue Jul 15, 2025 that may be closed by this pull request

Invariant tests of sumtree #45

Open

rjan90 assigned Hany-Almnaem Jul 15, 2025

rjan90 added this to the MX: Priority and sequencing TBD milestone Aug 19, 2025

wjmelements reviewed Oct 25, 2025

View reviewed changes

Hany-Almnaem added 2 commits December 4, 2025 17:44

Merge upstream/main into feat/invariant-sumtree-tests

ea438ca

- Resolve conflict in test/PDPVerifier.t.sol - Fix failing SumTreeEnhancedTest tests - Apply forge fmt formatting

Enhance SumTree invariant tests with pattern coverage

d75d2de

rjan90 removed the request for review from aarshkshah1992 December 8, 2025 08:10

BigLep added this to FilOz Dec 17, 2025

github-project-automation bot moved this to 📌 Triage in FilOz Dec 17, 2025

BigLep moved this from 📌 Triage to 🔎 Awaiting Review in FilOz Jan 13, 2026

ZenGround0 reviewed Jan 13, 2026

View reviewed changes

BigLep moved this from 🔎 Awaiting review to ⌨️ In Progress in FOC Jan 13, 2026

test: Add invariant tests for SumTree structure in PDPVerifier #166

Are you sure you want to change the base?

test: Add invariant tests for SumTree structure in PDPVerifier #166

Uh oh!

Conversation

Hany-Almnaem commented May 26, 2025

Summary

Summary of Changes

Core Tests (Basic Coverage)

Advanced Suite (Tagged [ADVANCED])

Benefits

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvagg commented Dec 4, 2025

Uh oh!

Hany-Almnaem commented Dec 4, 2025

Uh oh!

rvagg commented Dec 5, 2025

Uh oh!

ZenGround0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BigLep commented Jan 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Advanced Suite (Tagged `[ADVANCED]`)