lnd: ensure that LND won't start in native SQL mode if it has any KV invoices #8568

bhandras · 2024-03-19T14:23:14Z

This PR is a temporary measure for 0.18 as we don't plan to add invoice migration yet for native SQL invoice DB.

coderabbitai · 2024-03-19T14:23:23Z

Important

Auto Review Skipped

Auto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit-tests for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

ellemouton

LGTM! tACK 🚀

One comment about re-using the passed in context instead.

Then as mentioned offline: realised that we dont do this check the other way around (ie it is possible to add native sql invoice & then remove the flag and it will go back to adding kvdb invoices). But Andras pointed out that this is already the case with any backend change we have today. So potentially not needed to be that defensive especially cause in future when we add migration scripts, then we do want to allow this 👍

ellemouton · 2024-03-20T07:07:31Z

config_builder.go

+		// start lnd with native SQL enabled, as we don't currently
+		// migrate the invoices to the new database schema.
+		invoiceSlice, err := dbs.GraphDB.QueryInvoices(
+			context.Background(), invoices.InvoiceQuery{


i think you can instead use the context that is already available

Fixed, thanks :)

ellemouton · 2024-03-20T07:40:17Z

lncfg/db.go

+			return fmt.Errorf("cannot use native SQL with bolt " +
+				"backend")


This commit adds a check to ensure that we don't start LND with native SQL invoice DB if we already have any invoices in our KV channeldb. This is needed as native SQL invoices is an experimental feature and we do not yet support migration.

bitromortac

LGTM 🎉. Only to clarify, it doesn't matter if the database contained any invoices that were deleted, we just require that it is empty?

bhandras · 2024-03-20T08:44:39Z

LGTM 🎉. Only to clarify, it doesn't matter if the database contained any invoices that were deleted, we just require that it is empty?

Good question! As deleted invoices are completely removed from the KV invoice DB, it'd be fine. We could be more strict by not even caring about the invoice DB, but only about whether this is a first ever start of the node. I believe the current approach is quite liberal in that it allows "old" nodes to also use the native schema (given no invoices).

bhandras · 2024-03-20T08:47:19Z

LGTM! tACK 🚀

One comment about re-using the passed in context instead.

Then as mentioned offline: realised that we dont do this check the other way around (ie it is possible to add native sql invoice & then remove the flag and it will go back to adding kvdb invoices). But Andras pointed out that this is already the case with any backend change we have today. So potentially not needed to be that defensive especially cause in future when we add migration scripts, then we do want to allow this 👍

Thanks for the testing and discussion! :) I agree that we could be more strict about preconditions, see also my other comment above. I'm also open to changing things up if you both think more strict checking is better.

bitromortac · 2024-03-20T09:06:46Z

Good question! As deleted invoices are completely removed from the KV invoice DB, it'd be fine. We could be more strict by not even caring about the invoice DB, but only about whether this is a first ever start of the node. I believe the current approach is quite liberal in that it allows "old" nodes to also use the native schema (given no invoices).

The only thing that would get out of sync would be the add index (if I understand that correctly). I think keeping that liberal approach is ok, while being somewhat defensive against accidentally mixing native with the preexisting kv db 👍.

bhandras added the database Related to the database/storage of LND label Mar 19, 2024

bhandras self-assigned this Mar 19, 2024

bhandras added this to the v0.18.0 milestone Mar 19, 2024

bhandras force-pushed the native-sql-ensure-empty-invoicedb branch from a6e6a00 to 55b3b85 Compare March 19, 2024 14:30

saubyk requested review from a team, bitromortac, ziggie1984 and ellemouton and removed request for a team and ziggie1984 March 19, 2024 14:38

bhandras force-pushed the native-sql-ensure-empty-invoicedb branch from 55b3b85 to 751caa1 Compare March 20, 2024 07:33

ellemouton approved these changes Mar 20, 2024

View reviewed changes

bhandras added 4 commits March 20, 2024 08:46

lncfg: make sure to only use native SQL with SQL backends

65c1f54

itest: ensure that native SQL LND won't start if it has any KV invoices

2fbffab

docs: update release notes for 0.18

483f5f3

bhandras force-pushed the native-sql-ensure-empty-invoicedb branch from 751caa1 to 483f5f3 Compare March 20, 2024 07:46

bitromortac approved these changes Mar 20, 2024

View reviewed changes

guggero merged commit 23f5f3c into lightningnetwork:master Mar 20, 2024
25 of 27 checks passed

bhandras deleted the native-sql-ensure-empty-invoicedb branch March 20, 2024 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lnd: ensure that LND won't start in native SQL mode if it has any KV invoices #8568

lnd: ensure that LND won't start in native SQL mode if it has any KV invoices #8568

bhandras commented Mar 19, 2024

coderabbitai bot commented Mar 19, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

ellemouton left a comment •

edited

ellemouton Mar 20, 2024

bhandras Mar 20, 2024 •

edited

ellemouton Mar 20, 2024

bitromortac left a comment

bhandras commented Mar 20, 2024

bhandras commented Mar 20, 2024 •

edited

bitromortac commented Mar 20, 2024

		return fmt.Errorf("cannot use native SQL with bolt " +
		"backend")

lnd: ensure that LND won't start in native SQL mode if it has any KV invoices #8568

lnd: ensure that LND won't start in native SQL mode if it has any KV invoices #8568

Conversation

bhandras commented Mar 19, 2024

coderabbitai bot commented Mar 19, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

ellemouton left a comment • edited

Choose a reason for hiding this comment

ellemouton Mar 20, 2024

Choose a reason for hiding this comment

bhandras Mar 20, 2024 • edited

Choose a reason for hiding this comment

ellemouton Mar 20, 2024

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

bhandras commented Mar 20, 2024

bhandras commented Mar 20, 2024 • edited

bitromortac commented Mar 20, 2024

CodeRabbit Configration File (`.coderabbit.yaml`)

ellemouton left a comment •

edited

bhandras Mar 20, 2024 •

edited

bhandras commented Mar 20, 2024 •

edited