fix(network-fabric): widen the ledger-height retry budget and correct its docs

Three corrections to the starting-block handling added earlier in this branch.

The retry budget was too short to serve its purpose. Nothing retries ScanBlock —
FSC's events.ListenerManager calls it once from a goroutine that only logs the
result — so a height that stays unreadable costs the channel its block-based
finality until the process restarts. At 3 attempts over ~1.5s a peer restart
outlasted the retries, trading a genesis rescan for a silently dead listener.
The default is now 7 attempts over ~31.5s.

ErrLedgerHeightUnavailable is now joined on the cancellation path as well, not
only on budget exhaustion. It therefore accompanies every failure to resolve the
starting block, which makes it a single reliable test for "no scan started" and
makes "an error without it came from the scan" true.

The documented error taxonomy claimed more than it delivered: a context error
does not identify which phase ended, since the same context governs the scan,
and ErrNoLedgerInfo reports only the last observed attempt. Both godoc and
docs/services/network-fabric.md now state those limits, and the doc no longer
recommends a fallback to query-based finality that no caller can perform.

Adds three tests covering the cancellation/nil-info overlap, the last-failure
-only classification, and the default attempt budget.

Signed-off-by: AkramBitar <akram@il.ibm.com>

Author: AkramBitar

docs/services/network-fabric.md

@@ -299,27 +299,50 @@ sequenceDiagram
 The scan resumes at the peer's current ledger height, read via `GetLedgerInfo`
 ([`delivery.go`](../../token/services/network/fabric/finality/delivery.go)). Because that
 one RPC decides the starting block, a failure is retried with an exponentially growing
-delay (`LedgerInfoAttempts`, `LedgerInfoRetryDelay`; 3 attempts over ~1.5s by default) and
-aborts early if the context is cancelled.
+delay — 7 attempts spanning ~31.5s, aborting early if the context is cancelled.
+
+That budget is deliberately long. Nothing retries `ScanBlock`: FSC's
+`events.ListenerManager` calls it once from a goroutine that only logs the result, so an
+error escaping the retry loop leaves the channel with **no block-based finality until the
+process restarts**. The retries therefore have to outlast a peer restart, not merely a
+dropped packet.
 
 If the height is still unavailable after the last attempt, `ScanBlock` returns the error
 rather than defaulting to block 0. Starting at genesis would rescan the entire chain and
 replay finality notifications for every historical transaction, while the caller would have
 no way to tell a transient RPC failure from a genuinely fresh chain. Block 0 is used only
 when no ledger is configured at all.
 
-The returned error is classifiable with `errors.Is`, so a caller does not have to match on
-its message:
-
-| Sentinel | Meaning | Reaction |
-| --- | --- | --- |
-| `finality.ErrLedgerHeightUnavailable` | the height could not be read within the attempt budget, so no scan started | block-delivery finality is degraded; fall back to query-based finality and retry later |
-| `finality.ErrNoLedgerInfo` | the ledger returned neither info nor an error — a driver contract violation | will not self-heal; report as a bug |
-| `context.Canceled` / `context.DeadlineExceeded` | the context ended the retries | shutdown, not a failure |
+The budget is tunable through the `LedgerInfoAttempts` and `LedgerInfoRetryDelay` fields of
+`finality.Delivery`. These are Go fields with no YAML counterpart — neither construction site
+sets them, so deployments always run the defaults and only tests vary them.
 
-`ErrNoLedgerInfo` is always accompanied by `ErrLedgerHeightUnavailable`, and the underlying
-ledger error is preserved in every case. An error matching none of these comes from the
-block scan itself rather than from resolving the starting block.
+The returned error is classifiable with `errors.Is`, so a caller does not have to match on
+its message. `ErrLedgerHeightUnavailable` is the single test for "the starting block could
+not be resolved, so no scan started" — it accompanies every such failure, including a
+cancelled one:
+
+| Sentinel | Meaning |
+| --- | --- |
+| `finality.ErrLedgerHeightUnavailable` | the height could not be read, so no scan started |
+| `finality.ErrNoLedgerInfo` | the **last** attempt saw the ledger return neither info nor an error — a driver contract violation |
+| `context.Canceled` / `context.DeadlineExceeded` | a wait between attempts was cut short, or the scan itself was cancelled |
+
+The two refining sentinels are not discriminators on their own. `ErrNoLedgerInfo` reports
+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.
+And because the same context governs the scan, a context error alone does not say which
+phase ended; pair it with `ErrLedgerHeightUnavailable` to tell a cancelled height read from
+a cancelled scan.
+
+The underlying ledger error is preserved in every case. An error that does not match
+`ErrLedgerHeightUnavailable` comes from the block scan itself rather than from resolving the
+starting block.
+
+There is no in-tree caller that inspects these sentinels yet — today the sole consumer logs
+the error and stops. Classifying the failure is what a future caller needs to react
+(fall back to query-based finality, retry, or restart the manager) rather than a description
+of current behaviour.
 
 ### Notification Mode
 

token/services/network/fabric/finality/delivery.go

