Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More clarifications around channel_announcement handling #1220

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

t-bast
Copy link
Collaborator

@t-bast t-bast commented Jan 15, 2025

This is a follow-up of #1215, where we keep reworking the announcement requirements.

We remove the rationale around deferring after channel_ready since that has been changed years ago and shouldn't be an issue anymore.

We slightly rework the wording to minimize future conflicts with the splicing PR.

While nodes are free to send announcement_signatures whenever they feel that the channel is safe from reorg, we disallow broadcasting the channel_announcement before at least 6 confirmations: this gives nodes a simple heuristic to ignore channel_announcements for remote channels that aren't spec-compliant, without having to deal with the extra cost of managing reorgs.

This is a follow-up of lightning#1215, where we keep reworking the announcement
requirements.

We remove the rationale around deferring after `channel_ready` since
that has been changed years ago and shouldn't be an issue anymore.

We slightly rework the wording to minimize future conflicts with the
splicing PR.

While nodes are free to send `announcement_signatures` whenever they
feel that the channel is safe from reorg, we disallow broadcasting the
`channel_announcement` before at least 6 confirmations: this gives nodes
a simple heuristic to ignore `channel_announcement`s for remote channels
that aren't spec-compliant, without having to deal with the extra cost
of managing reorgs.
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall seems like a good clarification. A few comments nits:

07-routing-gossip.md Show resolved Hide resolved
07-routing-gossip.md Outdated Show resolved Hide resolved
07-routing-gossip.md Outdated Show resolved Hide resolved
- remove conflicting `announcement_signatures` requirements
- allow a buffer of a few blocks to account for late block arrival
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

07-routing-gossip.md Outdated Show resolved Hide resolved
07-routing-gossip.md Show resolved Hide resolved
07-routing-gossip.md Show resolved Hide resolved
We don't need more than 6 confirmations, we need at least that.
Copy link
Contributor

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM🌹

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants