paymentsdb: normalize orphaned blinded total

[ziggie1984]

Change Description

Fix a KV-to-SQL payment migration failure for historical routes containing a
non-zero blinded total_amt_msat without encrypted recipient data.

SendToRouteV2 historically copied the blinded total amount independently of
the encrypted data. LND did not classify such a hop as blinded at runtime, but
the migration treated the total amount as proof of a blinded hop and attempted
to insert a row with a NULL encrypted_data value. This violated the existing
SQL NOT NULL constraint and prevented LND from starting.

This PR:

• requires encrypted recipient data when SendToRouteV2 supplies either a
  blinding point or blinded total amount;
• omits the orphaned total when migrating the historically accepted total-only
  representation;
• rejects malformed blinding-point-only migration records with payment,
  attempt and hop context;
• applies the same normalization during KV/SQL migration validation;
• emits a PYDB warning identifying the affected payment, attempt, hop and
  discarded total amount;
• keeps the strict SQL schema unchanged.

Steps to Test

make unit-debug log="stdlog trace" \
  pkg=lnrpc/routerrpc \
  case=TestUnmarshallHopBlindedFieldsRequireEncryptedData \
  timeout=10s nocache=1

go test -v -tags="dev test_db_sqlite stdlog trace" \
  ./payments/db/migration1 \
  -run '^(TestMigrateOrphanedBlindedTotalAmount|TestMigrateBlindingPointWithoutEncryptedData)$' \
  -count=1 -timeout=10s

make unit pkg=lnrpc/routerrpc timeout=5m nocache=1

go test -tags=test_db_sqlite ./payments/db/migration1 \
  -count=1 -timeout=5m

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative error paths are included.
• The bug fix contains a regression test triggering the historical case.

Code Style and Documentation

• The change follows the documentation, commenting and 80-column rules.
• Commits follow the ideal Git commit structure.
• New logging uses the PYDB subsystem at warning level.
• A change description is included in the v0.21.2 release notes.

[github-actions]

🟠 PR Severity: HIGH

Classification | 6 files | 143 additions, 10 deletions (49 lines / 4 files excluding tests & docs)

🟠 High (1 file)

