Re-send AnnouncementSignature not more than once per connection. #9957

gijswijs · 2025-06-17T12:59:40Z

This PR aligns our behavior with latest protocol guidelines. That
is, when we receive a signature announcement when we already have the
full proof we reply with our signature announcement once per
(re)connection.

see: lightning/bolts#1256

fixes: #9843

coderabbitai · 2025-06-17T12:59:46Z

Important

Review skipped

Auto reviews are limited to specific labels.

🏷️ Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

discovery/gossiper.go

lnwire/announcement_signatures.go

funding/manager.go

discovery/gossiper.go

discovery/gossiper_test.go

discovery/syncer.go

discovery/gossiper.go

gijswijs

Pushed new code. The only thing that still doesn't sit with me well is the simulation of the peer disconnecting.

https://github.com/gijswijs/lnd/blob/b01eea655ac4ba8d93d66c7696dc9049d36f2f7a/discovery/gossiper_test.go#L2004-L2009

Obviously when I recreate the mockPeer the state of the previous mockPeer is gone, so that reasoning is almost circular. (That also holds true for the way we simulated the gossiper before tho).

I think to truly test this we should make an itest. @ellemouton what do you think?

ellemouton

looking good - left one more suggestion regarding approach.

I think the suggestion also answers your question:

doesn't sit with me well is the simulation of the peer disconnecting.

cause now with the suggestion, simulating this is quite easily done from a unit test since the "PeerDisconnected" is just a call-back on the gossiper passed to the peer in question.

ellemouton · 2025-07-08T12:57:18Z

discovery/gossiper.go

+					remoteNSB,
+					remoteBSB, nil,
 				)
+


unaddressed

discovery/gossiper.go

ellemouton · 2025-07-16T10:59:02Z

discovery/gossiper.go

-					chanInfo.AuthProof, chanInfo, e1, e2,
+				chanAP := chanInfo.AuthProof
+				var remoteNSB []byte
+				var remoteBSB []byte


i think the name remote* is confusing cause it makes it sound like we are sending them their own proof. local* would make more sense no?

i think this was resolved without a reply? 😊

discovery/gossiper.go

ellemouton · 2025-07-16T11:04:07Z

discovery/gossiper_test.go

+	// We create an active syncer for our remote peer.
+	err = syncMgr.InitSyncState(remotePeer)
+	require.NoError(t, err, "failed to init sync state")
+	remoteSyncer := assertSyncerExistence(t, syncMgr, remotePeer)
+	assertTransitionToChansSynced(t, remoteSyncer, remotePeer)
+	assertActiveGossipTimestampRange(t, remotePeer)
+	assertSyncerStatus(t, remoteSyncer, chansSynced, ActiveSync)


hmm do we still need this?

I changed it into tCtx.gossiper.InitSyncState(remotePeer) which now handles the clearing of the map[peer][chanID]bool and also calls into syncMgr.InitSyncState(remotePeer). I've kept the assertions, which although slight overkill, are an assertion that everything went aOK so far.

discovery/mock_test.go

discovery/gossiper.go

The config file format changed. The tool golangci-lint migrate was used to migrate the old config. However old comments and also the structure of the disabled linters was preserved. Moreover the new v2 version introduced new linters, we disable 3 of them because they are very noise and we do not really want to check for them: funcorder, noinlineerr, embeddedstructfieldcheck.

The custom file is only needed in the tools directory.

Roasbeef

Straight forward solution.

Main comments are re better encapsluation, and reducing duplication in the unit test.

Roasbeef · 2025-10-03T01:18:17Z

discovery/mock_test.go

+		sentMsgs: sentMsgs,
+		quit:     quit,
+	}
+	p.disconnected.Store(disconnected)


A new addition?

Not sure if I follow. It's just a small refactor that introduces a constructor instead of a ton of &mockPeer{someKey.PubKey(), nil, nil, atomic.Bool{}}

It's never called with anything other than disconnected == false so in theory we could do without that parameter and just initialize it with the zero value of atomic.Bool. It probably doesn't make sense to construct a disconnected peer from the get go.

What do you think?

discovery/gossiper.go

Roasbeef · 2025-10-03T01:19:16Z

discovery/gossiper.go

+					"announcement signatures to peer=%x",
 					ann.ChannelID, peerID)

