-
Notifications
You must be signed in to change notification settings - Fork 2.2k
migration1: freeze serialization boundary types and helpers#10600
Closed
ziggie1984 wants to merge 97 commits intolightningnetwork:elle-payment-sql-series-newfrom
Closed
migration1: freeze serialization boundary types and helpers#10600ziggie1984 wants to merge 97 commits intolightningnetwork:elle-payment-sql-series-newfrom
ziggie1984 wants to merge 97 commits intolightningnetwork:elle-payment-sql-series-newfrom
Conversation
Collaborator
Depends on #10485
Summary
This PR ensures the payments/db/migration1 package has a fully frozen
serialization boundary. The goal is that once this migration is released,
no future changes to upstream packages can silently corrupt the migration
or require modifying the migration code.
Three categories of dependencies are frozen:
- lnwire serialization helpers: frozen local copies of
ParseCustomRecordsFrom,MergeAndEncode,ParseAndExtractCustomRecords,
EncodeFailureMessage,DecodeFailureMessageandmakeEmptyOnionError - codec primitives: frozen local
ReadElement/WriteElement/
UnknownElementType, removing the dependency onchanneldbcodec - Route/Hop/Vertex types: local frozen snapshots of
route.Route,
route.Hopandroute.Vertexat migration write time, with conversion
helpers for live code paths
Types that were intentionally not frozen (stable primitives unlikely
to change): lnwire.MilliSatoshi, lntypes.Hash/Preimage,
lnwire.CustomRecords, record.MPP/record.AMP, btcec.PublicKey.
Summary of ChangesHello @ziggie1984, 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 introduces a robust framework for migrating payment data from the existing KV store to a new native SQL schema. The core change involves freezing critical serialization and data types within a dedicated Highlights
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. Changelog
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
|
lightninglabs-deploy
added
the
severity-critical
Requires expert review - security/consensus critical
label
Feb 19, 2026
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 a frozen migration1 package for the payments database migration from KV to SQL, a commendable approach for ensuring long-term migration stability. The changes are extensive and well-executed, including freezing data types, serialization logic, and database interfaces. The migration logic is robust, handling historical edge cases like duplicate payments, and is accompanied by thorough validation and extensive testing. Additionally, the PR brings welcome performance improvements to SQL queries and adds a useful omit_hops flag to the listpayments API. Overall, this is a high-quality contribution that significantly improves the database architecture and maintainability.
ziggie1984
force-pushed
the
kvdb-sql-payments-migration-final
branch
2 times, most recently
from
February 20, 2026 09:28
aa909a7 to
0412b29
Compare
5 tasks
We prepare the code for the sql payment backend. However no
payment db interface method for the sql backend is implemented
yet. This will be done in the following commits. They currently
use the embedded KVStore to satify the build environment.
payment db interface method for the sql backend is implemented
yet. This will be done in the following commits. They currently
use the embedded KVStore to satify the build environment.
This does not include duplicate payments yet. They will be added
when the migration code is introduced for payments.
when the migration code is introduced for payments.
We allow the migration of the payment schema to be applied when
the test_native_sql is active to make sure the tables are properly
contructed.
the test_native_sql is active to make sure the tables are properly
contructed.
Previously a one(intent)-to-many(payment) relationship it is now
changed to a one-to-one relationship because a payment request
only can have 1 payment related to it. Looking into the future
with BOLT12 offers, the fetched invoice from the offer could be
stored here as well and the relationship would still hold.
changed to a one-to-one relationship because a payment request
only can have 1 payment related to it. Looking into the future
with BOLT12 offers, the fetched invoice from the offer could be
stored here as well and the relationship would still hold.
In this commit we add all queries which we will need to insert
payment related data into the db.
payment related data into the db.
Add a migration specific query which allows to set the failure
reason when inserting a payment into the db.
reason when inserting a payment into the db.
Older LND versions could create multiple payments for the same hash.
We need to preserve those historical records during KV-SQL migration,
but they don't fit the normal payment schema because we enforce a
unique payment hash constraint. Introduce a lean payment_duplicates
table to store only the essential fields (identifier, amount,
timestamps, settle/fail data).
This keeps the primary payment records stable and makes the migration
deterministic even when duplicate records lack attempt info. The table
is intentionally minimal and can be dropped after migration if no
duplicate payments exist.
For now there is no logic in place which allows the noderunner to
fetch duplicate payments after the migration.
We need to preserve those historical records during KV-SQL migration,
but they don't fit the normal payment schema because we enforce a
unique payment hash constraint. Introduce a lean payment_duplicates
table to store only the essential fields (identifier, amount,
timestamps, settle/fail data).
This keeps the primary payment records stable and makes the migration
deterministic even when duplicate records lack attempt info. The table
is intentionally minimal and can be dropped after migration if no
duplicate payments exist.
For now there is no logic in place which allows the noderunner to
fetch duplicate payments after the migration.
Copy the core payments/db code into payments/db/migration1 and
add the required sqlc-generated types/queries from sqldb/sqlc.
This effectively freezes the migration code so it stays robust
against future query or schema changes in the main payments package.
add the required sqlc-generated types/queries from sqldb/sqlc.
This effectively freezes the migration code so it stays robust
against future query or schema changes in the main payments package.
Implement the KV-SQL payment migration and add an in-migration
validation pass that deep-compares KV and SQL payment data in batches.
Duplicate payments are migrated into the payment_duplicates table,
and duplicates without attempt info or explicit resolution are marked
failed to ensure terminal state. Validation checks those rows as well.
validation pass that deep-compares KV and SQL payment data in batches.
Duplicate payments are migrated into the payment_duplicates table,
and duplicates without attempt info or explicit resolution are marked
failed to ensure terminal state. Validation checks those rows as well.
Add test helpers plus sql_migration_test coverage for KV-SQL migration.
Basic migration, sequence ordering, data integrity, and feature-specific cases
(MPP/AMP, custom records, blinded routes, metadata, failure messages). Also
cover duplicate payment migration to payment_duplicates, including missing
attempt info to ensure terminal failure is recorded.
This gives broad regression coverage for the migration path and its edge-cases.
Basic migration, sequence ordering, data integrity, and feature-specific cases
(MPP/AMP, custom records, blinded routes, metadata, failure messages). Also
cover duplicate payment migration to payment_duplicates, including missing
attempt info to ensure terminal failure is recorded.
This gives broad regression coverage for the migration path and its edge-cases.
Add a developer-facing migration_external_test that allows
running the KV-SQL payments migration against a real channel.db
backend to debug migration failures on actual data. The accompanying
testdata README documents how to supply a database file and configure
the test, so users can validate their data and confirm the migration
completes successfully.
The test is skipped by default and meant for manual diagnostics.
running the KV-SQL payments migration against a real channel.db
backend to debug migration failures on actual data. The accompanying
testdata README documents how to supply a database file and configure
the test, so users can validate their data and confirm the migration
completes successfully.
The test is skipped by default and meant for manual diagnostics.
Hook the payments KV-SQL migration into the SQL migration config.
The migration is still only available when building with the build tag
"test_native_sql".
Moreover a tombstone protection similar to the invoice migration is added
to prevent re-runningi with the KV backend once migration completes.
The migration is still only available when building with the build tag
"test_native_sql".
Moreover a tombstone protection similar to the invoice migration is added
to prevent re-runningi with the KV backend once migration completes.
For legacy payments, the HTLC Hash field may be nil in the bbolt
backend. Previously, the migration would fail with "HTLC attempt X
missing payment hash" when encountering such payments.
This commit fixes the migration by falling back to the parent payment
hash when the HTLC-specific hash is nil. This is consistent with how
the router handles legacy payments (see patchLegacyPaymentHash in
payment_lifecycle.go).
The validation logic is also updated to apply the same fallback when
comparing bbolt data with migrated SQL data, ensuring the comparison
succeeds.
backend. Previously, the migration would fail with "HTLC attempt X
missing payment hash" when encountering such payments.
This commit fixes the migration by falling back to the parent payment
hash when the HTLC-specific hash is nil. This is consistent with how
the router handles legacy payments (see patchLegacyPaymentHash in
payment_lifecycle.go).
The validation logic is also updated to apply the same fallback when
comparing bbolt data with migrated SQL data, ensuring the comparison
succeeds.
When duplicatePaymentSequenceKey is missing from a duplicate payment
sub-bucket, the code returned the outer function's err variable which
is nil at that point. This caused corrupted duplicate entries to be
silently treated as "not found" instead of failing loudly.
Return a new dedicated ErrNoDuplicateSequenceNumber error so malformed
data is detected immediately.
sub-bucket, the code returned the outer function's err variable which
is nil at that point. This caused corrupted duplicate entries to be
silently treated as "not found" instead of failing loudly.
Return a new dedicated ErrNoDuplicateSequenceNumber error so malformed
data is detected immediately.
The defer closure checked a local err variable for commit/rollback
decisions, but err remained nil after a successful BeginTx. When
txBody failed, the error was returned directly without assigning to
err, so the defer always committed instead of rolling back.
Additionally, since err was not a named return value, the defer's
Commit error assignment was silently swallowed.
Replace the error-prone defer pattern with explicit rollback on
txBody failure and a direct Commit return.
decisions, but err remained nil after a successful BeginTx. When
txBody failed, the error was returned directly without assigning to
err, so the defer always committed instead of rolling back.
Additionally, since err was not a named return value, the defer's
Commit error assignment was silently swallowed.
Replace the error-prone defer pattern with explicit rollback on
txBody failure and a direct Commit return.
Tests that all migration files follow the defined schema and that
there are not duplicates which could cause collision.
there are not duplicates which could cause collision.
Add a self-contained, frozen copy of the lnwire serialization functions
used by the migration1 KV store. These local copies ensure that the
serialization boundary of this migration package is independent of live
lnwire package changes that may occur after this migration is released.
The following functions are frozen locally:
- ParseCustomRecordsFrom
- MergeAndEncode
- ParseAndExtractCustomRecords
- EncodeFailureMessage / DecodeFailureMessage
- makeEmptyOnionError and the ErrParsingExtraTLVBytes sentinel
used by the migration1 KV store. These local copies ensure that the
serialization boundary of this migration package is independent of live
lnwire package changes that may occur after this migration is released.
The following functions are frozen locally:
- ParseCustomRecordsFrom
- MergeAndEncode
- ParseAndExtractCustomRecords
- EncodeFailureMessage / DecodeFailureMessage
- makeEmptyOnionError and the ErrParsingExtraTLVBytes sentinel
Replace the delegation to channeldb.ReadElement/WriteElement with
self-contained, frozen implementations that only handle the exact types
required by this migration package. This removes the dependency on the
live channeldb codec so that future changes to channeldb serialization
cannot silently corrupt or break the migration.
UnknownElementType is also defined locally for the same reason.
self-contained, frozen implementations that only handle the exact types
required by this migration package. This removes the dependency on the
live channeldb codec so that future changes to channeldb serialization
cannot silently corrupt or break the migration.
UnknownElementType is also defined locally for the same reason.
Add local frozen snapshots of route.Route, route.Hop and route.Vertex
to the migration1 package. These types capture the exact field set that
existed at the time this migration was written, ensuring that future
additions or removals to the upstream routing types cannot require
changes to this migration or silently alter its serialization behavior.
Conversion helpers (routeToLocal, hopToLocal, toRouteRoute, toRouteHop)
are provided so that the live code paths (e.g. NewHtlcAttempt,
generateSphinxPacket) can continue to accept and return the upstream
route package types at their boundaries.
All files within the migration1 package are updated to use the local
types, removing the routing/route dependency from the package.
to the migration1 package. These types capture the exact field set that
existed at the time this migration was written, ensuring that future
additions or removals to the upstream routing types cannot require
changes to this migration or silently alter its serialization behavior.
Conversion helpers (routeToLocal, hopToLocal, toRouteRoute, toRouteHop)
are provided so that the live code paths (e.g. NewHtlcAttempt,
generateSphinxPacket) can continue to accept and return the upstream
route package types at their boundaries.
All files within the migration1 package are updated to use the local
types, removing the routing/route dependency from the package.
ziggie1984
force-pushed
the
kvdb-sql-payments-migration-final
branch
from
February 24, 2026 16:11
2877326 to
634c266
Compare
ziggie1984
added
severity-override-medium
Manual override to medium
and removed
severity-critical
Requires expert review - security/consensus critical
labels
Feb 24, 2026
Member
|
Can you rebase and/or update the base branch for this? |
ziggie1984
force-pushed
the
elle-payment-sql-series-new
branch
from
February 25, 2026 07:21
2589333 to
190893d
Compare
Collaborator
Author
|
will be squashed into PR #10485 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.