fix(sync)!: stop generating the session percentages that crashed sync_status - #2643
fix(sync)!: stop generating the session percentages that crashed sync_status#2643zancas wants to merge 8 commits into
Conversation
…_status An interactive sync died with "attempt to subtract with overflow" at the session-percentage divisor in sync_status. The two operands come from different sources: the span is measured once at sync start, while the session baseline is a live sum over scanned ranges. SyncStatus already documents that the live count passes the frozen span when scanning continues into chain growth, so the subtraction underflows in ordinary operation. A debug build panicked and took the calling thread down; a release build wrapped the divisor and pinned every derived percentage at zero. The two session percentages are removed rather than guarded. They were the only fields whose computation could abort the process, and nothing read them: the CLI's progress surfaces take the raw counts and is_complete, and SyncResult carries the total percentage only. The session counts that survive now saturate, which covers the rarer case of a re-org truncating the scanned ranges below the baseline. Three tests span the blast radius. The narrowest pins the class of bug itself, asserting that the unguarded subtraction panics where overflow checks are on and wraps where they are off. The middle one drives sync_status over a wallet in the documented overshoot condition. The last renders a CLI prompt over such a wallet, which is the surface that actually failed: scan_progress guards sync_status with .ok(), and .ok() catches a typed error but never an unwind. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| } | ||
| // Saturating because the baseline is a stash taken at session start | ||
| // while the scanned count is a live sum over scan ranges: a truncation | ||
| // on re-org shrinks the live sum below the stash. |
There was a problem hiding this comment.
this is incorrect. the InitialSyncState is always re-calculated after re-org. the chain height is never updated until sync is called again, which triggers a new InitialSyncState to be created after the new chain height is updated. this also covers when scan ranges are truncated in "checked_wallet_height"
| let session_blocks_scanned = | ||
| total_blocks_scanned - sync_state.initial_sync_state.previously_scanned_blocks; | ||
| let mut percentage_session_blocks_scanned = ((session_blocks_scanned as f32 | ||
| / (total_blocks - sync_state.initial_sync_state.previously_scanned_blocks) as f32) |
There was a problem hiding this comment.
i dont understand the issue here. total blocks is based on last_known_chain_height (only updated once at sync start) and birthday (immutable). the initial sync state is calculated after "checked_wallet_height" (potentially re-org truncations) and re-calculated in the case of re-org detection during scanning. this means the scan ranges will always align with the last_known_chain_height and no scan ranges will ever span above this height or below the birthday. this means that previously_scanned_blocks must always be lower than total_blocks_scanned
There was a problem hiding this comment.
there is one exception that i am looking into here. as the wallet lock is now given back between checked_wallet_hieght and set_initial_state at sync initialization. if sync_status is called after a re-org truncation has just occured but before it can re-calculate the initial sync state, it may result in previously scanned blocks to become larger than total blocks
| .previously_scanned_ironwood_outputs, | ||
| ); | ||
| let mut percentage_session_outputs_scanned = ((session_outputs_scanned as f32 | ||
| / (total_outputs - previously_scanned_outputs) as f32) |
There was a problem hiding this comment.
the same logic as above applies to outputs too
| /// `SyncStatus::total_outputs_scanned` documents that a live scanned | ||
| /// count passes the span frozen at sync start "when scanning continues | ||
| /// past them into chain growth". Any code that subtracts one from the | ||
| /// other therefore meets an unsigned underflow in ordinary operation. |
There was a problem hiding this comment.
this is incorrect. the chain does not grow during scanning. the chain height is only updated once in sync initialization and does not scan past the "last_known_chain_height" until sync is called again
| let mut wallet = client.wallet().write().await; | ||
| wallet | ||
| .sync_state | ||
| .set_session_baseline_for_test(BlockHeight::from_u32(1_000), 70); |
There was a problem hiding this comment.
this test isnt useful IMO. if we are going to add tests they should be verifying that the is no edge case where the scan range span ever exceeds the initial sync state bounds
|
i don't believe the hypothesis is correct although my investigation has lead me to another potential cause which i described in one of my comments i think the correct course of action is to still add saturating subs for safety but also hold the wallet lock between |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
As Far As I Know, we're all in agreement with this move: So that perspectives are in their own mod. |
|
The perspectives mod has now merged. |
… rig The nym-feature CI job lints test targets and rejected the three RT.block_on calls in scan_progress_blast_radius under the ADR 0030 disallowed-methods rule. The rig and its test now carry the same explicit allowance and crossing justification the file's config-builder helper established. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
An interactive sync died with
attempt to subtract with overflowatpepper-sync/src/sync.rs, in the session-percentage divisor ofsync_status.Root cause
The two operands come from different sources. The span is measured once at sync start. The session baseline is a live sum over scanned ranges.
SyncStatus::total_outputs_scannedalready documents the consequence: the live count "May exceedtotal_outputs, whose tree bounds are fixed at sync start, when scanning continues past them into chain growth." The subtraction therefore underflows in ordinary operation.The failure differs by build profile. A debug build panics and takes the calling thread down. A release build wraps the divisor to about 1.8e19 and pins every derived percentage at zero, which reads to a user as a sync that never progresses.
The fix is removal, not a guard
The two session percentages are deleted. They were the only fields whose computation could abort the process, and no consumer read them. The CLI's progress surfaces take
total_outputs_scanned,total_outputs, andis_complete().SyncResultcarries the total percentage only. A consumer that wants a session percentage can compute it from the session and total counts the struct still reports.The surviving session counts now saturate. That covers a different and rarer cause: a re-org truncating the scanned ranges below the baseline.
Blast radius, and the three tests that span it
scan_progressin zingo-cli already guardssync_statuswith.ok(). That catches a typed error and never an unwind, so the panic escaped intoprompt_indicator, reached on every REPL prompt, and intoawait_sync_narrated. A cosmetic statistic killed the whole session, and the session never read it.sync_statusover a wallet in the documented overshoot condition and asserts the exact ratio is reported unreduced.Breaking change
SyncStatus::percentage_session_blocks_scannedandSyncStatus::percentage_session_outputs_scannedare removed, along with their entries in the JSON rendering.zingolib::syncre-exportssync_status, so out-of-repo consumers can see these fields; zingo-mobile and zingo-pc should be checked before release. Recorded in the pepper-sync CHANGELOG.SyncState::set_session_baseline_for_testis added behindtest-featuresso a consumer can regression-test its own progress surface against the overshoot.A question this raises about where summary statistics belong
Removing these two fields was easy precisely because nothing consumed them, which suggests the sync engine may be the wrong home for summary statistics generally.
sync_statuscurrently computes percentages eagerly for every caller, and its three callers want three different things:sync statuswants the percentages,SyncResultwants the session counts, and the CLI's progress path wants the raw totals. All three pay for all three on every call.A percentage is a rendering of a measurement, not a measurement. The engine could report only what it measures, leaving the derived statistics to a domain that owns presentation: a consumer, or the not-yet-landed
zingolib::perspectivemodule, which exists for exactly this kind of consumer-facing projection. That would also mean a caller who never asks for a statistic can never be harmed by one, which is the general form of the bug fixed here.Verified: pepper-sync 104/104, zingo-cli 219/219, libtonode-tests compile, clippy and fmt clean.
🤖 Generated with Claude Code