Repository navigation
fix(subtreevalidation): validate peer subtrees skipped during catchup once RUNNING again - #2055
Merged
icellan merged 8 commits intoOct 27, 2026
Conversation
Contributor
|
🤖 Claude Code Review Status: Complete Current Review:
Docs: The CHANGELOG entry under History:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-10-27 17:42 UTC |
oskarszoon
requested changes
Oct 26, 2026
oskarszoon
left a comment
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
The backfill has a lost replay trigger, can fetch subtrees beyond its stated 30-minute limit, and can count an unvalidated subtree as replayed. Please address these before merging.
services/subtreevalidation/skipped_subtrees.go:205: If another catchup records entries while a replay is active, its RUNNING watcher can exit on thereplayingguard. The active replay then finishes without draining those entries. Arrange a follow-up replay; a scoped test reproduced one entry left queued with no new event.services/subtreevalidation/skipped_subtrees.go:221: Recheck each entry's age after it obtains a replay worker slot. The current batch-start check can let an entry wait past 30 minutes, fetch from a peer that has pruned it, and report that peer.services/subtreevalidation/skipped_subtrees.go:232: Countreplayedonly after validation completes or an existing subtree is confirmed. A requeue after another catchup, or a held quorum lock with no stored file, currently increments it.services/subtreevalidation/skipped_subtrees.go:74: Preserve the original announcement time when replay requeues an entry during catchup. Resetting it can fetch from and report the original peer more than 30 minutes after its announcement.
ordishs
approved these changes
Oct 27, 2026
freemans13
requested changes
Oct 27, 2026
freemans13
left a comment
freemans13
left a comment
Collaborator
There was a problem hiding this comment.
Request changes on e903bee6906debd2eb7193cecc14aa36390dd15a. I reproduced the lost-trigger and expiry problems in oskarszoon's review. The inline comment adds a related queue-ordering failure that drops newer announcements.
- A second RUNNING watcher can finish while the first replay is active. After that replay finishes, the regression finds one queued entry with both
watching=falseandreplaying=false. Nothing drains it without another external event. - An entry can expire while waiting for admission. A regression using the real replay/admission path, SQLite blockchain client and in-memory Kafka fetched it after 30 minutes. A simulated 404 then published
peer_cannot_provide_subtreeagainst the original valid peer ID. Check age after admission, before network work, as well as preserving the original timestamp across catchups. - With a full queue, requeuing old replay entries can evict newer announcements. Preserve their original ordering as described inline.
Each focused regression reproduced twice with the race detector. The existing SubtreeValidation and settings suites passed with SETTINGS_CONTEXT=test go test -race -tags testtxmetacache -count=2 ./services/subtreevalidation/... ./settings. I did not run the full repository suite or verify CI.
Contributor
Author
|
Addressed in ec8aa0b, with a regression test for each point. Each test fails on e903bee.
New metric outcomes
|
freemans13
approved these changes
Oct 27, 2026
freemans13
left a comment
freemans13
left a comment
Collaborator
There was a problem hiding this comment.
Approve on ec8aa0b7f. All three points from my review on e903bee69 are fixed, and so are oskarszoon's four. Two things need doing before merge, and neither is a code finding:
- No CI has run on
ec8aa0b7f. Only Socket Security posted a check run. The last full run was one903bee69. - GitHub reports the PR as
DIRTY, meaning it now conflicts withmain. That conflict is probably why CI did not start.
How I checked the fixes. I undid each fix in a scratch worktree and ran its regression test:
| Fix undone | Test | Result |
|---|---|---|
| Age check after admission removed | ExpiresWhileWaitingForAdmission |
fails |
record() inserts at the tail again |
KeptAgainKeepsAnnouncementOrder |
fails |
A held lock counts as replayed |
HeldLockIsNotReplayed |
fails |
A requeue counts as replayed |
KeptAgainIsNotReplayed |
fails |
| End-of-replay recheck removed | KeptDuringReplayAreReplayedAfter |
fails |
Watcher clears watching after the replay call (the old defer) |
KeptDuringReplayAreReplayedAfter |
passes |
The last row is the first inline comment. The fix is correct, but nothing pins its ordering half. The second inline comment is a small observability gap. Both are non-blocking.
I read the subtreesHandler split with the live path in mind. Both lock outcomes still return ErrSubtreeExists, as before, and the admission-wait metric and span placement are unchanged.
SETTINGS_CONTEXT=test go test -race -tags testtxmetacache -count=1 ./services/subtreevalidation/... passed (440 s, race detector on). The skipped-subtree and replay tests passed five times under -race. I did not run the full repository suite.
oskarszoon
requested changes
Oct 27, 2026
oskarszoon
left a comment
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
Two issues still need changes. A live subtree announcement can acquire a fresh age after waiting for admission, allowing replay beyond the stated 30 minutes. The new handoff regression test also passes when the watcher ordering that prevents stranded entries is removed. The earlier replay fixes otherwise hold. This HEAD has no Go CI results and currently conflicts with main.
services/subtreevalidation/subtreeHandler.go:148: Capture the announcement time before the admission wait and carry it into the catchup queue. The currenttime.Now()starts the age limit after the wait, so a stale announcement can be fetched and its peer reported after pruning.services/subtreevalidation/skipped_subtrees_test.go:465: Make the handoff test hold the second watcher after its failed replay attempt while the first replay tries to start a replacement watcher. Restoring the old deferredwatchingclear currently leaves this test passing, despite reopening the lost-trigger race.
… once RUNNING again
While the node was not RUNNING, the subtree validator dropped every
peer-announced subtree and committed the Kafka offset, so nothing came back
for them. After a 6-minute catchup on scale-3 the next block arrived with 286
subtrees unvalidated and checking it read 300M transactions from the UTXO
store, which made that block slow enough to start the next catchup.
Subtrees announced while not RUNNING, or found not RUNNING after waiting for
an admission slot, are now kept (deduplicated, up to
subtreevalidation_skippedSubtreeBackfillMax, default 4096; 0 keeps none). The
first peer subtree handled in RUNNING replays them through subtreesHandler, so
the admission limit, the quorum lock and the RUNNING check all apply and a
subtree validated meanwhile is skipped. The live path is unchanged: no
conflicting txs are created and txmeta ingestion is not touched. New metric
teranode_subtreevalidation_skipped_peer_subtrees_total{outcome}.
Also re-indents four nested bullets of an existing CHANGELOG entry that the
markdown lint hook rejects.
… replay from the block notification too Review follow-ups on the backfill: - Keep subtrees only while CATCHINGBLOCKS. IDLE can last days; by then the peers may have pruned the subtrees and a replay that cannot fetch one reports the peer. - When full, drop the oldest entry instead of refusing the newest: the newest announcements are the ones the next block needs. Replay in announcement order, so parents start before the subtrees that spend from them. - Start the replay from the block notification as well, so a block arriving before any peer subtree message still sets it going. - Use at most half the peer-subtree admission slots, leaving the rest to live subtrees; stop launching on shutdown and don't log cancellations. - Metric outcomes: recorded (new entries only), evicted, replayed, replay_failed.
…UNNING, drop entries older than 30 minutes Catchup sends RUN only after its last block, and the blockchain client keeps FSM notifications for its own state cache, so the replay could only start on the next peer subtree message or block. On a quiet network that is the next block, validated with the kept subtrees still unvalidated. The first kept subtree now starts a watcher that waits for RUNNING and replays. The replay runs on the parent context rather than an errgroup context, so a watcher started by a subtree kept again during the replay outlives it. Entries older than 30 minutes are not replayed: a catchup followed by a long operator STOP would otherwise replay subtrees the peers may have pruned, and a failed fetch reports the peer.
…catchups - A replay that ends with subtrees kept meanwhile starts a new RUNNING watcher; the watcher that found the replay running had given up, leaving them queued. The watcher now clears its flag before replaying, so the new one can start. - The age limit is checked again once a replayed subtree has an admission slot, so a subtree that waited past 30 minutes is not fetched from a peer that may have pruned it. - A subtree kept again because the node is catching up again keeps its announcement time and goes back in announcement order; with the queue full it is the one dropped, not newer announcements. - replayed counts only validated or stored subtrees, not ones kept again or locked by another validation. New outcomes: expired, requeued.
…play drops - subtreesHandler takes the announcement time before the admission wait, so a subtree kept for replay is aged from its arrival. - A replayed subtree that finds the node IDLE is counted as dropped. - A test holds the second RUNNING watcher right after its replay call, so the watcher clearing its flag before the call is pinned: the old deferred clear strands the subtree kept during the replay.
icellan
force-pushed
the
fix/subtree-backfill-after-catchup
branch
from
October 27, 2026 15:54
ec8aa0b to
e218573
Compare
Contributor
Author
|
Addressed in 2a26227. The branch is rebased onto
Rebase conflicts:
|
oskarszoon
requested changes
Oct 27, 2026
oskarszoon
left a comment
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
The new watcher test, admission-wait timestamp, and IDLE outcome address the issues I raised on the previous commit, and current-head CI is green. One age-limit path remains: a received RUNNING message can wait in the bounded handler group before its timestamp starts, then enter catchup and be replayed more than 30 minutes after receipt. Please carry receipt time across that wait before merging.
services/subtreevalidation/subtreeHandler.go:102and:129:g.Gocan block while the handler group is full, butannouncedAtis set only inside the launched goroutine. I reproduced a 300 ms uncounted wait; the same sequence spanning the 30-minute cutoff can fetch from a peer after it has pruned the subtree and report that peer on a 404. Capture the time beforeg.Go, pass it intosubtreesHandler, and cover the saturated-group catchup path.
…the handler group wait subtreeMessageHandler takes the receipt time before g.Go, which blocks while the handler group is full, and passes it into subtreesHandler. A subtree that waited there and then met a catchup was aged from the end of that wait, so a replay could fetch it more than 30 minutes after receipt.
Contributor
Author
|
Fixed in be6d5a3.
The full |
|
oskarszoon
approved these changes
Oct 27, 2026
oskarszoon
left a comment
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
The handler-group receipt-time gap is fixed, and the new regression test fails when the timestamp is moved back after that wait. I found no remaining blocking issue at be6d5a33c.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
While the FSM is not RUNNING,
subtreeMessageHandlerreturned nil for every peer-announced subtree, and the offset was auto-committed. Nothing ever came back for them.On the scaling env on 2026-10-26, scale-3 sat in CATCHINGBLOCKS for 6 minutes (08:34:36–08:40:34). The next block (955) arrived with 286 subtrees the node had never validated.
CheckBlockSubtreesread their 300M transactions from Aerospike (txmeta misses peaked at 3.65M/s), and validation took 6m21s. The next block arrived while 955 was still in flight and went to catchup too.Change
skippedSubtrees).subtreevalidation_skippedSubtreeBackfillMaxsets the cap (default 4096; 0 keeps none). When full, the oldest is dropped.WaitForFSMtoTransitionToGivenState(RUNNING). Catchup sends RUN only after its last block, and the blockchain client keeps FSM notifications for its own state cache, so neither the subscription nor the next block would be early enough. A peer subtree handled in RUNNING and the block notification also start a replay.subtreesHandler, so the admission limit, the quorum lock (which skips a subtree a block already validated) and the RUNNING re-check all apply. It uses at most half ofsubtreevalidation_peerSubtreeConcurrency, leaving the rest to live subtrees.WithCreateConflicting. Txmeta ingestion is not touched.teranode_subtreevalidation_skipped_peer_subtrees_total{outcome}, with outcomes recorded, evicted, replayed and replay_failed.The CHANGELOG diff also re-indents four nested bullets of the #2037 entry (whitespace only), because markdownlint rejects the file otherwise.
Tests
Covered in
skipped_subtrees_test.go:Each one fails with its piece reverted.
go test -race ./services/subtreevalidation/...passes, andmake lintreports 0 issues../settingspasses exceptTestBlockAssemblyTxMapDirs_DefaultEmptyandTestSubtreeMmapDir_DefaultEmpty, which fail locally only because of mysettings_local.conf.Limits