Remove invoice_id from static invoice server protocol
#4009
+158
−162
Conversation
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
Missed removing some _ prefixes from vars that were previously cfg-gated.
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.
|
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4009 +/- ##
==========================================
- Coverage 88.85% 88.83% -0.02%
==========================================
Files 175 175
Lines 127710 127706 -4
Branches 127710 127706 -4
==========================================
- Hits 113475 113454 -21
- Misses 11672 11696 +24
+ 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:
|
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
713b5fb to
d3c9a03
Compare
tnull
reviewed
Aug 15, 2025
| if !needs_new_offers { | ||
| return Ok(()); | ||
| } | ||
| let needs_new_offer_idx: u16 = |
There was a problem hiding this comment.
Rather than having to convert at the call sites, would it make sense for the internal APIs to use/return the invoice slot number as a u16 where possible?
| }); | ||
| }, | ||
| 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?
|
🔔 1st Reminder Hey @joostjager! This PR has been waiting for your review. |
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.
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.