Fixes for nipopows + utxo set snapshot bootstrapping

[kushti]

Close #2464 : nipopows + utxo set snapshot bootstrapping fixes for p2p layer done in this PR

Also contains #2468

[a-shannon]

Thanks @kushti. Reviewed 85044dc444f18235f2e33393acf005e3a6db4979. The new restart change addresses the lost in-memory synchronization flag for NiPoPoW + UTXO bootstrap. I found one blocking regression in the cumulative PR, separate from that latest change:

UTXO snapshot bootstrap without NiPoPoW loses its synchronization transition. With utxoBootstrap=true, nipopowBootstrap=false, verifyTransactions=true and blocksToKeep=-1, the new early return in ToDownloadProcessor.toDownload bypasses the fresh-header branch that sets isHeadersChainSynced. Consequently, nextModifiersToDownload keeps returning empty before it can request snapshot information. The new startup recovery condition requires NiPoPoW, so it cannot recover this configuration either.

Please preserve ordinary header-synchronization detection while suppressing premature block-section downloads. The focused regression should use this configuration, deliver headers through the normal append path until a fresh tip is reached, and assert that snapshot information is requested while block sections remain suppressed. It should not call setHeadersChainSynced() itself: the current tests do so and therefore bypass the missing transition. This guard is also carried by #2424, so fixing it here will let the descendant inherit the correction.

Two focused improvements would make the restart evidence reproducible:

• In UtxoBootstrapRestartSpecification, close the original history storage before reopening it, and assert the next snapshot request as well as the flag. StoreRegistry otherwise reuses the already-open underlying DB.
• Extend the functional bootstrap test to observe partial chunk download, restart with the same data directory, and require subsequent snapshot completion and full-block progress. The committed version currently only starts once and waits for synchronization.

The current 8/8 CI checks are green, but the integration job runs it:test; the separate bootstrap workflow runs it2 on release/manual dispatch. Your successful restart run may cover this separately. My feedback above is based on source review and visible CI; I have not independently rerun the mainnet scenario.

[jozanek]

What I understood this to be for

With blocksToKeep = 1440, the first fresh header after the proof reaches updateBestFullBlock, which writes minimalFullBlockHeight — and since isUtxoSnapshotApplied is derived from that height (UtxoSetSnapshotProcessor.scala:50), it flips true with no snapshot applied. The snapshot request is then skipped forever and the node downloads full blocks from 1.8M against a genesis state, which matches the report.

If that's right, the five changes are one mechanism rather than a bundle: the suppression keeps updateBestFullBlock from poisoning the flag the snapshot path keys off, and setHeadersChainSynced() is its side-effect-free replacement. Worth stating in the body — it's what makes the shape defensible, and reviewers have to re-derive it today.

Verdict: right change, wrong shape.

The blocking concern

@a-shannon's regression traces end to end, and I reached it independently. With utxoBootstrap=true, nipopowBootstrap=false — permitted by ErgoSettingsReader.scala:191-194, documented in README — the early return at ToDownloadProcessor.scala:114 precedes L122-124, the only remaining writer of the flag. Neither new setter covers that config, so the node stalls permanently and a restart doesn't clear it. Two possible fixes inline. #2424 is rebased on this branch and carries the same guard, so fixing it here propagates.

Questions I'm holding

1. Does this close #2464, or a neighbour? The issue reproduces from a wiped data dir; UtxoBootstrapRestartSpecification's comment describes a restart mid-download. Untouched by the diff: the recurring Extension is empty while comparison is younger, headersHeight reporting lower than a just-applied proof's tip, the 52 → 11 peer collapse, and the quorum-without-fallback the reporter hypothesised. Would you keep it open pending a mainnet run?

2. Regression, or never worked end to end? git diff v5.0.13..v6.1.3 over the two processor files shows only import moves — this deadlock has been identical since 2023, it2 has never run in CI, and #1995 is still open. Your issue comment mentions instability "after some changes in p2p layer"; naming the commit would say whether a second regression is still out there.

