Add --check-lcm mode to verify captured LedgerCloseMeta golden data in CI - #5435
Add --check-lcm mode to verify captured LedgerCloseMeta golden data in CI#5435sisuresh wants to merge 17 commits into
Conversation
…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.
There was a problem hiding this comment.
Pull request overview
Adds CI verification for captured LedgerCloseMeta golden data.
Changes:
- Adds
--check-lcmwith 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.
| # 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}" |
| if (startIndex >= allMetas.size()) | ||
| { | ||
| // Capture mode would not have written a file for this leaf. | ||
| return; | ||
| } |
There was a problem hiding this comment.
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.
--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/check now fails loudly when two distinct sections produce the same golden file path, but only for leaves that actually accumulate LCM — leaves with no meta never touch a file, so their name collisions are harmless.
The checked-in data was captured at protocol 27 before the bump in 3a59b3e. Also picks up the index.json header stamps, HerderTests, and the CAP-85 external-ref vector which now runs ungated at protocol 28.
The checked-in data was captured when vnext meant protocol 28, before the bump in 3a59b3e.
There was a problem hiding this comment.
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.
Dropping the duplicate 'missing signature field' section means the test case re-runs once less, so the parent contexts of 'Soroban classic account authentication' accumulate one fewer set of setup-tx hashes. Both tiers change by exactly the 25 hashes CI reported as un-checked; no other baseline content moved.
There was a problem hiding this comment.
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-lcmsilently 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 acloseLedger/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.
| 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. |
There was a problem hiding this comment.
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.
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. Also drop the copilot-instructions.md edit.
--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). Deriving NODE_SEED from the un-offset instance number makes captured meta invariant to --base-instance while remaining distinct per logical instance within multi-node tests. Base instance 0 derivation is unchanged, so existing golden data stays valid.
There was a problem hiding this comment.
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
writeLcmToFileexplicitly 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.
| // 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 |
There was a problem hiding this comment.
Fixed in f6be648 — the TxTests.h declaration comment now mentions both --capture-lcm and --check-lcm.
| in >> root; | ||
| for (auto const* key : {TESTKEY_PROTOCOL_VERSION, TESTKEY_RNG_SEED, | ||
| TESTKEY_ALL_VERSIONS, TESTKEY_VERSIONS_TO_TEST}) | ||
| { |
There was a problem hiding this comment.
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.
Only the LCM golden data needs instance-invariant node identity; every other test run keeps the historical offset-derived seed, so non-LCM behavior is unchanged from master.
There was a problem hiding this comment.
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.
loadLcmHeadersdiscards 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; sincemake checkpartitions tests, either aggregate consumption across partitions or run this check unpartitioned.
Its meta is not deterministic across platforms: randomized nomination in a 4-node simulation goes through stdlib distributions that differ between libc++ and libstdc++, so the golden vector failed --check-lcm on Linux while passing on macOS. Deterministic empty-tx-set ledgers are already covered by other captured vectors.
| # 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}" |
There was a problem hiding this comment.
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.
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 (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. This replaces the hand-removal of the herder simulation's capture call, which is restored: the rules now exclude it mechanically, so if that simulation is ever made deterministic the vector comes back for free.
287 files: 272 leaves left behind by regens at protocol 26 and 27, the CAP-85 vector (its test injects the executable-reference entry directly into the bucket list, so the meta never showed the entry being created), and leaves orphaned by the duplicate-section renames earlier in this branch.
The same 288 stale files as the current tier.
There was a problem hiding this comment.
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-lcmis not set, this still replacesindex.jsonwith 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.xdrfiles orphaned, contrary to the documented non-pruning behavior. Merge existing non-header mappings when pruning is disabled, and rebuild from scratch only whengLcmMayPruneis true.
| // 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"); |
There was a problem hiding this comment.
Fixed in 85f8eda — setNextArchiveBatchForBucketTesting now applies the same taint.
There was a problem hiding this comment.
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.
setNextArchiveBatchForBucketTestingalso injects archive/restored entries directly (BucketTestUtils.cpp:247-248), and theaddLiveBatchAndUpdateSnapshot/addHotArchiveBatchAndUpdateSnapshothelpers 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.
| std::ofstream out(dir + "/index.json", std::ios_base::trunc); | ||
| out << writer.write(root); |
There was a problem hiding this comment.
Fixed in 94644d2 — the index write now validates the stream, enables exceptions and closes explicitly, matching saveTestTxMeta.
| finalizeLcmCapture() | ||
| { | ||
| size_t pruned = 0; | ||
| for (auto const& [dir, entries] : gLcmCapturedIndex) |
There was a problem hiding this comment.
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.
| size_t pruned = 0; | ||
| for (auto const& [dir, entries] : gLcmCapturedIndex) | ||
| { | ||
| writeLcmIndex(dir, entries); |
There was a problem hiding this comment.
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.
setNextArchiveBatchForBucketTesting injects ledger state as directly as the live-bucket setter and can be called on its own, so tests using only it were still eligible for capture even though their meta omits how the archived state came to be.
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. Adds the parallel section: what the two modes do, where the data lives, that CI enforces it and fails fast after a protocol bump, the regeneration commands for both tiers, why --prune-stale-lcm belongs only with the full run, and which tests are skipped as ineligible.
Adds a
--check-lcm DIRNAMEtest mode mirroring--check-test-tx-meta: eachindex.jsonundertest-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 extramake checkpass 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.