fix(network-fabric): propagate ledger info failures instead of rescanning from genesis - #2119
Conversation
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
3dfa081 to
0ae75e6
Compare
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
|
@adecaro I flagging one change: the retry budget is now configurable instead of hardcoded. Nothing retries ScanBlock — FSC's ListenerManager.start() calls it once and only logs the error — so an unreadable height costs the channel its block-based finality until restart. The old 3 attempts / ~1.5s didn't survive a peer restart, trading the genesis rescan for a silently dead listener. Default is now 7 / ~31.5s, but that's a guess, not a measurement — hence: token.finality.delivery.ledgerInfoAttempts: 7 Do you have a better sense of how long a peer usually takes to come back? Thanks a million, |
f4597a3 to
3b40dd6
Compare
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
7e40614 to
7b7cbb4
Compare
…ning from genesis Signed-off-by: AkramBitar <akram@il.ibm.com>
7b7cbb4 to
da1a00c
Compare
Fixes #2058
The issue
ScanBlockasks the peer for the current ledger height to know where to resume the block scan. If that call failed, the error was only logged and the height stayed 0, so the scan silently restarted from genesis — rescanning the whole chain and replaying finality notifications for every past transaction, with no error for the caller.The fix
Retry the height lookup a few times, and if it still fails return the error instead of starting from block 0. A passing RPC hiccup is absorbed by the retry; a real outage is reported.
Before
One failed RPC → scan restarts from genesis, caller sees no error.
After
What an attempt does
One attempt is one
GetLedgerInfoRPC to the peer:(nil, nil), which used to nil-deref — remember the error and log it asattempt N/M.delay, then double it. A cancelled context ends the wait immediately and returns.Defaults: 7 attempts, 0.5s + 1s + 2s + 4s + 8s + 16s of waiting, ~31.5s worst case. Both bounds are configurable — see below.
If every attempt fails
errors.Isstill reaches it:failed to get ledger info after 7 attempt(s), refusing to rescan from genesis.ScanBlockFromis never called — no scan, so no genesis rescan and no duplicate finality notifications.ListenerManager.start(), logs the error, so block-delivery finality does not start for that manager. Finality is degraded, not lost: timed-out listeners are queried directly against the ledger (QueryByID→fetchTxs), and the recovery service re-registers listeners for pending transactions.GetLedgerInfoacross ~31.5s of retries, theScanBlockFromstream to that same peer would have failed too. The old code just failed later, after starting a full-chain rescan.Block 0 is still used in one case only: no ledger configured at all.
Notes
ListenerManager.start()callsScanBlockonce and only logs the error, so returning on the first hiccup would leave block-based finality dead until the process restarts. That is also why the budget has to outlast a peer restart rather than a dropped packet.*fabric.Delivery/*fabric.Ledgerreplaced by two small interfaces so the code is unit-testable. Field names unchanged, wiring untouched.Tests
delivery_test.go, 11 cases: height resumption, error propagated with no scan started, transient failure retried, attempt budget honoured, nil info, cancelled context, nil ledger → block 0, scan error propagated, plus the cancellation/nil-info sentinel overlap, last-failure-only classification, and the default attempt budget.config/config_test.go, 4 cases (the package had none): defaults, configured values, non-positive rejection, and the budget appearing inString().Against the old code 4 fail and one panics. All pass with the fix under
-count=2 -race;gofmt,go vet,golangci-lint,go build ./...clean.Configurable retry budget
The 7-attempt default is a judgement call, not a measurement: it is the smallest count on the existing doubling schedule that clears ~30s, chosen as a rough proxy for how long a peer restart takes. How long a peer actually takes to come back is a deployment property, so both bounds are exposed alongside the five existing
token.finality.delivery.*settings rather than baked in:Non-positive values fall back to the defaults: zero attempts would refuse every scan, and a non-positive delay would busy loop.
lookupis wired too, not justfinality— it constructs the sameDelivery, so leaving it out would have silently kept the old budget for lookup listeners. The exported provider constructors (NewDeliveryBasedFLMProvider,NewDeliveryBasedLLMProvider) take the values through a variadic option, so their existing signatures stay source-compatible for downstream callers.Error classification
Failures are classifiable with
errors.Is, so a caller never has to match on message text:ErrLedgerHeightUnavailableErrNoLedgerInfocontext.Canceled/context.DeadlineExceededErrLedgerHeightUnavailableaccompanies every failure to resolve the starting block, including a cancelled one, which makes it the single test for "no scan started" — and makes "an error without it came from the scan" true.The two refining sentinels are deliberately documented as not discriminators on their own:
ErrNoLedgerInforeports only the last observed failure, so a driver that violates the contract intermittently may surface an ordinary ledger error instead. Its absence is not proof the contract was kept.ErrLedgerHeightUnavailableto tell a cancelled height read from a cancelled scan.No in-tree caller inspects these sentinels yet — today the sole consumer logs the error and stops. Retry or restart around
ScanBlockbelongs to FSC'sevents.ListenerManagerand is out of scope here: the wider budget shrinks that window but does not close it.