lnwire: pull gossip v2 messages in line with BOLT taproot-gossip review updates#10837
lnwire: pull gossip v2 messages in line with BOLT taproot-gossip review updates#10837ellemouton wants to merge 10 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.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the lnwire gossip v2 implementation to align with the latest specifications from the BOLT taproot-gossip extension. The changes focus on protocol consistency, improved message validation, and refining data structures for better interoperability and compliance with the evolving gossip v2 standard. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
the
severity-critical
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Taproot gossip implementation to match the latest BOLT specifications. Major changes include transitioning AnnounceSignatures2 to use AnnouncementSigPair and FundingTxID, adopting the Sciddir format in ChannelUpdate2 to replace the SecondPeer flag, and splitting inbound fees into distinct base and proportional TLV records. Additionally, the signature TLV type was moved to 240, and mandatory TLV validation was added. Review feedback highlighted a potential integer overflow when casting uint32 inbound fees to int32 in the database models. Furthermore, several new encoder/decoder functions and exported constructors require more comprehensive documentation to comply with the repository's style guide.
| BaseFee: int32(upd.InboundFeeBaseMsat.Val), | ||
| FeeRate: int32( | ||
| upd.InboundFeeProportionalMillionths.Val, | ||
| ), |
There was a problem hiding this comment.
The cast from uint32 to int32 for inbound fees is potentially unsafe. While the PR description mentions these are "positive-only", a uint32 value exceeding math.MaxInt32 (approx. 2.1 billion) will result in a negative value when cast to int32. Given that inbound_fee_base_msat is in millisatoshis, a value of 2.1 million satoshis is plausible for a base fee. We should ensure the value fits within the destination type's range or use a larger type if lnwire.Fee allows it.
| func blockHeightRangeEncoder(w io.Writer, val interface{}, | ||
| buf *[8]byte) error { |
There was a problem hiding this comment.
Missing function comment for blockHeightRangeEncoder.
| func blockHeightRangeEncoder(w io.Writer, val interface{}, | |
| buf *[8]byte) error { | |
| // blockHeightRangeEncoder is a custom TLV encoder for the BlockHeightRange | |
| // type. | |
| func blockHeightRangeEncoder(w io.Writer, val interface{}, | |
| buf *[8]byte) error { |
References
- Every function must be commented with its purpose and assumptions, beginning with the function name. (link)
| func blockHeightRangeDecoder(r io.Reader, val interface{}, buf *[8]byte, | ||
| l uint64) error { |
There was a problem hiding this comment.
Missing function comment for blockHeightRangeDecoder.
| func blockHeightRangeDecoder(r io.Reader, val interface{}, buf *[8]byte, | |
| l uint64) error { | |
| // blockHeightRangeDecoder is a custom TLV decoder for the BlockHeightRange | |
| // type. | |
| func blockHeightRangeDecoder(r io.Reader, val interface{}, buf *[8]byte, | |
| l uint64) error { |
References
- Every function must be commented with its purpose and assumptions, beginning with the function name. (link)
| ) | ||
| } | ||
|
|
||
| func sciddirEncoder(w io.Writer, val interface{}, buf *[8]byte) error { |
There was a problem hiding this comment.
Missing function comment for sciddirEncoder.
| func sciddirEncoder(w io.Writer, val interface{}, buf *[8]byte) error { | |
| // sciddirEncoder is a custom TLV encoder for the Sciddir type. | |
| func sciddirEncoder(w io.Writer, val interface{}, buf *[8]byte) error { |
References
- Every function must be commented with its purpose and assumptions, beginning with the function name. (link)
| func sciddirDecoder(r io.Reader, val interface{}, buf *[8]byte, | ||
| l uint64) error { |
There was a problem hiding this comment.
Missing function comment for sciddirDecoder.
| func sciddirDecoder(r io.Reader, val interface{}, buf *[8]byte, | |
| l uint64) error { | |
| // sciddirDecoder is a custom TLV decoder for the Sciddir type. | |
| func sciddirDecoder(r io.Reader, val interface{}, buf *[8]byte, | |
| l uint64) error { |
References
- Every function must be commented with its purpose and assumptions, beginning with the function name. (link)
| // NewAnnouncementSigPair creates a new AnnouncementSigPair. | ||
| func NewAnnouncementSigPair(node, bitcoin btcec.ModNScalar) AnnouncementSigPair { |
There was a problem hiding this comment.
The comment for NewAnnouncementSigPair is sparse. Per the LND Style Guide, exported functions require detailed comments for the caller.
// NewAnnouncementSigPair creates a new AnnouncementSigPair from the provided
// node and bitcoin partial signatures.
func NewAnnouncementSigPair(node, bitcoin btcec.ModNScalar) AnnouncementSigPair {References
- Exported functions require detailed comments for the caller. (link)
| func announcementSigPairEncoder(w io.Writer, val interface{}, | ||
| _ *[8]byte) error { |
There was a problem hiding this comment.
Missing function comment for announcementSigPairEncoder.
| func announcementSigPairEncoder(w io.Writer, val interface{}, | |
| _ *[8]byte) error { | |
| // announcementSigPairEncoder is a custom TLV encoder for the | |
| // AnnouncementSigPair type. | |
| func announcementSigPairEncoder(w io.Writer, val interface{}, | |
| _ *[8]byte) error { |
References
- Every function must be commented with its purpose and assumptions, beginning with the function name. (link)
| func announcementSigPairDecoder(r io.Reader, val interface{}, buf *[8]byte, | ||
| l uint64) error { |
There was a problem hiding this comment.
Missing function comment for announcementSigPairDecoder.
| func announcementSigPairDecoder(r io.Reader, val interface{}, buf *[8]byte, | |
| l uint64) error { | |
| // announcementSigPairDecoder is a custom TLV decoder for the | |
| // AnnouncementSigPair type. | |
| func announcementSigPairDecoder(r io.Reader, val interface{}, buf *[8]byte, | |
| l uint64) error { |
References
- Every function must be commented with its purpose and assumptions, beginning with the function name. (link)
🔴 PR Severity: CRITICAL
🔴 Critical (9 files) — lnwire/*
🟠 High (1 file) — graph/*
AnalysisThis PR modifies multiple files in the lnwire package, which implements the core Lightning Network wire protocol messages. Changes span announcement signatures, channel announcements, channel updates, gossip timestamp range, node announcements, partial signatures, TLV encoding, and adds a new sciddir.go for SCID direction handling. The lnwire package is CRITICAL as it defines fundamental protocol message types used throughout the daemon. A line-count bump also applies: 584 non-test lines changed exceeds the 500-line threshold. The base classification is already CRITICAL so no further escalation occurs. The breadth of changes across multiple lnwire v2 gossip message types suggests this is part of the gossip v2 / Taproot Channels initiative. Protocol-level serialization and deserialization changes require careful review for correctness and compatibility. To override, add a severity-override-{critical,high,medium,low} label. |
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.
…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.
ellemouton
force-pushed
the
bolt-1059-spec-updates
branch
from
834c03d to
64acef0
Compare
1 participant
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