-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[g175] graph/db: v2 columns and v2 node CRUD #10339
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
base: master
Are you sure you want to change the base?
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 lays crucial groundwork for the upcoming V2 gossip protocol by introducing version-aware database operations and extending the node data model. It refactors the graph store interface to support explicit gossip versions, adds new fields to the node model for V2 announcements, and updates all relevant CRUD methods and tests to handle these changes. This ensures that the system can properly store and retrieve node information for both V1 and V2 gossip, facilitating a smooth transition and coexistence of different protocol versions. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces support for v2 gossip messages in the graph database. It adds new columns to the database schema, refactors the database store interface to be version-aware, and introduces a VersionedReader to abstract gossip versioning for consumers. The changes are extensive and well-structured. I've found a couple of issues: a potential overflow bug due to an incorrect type cast for block height, and a bug in a database migration script where a CHECK constraint is applied to the wrong column. I've also included some suggestions to improve documentation according to the project's style guide.
273d8b8 to
ae446b8
Compare
7988c44 to
9306c58
Compare
ae446b8 to
70894d3
Compare
c8cd4da to
edd9890
Compare
The config file format changed. The tool golangci-lint migrate was used to migrate the old config. However old comments and also the structure of the disabled linters was preserved. Moreover the new v2 version introduced new linters, we disable 3 of them because they are very noise and we do not really want to check for them: funcorder, noinlineerr, embeddedstructfieldcheck.
The custom file is only needed in the tools directory.
And ensure that both versions 1 and 2 implement it.
Define a GossipVersion enum along with a GossipMessage interface to be satisfied by all gossip related messages. This will be useful later on when we want to make decisions based on the protocol version that a message is part of.
Since the gossip protocols are completely disjoint, we need to treat messages on the two protocols completely separately and should not let rejections on one protocol affect how we treat messages on the other.
We now make sure we only set the custom channel data in the lnrpc.Route only if it contains relevant data.
We now return an error when blinded and non blinded attempts are combined. This was theoretically possible to register a legacy attempt in combination with a blinded payment. This would have been prevented by other checks in the code because legacy payments are not split into shards.
This message type is a message that carries an onion-encrypted payload used for BOLT12 messages.
This commit creates the necessary endpoints for onion messages. Specifically, it adds the following: - `SendOnionMessage` endpoint to send onion messages. - `SubscribeOnionMessages` endpoint to subscribe to incoming onion messages. It uses the `msgmux` package to handle the onion messages.
The only way to unblock SendCustomMessage is if the peer activates, disconnects or the server shuts down. This means that if the context is cancelled, we will still wait until one of those other events happen. With this commit we thread the context through to SendCustomMessage, so that if the context is cancelled, we can return early. This improves the cancellation semantics.
This tests was a temporary helper to let devs test the graph SQL migration before it was plugged in to LND. But that migration has now shipped and so we can remove this.
Copy over all the code that the graph SQL migration needs to a separate folder. This will let us advance the main graph SQL CRUD code without worrying about changing the sql migration code. It will also let us change the SQL queries without changing the migration. In this commit, only the migration logic is "frozen" but in an upcoming commit, the sqlc queries & models will be frozen too.
…Freeze graph/db: freeze the SQL migration code
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 date 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.
In this commit, we update TestNodeInsertionAndDeletion so that it is run twice: once with V1 nodes and another with V2 nodes. This way, we are testing the basic CRUD for V2 nodes. In later commits we can add tests that tests the interaction between V1 and V2 nodes.
Convert a few more CRUD methods to handle v2 nodes. Then update some more tests to be run against both v1 and v2 nodes.
c8cd4da to
89f9bc0
Compare
89f9bc0 to
989e778
Compare
|
@ellemouton, remember to re-request review from reviewers when ready |
706b22a to
95b84a8
Compare
This PR:
models.Node.part of #10293