improve speed of retrieval of payments#10535
improve speed of retrieval of payments#10535ziggie1984 wants to merge 16 commits intolightningnetwork:elle-payment-sql-series-newfrom
Conversation
ziggie1984
changed the base branch from
master
to
elle-payment-sql-series-new
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 significantly overhauls the payment storage and retrieval mechanisms by transitioning from a key-value store to a native SQL database. The primary goal is to enhance the speed and efficiency of payment data access, particularly for querying and managing payment lifecycle events. The changes introduce a more modular and robust database layer, improve context propagation throughout payment operations, and ensure backward compatibility by handling legacy data structures during migration. 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. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a significant and well-executed pull request that introduces a SQL backend for the payments database, along with a comprehensive migration from the existing KV store. The migration logic is thorough, handling edge cases like legacy duplicate payments, and includes robust validation and progress reporting. The new SQL store implementation is well-optimized, leveraging batch queries for better performance. The approach of freezing the old KV store logic in a separate package for the migration is a sound engineering practice that ensures stability. The code quality is high, and the changes are well-tested. I have one minor suggestion regarding a typo in a comment.
| // BatchedSQLPaymentsQueries storage backend. | ||
| func NewSQLStore(cfg *SQLStoreConfig, db BatchedSQLQueries, |
There was a problem hiding this comment.
There's a small typo in the comment. The type is BatchedSQLQueries, not BatchedSQLPaymentsQueries.
| // BatchedSQLPaymentsQueries storage backend. | |
| func NewSQLStore(cfg *SQLStoreConfig, db BatchedSQLQueries, | |
| // BatchedSQLQueries storage backend. | |
| func NewSQLStore(cfg *SQLStoreConfig, db BatchedSQLQueries, |
References
- The function comment should accurately describe its purpose and parameters. The type name for the
dbparameter is incorrect in the comment. (link)
ziggie1984
force-pushed
the
kvdb-sql-payments-improvements
branch
from
c7173ef to
a057853
Compare
🔴 PR Severity: CRITICAL
🔴 Critical (4 files)
🟠 High (6 files)
🟡 Medium (28 files)
🟢 Low (5 files)
AnalysisThis PR is CRITICAL severity due to:
Recommendation: This PR requires expert review from engineers familiar with:
To override, add a |
|
Do you want to change https://github.com/ziggie1984/lnd/blob/a0578535dd97cc48c23d03f01d6301c2151a05ac/go.mod#L222 to match master Line 222 in 44c6cc5 ? |
ziggie1984
force-pushed
the
elle-payment-sql-series-new
branch
from
8c106c7 to
2104014
Compare
Add a migration specific query which allows to set the failure 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.
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.
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.
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.
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.
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.
Add a config flag to skip in-migration validation for the KV->SQL payments migration. This is added as an option in case bigger payment databases don't require strict validation but instead prefer speed. This commit wires the option through the config, documents it in the sample config, and disables batch/count validation when requested.
Instead of using the OR case we use COALESCE to make sure the index are used effieciently when querying the payments.
during route retrieval don't query for the channel capacity. We don't set the channel capacity anymore. That is ok because on the one hand this value is deprecated and should be removed and on the other hand for old routes this will not succeed anyways since the channel has already been closed and we cannot lookup the channel capactiy anyways.
The previous commit stopped setting the channel capacity when retrieving the route. This commit makes sure that in the next release we remove the entries from the rpc interface.
Drop two explicit indexes in the payments migration that duplicate indexes already created by UNIQUE constraints: - payment_htlc_attempts(attempt_index) duplicates UNIQUE(attempt_index) - payment_route_hops(htlc_attempt_index) duplicates UNIQUE(htlc_attempt_index, hop_index) This reduces write/index maintenance overhead without changing query capabilities, since the UNIQUE-backed autoindexes already satisfy these lookups via exact and leftmost-prefix matching. Also remove the corresponding DROP INDEX statements in the down migration. We don't have to write a migration because the the main change is still not merged into master to we can just change it.
Add two composite indexes to the payments schema migration to better match
high-frequency query patterns observed in the payment lifecycle:
- idx_htlc_payment_id_attempt_time on payment_htlc_attempts(payment_id, attempt_time)
Matches batched attempt reads that filter by payment_id and order by
payment_id, attempt_time (FetchHtlcAttemptsForPayments).
- idx_htlc_resolutions_type_attempt_index on
payment_htlc_attempt_resolutions(resolution_type, attempt_index)
Matches failed-attempt cleanup paths that filter by resolution_type and
then join/delete by attempt_index (DeleteFailedAttempts).
Also update the down migration to drop these new indexes.
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.
ziggie1984
force-pushed
the
kvdb-sql-payments-improvements
branch
from
a057853 to
adf8264
Compare
3 participants
builds on top off #10485
Adds improvements how we retrieve payments: