Add GPRC methods for the duplicate payments table for the SQL backend #10489
Conversation
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 helpers 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/channel.sqlite (or a KV postgres 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 and config builder, add logging for the migration path, and introduce tombstone protection to prevent re-running once migration completes.
Add SQL queries and store APIs to fetch duplicate payments by hash or across all records. This introduces the duplicate payment domain type, conversion helpers, and the SQL-backed fetch methods, and wires them into the SQL query interface/SQLC bindings.
Add SQL-only tests for fetching duplicate payments, covering per-payment lookup and pagination across multiple payments.
Introduce RPCs for listing duplicate payments and wire them into the main server. This adds the new proto messages, REST bindings, and rpcserver handlers, and reuses a shared failure-reason marshal helper for consistent enum mapping.
Add hidden lncli commands to call the duplicate payment list RPCs for debugging and inspection.
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 enhances the payment system by exposing legacy duplicate payment records through new gRPC endpoints and 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 pull request introduces GPRC methods for handling duplicate payments within the SQL backend, a significant step in the migration from a KV store. The changes are extensive, encompassing new RPCs, CLI commands, database schema updates, and a dedicated migration package. The implementation appears robust, with thorough testing and a clear focus on data integrity. My feedback primarily addresses opportunities to enhance code clarity and ensure full adherence to the repository's style guide, specifically regarding function documentation and error message accuracy.
| Action: actionDecorator(listPaymentDuplicates), | ||
| } | ||
|
|
||
| func listPaymentDuplicates(ctx *cli.Context) error { |
There was a problem hiding this comment.
This function is missing a comment explaining its purpose. According to the style guide, every function must be commented.
// listPaymentDuplicates is the action for the `listpaymentduplicates` command.
// It retrieves and displays duplicate payment records for a given payment hash.
func listPaymentDuplicates(ctx *cli.Context) error {References
- Every function must be commented with its purpose and assumptions. Function comments must begin with the function name and should be complete sentences. (link)
| Action: actionDecorator(listAllPaymentDuplicates), | ||
| } | ||
|
|
||
| func listAllPaymentDuplicates(ctx *cli.Context) error { |
There was a problem hiding this comment.
This function is missing a comment explaining its purpose. According to the style guide, every function must be commented.
// listAllPaymentDuplicates is the action for the `listallpaymentduplicates`
// command. It retrieves and displays all duplicate payment records across all
// payments.
func listAllPaymentDuplicates(ctx *cli.Context) error {References
- Every function must be commented with its purpose and assumptions. Function comments must begin with the function name and should be complete sentences. (link)
| return fmt.Errorf("payments bucket does not "+ | ||
| "exist: %w", err) |
There was a problem hiding this comment.
The error message "payments bucket does not exist" is confusing here, as it's returned from a code block that attempts to create the bucket if it doesn't exist. A more accurate message would clarify that the creation failed.
| return fmt.Errorf("payments bucket does not "+ | |
| "exist: %w", err) | |
| return fmt.Errorf("failed to create payments "+ | |
| "bucket: %w", err) |
| return c, nil | ||
| } | ||
|
|
||
| func fetchDuplicatePayment(bucket kvdb.RBucket) (*MPPayment, error) { |
There was a problem hiding this comment.
This function is missing a comment explaining its purpose. According to the style guide, every function must be commented.
// fetchDuplicatePayment deserializes a legacy duplicate payment from the
// given bucket.
func fetchDuplicatePayment(bucket kvdb.RBucket) (*MPPayment, error) {References
- Every function must be commented with its purpose and assumptions. Function comments must begin with the function name and should be complete sentences. (link)
| return payment, nil | ||
| } | ||
|
|
||
| func fetchDuplicatePayments(paymentHashBucket kvdb.RBucket) ([]*MPPayment, |
There was a problem hiding this comment.
This function is missing a comment explaining its purpose. According to the style guide, every function must be commented.
// fetchDuplicatePayments fetches all legacy duplicate payments for a given
// payment hash bucket.
func fetchDuplicatePayments(paymentHashBucket kvdb.RBucket) ([]*MPPayment,References
- Every function must be commented with its purpose and assumptions. Function comments must begin with the function name and should be complete sentences. (link)
| return fmt.Errorf("non bucket element" + | ||
| "in duplicate bucket") |
There was a problem hiding this comment.
There's a missing space in the error string concatenation, which will result in the message "non bucket elementin duplicate bucket".
return fmt.Errorf("non bucket element " +
"in duplicate bucket")|
|
||
| // parseDuplicateSettleData extracts settle data from either legacy or modern | ||
| // duplicate formats. | ||
| func parseDuplicateSettleData(settleData []byte) ([]byte, sql.NullTime, error) { |
There was a problem hiding this comment.
This function is missing a comment explaining its purpose. According to the style guide, every function must be commented.
| func parseDuplicateSettleData(settleData []byte) ([]byte, sql.NullTime, error) { | |
| // parseDuplicateSettleData extracts settle data from either legacy or modern | |
| // duplicate formats. Legacy formats only contain the preimage, while modern | |
| // formats contain a serialized HTLCSettleInfo. | |
| func parseDuplicateSettleData(settleData []byte) ([]byte, sql.NullTime, error) { |
References
- Every function must be commented with its purpose and assumptions. Function comments must begin with the function name and should be complete sentences. (link)
| return resp, nil | ||
| } | ||
|
|
||
| func paymentDuplicateToRPC(dup *paymentsdb.DuplicatePayment) ( |
There was a problem hiding this comment.
This function is missing a comment explaining its purpose. According to the style guide, every function must be commented.
// paymentDuplicateToRPC converts a paymentsdb.DuplicatePayment to its
// corresponding lnrpc.PaymentDuplicate representation.
func paymentDuplicateToRPC(dup *paymentsdb.DuplicatePayment) (References
- Every function must be commented with its purpose and assumptions. Function comments must begin with the function name and should be complete sentences. (link)
1 participant
Builds on top of: #10485
This PR is just for completion in case users have duplcate payments and want to access them after the migration.
Before the duplicate payments would be always fetched as well, but since this is a very old bug and most of the nodes should not have any of those duplicate payments I decided to pack them into a different lean table. This PR makes sure people can still access them if they need to.