[g175:3] graph/db: continue prepping models for V2 data
#10379
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 advances the ongoing effort to prepare the system for V2 channel data models. It involves a significant refactoring of fundamental channel-related data structures to be version-aware, introducing new, more robust ways to construct and interact with channel information. The changes propagate throughout the codebase, updating existing components to align with the new model. Furthermore, the database schema is proactively modified to accommodate future V2 channel data, ensuring a smooth transition when V2 is fully implemented. 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 effort to prepare the graph database models for V2 data. The changes primarily involve replacing manual struct instantiations with new constructors and helper methods, which greatly improves code encapsulation, readability, and maintainability. The refactoring is consistently applied across a large number of files. I've found one minor area where a new helper method could be used to further simplify the code, but overall, this is a solid improvement.
ellemouton
force-pushed
the
g175Prep4
branch
from
98b033b to
b79fb2a
Compare
ellemouton
changed the title
models for V2 datamodels for V2 data
ellemouton
force-pushed
the
g175Prep4
branch
from
b79fb2a to
faabb01
Compare
ellemouton
force-pushed
the
g175Prep4
branch
from
faabb01 to
4299389
Compare
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 use taproot where the node keys serve as the bitcoin keys. 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
g175Prep4
branch
2 times, most recently
from
a41b1b8 to
88a1a85
Compare
|
@ellemouton, remember to re-request review from reviewers when ready |
2 participants
Description TBD
Depends on #10339
Part of #10293