-				ca, _, _, err := netann.CreateChanAnnouncement(


Any reason to not also send the chan ann as well? So send the ann sig, and the channel ann.

BOLT07 requires us to send the ann sig: https://github.com/ddustin/lightning-rfc/blob/a2d314df9a791dd1a38be7c34d201bf798ad3525/07-routing-gossip.md?plain=1#L93

Is there a good reason to also send the channel ann? We should assume that our peers do not expect a channel ann.

Roasbeef · 2025-10-03T01:20:55Z

discovery/gossiper.go

+	// sentAnnSigs tracks which announcement signatures we've sent to which
+	// peers. We'll use this to ensure we don't re-send the same signatures
+	// to a peer during a single connection.
+	sentAnnSigs map[route.Vertex]map[lnwire.ChannelID]struct{}


Do we need to limit the size of this map?

Let's also further encapsulate this new data structure. We can expose easy to use methods that: delete, check existence, and add. All while handling thread safety on behalf of the caller.

I don't think it's smart to limit the size of the map. We need to keep state for every channel anyway, so I wouldn't inadvertently run into the limit of the map. It's hardly a DOS-vector since we only ever hit this map after channel_ready has been sent and received AND the funding transaction has enough confirmations. That's enough disincentivization if you'd ask me.

I agree with the proposed methods tho.

I've encapsulated the map in its own struct sentAnnSigsTracker. It uses its own sync.Mutex to provide better lock granularity. The memory overhead of an extra mutex is very small, and it (hopefully) reduces contention.

discovery/gossiper_test.go

…ls-feature Use golangs new tool feature

And ensure that both versions 1 and 2 implement it.

Define a GossipVersion enum along with a GossipMessage interface to be satisfied by all gossip related messages. This will be useful later on when we want to make decisions based on the protocol version that a message is part of.

Since the gossip protocols are completely disjoint, we need to treat messages on the two protocols completely separately and should not let rejections on one protocol affect how we treat messages on the other.

[g175] multi: small G175 preparations

Use a blank identifier when values are intentionally unused.

two constructor funcs that create and return a pointer to an AnnounceSignatures1. `NewAnnSigFromSignature` takes `nodeSig` and `bitcoinSig` as pointers to `*ecdsa.Signature`. `NewAnnSigFromWireECDSA` takes those arguments in the raw 64-byte format we expect.

We change the discovery gossiper to send a signature announcement in response to receiving a signature announcement, but only if the full proof is available. Previously the behavior was to send the channel announcement.

This change aligns our behavior with latest protocol guidelines. That is, when we receive a signature announcement when we already have the full proof we reply with our signature announcement once per (re)connection. see: lightning/bolts#1256

gijswijs · 2025-10-08T15:53:00Z

I think I addressed all comments. Ready for a new round of code review.

ellemouton

looks good! i think just some commit restructuring needed. Other than that, logic LGTM!

discovery/gossiper.go

ellemouton · 2025-10-09T11:28:50Z

discovery/gossiper.go

-					chanInfo.AuthProof, chanInfo, e1, e2,
+				chanAP := chanInfo.AuthProof
+				var remoteNSB []byte
+				var remoteBSB []byte


i think this was resolved without a reply? 😊

ellemouton · 2025-10-09T11:29:09Z

discovery/gossiper.go

+					remoteNSB,
+					remoteBSB, nil,
 				)
+


ellemouton · 2025-10-09T11:29:58Z

discovery/gossiper.go

