Add --check-lcm mode to verify captured LedgerCloseMeta golden data in CI by sisuresh · Pull Request #5435 · stellar/stellar-core

sisuresh · 2026-08-28T19:06:52Z

Adds a --check-lcm DIRNAME test mode mirroring --check-test-tx-meta: each index.json under test-lcm-{current,next}/ is stamped with the protocol version / rng seed that produced it (validated up front, so a protocol bump without regeneration fails fast), and the LCM captured at each leaf section is compared against the checked-in golden file. Wired into ci-build.sh as an extra make check pass reusing the already-built binary.

The first CI run on this PR is expected to fail the new check — the existing golden data predates the header stamp. A follow-up commit regenerates it under both build configurations.

Copilot

Pull request overview

Adds CI verification for captured LedgerCloseMeta golden data.

Changes:

Adds --check-lcm with header validation and semantic comparison.
Stamps captured indexes with protocol and RNG configuration.
Adds CI integration and documentation.

Reviewed changes

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

Show a summary per file

File	Description
`src/test/test.cpp`	Implements LCM checking and header validation.
`src/test/test.h`	Updates LCM tracking documentation.
`ci-build.sh`	Adds the CI golden-data check.
`docs/software/commands.md`	Documents `--check-lcm`.
`.github/copilot-instructions.md`	Documents CI enforcement.

Suppressed comments (1)

ci-build.sh:226

The repository's required test invocation flags include --abort, but this newly added CI pass omits it. Add the flag so an ordinary Catch failure stops the pass immediately rather than spending the remainder of this expensive [tx] run after the outcome is known.

export STELLAR_CORE_TEST_PARAMS="--ll fatal -r simple --disable-dots --rng-seed 12345 --check-lcm ${SRC_DIR}"

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

sisuresh · 2026-08-28T21:08:20Z

+# protocol version only.
+echo Running fixed check-lcm tests
+export TEST_SPEC='[tx]'
+export STELLAR_CORE_TEST_PARAMS="--ll fatal -r simple --disable-dots --rng-seed 12345 --check-lcm ${SRC_DIR}"


The regenerated golden trees (with the header stamps) landed in later commits on this PR — 50f847b for test-lcm-current and b18addf for test-lcm-next. The first CI round intentionally ran without them to demonstrate the failure mode.

sisuresh · 2026-08-28T21:08:22Z

+    if (startIndex >= allMetas.size())
+    {
+        // Capture mode would not have written a file for this leaf.
+        return;
+    }


Good catch — fixed. The no-meta path now records a failure when a golden file exists for the leaf: capture mode never writes files for no-meta leaves, so an existing file means the test used to produce LCM and no longer does. This mirrors how the tx-meta check catches disappearance via un-consumed hashes at end of run.

Copilot

Pull request overview

Copilot reviewed 9 out of 1636 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/test/test.cpp:394

If a leaf previously produced a golden LCM but now produces none, this return treats it as success and leaves the existing golden file unchecked. That allows loss of LCM capture to pass CI; record a failure when the expected file exists.

Copilot

Pull request overview

Copilot reviewed 9 out of 1638 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/test/test.cpp:394

--check-lcm silently succeeds when a leaf that has a checked-in golden file stops producing any LCM. This early return skips both the existence check and comparison, so a regression that removes a closeLedger/capture call is not detected. If the golden path exists, record a mismatch before returning; leaves that have never had golden data can still be ignored.

sisuresh · 2026-09-02T00:13:39Z

      Meta is normalized (sorted) before writing so that output is
-      deterministic given a fixed `--rng-seed`.
+      deterministic given a fixed `--rng-seed`. Each `index.json` is stamped
+      with the protocol version, rng seed and protocol-version list that
+      produced the data.


Fixed in f6be648 — the docs now say files preserve original entry order (with diagnostics zeroed) and that normalization happens only in the comparison copies, both for the capture-time skip-rewrite check and for --check-lcm.

Copilot

Pull request overview

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

Suppressed comments (1)

docs/software/commands.md:238

This still states that meta is normalized before writing, but writeLcmToFile explicitly preserves original ordering and only normalizes copies for semantic comparison. Update the description so users do not expect the checked-in XDR stream itself to be sorted.

sisuresh · 2026-09-02T00:13:41Z

+// Returns true if --capture-lcm or --check-lcm was passed to the test
+// command. When enabled, LedgerCloseMeta from closeLedger/closeLedgerOn is
+// accumulated and, at leaf section boundaries (or test case end for tests