3. Why does ErgoHistory.scala:303 require nipopowBootstrap when the other guards don't? Nothing explains it, and that asymmetry is what makes the blocker unrecoverable.

One note without a line to sit on

Flipping the flag early moves rejection of a section below minimalFullBlockHeight from rule 304 (recoverable, no penalty) to rule 305 (fatal, penalizeMisbehavingPeer). Unsolicited modifiers are filtered at :1261, so I don't think it's reachable today — but the flag now gates a download policy and a peer-penalising validation rule at once. Free while minimalFullBlockHeight == 1; not once it can be raised.

[jozanek]

release: published fires after the release ships, so a red run can't stop a bad one — and this is exactly the job that would catch #2464. Thanks for adding it; it's the first CI to touch this path.

Also no timeout-minutes: the awaits total 6h (1h plus the 5h at the it2 spec L57) against a 360-minute default, so a stall gets cancelled rather than reported. Would a nightly schedule: plus a timeout fit?

[jozanek]

Following up. This matters a little more after 9dca805: the it2 spec now carries the restart scenario, and it2 still is not in ci.yml, so nothing exercised it before the green checks on this head.

A nightly schedule: plus a timeout-minutes would turn it into a signal you can act on. If you would rather keep it manual-dispatch only for now, say so and I will resolve.

[kushti]

Checking on release is ok for now maybe, more frequent checks could be too heavy . On my laptop this kind of bootstrap takes ~15 min, so default timeout should be ok

[jozanek]

At debug level, a bootstrap suppressing invs looks identical to a healthy one at default logging — the same blind spot that made #2464 take two clean-state reproductions to diagnose.

Would one INFO heartbeat naming the phase and what's awaited (snapshot info / manifest / N of M chunks) be worth adding here?

[jozanek]

Partly addressed — the new log.info in ToDownloadProcessor covers the sync transition, which was the part I most wanted. Thank you.

What is still missing is the bootstrap phase itself: this line stays at debug, so a node waiting on manifests or chunks still looks idle at default logging. One INFO line naming the phase would close it. Not a blocker — I will resolve if you would rather hold the log volume down.

[a-shannon]

@jozanek, @kushti_ru, thanks for the detailed review. The ordinary-header blocker and the two test issues are addressed in #2545, targeting this PR's i2464 branch.

The fix calls setHeadersChainSynced() when an ordinary fresh header arrives, while preserving the retention floor and suppressing premature block-section downloads. The regressions drive normal header append without manually setting the flag, and close the original history store before reopening it and checking the next snapshot request. A mutation using updateBestFullBlock fails the retention test. Nine focused tests pass, with 8/8 CI on the current head.

The existing restart predicate is unchanged in this follow-up. A persisted header alone does not establish freshness; ordinary mode resumes discovery when the next fresh header arrives. Immediate recovery without another header remains outside this evidence.

The same shared commits are included in #2424 and the combined wallet review, both also at 8/8. The tracker links the review increments.

I would keep #2464 open pending the separate end-to-end bootstrap/restart validation. This follow-up does not resolve the other trust/configuration, workflow, observability or state-recreation coverage questions, or identify a separate P2P regression commit.

[jozanek]

Re-review at 9dca805

The blocker is fixed, and fixed well. setHeadersChainSynced() inside the bootstrap branch, not updateBestFullBlock — so minimalFullBlockHeight stays at GenesisHeight and isUtxoSnapshotApplied can't flip early. I traced toDownload → nextModifiersToDownload L94 → requestSnapshotsInfo() and it reaches the snapshot request for utxoBootstrap=true, nipopowBootstrap=false. headerChainDiff and chainSettings are in scope, verifyTransactions=false still short-circuits first, and the flag write is in-memory only.

Three regression tests now fail on the parent commit — the new UtxoBootstrapNoNipopowSyncSpecification property 1, the parameterised restart spec, and the corrected UtxoBootstrapToDownloadSpecification. None of them sets the flag by hand. The restart spec's close() does evict from StoreRegistry, so the reopen is a real cold one. Thank you also for putting the minimalFullBlockHeight derivation into the code comments — that was the part reviewers had to re-derive.

From the previous review