+				if peerIsFirstNode {
+					localNSB = chanAP.NodeSig2Bytes
+					localBSB = chanAP.BitcoinSig2Bytes
 				} else {
-					remoteNSB = chanAP.NodeSig1Bytes
-					remoteBSB = chanAP.BitcoinSig1Bytes
+					localNSB = chanAP.NodeSig1Bytes
+					localBSB = chanAP.BitcoinSig1Bytes


ah, ok yeah, for sake of review, rather make these changes in the commit where the variables are introduced

lightninglabs-deploy · 2026-01-22T21:00:41Z

@Roasbeef: review reminder
@gijswijs, remember to re-request review from reviewers when ready

gijswijs changed the title ~~Resend-annsig~~ Re-send AnnouncementSignature not more than once per connection. Jun 17, 2025

gijswijs force-pushed the resend-annsig branch from 6b7c85c to 14fe1b0 Compare June 17, 2025 14:48

saubyk requested review from bitromortac and ellemouton June 17, 2025 16:40

saubyk assigned gijswijs Jun 17, 2025

saubyk added this to lnd v0.20 Jun 17, 2025

saubyk added this to the v0.20.0 milestone Jun 17, 2025

saubyk moved this to In review in lnd v0.20 Jun 17, 2025

gijswijs force-pushed the resend-annsig branch from 14fe1b0 to 736087e Compare June 17, 2025 17:27

gijswijs added the 32-bit label Jun 18, 2025

ellemouton requested changes Jun 18, 2025

View reviewed changes

saubyk removed this from lnd v0.20 Jun 30, 2025

gijswijs force-pushed the resend-annsig branch from 736087e to b01eea6 Compare July 7, 2025 19:30

This comment was marked as duplicate.

Sign in to view

gijswijs commented Jul 8, 2025

View reviewed changes

gijswijs requested a review from ellemouton July 8, 2025 13:23

ellemouton reviewed Jul 16, 2025

View reviewed changes

bitromortac removed their request for review August 27, 2025 07:19

saubyk modified the milestones: v0.20.0, v0.21.0 Sep 5, 2025

tools: use new golang 1.24 tool directive

c553749

gijswijs force-pushed the resend-annsig branch 2 times, most recently from 462f1ee to f0dc8d3 Compare September 22, 2025 21:06

ziggie1984 added 5 commits September 23, 2025 08:40

linter: remove unsupported linters from the codebase

0a7cc02

CI: also use go tool for gosimports

055da5f

CI: remove unnecessary custom-gcl.yml file

a21f441

The custom file is only needed in the tools directory.

makefile: add linter config check

8cbfca8

Roasbeef requested changes Oct 3, 2025

View reviewed changes

Roasbeef and others added 15 commits October 6, 2025 14:08

Merge pull request lightningnetwork#10209 from ziggie1984/use-new-too…

67c9dbb

…ls-feature Use golangs new tool feature

lnwire: add NodeAnnouncement interface

211e6ad

And ensure that both versions 1 and 2 implement it.

lnwire: define GossipVersion and GossipMessage

49b39f1

Define a GossipVersion enum along with a GossipMessage interface to be satisfied by all gossip related messages. This will be useful later on when we want to make decisions based on the protocol version that a message is part of.

lnwire: let gossip messages implement GossipMessage

349dedb

graph/db: use lnwire.GossipVersion instead of ProtocolVersion

eab2614

discovery: let reject cache use gossip version in key

6dbefd9

Since the gossip protocols are completely disjoint, we need to treat messages on the two protocols completely separately and should not let rejections on one protocol affect how we treat messages on the other.

Merge pull request lightningnetwork#10267 from ellemouton/g175Prep

30decfc

[g175] multi: small G175 preparations

channeldb: fix typo

54a5498

discovery: improve wording error message

5276413

discovery: use blank identifier when possible

0eb8dd5

Use a blank identifier when values are intentionally unused.

discovery: add constructor for mockPeer

6dd32af

discovery: send sig ann when full proof available

511be4c

We change the discovery gossiper to send a signature announcement in response to receiving a signature announcement, but only if the full proof is available. Previously the behavior was to send the channel announcement.

discovery: send signature announcement once

9402ec9

This change aligns our behavior with latest protocol guidelines. That is, when we receive a signature announcement when we already have the full proof we reply with our signature announcement once per (re)connection. see: lightning/bolts#1256

docs: update release notes for 0.21.0

2e601d8

gijswijs force-pushed the resend-annsig branch from f0dc8d3 to 2e601d8 Compare October 8, 2025 15:45

gijswijs changed the base branch from master to 0-21-0-staging October 8, 2025 15:48

gijswijs requested review from Roasbeef and ellemouton October 8, 2025 19:03

ellemouton reviewed Oct 9, 2025

View reviewed changes

yyforyongyu force-pushed the 0-21-0-staging branch from 496c5e3 to 8eb29f1 Compare October 13, 2025 16:39

yyforyongyu force-pushed the 0-21-0-staging branch 3 times, most recently from 57eb251 to 6fcdbbb Compare November 7, 2025 09:15

yyforyongyu force-pushed the 0-21-0-staging branch from 706b22a to 95b84a8 Compare November 12, 2025 14:54

yyforyongyu changed the base branch from 0-21-0-staging to master November 13, 2025 08:59

Re-send AnnouncementSignature not more than once per connection. #9957

Are you sure you want to change the base?

Re-send AnnouncementSignature not more than once per connection. #9957

Conversation

gijswijs commented Jun 17, 2025

Uh oh!

coderabbitai bot commented Jun 17, 2025

Review skipped

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

gijswijs left a comment

Choose a reason for hiding this comment

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gijswijs commented Oct 8, 2025

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodeRabbit Configuration File (`.coderabbit.yaml`)