Fixed in f6be648 — the TxTests.h declaration comment now mentions both --capture-lcm and --check-lcm.

sisuresh · 2026-09-02T00:13:43Z

+        in >> root;
+        for (auto const* key : {TESTKEY_PROTOCOL_VERSION, TESTKEY_RNG_SEED,
+                                TESTKEY_ALL_VERSIONS, TESTKEY_VERSIONS_TO_TEST})
+        {


Acknowledged as a known limitation, matching the tx-meta baseline flow: under partitioned make check, a single process cannot distinguish 'leaf not run in this partition' from 'leaf deleted', and there is no cross-process aggregation point, so unconsumed-entry detection would need infrastructure that doesn't exist today. Stale files/index entries are inert (never checked against anything), and the dangerous variant — a test that still runs but stops producing meta — does fail via the golden-exists-but-no-meta check. A sequential garbage-collect mode for capture could prune orphans; leaving that for a follow-up.

Copilot

Pull request overview

Copilot reviewed 8 out of 1637 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/test/test.cpp:1519

This only reports mismatches for leaves that executed. loadLcmHeaders discards every hash entry, so deleting a captured section/test (or removing its [tx] tag) leaves the old index/XDR entry unconsumed and CI still succeeds. Track expected hashes and fail on leftovers; since make check partitions tests, either aggregate consumption across partitions or run this check unpartitioned.

Copilot

Pull request overview

Copilot reviewed 8 out of 1636 changed files in this pull request and generated 1 comment.

sisuresh · 2026-09-02T00:13:45Z

+# protocol version only.
+echo Running fixed check-lcm tests
+export TEST_SPEC='[tx]'
+export STELLAR_CORE_TEST_PARAMS="--ll fatal -r simple --disable-dots --rng-seed 12345 --check-lcm ${SRC_DIR}"


Intentional, matching the check-test-tx-meta pass above it: for golden-data checks we want the run to continue and aggregate every mismatch into one end-of-run report (that is how multiple issues were diagnosed in single CI runs while iterating on this PR), rather than abort at the first failure. An ordinary test failure still fails make check via the nonzero exit.

Copilot

Pull request overview

Copilot reviewed 9 out of 1637 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 12 out of 1978 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/test/test.cpp:1594

When --prune-stale-lcm is not set, this still replaces index.json with only the entries observed in the current run. A filtered capture that touches one test file therefore drops mappings for all unrun leaves in that file and leaves their retained .xdr files orphaned, contrary to the documented non-pruning behavior. Merge existing non-header mappings when pruning is disabled, and rebuild from scratch only when gLcmMayPrune is true.

sisuresh · 2026-09-02T22:17:05Z

+        // Entries injected straight into the bucket list appear in the ledger
+        // without any transaction producing them, so the resulting meta is
+        // not a faithful record of how the state came to be.
+        taintLcmCapture("injects ledger entries directly into the bucket list");


Fixed in 85f8eda — setNextArchiveBatchForBucketTesting now applies the same taint.

Copilot

Pull request overview

Copilot reviewed 12 out of 1978 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

src/bucket/test/BucketTestUtils.h:92

This only taints the live-bucket setter. setNextArchiveBatchForBucketTesting also injects archive/restored entries directly (BucketTestUtils.cpp:247-248), and the addLiveBatchAndUpdateSnapshot / addHotArchiveBatchAndUpdateSnapshot helpers directly modify bucket state as well. Those paths can still produce golden LCM that omits how its starting state was created, despite the documented automatic exclusion; apply the taint hook to every direct-injection path.

sisuresh · 2026-09-02T22:17:08Z

+    std::ofstream out(dir + "/index.json", std::ios_base::trunc);
    out << writer.write(root);


Fixed in 94644d2 — the index write now validates the stream, enables exceptions and closes explicitly, matching saveTestTxMeta.

sisuresh · 2026-09-02T22:17:10Z

+finalizeLcmCapture()
+{
+    size_t pruned = 0;
+    for (auto const& [dir, entries] : gLcmCapturedIndex)


Fixed in 94644d2 — visited output directories are now tracked independently of captured entries, so a test file whose leaves have all become ineligible is still visited. Verified by planting a stale .xdr in such a directory: the full prune removes the file, the index and the directory, rather than leaving one holding only header keys.

sisuresh · 2026-09-02T22:17:11Z

+    size_t pruned = 0;
+    for (auto const& [dir, entries] : gLcmCapturedIndex)
+    {
+        writeLcmIndex(dir, entries);


Good catch, this was the worst of the batch — it defeated the reason pruning is opt-in. Fixed in 94644d2: existing non-header entries are merged when not pruning, and the index is replaced outright only in prune mode. Verified that a single-test capture of InvokeHostFunctionTests leaves all 680 index entries intact.

Copilot

Copilot reviewed 12 out of 1978 changed files in this pull request and generated no new comments.

Copilot

Copilot reviewed 13 out of 1979 changed files in this pull request and generated no new comments.

…n CI The test-lcm-current/ and test-lcm-next/ trees were write-only: nothing recorded what protocol version (or seed) produced them and nothing in CI ever read them back, so a protocol bump silently invalidated both tiers. Mirror the --check-test-tx-meta flow: stamp each index.json with the same header keys the tx-meta baselines use ('!cfg protocol version', '!rng seed', '!test all versions', '!versions to test'), validate them up front in check mode so stale data fails fast, and compare the LCM captured at each leaf section against the golden file, reporting mismatches at end of run. Wire the check into ci-build.sh as an extra make check pass reusing the binary already built for the check-test-tx-meta run.

--check-lcm surfaced that 'Soroban classic account authentication' had two sibling SECTIONs named 'wrong key type' (silently sharing one golden file, last writer wins) and two identical SECTIONs named 'missing signature field'. Rename the former to 'wrong signature key type' and drop the redundant twin of the latter. Capture and check now fail when two distinct sections would produce the same golden file, for leaves that actually accumulate LCM. Dropping a section means the test case re-runs once less, so the parent contexts of that test accumulate one fewer set of setup-tx hashes; both tx-meta baseline tiers are re-recorded accordingly, changing by exactly the 25 hashes the check reported as un-checked.

Review feedback: the no-meta early return let a regression that eliminates expected LedgerCloseMeta pass silently. Capture never writes a file for a no-meta leaf, so an existing golden means the meta disappeared.

--base-instance exists to keep OS-level resources (ports, databases, directories) distinct across concurrent test processes, but it also shifted NODE_SEED, whose identity lands in the signed StellarValue of ledger headers and cascades through header hashes, previousLedgerHash chains and txSetHash. Golden LCM captured at base instance 0 therefore failed --check-lcm under partitioned CI runs (which use --base-instance j*50). Only LCM capture/check runs need instance-invariant node identity, so the change is scoped to them; every other run keeps the historical offset-derived seed, and base instance 0 derivation is unchanged.

Files are written in original entry order with diagnostics zeroed; normalization happens only in comparisons. Accumulation happens under --check-lcm as well as --capture-lcm.

updateLcmIndex read-merge-wrote each index.json and writeLcmToFile never deleted anything, so leaves that stopped being produced kept their files and index entries forever: the checked-in data still carried 272 protocol 26 and 27 leaves from regens predating the protocol 28 bump, and every bump stacked another set. Capture now accumulates its index entries in memory and, at end of a clean run, rewrites each touched directory's index from scratch and (with --prune-stale-lcm) deletes .xdr files the run did not write. This mirrors saveTestTxMeta, which rebuilds each baseline JSON from what the run observed rather than merging into the existing file. Pruning is opt-in because a filtered run visits only some leaves, and is per touched directory only, matching the granularity of one baseline file per test file.

taintLcmCapture marks a test ineligible when it injects ledger entries straight into the bucket list or the archive (the meta never shows the entries being created), runs a multi-node Simulation, or uses a config whose ledger content depends on thread scheduling or randomized nomination. Capture skips those leaves with a named warning; --check-lcm fails if golden data for them still exists, so a vector from before a test became ineligible cannot linger. The multi-node herder simulation is excluded by these rules rather than by hand, so if that simulation is ever made deterministic its vector comes back for free.

Three defects in the prune pass, all found in review: Without --prune-stale-lcm the index was still rewritten from only the entries the run observed, so a filtered capture dropped the hash-to-name mappings of untouched leaves while leaving their .xdr files behind, unidentifiable. Existing non-header entries are now merged when not pruning; the index is replaced outright only in prune mode. The prune pass iterated the directories that produced entries, so a test file whose leaves had all become ineligible was never visited and kept its stale goldens through a full-corpus prune. Visited output directories are now tracked independently, and such a directory has its index and itself removed rather than being left holding only header keys. The index write checked neither that the file opened nor that the final flush succeeded, so an unwritable path could leave a truncated index while capture reported success. It now validates the stream, enables exceptions and closes explicitly, as saveTestTxMeta does.

The TxMeta baselines have a section explaining how to re-record them; the golden LCM data had nothing outside the command reference, which is part of why it went stale at protocol 27 for a month without anyone noticing.

Both tiers were captured before the protocol 28 bump, so test-lcm-current held protocol 27 meta and test-lcm-next held protocol 28. Re-captured at 28 and 29 respectively, with the index.json header stamps the check validates. The regeneration also drops what had accumulated: leaves left behind by regens at protocol 26 and 27, leaves orphaned by the section renames, and the CAP-85 vector, whose test injects the executable-reference entry directly into the bucket list so the meta never showed the entry being created. Each tier goes from 1131 files to 844.

Copilot

Pull request overview

Copilot reviewed 13 out of 1979 changed files in this pull request and generated 1 comment.

+    return prefix + lcmTierDirName() + "/" + file.filename().stem().string();
 }

 int32_t


dmkozh

Just a general thought to consider, not necessary for this PR. The existing test tx meta that we capture is basically just a subset of the full LCM, so we shouldn't need to keep both around given the CI integration. Also tx meta has pretty poor coverage for Soroban, as IIRC it mostly captures the classic transactions. Another benefit of the LCM capture is that it seems to be more parallel-friendly; I don't believe it runs with NUM_PARTITIONS>1 out of the box, but it doesn't seem too hard to make it do so, as the meta is now captured per scenario (instead of a single huge file). We'll still need to merge the index files, but that's hopefully not that hard to do.

dmkozh · 2026-09-09T22:04:03Z

+// reason; --check-lcm fails if golden data for them still exists, so a
+// stale or misleading vector cannot linger. Calling this when LCM tracking
+// is off is free, so hooks may call it unconditionally.
+void taintLcmCapture(std::string const& reason);


nit: this name is both too fancy and imprecise (because it disables meta and it does not spoil it). This should be just be called 'disableLcmCapture', and the underlying global variable should be called correspondingly.

dmkozh · 2026-09-09T22:13:06Z

+// Marks the running test as unsuitable for LedgerCloseMeta golden data,
+// naming the reason (e.g. injecting ledger entries straight into the bucket
+// list, or running a multi-node simulation whose progression depends on
+// thread scheduling). Capture skips such a test's leaves and logs the


Is reason logging achieving anything? We're anyways leaving comments around this, so I'm not sure if additional logging is necessary.

dmkozh · 2026-09-09T22:16:53Z

+   "!rng seed" : 12345,
+   "!test all versions" : false,
+   "!versions to test" : [ 29 ],
+   "17846470c1f6514e" : "sponsor_future_reserves-protocol_version_29-add_sponsored_entry_before_adding_first_sponsored_signer",


nit: Now that we're making this more productionized, I figured we might revisit the test name format. IIRC this semi-normalized format has been used for the raw test names, but since we need to hash the names anyways, can we use the proper human readable names, like the name format in txmeta (Soroban classic account authentication|account with required multisig etc.).

dmkozh · 2026-09-09T22:29:59Z

    META_TEST_CHECK
 };

+// Declared here (rather than with the other test globals below) because the


I don't understand this comment, if it needs to to move above some code, why not just declare it inside namespace without comments (declaration order justification is not particularly interesting information)

dmkozh · 2026-09-10T00:24:55Z

+    // In check mode the golden data lives under the directory passed to
+    // --check-lcm (the source tree root when building out-of-tree); in
+    // capture mode it is written relative to the current directory.
+    std::string prefix = gLcmCheckDir.empty() ? "" : gLcmCheckDir + "/";


nit: it's weird that we mix path with strings (especially the manual "/").

dmkozh · 2026-09-10T00:57:13Z

            REQUIRE(singleInvocation(signer, baseCredentials) ==
                    InvokeHostFunctionResultCode::INVOKE_HOST_FUNCTION_TRAPPED);
        }
-        SECTION("missing signature field")


Why did you remove this? We should just rename the sections to make the names unique.

Copilot AI balanced review requested due to automatic review settings August 28, 2026 19:06

Copilot started reviewing on behalf of sisuresh August 28, 2026 19:07 View session