sqldb+graph/db: add node related tables and implement some node CRUD#9866
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ellemouton
force-pushed
the
graphSQL7-nodes-tables
branch
3 times, most recently
from
ac4106d to
d96416a
Compare
graph/db/sql_store.go
Outdated
| } | ||
|
|
||
| // NewReadTx creates a new read transaction option set. | ||
| func NewReadTx() TxOptions { |
There was a problem hiding this comment.
We could return a pointer here, then we could use NewReadTx() directly in the call to ExecTx.
And we could move that to the sqldb for re-use (we define too many different types of them in tapd for example with no real reason). And perhaps shorten to ReadTx() and also add a WriteTx()?
There was a problem hiding this comment.
did the pointer part here.
for the re-use point: gonna do that in a separate PR that does it for both here and invoices if that's ok
| _, node, err = getNodeByPubKey(ctx, db, pubKey) | ||
|
|
||
| return err | ||
| }, func() {}) |
There was a problem hiding this comment.
Can we add a sqldb.NoOpReset and use that here? Then we can more easily find and replace them once we turn this into a functional option in sqldbv2.
There was a problem hiding this comment.
gonna do this in a separate PR that addresses it both here and for invoices if that's ok
bhandras
left a comment
bhandras
left a comment
There was a problem hiding this comment.
Excellent work! Looks good, just a few nits and comments.
| }, | ||
| } | ||
|
|
||
| return s.nodeScheduler.Execute(ctx, r) |
There was a problem hiding this comment.
I think we need to be careful here, as if there're two upserts in the same batch, they might get reordered?
There was a problem hiding this comment.
should be fine right? cause it will get added in order to the slice held by the batcher and executed in that order yeah?
There was a problem hiding this comment.
Okay discussed this offline, now with the addition that upserting a node will check the last update time we should be fine.
In this commit, the various SQL schemas required to store graph node related data is defined. Specifically, the following tables are defined: - nodes - node_extra_types - node_features - node_addresses
ellemouton
force-pushed
the
graphSQL7-nodes-tables
branch
from
d96416a to
3abea08
Compare
In this commit, we add the various sqlc queries that we need in order to implement the following V1Store methods: - AddLightningNode - FetchLightningNode - HasLightningNode - AddrsForNode - DeleteLightningNode - FetchNodeFeatures These are implemented by SQLStore which then lets us use the SQLStore backend for the following unit tests: - TestNodeInsertionAndDeletion - TestLightningNodePersistence
In this commit, we let the SQLStore implement LookupAlias. This then lets us run the TestAliasLookup unit test against the SQL backends.
In this commit we add the necessary SQL queries and then implement the SQLStore's NodeUpdatesInHorizon method. This lets us run the TestNodeUpdatesInHorizon unit tests against SQL backends.
In this commit, we add the `source_nodes` table. It points to entries in the `nodes` table. This table will store one entry per protocol version that we are announcing a node_announcement on. With this commit, we can run the TestSourceNode unit test against our SQL backends.
ellemouton
force-pushed
the
graphSQL7-nodes-tables
branch
from
3abea08 to
efc04b9
Compare
|
cc @guggero for override merge 🙏 (unit-race flake is an RBF test) |
4 participants
(Re-opening #9824 - base branch was accidentally deleted)
In this PR, the following schemas are defined for storing graph nodes:
(NOTE: the
block_heightcolumn is only shown as an example to show how v2 data might fit into this schema. It is not included in the initial schema definition).nodes: stores the main info about the node from a node_announcement. Note that we might sometimes insert a "shell node" for when we get a channel announcement that points to a node that we dont yet have a record for. In this case, we will only have the public key of the node along with the gossip version it was advertised on. So theversion:pub_keypair is always unique.node_addresses: normalised node addresses.node_extra_types: stores the normalised TLV records of anode_announcementfor any fields that we dont explicitly store in thenodestable.node_features: stores the normalised features advertised in a node announcement.source_nodes: we store a pointer to an entry in thenodestable to indicate which node belongs to this node. NOTE: there will be an entry here per gossip protocol that we are aware of.The following methods are implemented on the
SQLStore:This then lets us run a number of existing unit tests against the new SQLStore backend. Namely:
Part of #9795