[g175] graph/db: merge g175 types-prep side branch #10414
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary of ChangesHello @ellemouton, 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 focuses on preparing the lightning network daemon's graph database for future compatibility with gossip version 2 nodes. It introduces necessary schema changes, code modifications, and a versioned graph structure to ensure a smooth transition and graceful handling of different gossip versions. The changes primarily affect the graph database layer and aim to enhance the system's ability to adapt to evolving network standards. Highlights
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is a significant refactoring to prepare the graph database for handling gossip V2 nodes. The core changes include renaming V1Store to Store, introducing a VersionedGraph wrapper to handle different gossip versions, and updating various parts of the codebase to use this new abstraction. The changes are well-structured and consistently applied. My review found a couple of minor style guide violations regarding the formatting of switch statements, for which I've provided suggestions.
|
@ellemouton, remember to re-request review from reviewers when ready |
Add a new migration that updates the graph tables (nodes, channels and policies) in preparation for the new columns required for V2 announcements. This migration has to be added to the set of "live" migrations instead of "dev only" since it edits the columns of existing tables and so changes the existing sql models. We are going to prep the SQLStore code to handle the V2 types in the coming commits, so we need this migration to be in place. In this commit we also remove the TestSchemaMigrationIdempotency test since this test fails with the new "ALTER TABLE" migrations which dont have "IF NOT EXISTS" options like tables and indexes do. Migrations should be idempotent anyways due to the migration tracker file and/or the sqlc migration tracker.
The underling store will store gossip messages across gossip versions and we will instead expose version parameters on many of the methods. So this interface really just abstracts the underlying store/schema type.
We add a new NewV2Node constructor which takes a new NodeV2Fields as a parameter. This NodeV2Fields struct defines the fields that can be set in a models.Node if the version is V2.
Here we update the UpdateNode query so that it can be used to insert the new blockheight field for a v2 node.
Instead of embedding Store in ChannelGraph so that any methods of the Store interface not implemented by the ChannelGraph are redirected to the underlying Store, we update things in this commit to instead make the "redirection" explicit. This is in preparation for changes we will make soon where some underlying store methods will take an explicit "version" parameter but then we will keep the ChannelGraph methods as is so that existing call-sites dont all need to be updated. We will then add "Versioned" ChannelGraph wrapper which decides the version use. Initially, most call-sites will just create a wrapped V1 ChannelGraph so that the logic remains as it is today.
Update the SQLStore node writer and readers to handle V2 ndoes. Currently no logic will actually add such nodes. The following commits will update what is needed so that CRUD for v2 nodes can be tested.
ellemouton
force-pushed
the
elle-g175Prep-base
branch
from
1d85426 to
142689a
Compare
Collaborator
Author
|
^ rebase on master |
HasNode currently is very v1 specific since it returns a time.Time timestamp which is specific to V1 node announcements. However, it is mostly only used for the "exists" return value. So here we split it up into HasNode which just checks existence and HasV1Node which retains the same behavaiour as before.
Update some of the node related graph CRUD methods to take a version rather than hardcoding them in the SQLStore layer. Move the version up one layer instead. This will make it easier to make it configurable later on.
Add an isSQLDB variable that will let us quickly check in tests if the backing DB is KVStore or SQLStore.
For now, all instances will use V1 only so as not to change behaviour yet.
This will be used to run sub-tests with v2 data.
Convert a few more CRUD methods to handle v2 nodes. Then update some more tests to be run against both v1 and v2 nodes.
Remove the cached parsed signature field and its lazy getter method from ChannelEdgePolicy. This field was unused throughout the codebase and the signature is already stored as raw bytes in SigBytes. The SetSigBytes method is updated to remove the cache invalidation logic.
Replace [33]byte with route.Vertex for NodeKey1Bytes, NodeKey2Bytes, BitcoinKey1Bytes, and BitcoinKey2Bytes in ChannelEdgeInfo. Since route.Vertex is defined as [33]byte, this change is functionally equivalent but provides better type safety and consistency with the rest of the routing subsystem. OtherNodeKeyBytes is also updated to return route.Vertex.
Add a version parameter to maybeCreateShellNode to allow callers to specify which gossip protocol version should be used when creating shell nodes. Currently all callers pass GossipVersion1, but this change sets up the foundation for v2 support. Shell nodes are lightweight node entries containing only a protocol version and public key, created before full node information is available.
Add three new optional fields to the CreateChannel SQL query to support v2 channel announcements: - signature: single schnorr signature (replaces four ECDSA sigs) - funding_pk_script: the funding output script - merkle_root_hash: for taproot channels These fields are NULL for v1 channels and populated for v2 channels.
And set it to V1 version everywhere. Add a Version field to ChannelEdgeInfo to distinguish between v1 and v2 channel announcements. Set it to GossipVersion1 for all existing channels. Both KV and SQL stores now validate that only v1 channels are currently supported, returning an error for v2 channels. The KV store automatically sets version to v1 when deserializing (since all persisted channels in KV format are v1). This versioning is essential for handling the different field requirements and validation logic between v1 and v2 channels.
Also update it to more closely match the persisted version which has the v1 and v2 only fields as optional. Refactor ChannelAuthProof to support both v1 and v2 channel announcements: - Add Version field to distinguish v1 from v2 proofs - Wrap v1-specific fields (NodeSig1/2, BitcoinSig1/2) in fn.Option since v2 doesn't use them - Add optional Signature field for v2's single schnorr signature - Add constructor functions NewV1ChannelAuthProof and NewV2ChannelAuthProof to enforce correct initialization - Add getter methods (NodeSig1(), BitcoinSig1(), etc.) that safely unwrap options, returning empty slices when not present The IsEmpty() check is updated to handle both versions correctly. Both stores validate v1-only for now.
This makes it clear what fields must/can be set for a V1 channel. Introduce NewV1Channel constructor to create v1 channel edges with proper initialization and validation. The constructor: - Takes required fields (chanID, chainHash, node keys) as parameters - Takes v1-specific fields (bitcoin keys, extra opaque data) via ChannelV1Fields struct - Accepts optional fields (capacity, channel point, features, proof) via functional options (WithCapacity, WithChannelPoint, etc.) - Validates that if an AuthProof is provided, its version matches the channel version This makes it clear which fields are required vs optional for v1 channels and prevents incorrectly initialized channel edges. All tests and production code updated to use the constructor.
So that we have one place that converts from our `models` struct to the `lnwire.ChannelAnnouncement` struct. The commit also refactors netann.CreateChanAnnouncement to only take a ChannelEdgeInfo and get the proof from there instead of needing the proof to be passed in separately. Add ToChannelAnnouncement() method to ChannelEdgeInfo that converts the model struct to a lnwire.ChannelAnnouncement1 message. This: - Centralizes the conversion logic in one place instead of scattered across multiple call sites - Validates that AuthProof is present (can't create announcement without proof) - Currently only supports v1 channels, returning error for v2 Refactor netann.CreateChanAnnouncement to use this helper and remove the separate chanProof parameter since proof is now accessed from within ChannelEdgeInfo. This improves encapsulation and reduces parameter count.
Since not all will be required for V2 channels. Wrap BitcoinKey1Bytes and BitcoinKey2Bytes in fn.Option since these fields are only required for v1 channel announcements. V2 channels may or may not have bitcoin keys present in their announcement. NewV1Channel constructor wraps the bitcoin keys with fn.Some(). All access sites updated to unwrap the options, using UnwrapOr for non-critical paths and UnwrapOrErr where the keys must be present (e.g., KV serialization, ToChannelAnnouncement).
Add FundingPKScript() method that returns the funding output's pkScript for the channel. The implementation is version-aware: - V1: generates a 2-of-2 multisig P2WSH script from the two bitcoin keys - V2: will use taproot script (to be implemented) This encapsulates the script generation logic and makes it clear which bitcoin keys are being used. Replaces direct calls to genMultiSigP2WSH with the cleaner method call.
Update the Database section to reference both PRs that prepare the graph DB for gossip v2 support: - PR 10339: node handling - PR 10379: channel handling (this PR)
ellemouton
force-pushed
the
elle-g175Prep-base
branch
from
142689a to
ac96d94
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR represents all the gossip 1.75 prs that have been merged into the
elle-g175Prep-basebranch. We can merge this PR into master once the type preparations are complete (ie, all the CRUD has been updated to handle the new v2 types).Merged
modelsfor V2 data #10379In Review
TODO