lnwire: pull gossip v2 messages in line with BOLT taproot-gossip review updates - #11164
Open
ViktorT-11 wants to merge 12 commits into
Open
ViktorT-11 wants to merge 12 commits into
ViktorT-11 wants to merge 12 commits into
Conversation
The taproot-gossip BOLT extension was updated to align with BOLT 12, which long since moved its signature TLVs to type 240 and widened the signed TLV range from 0..=159 to 0..=239. Mirror that here: * pureTLVUnsignedRangeOneStart: 160 -> 240 * channel_announcement_2.Signature: TlvType160 -> TlvType240 * channel_update_2.Signature: TlvType160 -> TlvType240 * node_announcement_2.Signature: TlvType160 -> TlvType240 Update the pure-TLV test fixtures and the test message types correspondingly.
The BOLT taproot-gossip spec now requires each gossip v2 reader to
reject messages that are missing any of their compulsory fields with
a warning/close/ignore action. lnwire's three gossip v2 Decode
implementations silently zero-valued the missing TLVs, which made
later validation harder.
Add a small AssertRequiredPresent helper to pure_tlv.go and call it
from each Decode after DecodeWithParsedTypesP2P / ExtractRecords:
* channel_announcement_2: short_channel_id, outpoint, capacity,
node_id_1, node_id_2, signature.
* channel_update_2: short_channel_id, block_height, signature.
* node_announcement_2: features, block_height, node_id, signature.
The BOLT taproot-gossip spec was tightened so that the port-not-zero rule applies to every advertised address type, including tor_v3_address. Mirror that in the lnwire encoder/decoder for tor v3 addresses in node_announcement_2: fail encoding if the sender tries to emit a tor v3 address with port 0, and fail decoding if a received message contains one.
The taproot-gossip BOLT extension swapped the two-TLV (first_block @ type 2, block_height_range @ type 4) layout in gossip_timestamp_range for a single TLV at type 2 holding both fields: u32 first_block_height and tu32 num_blocks. The two fields are always set together, and the truncated u32 num_blocks saves a few bytes on the wire. Replace the FirstBlockHeight (type 2, u32) and BlockRange (type 4, u32) optional records on GossipTimestampRange with a single BlockHeightRange (type 2) optional record holding a new BlockHeightRange struct. The struct's Record() method uses MakeDynamicRecord with EUint32T+ETUint32T for the encoder and DUint32+DTUint32 for the decoder. The property-test factory in test_message.go is updated to draw a single optional BlockHeightRange instead of two independent fields.
The taproot-gossip BOLT extension added a required funding_txid TLV (type 6, sha256) to announcement_signatures_2 so each message names its funding transaction explicitly. For an initial channel open this is the original funding tx; for a spliced channel it is the txid of the splice transaction that triggered the new round of announcement signing. Using funding_txid directly (rather than inferring it via short_channel_id) is what makes splice announcements unambiguous when multiple candidate funding transactions may exist at different points. Add the field on AnnounceSignatures2 (encoded as [32]byte, to follow the same primitive-encoding pattern channel_announcement_2 uses for its chain hash), thread it through NewAnnSigs2, AllRecords and the Decode presence assertion, and update the test fixtures + the rapid property-test factory to set it. The two channeldb waitingproof tests that build AnnounceSignatures2 via NewAnnSigs2 are updated to pass a placeholder funding_txid.
ViktorT-11
changed the title
v0.2.19
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (9 files)
🟠 High (1 file)
🟢 Low (7 files)
AnalysisThe bulk of this PR modifies non-test files under Additionally, excluding test and auto-generated files, the PR changes ~605 lines across 11 files, which exceeds the 500-line bump threshold — though since CRITICAL is already the highest tier, this doesn't change the outcome. The one To override, add a |
6 tasks
ViktorT-11
force-pushed
the
2026-08-sync-gossip-with-spec
branch
from
September 3, 2026 08:57
ac6a7e6 to
fa3d115
Compare
bitromortac
reviewed
Sep 7, 2026
bitromortac
left a comment
bitromortac
left a comment
Collaborator
There was a problem hiding this comment.
Nice, I took a first look.
| return err | ||
| } | ||
|
|
||
| if err := AssertRequiredPresent( |
Collaborator
There was a problem hiding this comment.
nit: put into earlier commit that adds AssertRequiredPresent and then update here
| // InboundFeeBaseMsat record. | ||
| 0x14, // type. | ||
| 0x4, // length. | ||
| 0x0, 0x0, 0x0, 0x5, // value (5). |
Collaborator
There was a problem hiding this comment.
should we encode the fees minimally?
Collaborator
Author
There was a problem hiding this comment.
I checked the spec, and yes good catch, we should. As per the spec, that should also apply to the FeeBaseMsat& FeeProportionalMillionths. So I added a separate commit which updates those fields to use that as well.
Comment on lines
+262
to
+265
| // for a total of 64 bytes. The BOLT taproot-gossip extension dropped the | ||
| // previously pre-aggregated 32-byte form in favour of this layout so that | ||
| // receivers can verify each sig with the standard MuSig2 partial-sig verify | ||
| // routine. |
Collaborator
There was a problem hiding this comment.
nit: migration narrative
| // Inbound fees in gossip v2 are two required uint32 TLVs that | ||
| // default to 0. Treat the both-zero case as "no inbound fee" | ||
| // so the downstream Option semantics still hold. | ||
| // Inbound fees in gossip v2 are two uint32 TLVs that are |
|
|
||
| // SciddirLen is the wire length of a Sciddir: one direction byte followed by | ||
| // the 8-byte short_channel_id. | ||
| const SciddirLen = 9 |
Collaborator
There was a problem hiding this comment.
you could take a look at lnwire/intro_node.go, maybe we can reuse that or unify
Collaborator
Author
There was a problem hiding this comment.
I added a fixup commit which addresses this, as I'm not sure this change is actually cleaner than the previous version. Please let me know which version you prefer.
bitromortac
reviewed
Sep 7, 2026
Comment on lines
+137
to
+138
| BaseFee: int32(baseFee), | ||
| FeeRate: int32(propFee), |
Collaborator
There was a problem hiding this comment.
I think overflow is possible here
Collaborator
Author
There was a problem hiding this comment.
Good catch! I added a check which rejects such values which would overflow.
However, after adding that check, LND is now in theory not fully spec compliant as in theory someone could advertise an inbound fee between 2,147,483,648 and 4,294,967,295, which will be rejected by LND.
Since such high values for inbound fees are unrealistic to be used, we should consider if that's an ok trade-off or not.
The real fix for this would be to preserve the full unsigned range through LND’s inbound fee handling. That would be a big change though, so if we'd want ot go down that route I suggest that we do so through follow-ups.
Let me know your thoughts!
Collaborator
|
@yyforyongyu: review reminder |
ViktorT-11
force-pushed
the
2026-08-sync-gossip-with-spec
branch
from
September 14, 2026 14:24
fa3d115 to
237eb33
Compare
The taproot-gossip BOLT extension assigned the experimental inbound-fee field on channel_update_2 a real TLV layout: two separate uint32 records, type 20 (inbound_fee_base_msat) and type 22 (inbound_fee_proportional_millionths), both positive-only with a default of 0. Pull the implementation in line: - Replace the experimental InboundFee OptionalRecordT[TlvType55555, Fee] (which had a long-standing TODO to assign a real type) with two required uint32 RecordTs at types 20 and 22. - Suppress on encode when 0 and default-fill on decode, matching how the surrounding fee/htlc fields behave. - Update ChanEdgePolicyFromWire for ChannelUpdate2 to fold the two uint32 values into the existing fn.Option[lnwire.Fee] downstream contract: emit None when both are 0, Some otherwise. The Fee struct itself still uses int32 for the legacy ChannelUpdate1 case; ChannelUpdate2 inbound fees can only be non-negative so the uint32->int32 widening is safe in practice (and a v2 sender can't encode a negative fee anyway). - Update the channel_update_2 test fixture to carry valid type-20 and type-22 records, and move the previously-unknown extra TLV out of slot 20 to slot 24. - Update the rapid property factory to draw the two new uint32 fields from a non-zero range when including an inbound fee.
The taproot-gossip BOLT extension dropped the previously pre-aggregated 32-byte partial signature on announcement_signatures_2 in favour of emitting both raw musig2 partial sigs back-to-back -- one for the node_id key and one for the bitcoin key -- so the receiver can verify each half with the standard MuSig2 PartialSigVerify routine instead of the custom verifier the old layout required. Introduce an AnnouncementSigPair value type in partial_sig.go that encodes as `node || bitcoin` for a fixed 64 bytes, with a static- record builder via tlv.MakeStaticRecord. Swap announcement_signatures_2's PartialSignature (TlvType4, PartialSig) field for a new PartialSignatures (TlvType4, AnnouncementSigPair) field; update NewAnnSigs2 to take the pair; update the hardcoded test fixture (length 0x20 -> 0x40, 32 bytes -> 64 bytes of zero padding); update the rapid property factory to draw two independent partial sigs; and update the three channeldb waitingproof tests that build AnnounceSignatures2 directly. The existing 32-byte PartialSig type stays in place for the co-operative close flow and other call-sites that don't carry both sigs at once.
The channel_update_2 inbound-fee TLVs are not "required" -- they follow the same defaulted-on-the-wire pattern as fee_base_msat and friends, where the field is always present in the Go struct (a uint32 with the default-fill applied on decode) and suppressed from the wire when it equals the default of 0. Reword the comment to avoid implying that a sender MUST emit them. No code change.
…ddir
The taproot-gossip BOLT extension switched
channel_update_2.short_channel_id from a plain 8-byte SCID to BOLT 1's
sciddir_or_pubkey type constrained to the sciddir form: a 9-byte
<dirbyte><scid> encoding where the direction byte is 0 for node_id_1
and 1 for node_id_2. With the direction now part of the scid itself,
the previously-separate type-8 second_peer flag TLV is fully
redundant and is removed.
Add a Sciddir value type in lnwire/sciddir.go with a 9-byte static
record (custom encoder/decoder that rejects any direction byte other
than 0 or 1, so we can never accidentally accept the pubkey form of
sciddir_or_pubkey here).
In ChannelUpdate2:
* ShortChannelID now wraps a Sciddir instead of a ShortChannelID.
* The SecondPeer OptionalRecordT and all its Decode/AllRecords
wiring is removed.
* IsNode1() now derives from the dir byte (`Direction == 0`).
* SCID() projects the 8-byte scid portion so the ChannelUpdate
interface stays unchanged for callers.
* SetSCID() updates the scid portion while leaving the existing
dir byte in place.
ChanEdgePolicyFromWire now derives ChannelEdgePolicy.SecondPeer from
!upd.IsNode1() instead of the old upd.SecondPeer.IsSome() lookup.
The downstream SecondPeer field on ChannelEdgePolicy stays the same
shape since it is also used by ChannelUpdate1.
The channel_update_2 test fixture now carries a 9-byte sciddir at
type 2 and no longer has a type-8 SecondPeer record. The rapid
property factory draws a single bool for the direction byte instead
of separately drawing an isSecondPeer flag for the (now removed)
second_peer field.
Use minimal unsigned encoding for the existing channel_update_2 outbound fee fields, as required by the gossip v2 spec. Reuse the shared typed-record adapter and extend the encoding tests to cover canonical and malformed outbound values.
…proot-gossip The branch behind this PR pulls the lnwire gossip v2 messages in line with the review-driven updates on the BOLT taproot-gossip extension (lightning/bolts#1059). Record the change in the 0.22.0 release notes under "BOLT Spec Updates" so the PR check is satisfied and downstream implementors get a heads-up about the wire-format shifts.
ViktorT-11
force-pushed
the
2026-08-sync-gossip-with-spec
branch
from
September 14, 2026 14:44
237eb33 to
d366b60
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #10837
This PR builds on #10837 and makes the PR up to date.
Summary
This branch pulls the gossip v2 wire messages in
lnwirein line with thereview-driven updates on the BOLT taproot-gossip extension (lightning/bolts#1059).
The spec branch backing these changes is currently at
lightning/bolts#1059 (extension BOLT 7-style document
for gossip v2). The accompanying spec PR linked from each commit groups the
changes by reviewer rationale.
Each commit here is a single, independently-reviewable spec delta and is
written so that
make unit pkg=lnwireandgo build ./...both pass on itsown.
make installbuilds clean at the tip.Commits
align with BOLT 12's renumbering. Signed range expands from 0..=159 to
0..=239 and the signature TLVs in
channel_announcement_2,channel_update_2andnode_announcement_2move from type 160 to 240.add an
AssertRequiredPresenthelper and use it in the three gossip v2decoders so messages missing required TLVs are rejected up front.
extend the existing port-not-zero rule to cover tor v3 addresses too.
one — replace the split
first_block(type 2) /block_height_range(type 4) pair with a single
BlockHeightRangeTLV at type 2 holding{u32 first_block_height, tu32 num_blocks}.required
funding_txid(type 6, sha256-shaped[32]byte) so eachmessage names its funding tx explicitly.
NewAnnSigs2and the existingchanneldbtest callers are updated.drop the experimental
InboundFeeat TlvType55555 in favour of twodefaulted-on-the-wire uint32 records: type 20
inbound_fee_base_msatand type 22
inbound_fee_proportional_millionths. Positive-only.ChanEdgePolicyFromWirefolds the two fields into the existingfn.Option[lnwire.Fee]contract (None when both are 0).— introduce an
AnnouncementSigPairvalue type (node || bitcoin, 64bytes) and switch the
PartialSignaturefield to it. The previouslypre-aggregated 32-byte form is gone; receivers can now verify each half
with the standard MuSig2
PartialSigVerifyroutine.— tiny follow-up; the inbound-fee TLVs are defaulted on the wire, not
required.
sciddir — switch the
short_channel_idfield to BOLT 1'ssciddir_or_pubkeytype constrained to the 9-byte sciddir form(
<dirbyte><scid>). The separatesecond_peerTLV at type 8 becomesredundant and is removed;
IsNode1()now derives from the directionbyte.
Out of scope (deliberately)
channel_reestablish.announcement_noncesTLV at type 7my_current_funding_locked.retransmit_flags) and Make lnd go-gettable #3(
splice_lockedextensions carrying the announcement nonces) bothrely on splicing infrastructure that
lnddoes not implement yet.They are tracked but left as follow-ups.
— it is the spec-side change set's last item and is independent of
this implementation work.
Test plan
make unit pkg=lnwirepassesmake unit pkg=channeldbpasses (the only other unit suite thatbuilds AnnounceSignatures2 directly)
make unit pkg=graph/dbpasses (touchesChanEdgePolicyFromWire)make unit pkg=discoverypasses (gossip 2 consumers)make installbuilds clean at HEAD