discovery+graph: move funding tx validation to the gossiper #9478

ellemouton · 2025-02-05T09:20:19Z

Depends on:

This commit is a pure refactor. We move the transaction validation
(existence, spentness, correctness) from the `graph.Builder` to the
gossiper since this is where all protocol level checks should happen.
All tests involved are also updated/moved.

part of #9494

coderabbitai · 2025-02-05T09:20:27Z

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.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ 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.
- Generate unit testing code for this file.
- 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 generate unit testing code for this file.
- @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 generate unit testing code.
- @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.

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. (Beta)
@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.

graph/builder_test.go

bhandras

🌮

bhandras · 2025-02-10T12:45:21Z

graph/interfaces.go

@@ -88,6 +88,9 @@ type ChannelGraphSource interface {

 	// ForEachNode is used to iterate over every node in the known graph.
 	ForEachNode(func(node *models.LightningNode) error) error
+
+	// AddZombieEdge marks the channel with the given ID as a zombie edge.
+	AddZombieEdge(chanID uint64) error


nit: might as well be renamed to SetEdgeZombie or MarkEdgeZombie. wdyt?

yeah MarkEdgeZombie does feel more accurate

cool - will do 👍

bhandras · 2025-02-10T12:45:46Z

discovery/gossiper_test.go

@@ -107,6 +107,12 @@ func (r *mockGraphSource) AddNode(node *models.LightningNode,
 	return nil
 }

+func (r *mockGraphSource) AddZombieEdge(scid uint64) error {


nit: typo in the commit message (transction) 🤓

yyforyongyu

Thanks a lot for the PR! Been wanting to fix this for a while now, really happy to see this happening❤️ A few questions otherwise good to go!

yyforyongyu · 2025-02-11T02:09:30Z

graph/interfaces.go

@@ -88,6 +88,9 @@ type ChannelGraphSource interface {

 	// ForEachNode is used to iterate over every node in the known graph.
 	ForEachNode(func(node *models.LightningNode) error) error
+
+	// AddZombieEdge marks the channel with the given ID as a zombie edge.
+	AddZombieEdge(chanID uint64) error


yeah MarkEdgeZombie does feel more accurate

yyforyongyu · 2025-02-11T04:49:43Z

discovery/gossiper.go

@@ -2724,7 +2704,7 @@ func (d *AuthenticatedGossiper) handleChanAnnouncement(nMsg *networkMsg,
 				d.banman.incrementBanScore(nMsg.peer.PubKey())
 			}

-		default:
+		case err != nil:


think we can just keep the old default since the error must be non-nil here.

yyforyongyu · 2025-02-11T04:54:29Z

graph/builder.go

-			return fmt.Errorf("unable to add edge: %w", err)
-		}
+	if err := b.cfg.Graph.AddChannelEdge(edge, op...); err != nil {
+		return errors.Errorf("unable to add edge: %v", err)


Suggested change

return errors.Errorf("unable to add edge: %v", err)

return errors.Errorf("unable to add edge: %w", err)

good catch, thanks 🙏

yyforyongyu · 2025-02-11T05:00:25Z

discovery/gossiper.go

+func (d *AuthenticatedGossiper) validateFundingTransaction(
+	ann *lnwire.ChannelAnnouncement1,
+	tapscriptRoot fn.Option[chainhash.Hash]) (fn.Option[wire.OutPoint],
+	fn.Option[btcutil.Amount], error) {


don't think they need to be options?

yeah good point - i think in the PoC i have they made more sense as options but think that is for later functionality - so defs agree that for this PR they are not needed - will revert 👍

i think the og reason was case I had the if AssumeValid { return nil, nil } in here meaning we'd have a nil error but also nil return values. But in the latest iteration ive moved that check outside of the validate method (ie, now we only call validate if !AssumeValid.

yyforyongyu · 2025-02-11T05:06:42Z

discovery/gossiper.go

+			// we'll mark the edge itself as a zombie so we don't
+			// continue to request it. We use the "zero key" for
+			// both node pubkeys so this edge can't be resurrected.
+			zErr := d.cfg.Graph.AddZombieEdge(scid.ToUint64())


Think it'd be nice to move this outside of the method to keep the method's responsibility to be purely validation, plus we are catching these errors outside the validation anyway - but also non-blocking as this can be done in a future PR.

totally agree - but yeah let's do in follow up as this would be more than a code move then.

discovery/gossiper.go

yyforyongyu · 2025-02-11T05:12:27Z

discovery/gossiper.go

+		}
+
+		// Otherwise, this is just a regular rejected edge.
+		key := newRejectCacheKey(


looks like these lines are repetitive - another day another PR🤓

cool yeah - can do clean-up a follow up just to keep this commit a code-move 🫡

yyforyongyu · 2025-02-11T05:16:55Z

graph/builder.go

 	// As a new edge has been added to the channel graph, we'll update the
 	// current UTXO filter within our active FilteredChainView so we are
 	// notified if/when this channel is closed.
 	filterUpdate := []graphdb.EdgePoint{
 		{
 			FundingPkScript: fundingPkScript,
-			OutPoint:        *fundingPoint,
+			OutPoint:        edge.ChannelPoint,


If we move b.cfg.ChainView.UpdateFilter to the gossiper we can save us from doing the relatively expensive makeFundingScript again here.

with the latest update, we no longer re-calculate it and instead pass it through from the gossiper.

I think it is better to let the builder manage the ChainView anyways 👍

The `graph.Builder`'s `addZombieEdge` method is currently called during funding transaction validation for the case where the funding tx is not found. In preparation for moving this code to the gossiper, we export the method and add it to the ChannelGraphSource interface so that the gossiper will be able to call it later on.

This is in preparation for the commit where we move across all the funding tx validation so that we can test that we are correctly updating the zombie index.

in preparation for later on when we need to know when to skip funding transaction validation.

Here, we add a new fundingTxOption modifier which will configure how we set-up expected calls to the mock Chain once we have moved funding tx logic to the gossiper. Note that in this commit, these modifiers don't yet do anything.

In preparation for an upcoming commit which will move all channel funding tx validation to the gossiper, we first move the helper method which helps build the expected funding transaction script based on the fields in the channel announcement. We will still want this script later on in the builder for updating the ChainView though, and so we pass this field along with the ChannelEdgeInfo. With this change, we can remove the TapscriptRoot field from the ChannelEdgeInfo since the only reason it was there was so that the builder could reconstruct the full funding script.

As we move the funding transaction validation logic out of the builder and into the gossiper, we want to ensure that the behaviour stays consistent with what we have today. So we should aquire this lock before performing any expensive checks such as building the funding tx or valdating it.

This commit is a pure refactor. We move the transaction validation (existence, spentness, correctness) from the `graph.Builder` to the gossiper since this is where all protocol level checks should happen. All tests involved are also updated/moved.

bhandras

LGTM, very nice refactor! ✨

bhandras · 2025-02-12T15:37:36Z

discovery/gossiper.go

+			return wire.OutPoint{}, 0, nil, zErr
+		}
+
+		return wire.OutPoint{}, 0, nil, fmt.Errorf("%w: %w",


nit: previously we had some more context for this error: "output failed validation". afaict it's ok to remove but just flagging in case it wasn't intentional. Same in line 3737.

yyforyongyu

🚢

yyforyongyu · 2025-02-13T06:53:08Z

graph/db/models/channel_edge_info.go

-	TapscriptRoot fn.Option[chainhash.Hash]
+	// FundingScript holds the script of the channel's funding transaction.
+	//
+	// NOTE: this is not currently persisted and so will not be present if


ellemouton self-assigned this Feb 5, 2025

ellemouton added graph gossip refactoring validation labels Feb 5, 2025

ellemouton force-pushed the graph3 branch from e496c0d to 11aa2fc Compare February 5, 2025 09:59

ellemouton mentioned this pull request Feb 5, 2025

discovery+graph: various preparations for moving funding tx validation to the gossiper #9477

Merged

ellemouton force-pushed the graph3 branch 4 times, most recently from 68967bd to eec439a Compare February 10, 2025 12:21

ellemouton requested review from yyforyongyu and bhandras February 10, 2025 12:35

ellemouton commented Feb 10, 2025

View reviewed changes

graph/builder_test.go Show resolved Hide resolved

ellemouton force-pushed the graph3 branch 2 times, most recently from 7c10f43 to c3c9eb3 Compare February 10, 2025 12:51

bhandras approved these changes Feb 10, 2025

View reviewed changes

ellemouton mentioned this pull request Feb 10, 2025

Graph Abstraction & Gossiper->to->DB stack clean-up with Remote Graph & Gossip 1.75 projects in mind #9494

Open

8 tasks

yyforyongyu reviewed Feb 11, 2025

View reviewed changes

ellemouton added 6 commits February 12, 2025 13:15

graph: add IsZombieEdge method

00f5fd9

This is in preparation for the commit where we move across all the funding tx validation so that we can test that we are correctly updating the zombie index.

discovery: add AssumeChannelValid config option

22e391f

in preparation for later on when we need to know when to skip funding transaction validation.

discovery: prepare tests for preparing the mock chain

8a07bb0

Here, we add a new fundingTxOption modifier which will configure how we set-up expected calls to the mock Chain once we have moved funding tx logic to the gossiper. Note that in this commit, these modifiers don't yet do anything.

ellemouton force-pushed the graph3 branch from c3c9eb3 to e7bc5e1 Compare February 12, 2025 13:01

ellemouton force-pushed the graph3 branch from e7bc5e1 to e5db0d6 Compare February 12, 2025 13:48

ellemouton requested a review from bhandras February 12, 2025 14:54

ellemouton requested a review from yyforyongyu February 12, 2025 14:54

bhandras approved these changes Feb 12, 2025

View reviewed changes

yyforyongyu approved these changes Feb 13, 2025

View reviewed changes

yyforyongyu merged commit 9c2c95d into lightningnetwork:master Feb 13, 2025
30 checks passed

ellemouton deleted the graph3 branch February 13, 2025 07:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

discovery+graph: move funding tx validation to the gossiper #9478

discovery+graph: move funding tx validation to the gossiper #9478

ellemouton commented Feb 5, 2025 •

edited

Loading

coderabbitai bot commented Feb 5, 2025 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

bhandras left a comment

bhandras Feb 10, 2025

yyforyongyu Feb 11, 2025

ellemouton Feb 11, 2025

bhandras Feb 10, 2025

yyforyongyu left a comment

yyforyongyu Feb 11, 2025

yyforyongyu Feb 11, 2025

ellemouton Feb 11, 2025

yyforyongyu Feb 11, 2025

ellemouton Feb 11, 2025

yyforyongyu Feb 11, 2025

ellemouton Feb 11, 2025

ellemouton Feb 12, 2025

yyforyongyu Feb 11, 2025

ellemouton Feb 11, 2025

yyforyongyu Feb 11, 2025

ellemouton Feb 11, 2025

yyforyongyu Feb 11, 2025

ellemouton Feb 12, 2025

bhandras left a comment

bhandras Feb 12, 2025

yyforyongyu left a comment

yyforyongyu Feb 13, 2025

	return errors.Errorf("unable to add edge: %v", err)
	return errors.Errorf("unable to add edge: %w", err)

discovery+graph: move funding tx validation to the gossiper #9478

discovery+graph: move funding tx validation to the gossiper #9478

Conversation

ellemouton commented Feb 5, 2025 • edited Loading

coderabbitai bot commented Feb 5, 2025 • edited Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton commented Feb 5, 2025 •

edited

Loading

coderabbitai bot commented Feb 5, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)