Lumen snapshot invariant #5059

sisuresh · 2025-12-10T23:57:26Z

Description

Resolves #5028

Checklist

Reviewed the contributing document
Rebased on top of master (no merge commits)
Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
Compiles
Ran all tests
If change impacts performance, include supporting evidence per the performance document

sisuresh · 2025-12-10T23:59:10Z

src/invariant/ConservationOfLumens.cpp

+}
+
+std::string
+ConservationOfLumens::checkSnapshot(


Something that's important to note here - ConservationOfLumens isn't a strict invariant, so it won't take down the node. Instead, it'll log an error that our alerting will pick up. If we're not fine with this, we'll either need to make this invariant strict, or create a new invariant.

I think the invariant is still very likely to fail in this case since the balance would have to match (by luck) but I agree this should probably be handled more precisely by passing in another out-parameter for an error message.

I believe you meant to comment here right? I agree and updated that code.

Copilot

Pull request overview

This PR implements a snapshot-based invariant for conservation of lumens to validate that the total native asset supply matches the totalCoins value in the ledger header. The implementation scans both live and hot archive buckets to calculate the sum of all native balances and compares it against the recorded total.

Key changes:

Adds checkSnapshot method to ConservationOfLumens invariant that scans bucket lists
Introduces isStopping callback parameter to allow early termination during application shutdown
Adds test method getLastClosedLedgerStateForTesting to access ledger state for testing

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/main/AppConnector.h/cpp	Adds `isStopping()` method to expose application shutdown state
src/ledger/LedgerManager.h	Adds `getLastClosedLedgerStateForTesting()` virtual method declaration
src/ledger/LedgerManagerImpl.h/cpp	Implements `getLastClosedLedgerStateForTesting()` for test access and updates snapshot invariant call with `isStopping` callback
src/invariant/InvariantManager.h	Updates `runStateSnapshotInvariant` signature to include `isStopping` parameter
src/invariant/InvariantManagerImpl.h/cpp	Forwards `isStopping` callback to individual invariant checks
src/invariant/Invariant.h	Adds `isStopping` parameter to `checkSnapshot` virtual method
src/invariant/ConservationOfLumens.h/cpp	Implements snapshot validation by scanning live and hot archive buckets to sum native balances
src/invariant/ArchivedStateConsistency.h/cpp	Updates signature to accept `isStopping` parameter (unused in implementation)
src/invariant/test/ConservationOfLumensTests.cpp	Adds test case validating the snapshot invariant correctly detects mismatches in total lumens

src/invariant/ConservationOfLumens.cpp

SirTyson

Thanks for working on this! I think it looks good overall, just a few small nits and a suggestion on a memory/IO optimization. I think we should also improve the test to verify a non-empty and corrupt archive.

src/invariant/ArchivedStateConsistency.cpp

src/invariant/ConservationOfLumens.cpp

SirTyson · 2025-12-16T19:35:30Z

src/invariant/ConservationOfLumens.cpp

+    }
+
+    // Check if we should stop before scanning hot archive
+    if (isStopping())


Nit: I don't think this is necessary, since we check isStopping on the first iteration of scanHotArchiveBucket anyway.

I think this is still there?

Ah sorry I must've wiped the change during a rebase. Fixed.

src/invariant/ConservationOfLumens.cpp

SirTyson · 2025-12-16T21:15:38Z

src/invariant/test/ConservationOfLumensTests.cpp

+
+    // Note: We should figure out how to force the eviction of an SAC contract
+    // balance. The SAC auto extends balances, so simply waiting for TTL to
+    // expire is not practical. We need to update the in memory ttl to a lower


Given this is a BucketList specific invariant, I think we can test it by just manually populating the BucketList like we do in the "State archival eviction invariant" test. It's pretty easy to simulate a bad archival by just adding a DEADENTRY for the key in the live BucketList and an ARCHIVED key with the wrong value to the archive via the BucketTestUtils::BucketTestApplication interface. This test harness also automatically updates the in-memory state as well.

Added a test. I verified that it's triggering correctly in the test, but let me know if the code to populate the bucket can be improved.

graydon · 2025-12-20T03:18:52Z

src/invariant/ConservationOfLumens.cpp

