invoices/sqldb: query by ChanID when updating AMP invoice preimage #9022
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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? TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Please make sure that the linter is green too! |
I have questions on this: The lint task fails on checking submodule tidiness. However, locally, I can't reproduce this: The other thing, when I |
|
@aakselrod I get the following diff when I run diff --git a/go.mod b/go.mod
index 6652e6547..cbe4d40ba 100644
--- a/go.mod
+++ b/go.mod
@@ -19,7 +19,6 @@ require (
github.com/davecgh/go-spew v1.1.1
github.com/decred/dcrd/dcrec/secp256k1/v4 v4.3.0
github.com/go-errors/errors v1.0.1
- github.com/golang/protobuf v1.5.3
github.com/gorilla/websocket v1.5.0
github.com/grpc-ecosystem/go-grpc-middleware v1.3.0
github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
@@ -95,6 +94,7 @@ require (
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang-jwt/jwt/v4 v4.4.2 // indirect
github.com/golang-migrate/migrate/v4 v4.17.0 // indirect
+ github.com/golang/protobuf v1.5.3 // indirect
github.com/golang/snappy v0.0.4 // indirect
github.com/google/btree v1.0.1 // indirect
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirectMaybe a question of the Go version you're using? Hmm, normally we do this by doing a PR to the submodule first, merge that and push a tag, then bump the version in the main |
There was a problem hiding this comment.
make tidy-module-check is different from running the script directly as it involves one more check,
Lines 337 to 339 in cc9e2b7
That did it, thanks! |
I think my problem was that I wasn't running the
OK, I'll make this change. Thank you! |
aakselrod
force-pushed
the
add-channelid-ampsettle
branch
from
a6e1960
to
3e36cda
Compare
|
Lint is passing now! |
|
I think we should do a full CI run again once #9021 is in and this is rebased. |
|
I'll do that rebase today! |
aakselrod
force-pushed
the
add-channelid-ampsettle
branch
from
ea3b246
to
447e0d1
Compare
|
Rebase complete, hopefully should be good to merge after CI run. |
There was a problem hiding this comment.
Actually, this test failure very much looks like it could be affected by these changes:
--- FAIL: TestLightningNetworkDaemon/tranche05/107-of-168/bitcoind/sendpayment_amp_invoice_repeat (19.61s)
lnd_amp_test.go:300:
Error Trace: /home/runner/work/lnd/lnd/itest/lnd_amp_test.go:300
/home/runner/work/lnd/lnd/lntest/harness.go:386
/home/runner/work/lnd/lnd/itest/lnd_test.go:139
Error: Not equal:
expected: 1
actual : 2
Test: TestLightningNetworkDaemon/tranche05/107-of-168/bitcoind/sendpayment_amp_invoice_repeat
I ran the test locally and got the following errors in the Postgres container:
2024-08-26 08:45:15.430 UTC [88] ERROR: duplicate key value violates unique constraint "invoices_payment_addr_key"
2024-08-26 08:45:15.430 UTC [88] DETAIL: Key (payment_addr)=(\x68d295d5d6daa959147608463bb46f5ae877f0806d3af93ff48f681e79411700) already exists.
2024-08-26 08:45:15.430 UTC [88] STATEMENT: -- name: InsertInvoice :one
INSERT INTO invoices (
hash, preimage, memo, amount_msat, cltv_delta, expiry, payment_addr,
payment_request, payment_request_hash, state, amount_paid_msat, is_amp,
is_hodl, is_keysend, created_at
) VALUES (
$1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15
) RETURNING id
2024-08-26 08:45:15.440 UTC [88] ERROR: duplicate key value violates unique constraint "invoices_payment_addr_key"
2024-08-26 08:45:15.440 UTC [88] DETAIL: Key (payment_addr)=(\x68d295d5d6daa959147608463bb46f5ae877f0806d3af93ff48f681e79411700) already exists.
2024-08-26 08:45:15.440 UTC [88] STATEMENT: -- name: InsertInvoice :one
INSERT INTO invoices (
hash, preimage, memo, amount_msat, cltv_delta, expiry, payment_addr,
payment_request, payment_request_hash, state, amount_paid_msat, is_amp,
is_hodl, is_keysend, created_at
) VALUES (
$1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15
) RETURNING id
2024-08-26 08:45:15.686 UTC [88] ERROR: duplicate key value violates unique constraint "invoices_payment_addr_key"
2024-08-26 08:45:15.686 UTC [88] DETAIL: Key (payment_addr)=(\x68d295d5d6daa959147608463bb46f5ae877f0806d3af93ff48f681e79411700) already exists.
2024-08-26 08:45:15.686 UTC [88] STATEMENT: -- name: InsertInvoice :one
INSERT INTO invoices (
hash, preimage, memo, amount_msat, cltv_delta, expiry, payment_addr,
payment_request, payment_request_hash, state, amount_paid_msat, is_amp,
is_hodl, is_keysend, created_at
) VALUES (
$1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15
) RETURNING id
2024-08-26 08:46:10.586 UTC [346] ERROR: duplicate key value violates unique constraint "invoices_payment_addr_key"
2024-08-26 08:46:10.586 UTC [346] DETAIL: Key (payment_addr)=(\xc781996e8a209bfdd5e2775876b8626da735bfb088a94423985f8307a7aae2ac) already exists.
2024-08-26 08:46:10.586 UTC [346] STATEMENT: -- name: InsertInvoice :one
INSERT INTO invoices (
hash, preimage, memo, amount_msat, cltv_delta, expiry, payment_addr,
payment_request, payment_request_hash, state, amount_paid_msat, is_amp,
is_hodl, is_keysend, created_at
) VALUES (
$1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15
) RETURNING id
2024-08-26 08:46:10.806 UTC [346] ERROR: duplicate key value violates unique constraint "invoices_payment_addr_key"
2024-08-26 08:46:10.806 UTC [346] DETAIL: Key (payment_addr)=(\xc781996e8a209bfdd5e2775876b8626da735bfb088a94423985f8307a7aae2ac) already exists.
2024-08-26 08:46:10.806 UTC [346] STATEMENT: -- name: InsertInvoice :one
INSERT INTO invoices (
hash, preimage, memo, amount_msat, cltv_delta, expiry, payment_addr,
payment_request, payment_request_hash, state, amount_paid_msat, is_amp,
is_hodl, is_keysend, created_at
) VALUES (
$1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15
) RETURNING id
|
These errors are caused by lnd/invoices/invoiceregistry.go Line 888 in 8939a21 If this PR doesn't get merged before I find/fix this failing test, I'll include the fix here instead. |
Ah, right. I missed that part, sorry.
Sounds good. Though I think we'll want to block |
|
I think it's an issue with invoice numbering, the behavior is different between kvdb and sql here, so it looks like a real but separate bug to me. |
|
The |
|
Looks like the native SQL itest are still failing: |
I no longer think it's about invoice numbering but didn't have much time to keep digging into it today. Will keep going tomorrow. |
|
I've spent a few hours on this today, and have gotten farther. It looks like the SQL invoice updater isn't returning only the "projected" HTLCs for the newly-settled AMP invoice, but all of the past invoices. The newly-settled AMP invoice is also returned with the incorrect settle index and time, though it seems to be written correctly to the DB and returned correctly on I also noticed that in the KVDB code, the updater consistently logs the error Not 100% sure that's relevant, but I'm going to keep going on this tomorrow. |
|
Also will take a closer look at Line 234 in 1bf7ad9 ETA: it does run native SQL tests, so that's good. |
I believe the issue is that when we fetch the invoice during the update the Line 1301 in 5c3a8e9 setID won't work as the SQL query assumes that the AMP sub-invoice already exists. In the KV version the assumption is different as if the setID is passed it'll simply fetch the HTLCs in that set: Line 1508 in 5c3a8e9 |
This looks exactly right to me.
Sounds good, thank you! Do you want to make a separate PR or add it to this one? |
I think it's best to add to this one, given it fails the itest. |
I've pushed an itest change that checks for this, and then a fix. Hopefully they both run so we can see the difference... |
guggero
force-pushed
the
add-channelid-ampsettle
branch
from
8c81b5e
to
4a3a3c7
Compare
|
Did a quick rebase to fix a merge conflict in the release notes, so we can see the CI run. |
When fetching an AMP invoice we sometimes filter HTLCs to selected set IDs, however we always kept the full AMP state which is irrelevant as it contains state for all AMP payments. This was a side effect of UpdateInvoice needing to serialize the whole invoice when storing after an update but it is an unwanted "feature" as users will need to filter to relevant set when listing an AMP payment or subsribing to an update.
Previously SQL invoice updater ignored the set ID hint when updating an AMP invoice resulting in update subscriptions returning all of the AMP state as well as all AMP HTLCs. This commit synchornizes behavior with the KV implementation such that we now only return relevant AMP state and HTLCs when updating an AMP invoice.
|
Added fixes for the remaining issues. Note that the fix changes some previous behavior IMO for the better (cc @Roasbeef) |
Previously, the SQL implementation of the invoice query simply converted the start and end timestamps to time and used them in SQL queries to check for inclusivity. However, this logic failed when the start and end timestamps were equal. This commit addresses and corrects this issue.
|
I wonder if we should instead open another PR with the rest of the fixes since these seem to be unrelated to the original fix. |
Note that these logs are harmless and emitted when running lnd/invoices/invoiceregistry.go Line 889 in e8c5e7d It is def a bit ugly that we have these logs, but perhaps we can keep them? wdyt @guggero ? |
|
|
PR opened here: #9050 |
|
Closing in favor of #9050. |
Change Description
This PR fixes the following test:
Note that the
sendpayment_amp_invoice_repeattest is not fixed by this. Also note that this depends on #9021 to unmask the test failure.Steps to Test
Run the itest as above before and after applying this PR.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.