@@ -19,9 +19,14 @@ const (
 	// defaultLedgerInfoAttempts is how many times ScanBlock asks the ledger for
 	// its current height before giving up. A transient RPC hiccup must not
 	// decide the starting block, so the call is retried rather than trusted once.
-	defaultLedgerInfoAttempts = 3
+	//
+	// The budget is sized to outlast a peer restart, not just a dropped packet:
+	// nothing retries ScanBlock (see the ledgerHeight comment), so a failure that
+	// escapes here disables block-based finality until the process restarts.
+	defaultLedgerInfoAttempts = 7
 	// defaultLedgerInfoRetryDelay is the pause before the first retry. It doubles
-	// on each further attempt, so the defaults span ~1.5s in total.
+	// on each further attempt, so the defaults wait ~31.5s in total (0.5s + 1s +
+	// 2s + 4s + 8s + 16s over six retries).
 	defaultLedgerInfoRetryDelay = 500 * time.Millisecond
 )
 
@@ -31,10 +36,11 @@ var (
 	// than a transient failure: retrying it is not expected to help.
 	ErrNoLedgerInfo = errors.New("ledger returned no info")
 	// ErrLedgerHeightUnavailable reports that the current ledger height could not
-	// be read within the attempt budget, so the block scan was refused instead of
-	// restarted from genesis. It distinguishes a failure to decide where to scan
-	// from a failure of the scan itself; the last observed cause remains
-	// inspectable with errors.Is.
+	// be read, so the block scan was refused instead of restarted from genesis.
+	// Every failure to resolve the starting block carries it — whether the attempt
+	// budget ran out or the context ended the retries — so it is the single test
+	// for "no scan started"; an error without it comes from the scan. The last
+	// observed cause remains inspectable with errors.Is.
 	ErrLedgerHeightUnavailable = errors.New("ledger height unavailable")
 )
 
@@ -76,11 +82,19 @@ type Delivery struct {
 // A nil Ledger is the one case that still starts at block 0 — there is no height
 // to read, so the whole chain is the intended range.
 //
-// The returned error is classifiable without inspecting its message: it matches
-// ErrLedgerHeightUnavailable when the height could not be read (and additionally
-// ErrNoLedgerInfo when the ledger returned neither info nor an error), and
-// context.Canceled or context.DeadlineExceeded when the context ended the
-// retries. Any other error comes from the scan itself.
+// The returned error is classifiable without inspecting its message. Every
+// failure to resolve the starting block matches ErrLedgerHeightUnavailable, and
+// no scan has started in that case; an error that does not match it came from the
+// scan. Two further sentinels refine the height failure, and neither is a
+// discriminator on its own:
+//
+//   - ErrNoLedgerInfo, when the last attempt saw a ledger return neither info nor
+//     an error. Only the last attempt is reported, so a driver that violates the
+//     contract intermittently may fail with an ordinary ledger error instead.
+//   - context.Canceled or context.DeadlineExceeded, when the context ended the
+//     retries. The same context governs the scan, so a context error alone does
+//     not say which of the two ended: pair it with ErrLedgerHeightUnavailable to
+//     tell a cancelled height read from a cancelled scan.
 func (d *Delivery) ScanBlock(background context.Context, callback fabric.BlockCallback) error {
 	startingBlock := uint64(0)
 	if d.Ledger != nil {
@@ -98,11 +112,17 @@ func (d *Delivery) ScanBlock(background context.Context, callback fabric.BlockCa
 // with an exponentially growing delay. It gives up as soon as the context is
 // cancelled, and reports the last observed failure once the attempts run out.
 //
+// The budget is generous because nothing retries the caller: FSC's
+// events.ListenerManager calls ScanBlock once from a goroutine that only logs the
+// result, so an error escaping here leaves the channel without block-based
+// finality until the process restarts. Absorbing a peer restart here is therefore
+// worth more than failing fast.
+//
 // Failures are joined with a sentinel rather than described only in the message,
-// so callers classify them with errors.Is: ErrLedgerHeightUnavailable once the
-// budget is spent, ErrNoLedgerInfo for a driver returning (nil, nil), and the
-// context error for a cancelled wait. The underlying ledger error is preserved
-// in every case.
+// so callers classify them with errors.Is: ErrLedgerHeightUnavailable on both
+// exits, additionally ErrNoLedgerInfo for a driver returning (nil, nil) and the
+// context error for a cancelled wait. The underlying ledger error is preserved in
+// every case.
 func (d *Delivery) ledgerHeight(ctx context.Context) (uint64, error) {
 	attempts := d.LedgerInfoAttempts
 	if attempts <= 0 {
@@ -135,7 +155,7 @@ func (d *Delivery) ledgerHeight(ctx context.Context) (uint64, error) {
 
 		select {
 		case <-ctx.Done():
-			return 0, errors.Wrap(errors.Join(ctx.Err(), lastErr), "cancelled while getting ledger info")
+			return 0, errors.Wrap(errors.Join(ErrLedgerHeightUnavailable, ctx.Err(), lastErr), "cancelled while getting ledger info")
 		case <-time.After(delay):
 		}
 		delay *= 2

token/services/network/fabric/finality/delivery_test.go

@@ -182,13 +182,92 @@ func TestScanBlock_CancelledContextStopsRetrying(t *testing.T) {
 	case err := <-done:
 		require.ErrorIs(t, err, context.Canceled)
 		require.ErrorIs(t, err, transient, "the failure that was being retried must stay inspectable alongside the cancellation")
+		require.ErrorIs(t, err, finality.ErrLedgerHeightUnavailable,
+			"cancellation is still a failure to resolve the starting block: ErrLedgerHeightUnavailable is the single test for \"no scan started\"")
 		assert.Equal(t, int32(1), ledger.calls.Load(), "must not keep retrying after cancellation")
 		assert.False(t, delivery.called)
 	case <-time.After(10 * time.Second):
 		t.Fatal("ScanBlock kept retrying after its context was cancelled")
 	}
 }
 
+// TestScanBlock_CancellationDuringNilInfoKeepsBothSentinels pins the corner where
+// the two refining sentinels coincide: a (nil, nil) driver whose retry wait is
+// then cancelled. ErrNoLedgerInfo must survive alongside the context error, and
+// ErrLedgerHeightUnavailable must still be present — documentation states it
+// accompanies every height failure, so this path may not be the exception.
+func TestScanBlock_CancellationDuringNilInfoKeepsBothSentinels(t *testing.T) {
+	ledger := &fakeHeightLedger{results: []heightLedgerResult{{}}}
+	delivery := &recordingBlockDelivery{}
+
+	d := newTestDelivery(ledger, delivery)
+	d.LedgerInfoAttempts = 100
+	d.LedgerInfoRetryDelay = time.Hour
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	done := make(chan error, 1)
+	go func() { done <- d.ScanBlock(ctx, nil) }()
+
+	select {
+	case err := <-done:
+		require.ErrorIs(t, err, context.Canceled)
+		require.ErrorIs(t, err, finality.ErrNoLedgerInfo, "the contract violation being retried must stay inspectable")
+		require.ErrorIs(t, err, finality.ErrLedgerHeightUnavailable)
+		assert.False(t, delivery.called)
+	case <-time.After(10 * time.Second):
+		t.Fatal("ScanBlock kept retrying after its context was cancelled")
+	}
+}
+
+// TestScanBlock_OnlyTheLastFailureIsClassified documents the limit of the
+// refining sentinels: lastErr is overwritten on each attempt, so an earlier
+// (nil, nil) is not reported once a later attempt fails differently. The doc'd
+// contract is "the last attempt", not "any attempt".
+func TestScanBlock_OnlyTheLastFailureIsClassified(t *testing.T) {
+	transient := errors.New("peer connection reset")
+	ledger := &fakeHeightLedger{results: []heightLedgerResult{{}, {err: transient}}}
+	delivery := &recordingBlockDelivery{}
+
+	d := newTestDelivery(ledger, delivery)
+	d.LedgerInfoAttempts = 2
+
+	err := d.ScanBlock(context.Background(), nil)
+
+	require.ErrorIs(t, err, finality.ErrLedgerHeightUnavailable)
+	require.ErrorIs(t, err, transient)
+	require.NotErrorIs(t, err, finality.ErrNoLedgerInfo,
+		"only the last observed failure is classified; callers must not treat a missing ErrNoLedgerInfo as proof the driver never returned (nil, nil)")
+	assert.False(t, delivery.called)
+}
+
+// TestScanBlock_DefaultAttemptBudgetOutlastsAPeerRestart guards the sizing
+// decision: nothing retries ScanBlock, so a single transient failure must not
+// disable block-based finality, and the default budget has to cover a peer
+// restart rather than just a dropped packet. Only the attempt count is asserted —
+// the real delay would make this test sleep for the whole ~31.5s schedule, so it
+// is overridden and the schedule itself is pinned by the const comments.
+func TestScanBlock_DefaultAttemptBudgetOutlastsAPeerRestart(t *testing.T) {
+	const minimumAttempts = 7
+
+	ledger := &fakeHeightLedger{results: []heightLedgerResult{{err: errors.New("peer connection reset")}}}
+	delivery := &recordingBlockDelivery{}
+
+	// LedgerInfoAttempts left unset so the default applies.
+	d := &finality.Delivery{
+		Delivery:             delivery,
+		Ledger:               ledger,
+		Logger:               logging.MustGetLogger(),
+		LedgerInfoRetryDelay: time.Millisecond,
+	}
+
+	require.Error(t, d.ScanBlock(context.Background(), nil))
+	assert.GreaterOrEqual(t, int(ledger.calls.Load()), minimumAttempts,
+		"the default attempt budget must absorb a peer restart: nothing retries ScanBlock, so a failure here is terminal")
+	assert.False(t, delivery.called)
+}
+
 // TestScanBlock_NilLedgerScansFromGenesis pins the one remaining path that
 // legitimately starts at block 0: no ledger is configured, so there is no height
 // to resume from and the whole chain is the intended range.