Fix review comments

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

Author: AkramBitar

docs/services/network-fabric.md

@@ -308,6 +308,19 @@ replay finality notifications for every historical transaction, while the caller
 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 |
+
+`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.
+
 ### Notification Mode
 
 Uses asynchronous event notifications from the FSC layer:

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

@@ -25,6 +25,19 @@ const (
 	defaultLedgerInfoRetryDelay = 500 * time.Millisecond
 )
 
+var (
+	// ErrNoLedgerInfo reports that the ledger returned neither info nor an error,
+	// so no height could be read. It signals a driver contract violation rather
+	// 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.
+	ErrLedgerHeightUnavailable = errors.New("ledger height unavailable")
+)
+
 // blockFromScanner is the subset of fabric.Delivery used by Delivery: the block
 // scan, parameterized by the block to start from.
 type blockFromScanner interface {
@@ -62,6 +75,12 @@ 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.
 func (d *Delivery) ScanBlock(background context.Context, callback fabric.BlockCallback) error {
 	startingBlock := uint64(0)
 	if d.Ledger != nil {
@@ -78,6 +97,12 @@ func (d *Delivery) ScanBlock(background context.Context, callback fabric.BlockCa
 // ledgerHeight returns the current ledger height, retrying transient failures
 // 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.
+//
+// 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.
 func (d *Delivery) ledgerHeight(ctx context.Context) (uint64, error) {
 	attempts := d.LedgerInfoAttempts
 	if attempts <= 0 {
@@ -97,7 +122,7 @@ func (d *Delivery) ledgerHeight(ctx context.Context) (uint64, error) {
 		case info == nil:
 			// A driver that reports neither info nor error would otherwise nil
 			// deref below; treat it as a failure to read the height.
-			lastErr = errors.New("ledger returned no info")
+			lastErr = ErrNoLedgerInfo
 		default:
 			return info.Height, nil
 		}
@@ -110,11 +135,11 @@ func (d *Delivery) ledgerHeight(ctx context.Context) (uint64, error) {
 
 		select {
 		case <-ctx.Done():
-			return 0, errors.Wrapf(ctx.Err(), "cancelled while getting ledger info, last error [%v]", lastErr)
+			return 0, errors.Wrap(errors.Join(ctx.Err(), lastErr), "cancelled while getting ledger info")
 		case <-time.After(delay):
 		}
 		delay *= 2
 	}
 
-	return 0, errors.Wrapf(lastErr, "failed to get ledger info after %d attempt(s), refusing to rescan from genesis", attempts)
+	return 0, errors.Wrapf(errors.Join(ErrLedgerHeightUnavailable, lastErr), "failed to get ledger info after %d attempt(s), refusing to rescan from genesis", attempts)
 }

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

@@ -109,7 +109,8 @@ func TestScanBlock_PropagatesLedgerInfoErrorInsteadOfRescanningFromGenesis(t *te
 
 	require.Error(t, err, "GetLedgerInfo failure must be propagated, not swallowed after a log line")
 	require.ErrorIs(t, err, transient, "the underlying ledger error must stay inspectable")
-	assert.Contains(t, err.Error(), "refusing to rescan from genesis")
+	require.ErrorIs(t, err, finality.ErrLedgerHeightUnavailable, "the caller must classify this without matching on the message")
+	require.NotErrorIs(t, err, finality.ErrNoLedgerInfo, "an unreachable peer is not a driver returning no info")
 	assert.False(t, delivery.called, "no scan may start from genesis when the ledger height is unknown")
 }
 
@@ -154,14 +155,17 @@ func TestScanBlock_NilLedgerInfoIsReportedNotDereferenced(t *testing.T) {
 	err := newTestDelivery(ledger, delivery).ScanBlock(context.Background(), nil)
 
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "ledger returned no info")
+	require.ErrorIs(t, err, finality.ErrNoLedgerInfo, "the (nil, nil) contract violation must be identifiable, not just described")
+	require.ErrorIs(t, err, finality.ErrLedgerHeightUnavailable, "it is also a failure to read the height")
+	require.NotErrorIs(t, err, context.Canceled, "a nil-info driver is not a cancellation")
 	assert.False(t, delivery.called)
 }
 
 // TestScanBlock_CancelledContextStopsRetrying verifies shutdown does not have to
 // wait out the retry budget: a cancelled context ends the loop with its own error.
 func TestScanBlock_CancelledContextStopsRetrying(t *testing.T) {
-	ledger := &fakeHeightLedger{results: []heightLedgerResult{{err: errors.New("peer connection reset")}}}
+	transient := errors.New("peer connection reset")
+	ledger := &fakeHeightLedger{results: []heightLedgerResult{{err: transient}}}
 	delivery := &recordingBlockDelivery{}
 
 	d := newTestDelivery(ledger, delivery)
@@ -177,6 +181,7 @@ func TestScanBlock_CancelledContextStopsRetrying(t *testing.T) {
 	select {
 	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")
 		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):
@@ -206,5 +211,6 @@ func TestScanBlock_PropagatesScanError(t *testing.T) {
 	err := newTestDelivery(ledger, delivery).ScanBlock(context.Background(), nil)
 
 	require.ErrorIs(t, err, scanErr)
+	require.NotErrorIs(t, err, finality.ErrLedgerHeightUnavailable, "a scan failure must not be mistaken for a failure to read the height")
 	assert.Equal(t, uint64(3), delivery.startingBlock)
 }