Repository navigation
lnwire: pull gossip v2 messages in line with BOLT taproot-gossip review updates - #11164
ViktorT-11 wants to merge 15 commits into
Conversation
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 |
ac6a7e6 to
fa3d115
Compare
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
Nice, I took a first look.
|
|
||
| // SciddirLen is the wire length of a Sciddir: one direction byte followed by | ||
| // the 8-byte short_channel_id. | ||
| const SciddirLen = 9 |
There was a problem hiding this comment.
you could take a look at lnwire/intro_node.go, maybe we can reuse that or unify
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.
There was a problem hiding this comment.
I think we may be able to delete sciddir.go and reuse the existing data type a bit more, see other comment.
| BaseFee: int32(baseFee), | ||
| FeeRate: int32(propFee), |
There was a problem hiding this comment.
I think overflow is possible here
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!
There was a problem hiding this comment.
I think, rejecting values above math.MaxInt32 is a good solution for now and keep it for a follow-up.
There was a problem hiding this comment.
Added this as a part of the detailed check list for Type Representation in #10293
|
@yyforyongyu: review reminder |
237eb33 to
d366b60
Compare
|
@yyforyongyu: review reminder |
d366b60 to
059c3ea
Compare
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Left a few comments, think we are close!
| // DecodeWithParsedTypes helpers). It is used to enforce the spec's | ||
| // reader-side requirement that compulsory TLVs are present in a received | ||
| // pure-TLV message. | ||
| func AssertRequiredPresent(typeMap tlv.TypeMap, required ...tlv.Type) error { |
There was a problem hiding this comment.
Q: is there a plan to use this method outside lnwire pkg? if not we should keep it private.
There was a problem hiding this comment.
Edited this to keep it private. I think it's likely that this was planned by Elle to be used by some other package, but since I'm taking over and I don't have a clear answer to that at the moment, I changed for now and will update it in case we'd need it exported in a future PR.
| // type number is supplied by the wrapping RecordT. | ||
| func (b *BlockHeightRange) Record() tlv.Record { | ||
| sizeFunc := func() uint64 { | ||
| return 4 + tlv.SizeTUint32(b.NumBlocks) |
There was a problem hiding this comment.
nit: add some inline docs explaining where the 4 is coming from?
There was a problem hiding this comment.
Added a comment to explain that the 4 accounts for FirstBlockHeight, which is encoded as a fixed-width uint32.
|
|
||
| // FundingTxID is the txid of the funding transaction that this | ||
| // announcement signature covers. For an initial channel announcement | ||
| // this is the original funding transaction; for a spliced channel it |
There was a problem hiding this comment.
Q: are we designing this for splicing too? I don't think it's in the roadmap right?
There was a problem hiding this comment.
The spec PR does actually account for splicing after T-bast feedback, hence why I think this comment mentioned it. However, since it's currently not on our roadmap, I removed the comment for now, as it's probably just more confusing than helpful.
| ) | ||
| } | ||
|
|
||
| if c.InboundFeeBaseMsat.Val != defaultInboundFeeBaseMsat { |
There was a problem hiding this comment.
I think we need to preserve whether types 20/22 were present here. A peer may explicitly encode the default 0 (the spec only says SHOULD omit it), but AllRecords drops the record and reconstructs different signed bytes. Could we track presence separately and add an explicit-zero signature test?
There was a problem hiding this comment.
This is wider than 20 and 22. It affects every field with a default: types 0, 6, 10, 12, 16, 18, 20 and 22 of channel_update_2, and chain_hash of channel_announcement_2. I checked type 10 by execution: 0a 02 00 50 is dropped on re-encode. Type 14 fails the other way, because lnd always emits it.
The bolt12 package already solves this: every defaulted field is an OptionalRecordT, the decoder records presence with lnwire.SetOptFromMap, and the encoder emits with lnwire.AddOpt, so a message re-encodes exactly as it was sent. The default applies only when a caller reads an absent field. The same approach fixes the optional features of channel_announcement_2.
There was a problem hiding this comment.
Nice catch! I added @bitromortac suggestion, which uses the same approach as the bolt12 package.
| return err | ||
| } | ||
|
|
||
| v.Node.SetBytes(&nodeBytes) |
There was a problem hiding this comment.
I think we need to check the return value from SetBytes here. Values greater than or equal to the curve order are reduced modulo n, so invalid partial sigs are accepted. Could we reuse PartialSig for both halves and reject overflow there?
There was a problem hiding this comment.
Great catch! Addressed.
|
|
||
| // truncatedUint32Record preserves a typed record's value and type while using | ||
| // BOLT's tu32 encoding, which omits leading zero bytes. | ||
| func truncatedUint32Record[T tlv.TlvType]( |
There was a problem hiding this comment.
Q: should the tu32 encoding live in the fee value type instead? These exported fields are still RecordT[..., uint32], so calling Record() directly emits fixed-width u32 and every caller has to remember this wrapper.
There was a problem hiding this comment.
I'll address this specific feedback after the next review round, as I specifically missed addressing this one.
| func torV3AddrsEncoder(w io.Writer, val interface{}, _ *[8]byte) error { | ||
| if v, ok := val.(*TorV3Addrs); ok { | ||
| for _, addr := range *v { | ||
| if addr.Port == 0 { |
There was a problem hiding this comment.
I think this also needs to reject ports outside 1..65535 before the uint16 cast below. For example, 65536 passes this check and is encoded as port 0. Could we use a stable port error and add boundary tests?
|
|
||
| // SciddirLen is the wire length of a Sciddir: one direction byte followed by | ||
| // the 8-byte short_channel_id. | ||
| const SciddirLen = sciddirLen |
There was a problem hiding this comment.
nit: please squash f4bb4e91c into the preceding sciddir commit and fix the f - subject before merge
There was a problem hiding this comment.
I'll do that as soon as we've decided if we'd like to use the fixup approach or not :)
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
Thanks for the fixes. I cross-checked every TLV of the five gossip v2 messages against the spec and there are some discrepancies still.
| // `0` (refers to node_id_1 of the corresponding channel_announcement_2) or | ||
| // `1` (refers to node_id_2). The `pubkey` form of `sciddir_or_pubkey` is | ||
| // rejected wherever a Sciddir is used. | ||
| type Sciddir struct { |
There was a problem hiding this comment.
The spec types this field as BOLT 1's sciddir_or_pubkey, constrained to the sciddir form. It has no separate sciddir type. lnwire already implements sciddir_or_pubkey as IntroductionNode, so SciddirIntro could hold this field, with a small sciddir-only record that rejects the pubkey form with its own error. That removes the second implementation of the same 9 bytes.
There was a problem hiding this comment.
Thanks for the suggested fix. I'm still not sure that the applied fixup is more simple than the original implementation, as it removes some abstraction that was nice to separate.
I've therefore kept the fixup as a fixup, so that we can discuss if you have a strong preference that it should be applied, or if we should just go with the original implemenation.
There was a problem hiding this comment.
I'm not sure but would be nice to have a single implementation of the same type somewhere, so non-blocking.
| BaseFee: int32(baseFee), | ||
| FeeRate: int32(propFee), |
There was a problem hiding this comment.
I think, rejecting values above math.MaxInt32 is a good solution for now and keep it for a follow-up.
| ) | ||
| } | ||
|
|
||
| if c.InboundFeeBaseMsat.Val != defaultInboundFeeBaseMsat { |
There was a problem hiding this comment.
This is wider than 20 and 22. It affects every field with a default: types 0, 6, 10, 12, 16, 18, 20 and 22 of channel_update_2, and chain_hash of channel_announcement_2. I checked type 10 by execution: 0a 02 00 50 is dropped on re-encode. Type 14 fails the other way, because lnd always emits it.
The bolt12 package already solves this: every defaulted field is an OptionalRecordT, the decoder records presence with lnwire.SetOptFromMap, and the encoder emits with lnwire.AddOpt, so a message re-encodes exactly as it was sent. The default applies only when a caller reads an absent field. The same approach fixes the optional features of channel_announcement_2.
|
|
||
| // SciddirLen is the wire length of a Sciddir: one direction byte followed by | ||
| // the 8-byte short_channel_id. | ||
| const SciddirLen = 9 |
There was a problem hiding this comment.
I think we may be able to delete sciddir.go and reuse the existing data type a bit more, see other comment.
c048aa6 to
a10815f
Compare
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 taproot-gossip extension defines TLV 11 of node_announcement_2 as dns_hostnames, a list of dns_hostname entries. Each entry is a u16 hostname length, the hostname and a u16 port. lnd modelled the field as a single DNSAddress with no length prefix. A conformant peer therefore read the first two bytes of lnd's hostname as a length, and lnd read a conformant entry's length bytes as part of the hostname. ValidateDNSAddr then rejected the hostname, so lnd dropped any conformant announcement that carried a DNS address. A node could also only announce one hostname. The new DNSAddrs type encodes the list as the spec defines it. The codec does not validate the hostname or the port. The list is in the signed range and the signature digest is rebuilt from the decoded records, so the codec has to round-trip every entry exactly. Deciding which entries are usable happens above the codec. DNSAddress and ValidateDNSAddr stay as they are, because the v1 announcement shares them.
BOLT 7 forbids a zero port on every announced address type, and tells a receiver to ignore such an address while it keeps the rest of the announcement. Add NodeAnnouncement2.Addresses, which returns the addresses a node can be reached at and leaves out any entry with a zero port and any DNS entry whose hostname is not valid. Consumers of a received announcement use it instead of the raw address fields. The filter cannot sit in the codec. The address fields are in the signed range, and the signature digest is rebuilt by re-encoding the decoded records, so the codec round-trips every entry. For the same reason the codec does not enforce the sender rule. That rule belongs where lnd builds its own node_announcement_2.
Allocate a separate IP buffer for every IPv4 and IPv6 address decoded from a node_announcement_2. Reusing one buffer caused every address of the same type to reference the last IP decoded. Use io.ReadFull to require complete address and port values, and update the zero-port test to use distinct IPs so it detects buffer aliasing.
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.
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.
The gossip v2 spec types the outbound fee fields of channel_update_2 as tu32, its htlc_minimum_msat and htlc_maximum_msat as tu64, and the capacity_satoshis of channel_announcement_2 as tu64. These fields are older than this series and were fixed-width or BigSize, so lnd could not read a conformant message, and a conformant peer misread lnd's. Use the truncated encoding for all of them. truncatedUint32Record covers the fee fields. A new truncatedUint64Record covers the rest, and takes any type whose underlying type is uint64, such as MilliSatoshi. MilliSatoshi.Record keeps its BigSize form, which other messages rely on. The tests cover canonical and malformed values for the fee fields, and the fixtures carry the truncated bytes.
Gossip v2 signatures cover the exact TLV records sent. Decoding currently replaces absent fields with their defaults, while encoding decides whether to include those fields from their values. As a result, an explicitly encoded default is omitted when the message is re-encoded, changing the signed byte stream. Represent optional fields using OptionalRecordT and retain their presence during decoding. Apply protocol defaults only through message accessors, and emit every field that was present even when it contains its default.
…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.
a10815f to
5082b81
Compare
|
Thanks a lot for the reviews, and suggested fixes @bitromortac & @yyforyongyu 🙏! @bitromortac, I've looked through your suggested fixes and applied most of them, plus left a open question in #11164 (comment). This is now ready for another review. Also, as a reminder to my self, this comment needs to be addressed after the next review round: #11164 (comment) |
| // | ||
| // The codec does not validate the hostname or the port. The list is in the | ||
| // signed range and the signature digest is rebuilt from the decoded records, | ||
| // so the codec must round-trip every entry exactly. |
There was a problem hiding this comment.
nit: the signature digest is not relevant in this context, so could remove that comment
| // `0` (refers to node_id_1 of the corresponding channel_announcement_2) or | ||
| // `1` (refers to node_id_2). The `pubkey` form of `sciddir_or_pubkey` is | ||
| // rejected wherever a Sciddir is used. | ||
| type Sciddir struct { |
There was a problem hiding this comment.
I'm not sure but would be nice to have a single implementation of the same type somewhere, so non-blocking.
|
@yyforyongyu: review reminder |
6 participants
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