-
Notifications
You must be signed in to change notification settings - Fork 116
Static invoice server #621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Static invoice server #621
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this is in draft for good reason, just leaving some early comments here.
4bcdc95
to
e2f293b
Compare
src/rate_limiter.rs
Outdated
min_interval: Duration, | ||
} | ||
|
||
impl RateLimiter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping it simple here
src/static_invoice_store.rs
Outdated
|
||
pub(crate) struct StaticInvoiceStore { | ||
kv_store: Arc<DynStore>, | ||
rate_limiter: Mutex<RateLimiter>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to add Mutex
here even though I don't think it is used concurrently. Otherwise handle_event
needed to be made mutable, the Arc higher up no longer working, etc.
src/static_invoice_store.rs
Outdated
} | ||
} | ||
|
||
pub(crate) async fn handle_static_invoice_requested( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Async, prepare for async kv store.
0c92e33
to
5c43a3b
Compare
5c43a3b
to
472cf60
Compare
Added bindings |
472cf60
to
16ad843
Compare
Made rate limiter a bit less simplistic by adding a leaky bucket. Otherwise the second call of two consecutive calls immediately fails. |
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a first look, have yet to review the rate limiter logic and tests closer.
src/lib.rs
Outdated
/// [`StaticInvoice`]: lightning::offers::static_invoice::StaticInvoice | ||
pub fn blinded_paths_for_async_recipient( | ||
&self, recipient_id: Vec<u8>, | ||
) -> Result<Vec<u8>, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Rust API, we def. still want to return the actual BlindedMessagePaths
, just for the uniffi
interface it's okay to fallback to a bytes
for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it be required to know the details of the blinded paths for the Rust API and not for the other APIs? Not sure which use that unlocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it be required to know the details of the blinded paths for the Rust API and not for the other APIs? Not sure which use that unlocks.
Users might want to expect what they're paying to. It's fine to take a shortcut here for bindings, but please use the full Rust type in the Rust API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not about what they are paying to. It's the path that the receiver can use to contact the static invoice server. Because it is blinded, they still don't know where that server is potentially?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to use the proper Rust types where possible, and I really don't see why we shouldn't in this case. In case of set_paths_to_static_invoice_server
users could theoretically handcraft a blinded path somehow if their service implementation doesn't exactly meet our serialization and try to use it. Here we'd just match that API type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think consistency between languages is more important. Especially if the handcrafted blinded path is speculative. But no blocker, I've updated the code with distinct method signatures.
04a3c30
to
514676c
Compare
Unit tests added for static invoice store and rate limiter. |
Added async services config flag |
b1ca4e2
to
5b489c2
Compare
278669e
to
4225a5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, some comments/questions.
{ | ||
Ok(_) => {}, | ||
Err(e) => { | ||
log_error!(self.logger, "Failed to persist static invoice: {}", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to return Err(ReplayEvent())
here and below in case of persistence failure to be able to retry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Also static_invoice_persisted
shouldn't be called in that case. Fixed here and below.
src/io/mod.rs
Outdated
|
||
// Static invoices will be persisted at "static_invoices/<sha256(recipient_id)>/<invoice_slot>". | ||
// | ||
// Example: static_invoices/039058c6f2c0cb492c533b0a4d14ef77cc0f78abccced5287d84a1a2011cfb81/001f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is out-of-date now that we don't serialize invoice_slot
as hex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, let's move this example to the static invoice store, as this is just the primary namespace const. We should however cross-link to the the respective LDK docs here so that readers know what 'static invoices' are in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decimal invoice slot, example moved, LDK link added.
src/io/mod.rs
Outdated
// Static invoices will be persisted at "static_invoices/<sha256(recipient_id)>/<invoice_slot>". | ||
// | ||
// Example: static_invoices/039058c6f2c0cb492c533b0a4d14ef77cc0f78abccced5287d84a1a2011cfb81/001f | ||
pub(crate) const STATIC_INVOICES_PRIMARY_NAMESPACE: &str = "static_invoices"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub(crate) const STATIC_INVOICES_PRIMARY_NAMESPACE: &str = "static_invoices"; | |
pub(crate) const STATIC_INVOICE_STORE_PRIMARY_NAMESPACE: &str = "static_invoices"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I like this better. It's not the store that is stored, but the invoices. I don't really mind either. Changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the other new files need a module description and a copyright header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
mod tests { | ||
use std::{sync::Arc, time::Duration}; | ||
|
||
use bitcoin::{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: As mentioned elsewhere previously, module-level grouping of imports would be preferred.
I'm starting to wonder if we should go 'bleeding edge' here and introduce a nightly
rustfmt CI job, which would indeed support dealing with the import groups for us:
group_imports = "StdExternalCrate"
reorder_imports = true
imports_granularity = "Module"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great. FIxing import groups isn't something that humans should be required to do, especially because some people don't even care for them.
src/payment/bolt12.rs
Outdated
pub(crate) fn new( | ||
channel_manager: Arc<ChannelManager>, payment_store: Arc<PaymentStore>, | ||
is_running: Arc<RwLock<bool>>, logger: Arc<Logger>, | ||
async_payment_services_enabled: bool, is_running: Arc<RwLock<bool>>, logger: Arc<Logger>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, let's just forward the full Config
as we might need it for other things in the future, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to add a dependency to the full object if a bool suffices. Also makes it more clear what part of config matters. Maybe we don't need things in the future. But no strong objection, so changed.
/// Will only return an offer if [`Bolt12Payment::set_paths_to_static_invoice_server`] was called and we succeeded | ||
/// in interactively building a [`StaticInvoice`] with the static invoice server. | ||
/// | ||
/// Useful for posting offers to receive payments later, such as posting an offer on a website. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind adding an 'experimental feature' note here and on the other main APIs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that, but in what regard is it experimental? Added
) | ||
} | ||
|
||
pub(crate) async fn handle_persist_static_invoice( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check whether we actually know/have a channel open with the recipient? Or is all that fully handled on the LDK side already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. At this point, I think it is part of the integration of ldk-node into the wider system to ensure that blinded_paths_for_async_recipient
isn't called without restrictions.
4225a5f
to
e76d6d7
Compare
Squashed previous fixups and added new fix up that addresses review comments. |
e76d6d7
to
8ad197f
Compare
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, feel free to squash!
src/event.rs
Outdated
log_error!(self.logger, "Failed to send static invoice: {:?}", e); | ||
} | ||
}, | ||
Ok(None) => {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we log here, or do we think this would be very spammy? Also, does this point to a gap in the protocol that we don't seem to have a way to communicate to the client that a lookup failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's already more per-onion message trace logging, so spamming the logs is possible already. I've added a trace statement. At this stage it may be helpful in debugging production issues.
You could consider the lack of a failure onion message a gap in the protocol. Onion messages are problematic anyway because they may be dropped without notice.
A similar gap exists for the held_htlc_available
message. The sender won't get an ack indicating whether that message was processed. If that one gets dropped, the sender lsp will hold the htlc waiting for a release that will never come.
8ad197f
to
f471f11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, mind adding a bit more verbose commit message/title, then happy to land this!
This commit adds support for using ldk-node as a static invoice server. When configured as such, the node persists and retrieves invoices from the configured kv store. Access is guarded by a rate limiter to prevent overload and mitigate potential DoS attacks. In this mode, ldk-node also exposes blinded paths that can be shared with async recipients, allowing them to contact the static invoice server. When ldk-node functions as a recipient, it can communicate with the static invoice server to set up async payments.
f471f11
to
66f5c28
Compare
Commit msg/title added |
Wires up async payments functionality in ldk-node. This PR only covers the static invoice creation part. In a follow up, the full mechanism will be put in place to hold an htlc sender-side until the receiver comes online again.