[davelund110]

What

A small follow-up to #7, from the spec review on lightning-blake2b/bolts#17: a node that sets bit 266 (the even bit of option_independent_secrets) in its node_announcement is now treated as one requiring a feature lnd doesn't know. It isn't paid from its announcement, and it isn't routed through.

Why

The even bit is only valid in channel_type. lnd knows it, so a node announcing it passed ValidateRequired and was paid and routed through, while a node that doesn't know the bit refuses it. bolts#17 now says a receiver of 266 in node_announcement treats it as an unknown even bit, as BOLT 7 describes.

How

• feature.ValidateRoutable is ValidateRequired plus a refusal of 266.
• Pathfinding uses it for the destination's features (from the invoice, or the graph if there's none) and for every intermediate node, where ValidateRequired was used before.
• init keeps its own, clearer refusal of 266 from multi: add option_independent_secrets channel type #7.
• docs/independent-secrets.md has a line on it.

Tests

• New: a TestPathFinding case, "independent secrets even bit in node announcement", which runs the existing unknown-required-features test with a node announcing 266. It fails without the change. And TestValidateRoutableRefusesIndependentSecretsEvenBit: 266 refused, 267 accepted, unknown required bits still refused.
• go build ./... passes, and so do the feature, routing and peer tests and go vet on feature and routing.

[paulscode]

Thanks, this does the routing half of the new bolts#17 rule: the destination and every intermediate node go through ValidateRoutable, an invoice without 266 still pays (BOLT 7's BOLT 11 exception), and the new pathfinding case fails without the change. Merging.

Two notes for later, neither specific to this PR:

• "SHOULD NOT connect" isn't covered: the bootstrapper and autopilot don't look at announcement features, so lnd can still connect to a node that sets 266 only in its announcement. lnd treats every unknown even bit the same way there, so a filter in the bootstrapper would be a general change.
• findBlindedPaths checks only route blinding support, so our own blinded paths can still go through such a node (also true upstream for any unknown even bit).

[davelund110]

Thanks Paul, agreed on both notes. A security review of the lnd side (#7 and this PR) found the blinded paths one too, and nothing that could lose funds. Two more for later:

• Release notes: since multi: add option_independent_secrets channel type #7, protocol.custom-init, custom-nodeann and custom-invoice refuse any bit lnd defines, as you suggested. A node that already sets one there (say bit 19, wumbo, without protocol.wumbo-channels) won't start after upgrading, so it's worth a line.
• Older, not from this work: migration30 reads each channel's info with mig26.FetchChanInfo, which reads the channel type as one byte, while the current codec writes it as a varint. So any type of 0xfd or more, which includes every channel with the unified-sigs bit (1 << 13), misreads the fields after it. It only matters while old-format revocation logs exist.

[davelund110]

Two small follow-ups from another review of the lnd side (it found nothing that could lose funds, and confirmed the dev-tag build fix you already made in 204d9ad):

• sample-lnd.conf still shows protocol.custom-init=39 as its example. Since multi: add option_independent_secrets channel type #7 that's refused at startup (bit 39 is onion-messages, which lnd defines), so a node that copied it with onion messages off won't start. Worth changing the example along with the release note.

• A database used by a build from the middle of multi: add option_independent_secrets channel type #7, between "signal option_independent_secrets by default" (1d590b7) and "turn it off by default" (776735f), can hold add-on channels without the version-39 mark, which only came in 2d807f7; the mark is only set when a channel is written, so upgrading doesn't add it. That only matters for nodes that ran those intermediate builds (lab nodes, say). Marking at startup when any open or closed channel has the add-on would cover it.

[davelund110]

One more from a review of the lnd side, older than this work and probably upstream too, so you may want to check it there.

A zero-conf channel's funding confirmation can refresh the live channel state in the middle of ReceiveRevocation. When the funding confirms, waitForZeroConfChannel (funding/manager.go:4127) reports the short channel ID, and channelLink.UpdateShortChanID (htlcswitch/link.go:2541) calls Refresh() on the link's live OpenChannel from the funding manager's goroutine. ReceiveRevocation (lnwallet/channel.go:6240-6250) changes the revocation store and the remote revocation points in memory without the OpenChannel mutex, and only writes them in AdvanceCommitChainTail. A Refresh() in between reloads the old values from disk, and AdvanceCommitChainTail then writes them back while advancing the remote commitment height.

With the timing forced in a test (a hook calling Refresh() right after the in-memory update), on a shachain channel ReceiveRevocation succeeds but the database is left with the remote height advanced and the store missing the secret it just received: NewBreachRetribution for that state and ChanSyncMsg both fail, so that revoked state can't be punished and the channel can't reestablish. On a channel with option_independent_secrets the newest-secret check in AdvanceCommitChainTail refuses the write, the transaction rolls back, and after a reload the retransmitted revoke_and_ack is accepted, so those channels recover.

How often the real timing hits that window I don't know; it needs the confirmation to land while a revocation is being processed. A fix would be to refresh only the short channel ID fields rather than the whole state, or do the refresh on the link's goroutine, or hold the OpenChannel mutex across ReceiveRevocation's in-memory update and its write. Happy to send the test and hook if useful.

The same Refresh() call is still in UpdateShortChanID on lnd master, and I couldn't find an issue for it there. The closest is lightningnetwork#11259, a different race between two copies of the same OpenChannel while a channel opens.