🟠 Major F1: go.mod pins lightning-onion to a personal fork

The added replace github.com/lightningnetwork/lightning-onion => github.com/GeorgeTsagk/lightning-onion v0.0.0-... is self-described as temporary until the dependent lightning-onion PR merges, and it is the cause of the failing lint. This is a merge blocker: the dependency must be repointed to an upstream lightning-onion tag before this can land. Flagging so it is not merged in this state.

🟠 Major F2: Zero/invalid CreatedAt yields garbage hold time

getHoldTime computes uint32(time.Since(s.CreatedAt).Milliseconds() / 100) with no guard on CreatedAt. For a forwarded HTLC persisted by a pre-#9888 node, restoration goes Decode → Reextract: Decode's legacy branch returns early on len(typeMap) == 0, leaving CreatedAt at the zero time.Time (asserted by TestSphinxErrorEncrypterDecodeBackwardCompat), and Reextract only calls initialize(sharedSecret) — it never sets CreatedAt. time.Since(time.Time{}) overflows and the uint32 cast produces a nonsensical multi-decade hold time that is reported to the sender. The same cast wraps negatively if CreatedAt is ever in the future (clock skew, or a corrupt creationTime restored via time.Unix(0, ...) with no validation). Guard getHoldTime to return 0 when CreatedAt.IsZero() or the duration is negative, and default CreatedAt to now in Reextract/the legacy Decode branch.

🟠 Major F3: Intercepted Fail path drops attribution data

interceptedForward.Fail does obfuscatedReason, _, err := f.packet.obfuscator.IntermediateEncrypt(reason, nil) and resolves with &lnwire.UpdateFailHTLC{Reason: obfuscatedReason}, discarding the returned attrData and never setting ExtraData. Every other forward-failure site (mailbox.FailAdd, switch.handlePacketFail default case, FailWithCode) converts the returned attrData via lnwire.AttrDataToExtraData and populates ExtraData. As written, HTLCs failed through the interceptor lose attribution, silently breaking the feature for that path. Capture the attrData and set ExtraData on the UpdateFailHTLC.

🟠 Major F4: FailAdd abandons fail-back on attr-data conversion error, stranding the HTLC

In FailAdd, after EncryptFirstHop succeeds, extraData, err := lnwire.AttrDataToExtraData(attrData) is followed by if err != nil { log.Errorf(...); return }. The bare return aborts before the UpdateFailHTLC is ever queued, so the HTLC is neither settled nor failed — it stays live until timeout and forces an on-chain close, locking funds and a commitment slot. An incoming HTLC that cannot be forwarded is the attacker-influenced input to this path. The conversion error should still drive a fail-back (e.g. fail without attribution data) rather than silently returning.

🟠 Major F5: handlePacketFail proceeds with nil reason after EncryptFirstHop error

In the FailPermanentChannelFailure branch, reason, attrData, err := circuit.ErrorEncrypter.EncryptFirstHop(failure) only logs on error (if err != nil { log.Error(err) }) and falls through, assigning htlc.Reason = reason (zero-value OpaqueReason) and htlc.ExtraData from empty attrData. Both sibling branches (UpdateFailMalformedHTLC, the default forwarded case) return on encrypt error. The result is an all-zero, undecryptable UpdateFailHTLC sent upstream for a real HTLC, masking the true failure and breaking attribution. Add return err after the log to match the other branches.

🟡 Minor F7: Hold-time unit comments mislabel the encoded unit

The getHoldTime doc comment says "decaseconds" (10s) and the AttrErrorStruct comment says the payload holds "a 32-bit hold time in milliseconds," but the implementation encodes 100ms units (Milliseconds() / 100, i.e. deciseconds), which is what every other comment (failure.go, lightning.proto, payments/db/payment.go) and the ht*100 render agree on. Fix these two comments to say "100ms units" to match the implementation. Minor cleanup in the same constructor: time.Now().Truncate(time.Nanosecond) is a no-op since nanosecond is already time.Time's base resolution.

🟡 Minor F8: Constructor and interface semantics changed under the same names

NewSphinxErrorEncrypter changed meaning, not just signature: it was a no-arg blank constructor and is now an initialized constructor taking (ephemeralKey, sharedSecret), with the blank behavior moved to NewSphinxErrorEncrypterUninitialized (same for the Introduction/Relaying variants). A mechanically-remapped call site that needed the old deferred-Reextract semantics but got the initialized constructor (or vice versa) is a silent regression the type checker cannot catch — enumerate the former call sites and confirm each was mapped to the constructor matching its prior initialization state. The ErrorEncrypter interface also changed shape (the Encrypt* methods now return []byte, ErrorEncrypterExtracter → SharedSecretGenerator); this is an intentional breaking change that warrants a migration note in the release notes for out-of-tree implementers.

🟡 Minor F9: Two independent hold-time serialization formats on recently-churned DB code

Hold times are encoded twice by hand — WriteElements (uint16 count + uint32 each) in serializeHTLCFailInfo, and manual binary.BigEndian in sql_converters.encodeHoldTimes — feeding the same HTLCFailInfo.HoldTimes. Both land on DB code that was itself just modified (kv path PR #10153 Aug 2025; SQL hold_times column PR #10604 Oct 2025). If only one backend is exercised in tests, a format divergence ships undetected. Add a cross-backend round-trip assertion, or factor the two onto a shared encoder so they cannot drift.

🟡 Minor F10: Exported mutable AttrDataTlvType and nil/empty normalization

var AttrDataTlvType tlv.TlvType1 is an exported, mutable package var that determines the on-wire attribution TLV type — a protocol constant that any importer can silently reassign, breaking interop. Prefer a typed constant exposed via a getter. Also note AttrDataToExtraData(nil) and ([]byte{}) both round-trip to an empty record and ExtraDataToAttrData always returns nil for the empty case, so "no attribution present" and "present but empty" are indistinguishable; document this normalization (or return an fn.Option) so callers don't branch on == nil vs len == 0 incorrectly.

🟡 Minor F11: New/relocated branches lack test coverage

Two behavior-carrying changes need coverage. First, the blinded-path role selection (introduction vs relaying vs plain), which was only recently introduced in iterator.ExtractErrorEncrypter (PR #8485, 2024), is relocated into the CreateErrorEncrypter closure here as switch { case isIntroduction: ...; case hasBlindingPoint: ...; default: ... }; a swapped case reverts blinded-error behavior with no compile error. Add a test that exercises the introduction case with a blinding point also present (order-sensitivity). Second, IntermediateEncrypt's new errors.Is(err, sphinx.ErrInvalidAttrStructure) recovery branch (regenerating fresh attribution) has no visible test — add one asserting the reset behavior.