[g175] graph/db: merge g175 types-prep side branch #10414
Conversation
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 |
ellemouton
force-pushed
the
elle-g175Prep-base
branch
from
1d85426 to
142689a
Compare
|
^ rebase on master |
ellemouton
force-pushed
the
elle-g175Prep-base
branch
from
142689a to
ac96d94
Compare
ellemouton
force-pushed
the
elle-g175Prep-base
branch
from
ac96d94 to
3431043
Compare
|
^ just a rebase on master |
🔴 PR Severity: CRITICAL
🔴 Critical (5 files)
🟠 High (15 files)
🟡 Medium (3 files)
🟢 Low (20 files - tests, docs, auto-generated)
AnalysisThis PR receives a CRITICAL severity classification for the following reasons:
Recommendation: This PR requires review by engineers with deep expertise in LND's graph database architecture, SQL migrations, and server coordination. Special attention should be paid to the migration safety and the extensive changes to graph data models. To override, add a |
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.
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)
This commit extends ChannelEdgeInfo to support v2 (taproot) channel announcements by adding: - MerkleRootHash for the optional Merkle tree commitment - ExtraSignedFields for additional signed TLV fields - ChannelV2Fields struct to encapsulate v2-specific optional fields - NewV2Channel constructor for creating v2 channel edges - FundingPKScript handling for MuSig2 key aggregation with optional taproot tweaks
Extends the SQL store to support v2 (taproot) channel announcements: - Add version validation in AddChannelEdge - Store v2-specific fields: FundingPkScript, MerkleRootHash, Signature - Update buildEdgeInfoWithBatchData to reconstruct v2 channels from DB with optional bitcoin keys and funding script handling - Add WithMerkleRootHash edge modifier for ChannelV2Fields
Replace verbose t.Fatal patterns with concise require assertions following the project's testing guidelines.
Extend the createEdge test helper to accept a gossip version, enabling creation of both v1 and v2 test channels. V2 channels include the appropriate auth proof (single signature), merkle root hash, and funding script fields.
Thread the gossip version through DeleteChannelEdges to enable version-aware channel deletion. The KVStore rejects non-v1 versions while SQLStore properly passes the version to the underlying queries. This prepares for v2 channel zombie handling (strict zombie pruning for v2 is marked as TODO).
Add SQL query to update the signature column for v2 channel auth proofs. Unlike v1 which requires four separate signatures, v2 channels use a single aggregated signature.
Update AddEdgeProof to handle both v1 (four separate signatures) and v2 (single aggregated signature) channel authentication proofs using the appropriate SQL queries.
Convert TestPartialNode to a versioned test that runs for both v1 and v2 gossip versions, ensuring shell node creation works correctly for both channel types.
Add version-aware DeleteChannelEdges method to VersionedGraph and update call sites in graph builder, rpcserver, and server to use versioned graphs. This ensures channel deletion operations are properly scoped to the correct gossip version.
Add SQL query to determine if a node has public v2 channels. Unlike v1 which requires all four individual signatures, v2 channels are considered public when the single aggregated signature is present.
Make IsPublicNode version-aware by routing to the appropriate SQL query based on gossip version. V1 and v2 have different criteria for determining node publicity (v1 requires four signatures, v2 requires one). Convert TestNodeIsPublic to versioned test for both protocols.
Make channel edge fetching version-aware by adding gossip version parameter to FetchChannelEdgesByID and FetchChannelEdgesByOutpoint. Add corresponding methods to VersionedGraph. V2 policy building is marked as TODO.
Make IsZombieEdge version-aware and add corresponding method to VersionedGraph. Convert TestEdgeInsertionDeletion to versioned test using the createEdge helper for both v1 and v2 channel testing.
Add reference to the PR implementing v2 channel support in the graph database layer.
This commit improves handling of missing channel signatures in the database: - Return nil from auth proof accessors instead of empty slices so that missing signatures are stored as NULL in SQL. - Update public channel checks to require signature length > 0, which properly handles existing empty bytea values in the database. - Add regression test covering empty v1 and v2 channel signatures to prevent future issues.
ellemouton
force-pushed
the
elle-g175Prep-base
branch
from
236e6b9 to
509913e
Compare
|
^ merge of #10380 AND then rebase on master |
🔴 PR Severity: CRITICAL
🔴 Critical (5 files)
🟠 High (16 files)
🟡 Medium (3 files)
🟢 Low (18 test files)Test files including: AnalysisThis PR is classified as CRITICAL due to:
Recommended Review Approach:
To override, add a |
2 participants
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