+}
+
+std::string
+ConservationOfLumens::checkSnapshot(


I think the invariant is still very likely to fail in this case since the balance would have to match (by luck) but I agree this should probably be handled more precisely by passing in another out-parameter for an error message.

graydon

Just the one nit same as Garand, otherwise LGTM

Previously, processEntryIfNew would log overflow errors but allow the invariant to continue, causing it to pass despite data corruption. Now passes error string by reference and returns it from the invariant, causing proper failure on overflow. Changes: - Add errorMsg parameter to processEntryIfNew - Remove CLOG_ERROR calls, use fmt::format to set errorMsg - Update scanLiveBucket and scanHotArchiveBucket to accept and pass errorMsg - Check errorMsg after scan phases and return immediately if set 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Allows early termination during shutdown, preventing unnecessary work. Changes: - Check isStopping at start of checkIfLiveEntryInArchive lambda - Check isStopping between type iterations - Return empty string (not error) when stopping 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Add comprehensive bucket-level test using BucketTestApplication pattern. Tests three scenarios: 1. Invariant passes with correct bucket state 2. Invariant fails when bucket balance doesn't match totalCoins 3. Invariant handles shadowing correctly (LIVE entry shadows INIT entry) Addresses feedback from PR review to test actual bucket corruption rather than just modifying totalCoins. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

SirTyson

Overall LGTM, with a few small cleanups in the unit test.

SirTyson · 2025-12-23T23:29:47Z

src/invariant/test/ConservationOfLumensTests.cpp

+                app->getLedgerManager().getLastClosedLedgerStateForTesting();
+            auto& inMemoryState =
+                app->getLedgerManager().getInMemorySorobanStateForTesting();
+


I think we want to check that the test passes with a populated Hot Archive before corrupting it, as they other test case does not have any hot archive populated state.

Yeah good point. Added a second balance that is correctly archived.

SirTyson · 2025-12-23T23:32:19Z

src/invariant/test/ConservationOfLumensTests.cpp

+        acc1Modified.data.account().balance = 5000;
+
+        // Add modified acc1 as LIVE entry (shadows the INIT entry)
+        lm.setNextLedgerEntryBatchForBucketTesting(


I don't think this will actually create a shadow, since we'll just merge into the current snap immediately. I think if we close a few more ledgers in between the INIT and LIVE events (like 4 ledgers) we can guarantee an actual shadow.

Ah good point. Updated.

SirTyson · 2025-12-23T23:34:06Z

src/invariant/test/ConservationOfLumensTests.cpp

+    auto contractAddr = makeContractAddress(sha256("contract"));
+    REQUIRE(client.transfer(a1, contractAddr, 100));
+
+    // Note: We should figure out how to force the eviction of an SAC contract


This comment can be removed now right?

Yup removed

SirTyson · 2025-12-23T23:36:31Z

src/invariant/test/ConservationOfLumensTests.cpp

+        // Add entries to live bucket
+        lm.setNextLedgerEntryBatchForBucketTesting({acc1, acc2}, // init entries
+                                                   {},           // live entries
+                                                   {});          // dead entries


Nit: I don't think :: is required

SirTyson · 2025-12-23T23:37:54Z

src/invariant/test/ConservationOfLumensTests.cpp

+
+        // Remove from live buckets and inject corrupted entry into hot archive
+        lm.setNextLedgerEntryBatchForBucketTesting({}, {}, {balanceKey});
+        lm.setNextArchiveBatchForBucketTesting({balanceEntry}, {});


Nit: for correctness, we should also delete the TTL entry

Updated, although let me know if I did that right.

Copilot AI review requested due to automatic review settings December 10, 2025 23:57

Copilot started reviewing on behalf of sisuresh December 10, 2025 23:57 View session

sisuresh commented Dec 10, 2025

View reviewed changes

Copilot AI reviewed Dec 11, 2025

View reviewed changes

src/invariant/ConservationOfLumens.cpp Outdated Show resolved Hide resolved

src/invariant/ConservationOfLumens.cpp Show resolved Hide resolved

sisuresh force-pushed the lumen-inv branch 2 times, most recently from 26c82e7 to bbd59fa Compare December 11, 2025 18:54

sisuresh requested a review from SirTyson December 11, 2025 21:24

SirTyson requested changes Dec 16, 2025

View reviewed changes

graydon requested changes Dec 20, 2025

View reviewed changes

sisuresh and others added 7 commits December 22, 2025 13:29

Update ConservationOfLumens to add a snapshot invariant

f8524ce

add test

c0c1086

shutdown early

053a520

Optimization

90814e2

🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

sisuresh force-pushed the lumen-inv branch from bbd59fa to e12d9eb Compare December 22, 2025 21:29

Remove unnecessary check

78ff8f8

SirTyson previously approved these changes Dec 23, 2025

View reviewed changes

fixup! Improve ConservationOfLumens snapshot test

dcc1a0d

sisuresh dismissed SirTyson’s stale review via dcc1a0d December 24, 2025 01:48

Lumen snapshot invariant #5059

Are you sure you want to change the base?

Lumen snapshot invariant #5059

Uh oh!

Conversation

sisuresh commented Dec 10, 2025

Description

Checklist

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

SirTyson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

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!

graydon left a comment

Choose a reason for hiding this comment

Uh oh!

SirTyson 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants