Skip to content

Comments

[sql-57] Add accounts kvdb->SQL migration itest#1207

Open
ViktorT-11 wants to merge 2 commits intolightninglabs:sql-migration-basefrom
ViktorT-11:2025-01-account-migration-itest
Open

[sql-57] Add accounts kvdb->SQL migration itest#1207
ViktorT-11 wants to merge 2 commits intolightninglabs:sql-migration-basefrom
ViktorT-11:2025-01-account-migration-itest

Conversation

@ViktorT-11
Copy link
Contributor

Based on #1114

Implements part of step 6. of "Phase 3" in #917.

This PR adds the foundational components for testing the kvdb -> SQL migration within the itest framework, and implements itest coverage for the migration of the accounts store.

I'll add follow up PRs which will also add implement itest coverage for the sessions store, and the full firewalldb store.

The benefit of adding migration coverage through itests compared to just unit tests, is that this will test the full migration flow in litd, as it would be executed in production for a user who switches from a bbolt database backend to an SQL database backend.
Additionally, as itests have access to the full litcli, we can also assert parts of the migration through litcli commands, as the end user would experience it.

@gemini-code-assist
Copy link

Summary of Changes

Hello @ViktorT-11, 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 significantly advances the project's database infrastructure by introducing a robust integration test for the KVDB to SQL migration of the accounts store. It involves a comprehensive refactoring of the database interaction layer, adopting a new lnd/sqldb/v2 package for standardized SQL operations and migrations. This foundational work ensures a smooth and verifiable transition from BoltDB to a SQL backend, with a clear separation of migration-specific queries and enhanced control over the migration lifecycle.

Highlights

  • KVDB to SQL Migration Integration Test: Introduced a new integration test (itest/litd_migration_test.go) to validate the KVDB to SQL migration process specifically for the accounts store. This test covers data insertion into BoltDB, migration to SQL, and verification of data integrity in the SQL database.
  • Database Layer Refactoring: Refactored the core database interaction layer by integrating a new lnd/sqldb/v2 package. This change centralizes generic SQL database operations, transaction management, and migration stream handling, leading to a more robust and consistent database framework.
  • Versioned SQLC Queries for Migration: A new db/sqlcmig6 package was added, containing a snapshot of sqlc generated queries. This package is specifically used during the migration process to ensure compatibility with the schema at the time of migration, preventing potential issues with evolving schemas.
  • Enhanced Migration Stream Management: Implemented a new db/migrationstreams package to manage database migration streams and post-migration callbacks. This allows for executing custom Go code, such as the KVDB to SQL data transfer, as part of the database migration process in development environments.
  • Build System and Linter Updates: Modified the Makefile to include a dev build tag, enabling development-specific features. The .golangci.yml configuration was updated to exclude line length checks for the db/sqlcmig6 package, accommodating generated code.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a 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 the foundational components for testing KVDB to SQL migrations within the integration test framework, focusing on the accounts store. The changes are extensive, involving a significant refactoring of the database layer to align with lnd/sqldb/v2, introducing a dev build tag for migration-specific code, and adding a new integration test to validate the migration flow. The overall approach is solid and the new test provides valuable coverage. I have one minor suggestion for improving consistency.

@ViktorT-11 ViktorT-11 mentioned this pull request Jan 21, 2026
46 tasks
@ViktorT-11 ViktorT-11 added the no-changelog This PR is does not require a release notes entry label Jan 27, 2026
@ViktorT-11 ViktorT-11 changed the base branch from master to trigger-kvdb-to-sql-mig January 27, 2026 00:46
@ViktorT-11 ViktorT-11 force-pushed the 2025-01-account-migration-itest branch from 5247a8d to 891a015 Compare January 27, 2026 00:46
@ViktorT-11 ViktorT-11 changed the title [sql-47] Add accounts kvdb->SQL migration itest [sql-57] Add accounts kvdb->SQL migration itest Jan 27, 2026
@lightninglabs-deploy
Copy link

@ViktorT-11, remember to re-request review from reviewers when ready

@ViktorT-11 ViktorT-11 changed the base branch from trigger-kvdb-to-sql-mig to sql-migration-base February 19, 2026 10:37
@ViktorT-11 ViktorT-11 force-pushed the 2025-01-account-migration-itest branch from 891a015 to 3e0dda2 Compare February 19, 2026 10:40
Copy link
Contributor

@bitromortac bitromortac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see the work here on the itests 🔥. I have a few questions regarding what endpoints we should use. I think we should only use what's available via RPCs, which should be enough to cover most cases

ctxt, migNode.Cfg.LitAddr(), migNode.Cfg.LitTLSCertPath,
)
require.NoError(t.t, err)
defer rawConn.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need that as it's closed later explicitly

node.Cfg.LitDir, node.Cfg.NetParams.Name, "litd.db",
)

sqlStore, err := sqldb.NewSqliteStore(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is testing with Sqlite enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added coverage for postgres also here.

Comment on lines 265 to 272
// Mimics accounts/sql_migration_test.go "account with payments".
// NOTE: As there's no way to insert payments into the accounts store
// directly by just using `litcli`, we insert the payments outside
// of this function, by directly connecting to the bbolt db.
createAccount(50000, 0, "migration-payments")
paymentExpectation := expectations["migration-payments"]
paymentExpectation.payments = 4
expectations["migration-payments"] = paymentExpectation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we take use the RouterClient and add the account to the context to make some payments? payNode seems to be ready for that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I think this is a bigger questions for not just this PR, but the rest of the migration itest PRs as well.

We can either try to create all the data we can through the different clients, but with the drawback of not being able to mimic the unit tests as they cover more data variations than what's possible to generate through the clients. One example here is to manage to generate an account payment with the status unkonwn, which some users have in their DB due to having generated it with old releases, but which is hard to generate through the clients today.

Therefore I think we have to make the choice here if we want to try to cover as much data variation as possible in the itests, which requires a direct connection to the db, or if we're ok with just generating the data we can through the clients.

I can see the motivation for both alternatives:

  • If we generate through the clients, the data becomes much more realistic and may show errors with nil handling in the migrations for example. But we won't be able to mimic all unit tests.
  • If we do it through a direct connection to the db, we'll be able to cover more data variation.

Which variant do you think most sense? Would be interesting to hear @ellemouton opinion here as well after she has reviewed the PR.


// Step 4: Assert data via litcli where possible.
assertMigrationDataViaLitCLI()
assertMigrationDataViaLitCLI(ctxt, t, migNode, accountsData)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just use the RPC to query the contents? I'm not sure it adds a lot using litcli, as it's only a wrapper

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to use the RPCs directly instead. The reason I used litcli previously is that I intended to mimic the what most users experience when they query LiT, but I agree that it probably don't make most sense.

Comment on lines 111 to 171
assertMigrationDataSQL()
assertMigrationDataSQL(ctxt, t, migNode, accountsData)

// Step 7: Assert data via litcli where possible.
assertMigrationDataViaLitCLI()
assertMigrationDataViaLitCLI(ctxt, t, migNode, accountsData)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like here it would also be enough to query the RPC?

This commit adds the foundational components for testing the kvdb -> SQL
migration within the itest framework.

The benefit of adding migration coverage through itests compared to just
unit tests, is that this will test the full migration flow in litd, as
it would be executed in production for a user who switches from a bbolt
database backend to an SQL database backend.
Additionally, as itests have access to the full litcli, we can also
assert parts of the migration through litcli commands, as the end user
would experience it.
Implement itest coverage for migrating account data from kvdb to sql.
The itest coverage mimics the unit tests in
accounts/sql_migration_test.go, except for the randomized migration
tests.

This coverage ensures that the full kvdb->SQL migration flow for litd
works as expected for the accounts store.
@ViktorT-11 ViktorT-11 force-pushed the 2025-01-account-migration-itest branch from 3e0dda2 to 73d9fd8 Compare February 23, 2026 12:20
@ViktorT-11
Copy link
Contributor Author

Thanks for the review @bitromortac! I've addressed most of your feedback and responded to some comments above :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog This PR is does not require a release notes entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants