Remove invoice_id from static invoice server protocol #4009
Conversation
Missed removing some _ prefixes from vars that were previously cfg-gated.
|
👋 Thanks for assigning @joostjager as a reviewer! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4009 +/- ##
==========================================
- Coverage 88.85% 88.77% -0.09%
==========================================
Files 175 175
Lines 127710 127886 +176
Branches 127710 127886 +176
==========================================
+ Hits 113475 113525 +50
- Misses 11672 11805 +133
+ Partials 2563 2556 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
valentinewallace
force-pushed
the
2025-08-simplify-static-inv-server
branch
from
713b5fb to
d3c9a03
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Did a first high-level path.
IMO it would be preferable if we could refactor the offers field to be a helper type that wraps Vec<Option<AsyncReceiveOffer>>, as the internal API otherwise get's a bit hard to follow.
| }); | ||
| }, | ||
| None => return Err(()), | ||
| let slot_needs_new_offer = self.offers.get(cache_slot as usize) == Some(&None) |
There was a problem hiding this comment.
These option-in-option semantics get a bit hard to follow tbh. IMO it would be much more readable if we encapsulated self.offers into a dedicated helper object encapsulating the Vec and exposing 'proper' internal API methods that never leak the actual index usize.
Such a wrapper type could also more clearly assert the invariants, i.e., that we can never get more than u16::MAX/MAX_CACHED_OFFERS_TARGET entries?
There was a problem hiding this comment.
Looking through, ISTM the AsyncReceiveOfferCache is already basically just a wrapper around the Vec? I kinda don't want to overhaul the cache in this PR either way, tbh.
I'm not so sure this would improve readability either, it seems like that would just shift this not-so-readable code to inside the helper object? I think we'd have to move away from the pre-allocation of the cache or something like that to make a real dent here. Unless I'm misunderstanding the suggestion.
I agree the code is not too pretty as-is, though. I rewrote this commit, I think it's clearer? Let me know :)
There was a problem hiding this comment.
Thanks, the rewrite is def. more readable, although I still think the usize/u16 conversions and insertions into the cache could use a bunch more debug_asserts to make sure we're gonna catch if we'd ever violate the assumptions underlying the slot handling.
There was a problem hiding this comment.
One general remark about debug_assert: scenarios with a lot of invoices are perhaps most likely to occur in prod. Then we still do not have any indication that something is wrong. No log, no crash.
|
🔔 1st Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @joostjager! This PR has been waiting for your review. |
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
Straight forward change and I like the simplification of course.
Only thing that sticks a bit is whether a payer can get into an unexpected situation by requesting via a slot number, and that slot containing something completely different now?
If all slots are used for amount-less invoices, I don't think it matters much. But suppose we'd do invoices with an amount (which we talked about previously), perhaps this is unsafe?
valentinewallace
force-pushed
the
2025-08-simplify-static-inv-server
branch
from
d3c9a03 to
b419ce4
Compare
Yeah, it's a bit of a weird case. I think we'd need a fair amount of entirely new handling for that anyway. It may not even make sense unless the original offer has an amount, in which case even if the invoice was swapped out it should still match the offer amount, IIUC. |
Indeed, for swapping out invoices it doesn't matter. I was thinking specifically of reusing the slot for an offer with a different amount. But perhaps that can be solved by only reusing the slot when everything referencing it has expired? |
| /// allows us as the recipient to replace a specific invoice that is stored by the server, which | ||
| /// is useful for limiting the number of invoices stored by the server while also keeping all the | ||
| /// invoices persisted with the server fresh. | ||
| pub invoice_slot: u16, |
There was a problem hiding this comment.
"is 65536 enough for anyone"? Given tcp MTU, is there any reason to not just use u64 here?
There was a problem hiding this comment.
Hmm, would it be ok to revisit that in follow-up or on the bLIP since this is pre-existing? But I'm not sure supporting up to eighteen quintillion invoices makes sense lol. It feels like we'd want to go all the way to u128 and have it support being a randomly chosen number (but now we have a somewhat large key for the KV store), or choose a reasonable limit.
There was a problem hiding this comment.
Also note that u64 would at least disallow compilation on 32-bit systems (I think some embedded ones might still be out there) due to the usize convertion. And it def. wouldn't work for u128, as ~no system's usize could hold a u128.
There was a problem hiding this comment.
I think I'd normally just not use a relatively small value when it doesn't matter much, as a precaution for unforeseen use cases. Maybe there is a use where a ton of offers with different parameters/amount is pre-generated, and qr codes displayed on website, all while the receiver is offline. Or different offers that encode some metadata that can be useful to track back payments to where a qr was displayed.
But agreed that for our current amountless use case, there is no need to change. Not buying "pre-existing" for something that hasn't been released yet though. EIther way, no blocker.
| }; | ||
| invoice_slot, | ||
| ) { | ||
| return None; |
There was a problem hiding this comment.
Worth a log line, I think this should never happen? Or maybe the debug assert is sufficient.
There was a problem hiding this comment.
I think technically there could be a race where a new offer was separately inserted before this call and now we no longer need this current one, since we're not holding the cache lock for the whole method
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
valentinewallace
force-pushed
the
2025-08-simplify-static-inv-server
branch
from
b419ce4 to
8090af6
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Mostly looks good, mod pending comments.
| /// allows us as the recipient to replace a specific invoice that is stored by the server, which | ||
| /// is useful for limiting the number of invoices stored by the server while also keeping all the | ||
| /// invoices persisted with the server fresh. | ||
| pub invoice_slot: u16, |
There was a problem hiding this comment.
Also note that u64 would at least disallow compilation on 32-bit systems (I think some embedded ones might still be out there) due to the usize convertion. And it def. wouldn't work for u128, as ~no system's usize could hold a u128.
| }); | ||
| }, | ||
| None => return Err(()), | ||
| let slot_needs_new_offer = self.offers.get(cache_slot as usize) == Some(&None) |
There was a problem hiding this comment.
Thanks, the rewrite is def. more readable, although I still think the usize/u16 conversions and insertions into the cache could use a bunch more debug_asserts to make sure we're gonna catch if we'd ever violate the assumptions underlying the slot handling.
In the initially-merged version of the static invoice server protocol, the static invoice server would sometimes have to find a specific static invoice based on (recipient_id, invoice_slot) and sometimes based on (recipient_id, invoice_id). This made the API harder to use in terms of how the server would index into the KVStore. We'd like to transition to the server always finding a specific invoice based on (recipient_id, invoice_slot) and get rid of the invoice_id concept. Now that the invoice_slot is in the initial paths request, the server will be able to include the slot in the offer paths that they create in response, allowing the slot to be surfaced instead of the invoice_id when the invoice request comes in, in upcoming commits.
When we as an async recipient receive offer paths from the static invoice server, we create an offer and cache it, retrying persisting a corresponding invoice with the server until it succeeds. In the initially-merged version of this protocol, we would put this cached offer in any slot in the cache that needed an offer at the time the offer paths were received. However, in the last commit we started requesting offer paths for a specific slot in the cache, as part of eliminating the use of the invoice_id field in the overall protocol. As a result, here we put the cached offer in the specific cache slot that the original OfferPathsRequest indicated, rather than any slot that could use a new offer.
In the initially-merged version of the static invoice server protocol, the static invoice server would sometimes have to find a specific static invoice based on (recipient_id, invoice_slot) and sometime based on (recipient_id, invoice_id). This made the API harder to use in terms of how the server would index into the KVStore. We'd like to transition to the server always finding a specific invoice based on (recipient_id, invoice_slot) and get rid of the invoice_id concept. As part of this series of commits, include the invoice_slot in the ServeStaticInvoice blinded path context that the server creates when sending an offer_paths message. This is possible due to a previous commit including the invoice_slot in the initial offer_paths_request from the recipient, and lays the groundwork for removing the invoice_id field from this blinded path context.
In the initially-merged version of the static invoice server protocol, the static invoice server would sometimes have to find a specific static invoice based on (recipient_id, invoice_slot) and sometime based on (recipient_id, invoice_id). This made the API harder to use in terms of how the server would index into the KVStore. We'd like to transition to the server always finding a specific invoice based on (recipient_id, invoice_slot) and get rid of the invoice_id concept. In the previous commit the server began including the invoice_slot in the ServeStaticInvoice blinded path context that gets provided back to themselves. Therefore there is no need for the recipient to redundantly include it in the ServeStaticInvoice onion message itself.
In the initially-merged version of the static invoice server protocol, the static invoice server would sometimes have to find a specific static invoice based on (recipient_id, invoice_slot) and sometime based on (recipient_id, invoice_id). This made the API harder to use in terms of how the server would index into the KVStore. We'd like to transition to the server always finding a specific invoice based on (recipient_id, invoice_slot) and get rid of the invoice_id concept. Previously, when an invoice request would come in for the server on behalf of the often-offline recipient, they would need to find the static invoice based on the recipient_id and invoice_id. However, previous commits have now led to the server being able to use the invoice_slot in the initial offer paths that they create, which we do here, obviating the need for them to create their own randomly-generated invoice_id. The final dangling references to the invoice_id will be removed in the next commit.
In the initially-merged version of the static invoice server protocol, the static invoice server would sometimes have to find a specific static invoice based on (recipient_id, invoice_slot) and sometimetimes based on (recipient_id, invoice_id). This made the API harder to use in terms of how the server would index into the KVStore. Over the course of the previous commits we transitioned to the server always finding a specific invoice based on (recipient_id, invoice_slot). We still have a few dangling references to invoice_id in some messages and events though, so remove those here.
valentinewallace
force-pushed
the
2025-08-simplify-static-inv-server
branch
from
8090af6 to
e159ae4
Compare
valentinewallace
merged commit 1f4bd17
into
lightningdevkit:main
4 participants
In the initially-merged version of the static invoice server protocol, the
static invoice server would sometimes have to find a specific static invoice
based on
(recipient_id, invoice_slot)and sometime based on(recipient_id, invoice_id). This made the API harder to use in terms of how the server wouldindex into the KVStore.
Here we transition to the server always finding a specific invoice based on
(recipient_id, invoice_slot)and get rid of the invoice_id concept.