Repository navigation
Conversation
Every LSPS2 client test should end by checking the same invariants on what the client did, whatever the mock service misbehaved in. Add tests/lsps2_oracles.py with the first two: - Amount: a paid invoice received at least its amount minus the opening fee it owes (none for payments after the session), and the client holds no more in channels its peers opened than its paid invoices received. - Identity: invoices route only through known channels or through the LSP on a jit_channel_scid it issued to this client, and the only channel used before its funding confirms is the session's. The checks work on an Observation snapshot of plain data, gathered from the client and the mock's state, so they are unit tested on hand- written runs. For that the mock now timestamps its events, reports the params it issued and remembers which payment opened each session. The honest JIT tests now run the oracles.
Expired, contradictory or modified params must never lead to a payment. Two checks: - Every lsps2.buy the client sent uses params exactly as the LSP issued them, before valid_until, with usable payment size limits, a payment size the fee does not eat up, and a promise of at most 512 bytes of printable ASCII without '"' or '\'. - Every LSPS2 invoice follows the buy result the client was sent: the lsp_cltv_expiry_delta in a route hint without fees, an expiry no later than valid_until and the payment size as amount.
The last oracle for the LSPS2 client tests: the LSPS client plugin is still running and never crashed, no HTLC is left pending in the client's channels, and the client sent no more than 20 LSPS requests. check_oracles first waits for the client's HTLCs to settle, so that a check does not race a payment still in flight.
A client that accepts a part whose extra_fee does not match what was deducted from it breaks bLIP-52 even when the total deducted happens to be the right fee, and the amount oracle did not see that. Check that each part of a paid invoice arrived short by exactly its extra_fee and that the extra_fees add up to at most what the payment owes.
The LSPS2 service is to leave Core Lightning, so the client tests need a counterpart that does not depend on it. Run each client test that uses the in-tree service against both it and the mock service, which serves the same opening fee menu as the policy plugin. Nothing in the tests changes for the in-tree service; dropping it later only drops its parameter. With the mock, the JIT channel tests also check the oracles. The non-approved zero-conf test makes its invoice itself, so the checks on how the client makes invoices do not apply there.
The MPP+fixed-invoice test ran the in-tree service with an earlier Python mock taking over its HTLC handling, because the service does not support MPP. Run it against the mock service instead, which collects the parts, opens the channel and spreads the opening fee over the parts itself, and remove the earlier mock. The fee stays at 1000000 msat, now computed from a proportional rate, and the test gains the checks of the mock's own MPP test, which it replaces: how the fee is spread over the parts, the invoice status and the oracles.
The tests of the LSPS2 client against a misbehaving LSP go into their own file and need the same helpers to set up nodes and talk to the mock service. Move them out of test_cln_lsps.py, unchanged.
Split lsps2_mock_pay() out of lsps2_mock_pay_var_invoice(), so that a test can pay the same invoice twice, or several invoices. Changelog-None
Andezion
left a comment
Andezion
left a comment
There was a problem hiding this comment.
In on_channel_opened, why spend the approval that expires first, and not keep a small "pending opens per peer" set filled by the openchannel hook? also on_lsps_lsps2_approve, on_channel_opened and on_peer_connected each rebuild rpc_path and call ClnRpc::new. maybe we could add a small State::rpc() helper, it could remove about 5 copies
|
|
||
| let dir = p.configuration().lightning_dir; | ||
| let rpc_path = Path::new(&dir).join(&p.configuration().rpc_file); | ||
| let mut cln_client = cln_rpc::ClnRpc::new(rpc_path.clone()).await?; |
There was a problem hiding this comment.
what do you think about taking approvals.lock() as the first await in on_channel_opened (before ClnRpc::new). That makes the window smaller. it still does not guarantee order on a multi thread runtime. if a full fix is wanted, track "opens in progress" by peer id in the hook and clear them in the notification?
There was a problem hiding this comment.
Done: on_channel_opened takes the lock before connecting to lightningd. As you say, it narrows the window without closing it. See the summary for why I didn't go for the pending-opens set.
| key: entry.key, | ||
| record, | ||
| }), | ||
| _ => stale.push(entry.key), |
There was a problem hiding this comment.
maybe treat a record without expires_at as one unspent approval with a short default expiry, instead of dropping it?
There was a problem hiding this comment.
I'd rather not. The old record only matters if the node is upgraded while a JIT invoice is outstanding and its LSP hasn't opened yet, and that invoice lives at most until the fee params' valid_until. Migrating it means guessing an expiry, since the old record doesn't say which invoice it's for. It also means moving it from lsps/client/<lsp> to a key below it. For an experimental option, the cost of dropping it is one failed JIT payment that needs a new invoice, which seems better than carrying migration code. Happy to add it if you feel strongly.
| // The buy's channel is open: its spent approval is done with. | ||
| if let Some(lsp_id) = lsp_id { | ||
| let _lock = p.state().approvals.lock().await; | ||
| if let Err(e) = settle_spent_approvals(&mut cln_client, &lsp_id, false).await { |
There was a problem hiding this comment.
an approval given back can outlive the buy?
There was a problem hiding this comment.
Yes. Say the LSP disconnects after our funding_signed but before channel_ready. On reconnect the channel is still waiting for channel_ready, so the approval is given back. If channel_ready then goes through on that same channel and pays the invoice, the approval is unspent and stays until the invoice expires: one more zero-conf channel the LSP could open. Fixed: a given-back approval remembers the channels it came from (given_back_from) and is deleted like a spent one once one of them gets going. test_lsps2_mock_ready_disconnect covers it (it fails without the fix), and settle_deletes_approvals_whose_channel_got_going covers the cases.
| start: None, | ||
| }) | ||
| .await?; | ||
| let invoice = invoices |
There was a problem hiding this comment.
lsps-lsps2-approve now fails if the invoice does not exist. but today lsps-lsps2-invoice is its only caller, and it calls approve after creating the invoice, so it is fine
There was a problem hiding this comment.
Right, that's intended, and the PR description mentions it. If lsps-lsps2-approve needs to work on its own later, it can take an explicit expiry then.
| expires_at: u64, | ||
| /// The funding txid of the channel the approval was spent on, if any. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| funding_txid: Option<String>, |
There was a problem hiding this comment.
why String and not a Txid?
There was a problem hiding this comment.
cln-rpc models txids as String (listpeerchannels' funding_txid is Option<String>), and the channel_opened notification gives us a string too. We only store the txid and compare it with listpeerchannels, so a Txid would mean parsing on both sides for nothing. Serialized, it's the same hex string, so it's easy to switch if cln-rpc moves to Txid.
…hannel is open
The LSPS2 client approved zero-conf channels from an LSP with a single
datastore record, lsps/client/<lsp_id>, and deleted it in the
openchannel hook. So:
- if the open failed after the hook, e.g. because we disconnected
before sending funding_signed, the LSP's open for the payer's retry
was refused ("You required zeroconf, but you're not on our
allowlist"), and the buy could not be paid;
- two outstanding JIT invoices from the same LSP shared the record, so
the channel for the second one was refused.
Keep an approval per buy instead, lsps/client/<lsp_id>/<jit_channel_scid>,
until the buy's invoice expires. The openchannel hook allows a zero-conf
channel from the LSP while one of its approvals is left, but no longer
spends it; open_channel doesn't say which buy the channel is for anyway.
An approval is spent once lightningd has saved the channel: it sends
channel_opened before funding_signed, so the LSP still gets one channel
per buy. If that channel is still waiting for channel_ready when the LSP
reconnects, the LSP never got our funding_signed, and the approval is
given back for its retry. Once the channel got going, the spent
approval is deleted: when the buy's invoice is paid, or on reconnect.
An approval given back remembers the channel it came from, and is
deleted the same way if that channel gets going after all.
Expired approvals, and the record older versions kept, are deleted when
the LSP's approvals are next read.
Changelog-Experimental: lsps-plugin: the LSPS2 client accepts the JIT channel for a payment retried after a failed open, and for each of several JIT invoices from the same LSP.
a697d20 to
ead247a
Compare
|
@Andezion Thanks for your review! On which approval to spend: A pending-opens set filled in the
|
Builds on #9658, which should merge first. Until then, this PR's diff includes its commits.
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:
tools/lightning-downgradeFixes #9715.
The LSPS2 client accepted a zero-conf channel from an LSP with a single datastore record per LSP, and deleted it in the
openchannelhook. So if the open failed after the hook, e.g. because we disconnected before sendingfunding_signed, the LSP's open for the payer's retry was refused. And two outstanding JIT invoices from the same LSP shared the record, so the second invoice's channel was refused.This keeps an approval per buy,
lsps/client/<lsp_id>/<jit_channel_scid>, until the buy's invoice expires:openchannelhook allows a zero-conf channel while the LSP has an approval left, but doesn't spend it:open_channeldoesn't say which buy it is forchannel_opened, which comes beforefunding_signed), so the LSP still gets one channel per buychannel_readywhen the LSP reconnects (peer_connected), the LSP never got ourfunding_signed, and the approval is given back for its retryThe first commit lets the mock tests pay an invoice they already have. The second adds three tests against the mock LSP: the retry after a disconnect before
funding_signed, two invoices from the same LSP, and an LSP that opens a second channel for a buy (refused before and after, the test checks it is the zero-conf refusal).Limits: an LSP that disconnects before sending
channel_readygets the approval back on reconnect, at the cost of a funding output it never broadcasts. And any channel the LSP funds while a buy is outstanding spends an approval, which only errs towards refusing. Theopenchannelhook and thechannel_openednotification run as separate tasks. lightningd refuses a second open while one is in progress, so the LSP's next open can only start once the first channel is saved. An LSP that sends it right away may still have its hook run before the notification spends the approval, and get a second zero-conf channel for the buy.lsps-lsps2-approvenow looks up the buy's invoice bypayment_hashto take its expiry, so it fails if no such invoice exists.lsps-lsps2-invoicecalls it after creating the invoice.Changelog-Changed: lsps-plugin: the LSPS2 client accepts the JIT channel for a payment retried after a failed open, and for each of several JIT invoices from the same LSP.