Addressed: the blocker, the guard asymmetry, both test issues, and the it2 restart scenario.

Still open, all outside the blocker's path: the genesisId trust gate at ErgoNodeViewHolder.scala:322; digest mode falling through to the state-wiping branch at :594; the unconditional closeStorage() still unnamed in the description; the bootstrap-mainnet.yml post-release trigger and missing timeout; NodeViewTestConfig.allConfigs still carrying no bootstrap configuration. Also still open: whether this actually closes #2464 — the body still says it does, while @a-shannon and I both read the evidence as not yet establishing it.

One correction to my own earlier advice

I suggested dropping the nipopowBootstrap conjunct from ErgoHistory, and you did. @a-shannon declined that same change, and on re-examination their reasoning is the more precise one: with the toDownload fix in place, the startup set buys about one block interval rather than correctness, and it now marks a stale or partial header chain as synced. A freshness predicate satisfies both readings — details inline.

Not blocking

The new findings are all minor: one test assertion that is vacuous at the current constants, the staleness point above, a read-after-close in the restart spec, and a wall-clock window in the it2 spec. Two coverage notes without a line to sit on: the it2 spec keeps nipopowBootstrap = true, so the configuration that produced the blocker has no integration coverage — and it2 isn't in ci.yml, so this spec wasn't exercised by the green CI on this head.

Approving on the blocker being resolved and the regression tests that now cover it. That approval is scoped to this commit: the genesisId gate and digest mode from the previous round are still unanswered, and I'd like them addressed or explicitly ruled out of scope — but they predate this commit and needn't hold it.

[jozanek]

This predicate and its log line are now identical to the ones at L132-135. Two copies of the freshness rule can drift apart later.

Would extracting a small private helper — markHeadersSyncedIfFresh(header) — used by both branches be worth it here?

[jozanek]

The helper landed in 03aad1f4, thanks — and I owe you one here. Extracting it was my suggestion and I filed it as a nit, which was wrong: the two branches were only nearly identical, differing exactly in updateBestFullBlock vs setHeadersChainSynced. That is the difference 03aad1f4 flattened and cb40680 restored.

What is left is the updateBestBlock = false default: the pending-snapshot call site still takes it implicitly, so the value you get by saying nothing is the one that broke pruned bootstrap. @a-shannon suggested dropping the default so both choices are explicit, which looks right to me. Would making it a required parameter work? Keeping this thread open for that.

[a-shannon]

Update, 17 September: cb406808 applies the proposed ordinary-path correction in #2496, preserving the pending-snapshot path. The exact increment is one line. CI for the corrected head is in progress; validation of the correction remains pending. The finding and failed tests below refer to the previous head 03aad1f4.

@kushti_ru, the changes in #2496 address the ordinary-header snapshot transition and improve the restart coverage, but the helper extraction in 03aad1f4 introduced a regression in normal pruned bootstrap.

Both calls to markHeadersSyncedIfFresh use the default updateBestBlock = false. That is necessary while a UTXO snapshot is pending, but the ordinary branch previously called updateBestFullBlock(header). It now leaves minimalFullBlockHeight at genesis for a fresh pruned node. Download planning starts too early, and a supplied suffix cannot initialize the full-block chain at its intended starting height.

The current CI reproduces both consequences:

• Node job: 819 passed, 2 failed, 2 ignored. Both failures are PrunedNodeViewHolderSpec suffix-bootstrap cases, with the state version remaining zero.
• Integration job: 12 passed, 1 failed. PrunedDigestNodeSyncSpec fails at line 71 because the digest node applies a block below the expected pruning start, before the final pruning assertion.

Please pass updateBestBlock = true in the ordinary branch and keep it false in the pending-snapshot branch. Removing the default would make both choices explicit. The existing failing tests already cover the regression; the two snapshot-discovery properties should remain green too. I have not executed that proposed correction.

This should be fixed before merging. It affects pruned bootstrap, not a demonstrated consensus fault. No separate PR is needed. The earlier snapshot-discovery fix also overlaps #2545; its remaining test coverage can be reconciled after this branch is stable.