• lnrpc/routerrpc/router_backend.go - RPC/API definitions under lnrpc/*; changes here affect the router RPC surface.

🟡 Medium (2 files)

• payments/db/migration1/migration_validation.go - payments/* package, not one of the special always-critical migration paths (channeldb/migration*, sqldb/*, wtdb/*).
• payments/db/migration1/sql_migration.go - same as above.

🟢 Low (1 file, plus 2 excluded test files)

• docs/release-notes/release-notes-0.22.0.md - release notes.
• lnrpc/routerrpc/router_backend_test.go - test-only change, excluded from severity/line counting.
• payments/db/migration1/sql_migration_test.go - test-only change, excluded from severity/line counting.

Analysis

The highest-severity file touched is lnrpc/routerrpc/router_backend.go, which falls under lnrpc/* (RPC/API definitions), placing this PR at HIGH severity.

The payments/db/migration1/* changes look like migration-related code, but per the classification rules the "always CRITICAL" migration rule is scoped specifically to channeldb/migration*, sqldb/*, and wtdb/* — payments/db/migration1 isn't one of those paths, so it's treated as an ordinary payments/* package (MEDIUM).

No severity bump applies: only 4 non-test/non-doc files changed (well under the 20-file threshold) and ~49 lines changed excluding tests (well under the 500-line threshold), and no multiple distinct critical packages are touched.

Given the RPC-facing change to router backend logic alongside a payments DB migration adjustment, a knowledgeable engineer should review both the RPC behavior change and the migration validation/backfill logic for correctness.

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[ziggie1984]

/gateway review

[lightninglabs-gateway]

Gateway review — 5 findings

🔴 0 Blocker · 🟠 1 Major · 🟡 4 Minor · 🔵 0 Nit

Summary

This PR fixes a real, well-understood node-startup failure: historical routes where SendToRouteV2 copied a blinded total_amt_msat without encrypted recipient data caused the KV→SQL payment migration to attempt a NULL encrypted_data insert against a NOT NULL column, aborting the migration and preventing LND from starting. The RPC-side hardening, the orphaned-total normalization, and the matching compare-time normalization are the right shape, and the added tests cover the primary paths. The paymentHash → htlcHash rewrite in migrateHTLCAttempt is a behavior-preserving refactor.

The central concern is that the fix treats the two malformed-hop cases asymmetrically. An orphaned total is normalized so the node starts, but a hop with a blinding point and no encrypted data now returns a hard error that aborts the whole migration transaction — reintroducing the exact startup-blocking failure class this PR exists to eliminate, for any node whose historical data contains that combination.

The remaining findings are lower-severity: a breaking change on a deprecated RPC field, an undocumented cross-function normalization invariant, a misleading diagnostic label, and a test-coverage gap.

Bot commands

• /gateway re-review — re-run after pushing changes
• /gateway dismiss <id> — silence a finding (maintainers)
• /gateway explain <id> — elaborate on a finding (anyone)

[lightninglabs-gateway]

🤖 gateway audit metadata for this PR — auto-generated, please don't edit.

[ziggie1984]

/gateway re-review

[lightninglabs-gateway]

Gateway review — 3 findings

🔴 0 Blocker · 🟠 1 Major · 🟡 2 Minor · 🔵 0 Nit

Summary

The author addressed most of the prior review. The SendToRouteV2 release-note callout (F2), the attempt_index relabeling (F5), and the reverted hash-selection refactor (F4) are all resolved; the BOLT-4 rationale added to the RPC guards checks out under spec review. F3's requested documentation was added on the compare side but only in one direction, leaving the coupling half-documented.

The central concern (F1) remains unresolved. The blinding-point-only migration branch still aborts the entire at-startup, single-transaction migration rather than normalizing like its sibling orphaned-total case. The author added the justifying comment F1 asked for, but two independent reviews this round judge the underlying "this shape cannot exist historically" claim to be an unprovable assertion, and the code still bricks node startup with no recovery path if it is wrong — mitigated only by the fact that the migration itself has not yet shipped in a release.

One genuinely-new minor finding: the orphanedTotalAmt variable implies a control-flow role it does not have.

---

Status of prior findings

• F2 addressed: Fixed in docs/release-notes/release-notes-0.21.2.md — the note now states the fix "also affects callers replaying an affected historical route returned by ListPayments or TrackPayment," and the BOLT-4 justification for the new guards is spec-correct.
• F4 addressed: Fixed in payments/db/migration1/sql_migration.go — the htlcHash rewrite was reverted; migrateHTLCAttempt keeps the original paymentHash selection and threads parentPaymentHash only for diagnostics, so the untested-refactor concern no longer applies.
• F5 addressed: Fixed in payments/db/migration1/sql_migration.go — the parameter is now attemptIndex and both new diagnostics label the value attempt_index=%d, no longer implying it is the original on-disk attempt ID.

Bot commands

• /gateway re-review — re-run after pushing changes
• /gateway dismiss <id> — silence a finding (maintainers)
• /gateway explain <id> — elaborate on a finding (anyone)

[ziggie1984]

/gateway dismiss F1, as mentioned we want to abort this case because this cannot happen and signals some deeper issue with the underlying dataset

[lightninglabs-gateway]

🚫 Dismissed F1 (major) by @ziggie1984 — as mentioned we want to abort this case because this cannot happen and signals some deeper issue with the underlying dataset

Open findings on this PR: 🟡 F3 (minor) · 🟡 F6 (minor)

[ziggie1984]

/gateway approve

[lightninglabs-gateway]

❌ Cannot approve: the PR has new commits since the last review (last reviewed 9a90f07, current head 17ee638).

Run /gateway re-review first to evaluate the new commits, then /gateway approve.

[ziggie1984]

/gateway re-review

[lightninglabs-gateway]

Gateway review

No findings.

---

Status of prior findings

• F2 addressed: Fixed in docs/release-notes/release-notes-0.21.2.md — the bug-fix bullet now states "This also affects callers replaying an affected historical route returned by ListPayments or TrackPayment."
• F3 addressed: Fixed in payments/db/migration1/migration_validation.go:407 and payments/db/migration1/sql_migration.go:840 — both sites now document the invariant and cross-reference each other ("A blinding point without data is rejected during migration, so it cannot reach this comparison." / "Keep this rejection in sync with normalizePaymentForCompare, which only normalizes hops without a blinding point."), making the previously-implicit coupling explicit.
• F4 addressed: Fixed in payments/db/migration1/sql_migration.go:745 — the paymentHash/htlcHash rewrite was reverted to the pre-existing behavior-preserving hash-selection switch; parentPaymentHash is now threaded only for the new diagnostics, so the untested-AMP-branch concern no longer applies.
• F5 addressed: Fixed in payments/db/migration1/sql_migration.go:843 — the parameter is renamed attemptIndex and both the error and warning now label the value attempt_index=%d, no longer implying it is the original on-disk attempt ID.
• F6 addressed: Fixed in payments/db/migration1/sql_migration.go:848 — the orphanedTotalAmt variable was removed; the warning is now gated inline on !hasEncryptedData && hop.TotalAmtMsat != 0, and the row omission is driven by the if hasEncryptedData insert guard with a comment explaining the length-based check, so no variable implies a control-flow role it lacks.

Bot commands

• /gateway re-review — re-run after pushing changes
• /gateway dismiss <id> — silence a finding (maintainers)
• /gateway explain <id> — elaborate on a finding (anyone)

[ziggie1984]

/gateway approve

[lightninglabs-gateway]

✅ Approved — all prior findings addressed.

Findings recap

Findings:

• F1 (major) — unresolved: A single historical route hop with hop.BlindingPoint != nil and len(hop.EncryptedData) == 0 now causes migrateRouteHop to return `fmt.…
• F2 (minor) — addressed: The new TotalAmtMsat != 0 && len(EncryptedData) == 0 rejection in UnmarshallHopWithPubkey changes the behavior of the public `SendToRout…
• F3 (minor) — addressed: The migration round-trip holds only because of an undocumented coupling between two textually different predicates. `normalizePaymentForComp…
• F4 (minor) — addressed: The migrateHTLCAttempt rewrite to PaymentHash: htlcHash[:] with the new switch (htlc.Hash != nil → *htlc.Hash, else `parentPayment…
• F5 (minor) — addressed: The new error and warning strings label the value as attempt=%d, but migrateRouteHop receives attemptIndex (named attemptID in the s…
• F6 (minor) — addressed: orphanedTotalAmt and its comment ("Omit such an orphaned total rather than creating a blinded-hop row") read as if the variable controls w…

Dismissed:

• F1 by @ziggie1984 — as mentioned we want to abort this case because this cannot happen and signals some deeper issue with the underlying dataset

---

Approved by @ziggie1984 via /gateway approve. Last reviewed at 17ee638. Skill v0.3.0, model claude-opus-4-8.