Conversation
8407e32 to
395b784
Compare
Keep synchronization scenarios with their correction in ergoplatform#2511. Reuse the spendable sorting fixture already present in ergoplatform#2480.
395b784 to
c6e5669
Compare
Move the exact UtxoStateNodesSyncSpec correction from ergoplatform#2511 into the shared test-support prerequisite. Preserve topology, height and the deadline while observing eventual agreement of selected headers; retained sibling IDs are not selected-chain disagreement.
|
Prepared with Claude (Anthropic), model Claude Opus 5, building on earlier test-setup work with Claude Opus 4.8. @a-shannon, this is independent multi-node evidence for the case the description explicitly scopes out ("… not full-block network convergence …"). A follower on the lighter of two static forks, connected only to a peer that holds the heavier fork, stayed on the lighter fork for the whole 300 s window in 9 of 13 runs on a v6.0.5 release jar. On this head ( Setup. Four node processes run with
Across these runs, fork heights were 13–19, L held 2–23 blocks above the fork point, and Y led X by 9–36 in chain score. Each run is a single 300 s observation, and the counts are descriptive. Not counted: one early release run in which L stopped below height 16 (too low for any of its sync samples to reach the shared prefix), two release runs in which S stalled before it was ahead by 8 (inconclusive), and one further #2511 switch whose log was not kept. Reverse control on #2511, with X heavier than Y by 8–10: in 4 of 4 runs L stayed on X for the whole window, and S switched to X within 5 s. The warning is emitted when the continuation for a younger peer is empty. On the release jar it matched the outcome in all 13 runs: it was present in every run where L stayed on X and absent in every run where L switched. In the runs where L stayed on X, L and S were connected for the whole window, but L never received a header from S. In one of these runs L received two block sections from S (an extension and a transactions section); in the others it received nothing. The runs where L stayed on X are consistent with the empty continuation that the new history cases reproduce. The release runs where L switched are consistent with v6.0.5 sometimes sending a summary that does reach the shared prefix. Which summary form was sent was not captured. On #2511, L received Y's headers and block sections, rolled back its UTXO state once, and ended with the same best full block as S. Build. Reproduce. The script below is self-contained. It needs the following:
No Docker is required, and no root either, except The exact script below (sha256
|
jozanek
left a comment
There was a problem hiding this comment.
Thanks for this, and for stating the attribution limits up front rather than burying them. Three things I'd value your view on.
The cache hunks may be the whole mechanism. getV2SyncInfo(hr, full = false) at :741 fires after every header batch and, under a height-only key, poisoned the full = true broadcast with a one-header summary — disabling the 16/128/512 ladder during active sync. That explains the empty continuations, and it isn't in the description.
The evidence may not reach the genesis anchor. farAwayFromBeingSynced needs fb.height < headersHeight − 128, so at ≤70 blocks the ToDownloadProcessor change never fires in those runs. Could you run the script against a v6.0.5 jar carrying only the cache-key hunks? If L switches ~8/8, that settles it.
Nothing here looks blocked by #2535. All three production files auto-merge onto master; the four conflicts are in DeepRollBackSpec, CandidateGeneratorSpec and add/add on ConvergenceObservations{,Spec} — the latter two already upstream via #2512. git merge-tree --write-tree origin/master d62c945a4 is clean. Would you consider retargeting at that chain?
Also worth citing #1815 (open since 2022), with a note that this doesn't fix it. I'd be glad to see the cache change land on its own straight away.
| val headers = offsets.flatMap(offset => bestHeaderAtHeight(h - offset)) | ||
| // Keep a shared starting point in full summaries when it is available locally. | ||
| val genesisAnchor = if (full) { | ||
| bestHeaderAtHeight(GenesisHeight).filterNot(genesis => headers.exists(_.id == genesis.id)).toSeq |
There was a problem hiding this comment.
Genesis is picked only when no other rung matched — a fork rooted >512 back. continuationIdsV2 then answers heights 2..400, which can't reach a divergence at 1.5M, so it gets re-sent every round. And a fork that deep is fatally rejected by hdrTooOld (rule 209, keepVersions=200) on the peer we're serving.
Would you be open to clamping instead? offsets.map(o => Math.max(GenesisHeight, h - o)).distinct gives {50,34,1} at h=50 — the same fix your new tests cover — and today's exact four samples on mainnet, at no added cost.
| syncInfoV2CacheByHeadersHeight = Some(headersHeight -> v2SyncInfo) | ||
| v2SyncInfo | ||
| } | ||
| syncInfoV2Cache.getOrElseUpdate(history.bestHeaderIdOpt, full)(history.syncInfoV2(full)) |
There was a problem hiding this comment.
This looks like the load-bearing fix. getV2SyncInfo(hr, full = false) at :741 runs after every header batch and, under the old height-only key, stored a one-header summary that was then broadcast to the full = true callers at :358 and :492 — disabling the 16/128/512 ladder during active sync.
That mechanism isn't in the description, and it's the strongest thing here. Would you lead with it, and consider landing these three hunks alone? They merge onto master cleanly.
| val commonAncestor = if (isInBestChain(fb.id)) { | ||
| None | ||
| } else { | ||
| val parentSteps = Math.max(0L, nodeSettings.keepVersions.toLong) |
There was a problem hiding this comment.
keepVersions is the right concept here — FullBlockProcessor:105,133 and rule 209 already use it as the fork-root depth bound, so no objection to the value.
What I'd ask for is a name. Reached for directly, an operator raising it to 10000 for deep rollback turns a storage setting into a 10192-height scheduling walk — and the near-tip branch below hardcodes 100 for the same question.
Would a def maxRollbackDepth on the trait, used at all four sites, work for you?
| .getOrElse(fb.height + 1) | ||
| // Extending the scan backward must preserve forward progress beyond the existing full-chain tip. | ||
| val maxHeight = Math.min(fb.height.toLong + FullBlocksToDownloadAhead, Int.MaxValue.toLong).toInt | ||
| continuation(fromHeight, Map.empty, maxHeight) |
There was a problem hiding this comment.
Couldn't settle this by reading: downloadRequired filters in-flight ids, so once this window is fully requested the accumulator stays empty and the loop re-walks 21..252 on every call — on the synchronizer actor thread, at up to 20 calls/s (50 ms floor, ErgoNodeViewSynchronizer:1403).
isInBestChain(fb.id) gates it and continuation early-exits, so this may be nothing. Do you have a measurement? If not, would resuming from the last scanned height beat re-walking?
| object ErgoNodeViewSynchronizer { | ||
|
|
||
| /** Single-entry cache owned by the synchronizer actor. */ | ||
| private[network] final class SyncInfoV2Cache { |
There was a problem hiding this comment.
syncInfoV1CacheByHeadersHeight two lines up says the same thing as a tuple, and it keeps the identical stale-tip weakness — a stale 1000-id list to V1 peers after a same-height reorg.
The class does buy the SyncInfoV2CacheSpec seam, which I think justifies it. If it stays, would you route getV1SyncInfo through it too? Two cache shapes with two correctness properties in one file is the part I'd like to avoid.
| // add new modifiers to download to accumulator | ||
| val newAcc = toDownload.foldLeft(acc) { case (newAcc, (mType, mId)) => newAcc.adjust(mType)(_.fold(Vector(mId))(_ :+ mId)) } | ||
| continuation(height + 1, newAcc, maxHeight) | ||
| if (height == maxHeight) newAcc else continuation(height + 1, newAcc, maxHeight) |
There was a problem hiding this comment.
This took me a moment to place — an Int-overflow guard for the branches passing maxHeight = Int.MaxValue, where height + 1 wraps negative and the height > maxHeight check never fires. Correct, and otherwise behaviour-equivalent.
It's unrelated to the three changes the description covers, and unmentioned there. Could it get a sentence in the body, or land as its own one-liner?
A V2 sync cache keyed only by header height can return a reduced one-header summary to a later full-summary request at the same height. This happens when the reduced request populates the cache first; header delivery and history application are asynchronous. Keying by selected header ID and requested mode preserves the full sampling ladder and also refreshes summaries after a same-height tip replacement.
Review follow-up, 21 September: prioritize the cache-only unit on current master, separately from genesis sampling and download scheduling. Source review identifies the synchronizer cache hunks and their direct spec as a standalone unit. A merge-tree check of portable G against master
23aabeadis conflict-free; this is source composition evidence, not a compiled or rebased candidate. No full #2535 merge or #2501 merge is required to begin that focused work. The published branch remains unchanged pending this restructuring.The external short-chain experiments below compare the cumulative PR with v6.0.5. They do not isolate the cache change or exercise the far-behind download branch under the stated chain conditions. A cache-only execution remains an open attribution check. The requested clamped sampling alternative, explicit rollback-depth naming, scheduling scan measurement, V1 stale-tip cache handling and terminal-height guard description are recorded for their appropriate scope. None is claimed implemented by this status update. The related #1815 remains open; this PR does not claim to resolve it.
Full V2 synchronization summaries retain the locally available genesis header alongside recent samples. The cache is keyed by selected header ID and requested mode, so full requests cannot reuse reduced summaries and a replacement tip refreshes it even at the same height. Tip order, deduplication, reduced and empty summaries, wire encoding and validation rules are preserved.
During active synchronization, periodic body downloads can extend backward to a confirmed common ancestor within the configured search bound. The forward ceiling remains anchored to the best full block, so finding an older retained ancestor does not shorten forward progress. Pruning floors, filtering and request counts are preserved; arithmetic is bounded at the terminal height. A truncated search does not invent an ancestor, and rollback remains limited by retained state versions.
Current head:
8c00df3a9850660d2cfd13f5c23a99c5b5b070aa, targeting master. The latest one-file increment reuses the shared startup-reader fixture without changing production. The preceding history increment added two real-history regressions. All three production files remain unchanged from reviewed revisionc6e566900e5b12aeece8f6f3aa78d778da91e4e7.The synthetic-history fixture now owns its startup history/mempool responses, preventing a late empty real-holder response from replacing its populated readers. State/vault handling, all exact full-summary expectations and their deadlines remain intact. This is the exact shared reader increment R, also reused by #2433. Normal compilation and 47/47 tests passed on this actual master composition: synchronizer 26, cache 4, history 6 and scheduling 11. Independent composition review found no blockers.
The new cases construct and append actual header histories sharing prefixes of 3 and 18 headers, with fork tips 9/13 and 249/70. In both directions the old sparse summary has no recognizable common header and yields an empty continuation; the full summary with genesis returns the exact expected chain continuation, including the divergent section. Serializer round trips are checked. All six history tests pass; restoring only the old summary implementation makes both added cases fail, and restoring this correction passes both. These prove a header-continuation defect and its correction, not full-block network convergence or exclusive attribution of an earlier CI timeout.
Additional external evidence, 21 September: cafebedouin reports four-node devnet experiments on this exact head: the lighter-fork follower switched in 8/8 runs within 6 seconds, whereas the v6.0.5 release stayed on the lighter fork for the 300-second observation in 9/13 runs. Reverse controls passed 4/4. The report describes matching best full blocks and a UTXO rollback, extending beyond the local header-continuation fixtures above. These are contributor-reported measurements, not independently rerun here. The comparison uses the release jar versus this cumulative PR, and the runs span multiple script revisions; it does not isolate a single change or measure production incidence.
Shared test support remains owned by #2535. Review R once, then the portable seven-file synchronization increment G, which this PR owns. Both exact commits are ancestors of the current head and of #2535's release composition. This order requires no full #2535 merge before #2511. The master branch also retains historical support
5eedfdc91d7a515f66822643e730b4ca9b3197df; the older cumulative increment includes diagnostics. Upstream's full diff remains cumulative until prerequisites are integrated. Maintainers select release integration or a focused port of G.For release qualification, fork integration PR #3 at
168bd7e16860d3e3a6fbae80a000a4478f19c007passed all eight CI jobs. Its tree exactly equals #2535'sefbd1e917fe74f0f680a2e3c9e4d93595a59b06a, including #2549's separate cache wake-up prerequisite. The seven G source/direct-test blobs match this PR exactly; 49 local checks include the two wake-up regressions. Integration fixtures and deadlines are unchanged. Historical run 34724671004 covered the earlier d72/7d release composition, including 51/51 integration tests and one ignored case. This PR remains the synchronization correction's owner; native master CI remains separate.Earlier review increments remain available:
Native CI for
8c00df3a9passed all eight jobs, including node and integration. The preceding run at40c88de93passed integration, but wallet Scala 2.11 failed during Java setup before sbt and matrix siblings were cancelled. The new source update supersedes that old-head rerun request. The earlier 8/8 CI applies to1eaec0059; its 32 pure observation tests and integration compilation remain evidence for unchanged inputs. Earlier 8/8 CI and ForkResolution timeout likewise retain their original scope.The scheduler correction requested on #2508 remains here. Missing-parent handling remains with #2433 and #2312; pruning admissibility remains with #2359. Candidate expiry #2478 targets v6.0.7 separately. Wallet/support consumers do not require this production increment merely to acquire shared test fixtures. Review order, reviewed pins and current CI are tracked in #2533.