graph/db+sqldb: channel policy SQL schemas, queries and upsert CRUD #9887
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 (
|
4cc895b to
96e5716
Compare
bhandras
left a comment
There was a problem hiding this comment.
Looks good just one question.
|
note: im going to update the chan_policy table here to have explicit colums for inbound fee fields. So this will then depend on #9897 |
96e5716 to
b59e927
Compare
ellemouton
left a comment
There was a problem hiding this comment.
Thanks @bhandras 🙏
Updated this so that the newly added inbound fees columns are in place 👍
b59e927 to
e882735
Compare
e882735 to
1950bd5
Compare
Ensure that the graph tables are dropped in reverse dependency order.
Define the SQL tables for representing channel policies. Namely: - channel_policies - channel_policy_extra_types
In this commit, we introduce a SQLStoreConfig struct which for the time being only has the ChainHash of the genesis block of the chain this node is running on. This is used to reconstruct lnwire messages from what we have persisted in the DB. This means we dont need need to persist the chain-hash of gossip messages since we know it will always be the same for a given node. If a node were to be started with a different network, the lnwire messages it reconstructs for gossip will be invalid.
1950bd5 to
b1e8fe5
Compare
guggero
left a comment
There was a problem hiding this comment.
One potential optimization, otherwise LGTM 🎉
graph/db/sql_store.go
Outdated
| func upsertChanPolicyExtraSignedFields(ctx context.Context, db SQLQueries, | ||
| chanPolicyID int64, extraFields map[uint64][]byte) error { | ||
|
|
||
| // Get any existing extra signed fields for the channel policy. |
There was a problem hiding this comment.
Why can't we just delete all extra types and insert the new ones?
Would probably be more efficient from a SQL standpoint as well: 1 delete query and a couple of inserts.
There was a problem hiding this comment.
good point: my original thinking was that mostly the TLV data wont change very often and so we wont actually need to do deletes often. But now thinking about it, while that might be true for other messages, for channel_updates, the only TLV data we have right now (that we know of) is inbound fees which might actually change quite frequently - so yeah - I'll update this to just do a delete 👍
Probs something we can revisit again then at benchmark/tuning time.
In this commit, the various SQL queries are defined that we will need in order to implement the SQLStore UpdateEdgePolicy method. Channel policies can be "replaced" and so we use the upsert pattern for them with the rule that any new channel policy must have a timestamp greater than the previous one we persisted. As is done for the KVStore implementation of the method, we use the batch scheduler for this method.
b1e8fe5 to
c327988
Compare
|
cc @guggero for override merge 🙏 |
In this PR, the following schemas are defined for storing graph channel policy data. NOTE the nodes & channels tables here are only to show how they fit in - the full nodes & channels pictures can be seen here and here.
NOTE: here we only define the schemas and add enough queries so that we can implement the
UpdateEdgePolicymethod. So with this PR we are not really yet testing reading from these new tables. This is left to follow up PRs since those will be quite a lot of code and I want to keep this PR contained as it already contains new schemas etc.Part of #9795
See #9932 for the full picture that we are aiming at