chore: settings stream#1997
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87b6954be6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| service CdkPaymentProcessor { | ||
| rpc GetSettings(EmptyRequest) returns (SettingsResponse) {} | ||
| rpc GetSettings(EmptyRequest) returns (stream SettingsResponse) {} |
There was a problem hiding this comment.
Changing GetSettings from unary to server-streaming is a wire-level breaking API change, but this commit does not update the payment-processor protocol version gate (PAYMENT_PROCESSOR_PROTOCOL_VERSION remains 3.0.0). That means mixed-version peers can still pass the version interceptor and then fail at runtime on GetSettings with incompatible stubs instead of being rejected early as an explicit version mismatch.
Useful? React with 👍 / 👎.
| None => { | ||
| // Backend has no more updates; keep connection alive until | ||
| // shutdown or client disconnect | ||
| tokio::select! { | ||
| _ = shutdown.notified() => {} | ||
| _ = tx.closed() => {} | ||
| } | ||
| break; |
There was a problem hiding this comment.
When inner.wait_settings() ends (None), this branch parks until shutdown or client disconnect and never re-subscribes to the backend stream. In environments where the upstream settings stream can terminate transiently (e.g., chained processor restart), connected clients stop receiving updates permanently even though the server stays up; this differs from wait_payment_event, which re-enters subscription logic.
Useful? React with 👍 / 👎.
d4rp4t
force-pushed
the
chore/payment-processor-stream
branch
from
2dd38c3 to
49793d3
Compare
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Single-shot settings streams cause a hot mint_info update loop (high) - A standard mint using first-party in-process payment backends can enter a hot background loop that consumes CPU and continuously writes the mint_info record, causing avoidable load and potential database contention.
| /// | ||
| /// A single processor instance may handle multiple payment keys (e.g. arkade handles both | ||
| /// bolt11 and onchain). Each settings event is applied to every key the processor owns. | ||
| async fn wait_for_processor_settings( |
There was a problem hiding this comment.
wait_for_processor_settings re-subscribes immediately whenever the settings stream ends. That is fine for the gRPC client, but the new default MintPayment::wait_settings returns stream::once(...), so every in-process backend that does not override it emits one settings item, reaches None, breaks only the inner loop, and then immediately calls wait_settings() again with no sleep. Since this task is spawned for each processor and each item calls update_mint_info_from_settings, a normal mint using CLN/LND/LDK/fake/lnbits/bdk will spin continuously and perform repeated mint_info DB write transactions. Please either keep completed single-shot streams idle until shutdown, add a backoff before reconnecting, or only spawn this watcher for processors that provide a live settings stream.
| /// The first item is always the current settings. Subsequent items arrive | ||
| /// whenever the processor detects a change. Backends that do not support | ||
| /// live updates return a single-item stream with the current settings. | ||
| async fn wait_settings( |
There was a problem hiding this comment.
wait_for_processor_settings re-subscribes immediately whenever the settings stream ends. That is fine for the gRPC client, but the new default MintPayment::wait_settings returns stream::once(...), so every in-process backend that does not override it emits one settings item, reaches None, breaks only the inner loop, and then immediately calls wait_settings() again with no sleep. Since this task is spawned for each processor and each item calls update_mint_info_from_settings, a normal mint using CLN/LND/LDK/fake/lnbits/bdk will spin continuously and perform repeated mint_info DB write transactions. Please either keep completed single-shot streams idle until shutdown, add a backoff before reconnecting, or only spawn this watcher for processors that provide a live settings stream.
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Bolt12 settings updates are ignored by the live settings updater (medium) - Dynamic settings updates for Bolt12 payment processors are silently ignored, so the mint can continue advertising and enforcing stale NUT-04/NUT-05 amount limits after the backend changes its constraints.
|
|
||
| let mut mint_info: MintInfo = serde_json::from_slice(&mint_info_bytes)?; | ||
|
|
||
| match &key.method { |
There was a problem hiding this comment.
Should this updater also handle KnownMethod::Bolt12? The PR adds receive_limits/send_limits to Bolt12Settings and applies them during add_payment_processor, but live settings updates only match Bolt11 and Onchain, so a Bolt12 settings stream falls through _ => {} and leaves the advertised NUT-04/NUT-05 limits stale. Mirroring the Bolt12 branch from the builder here would keep dynamic Bolt12 processor limits in sync.
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Concurrent settings watchers can overwrite MintInfo updates (medium) - Mints configured with multiple payment processors can publish stale or incorrect payment method options/limits after concurrent settings updates, because one processor's MintInfo update may be lost.
| settings: cdk_common::payment::SettingsResponse, | ||
| operator_limits: Option<MintMeltLimits>, | ||
| ) -> Result<(), Error> { | ||
| let mint_info_bytes = localstore |
There was a problem hiding this comment.
The new settings watchers can race when a mint has more than one payment processor. Each watcher calls update_mint_info_from_settings, which reads the whole MintInfo before opening a transaction, updates only its own method entry, and then writes the whole blob back. If two processors emit their initial/current settings around the same time, both tasks can read the same original MintInfo; whichever task commits last overwrites the other task's changes to NUT-04/NUT-05/NUT-15 or amount limits. Please serialize these read-modify-write updates (for example, one settings-update worker or a mint-info mutex/transaction that covers the read and write) so independent processor updates are merged instead of lost.
| ); | ||
|
|
||
| tracing::info!("Starting payment wait task for {:?}", key); | ||
| { |
There was a problem hiding this comment.
The new settings watchers can race when a mint has more than one payment processor. Each watcher calls update_mint_info_from_settings, which reads the whole MintInfo before opening a transaction, updates only its own method entry, and then writes the whole blob back. If two processors emit their initial/current settings around the same time, both tasks can read the same original MintInfo; whichever task commits last overwrites the other task's changes to NUT-04/NUT-05/NUT-15 or amount limits. Please serialize these read-modify-write updates (for example, one settings-update worker or a mint-info mutex/transaction that covers the read and write) so independent processor updates are merged instead of lost.
3 participants
Description
Implements the GetSettings server-streaming RPC in the payment processor crate. The proto already declared rpc GetSettings(EmptyRequest) returns (stream SettingsResponse) but the server returned a single response and the client consumed it as such. This wires up the full streaming path so the mint can receive settings updates from the payment processor in real time without restarting.
Notes to the reviewers
The server is event-driven — it calls inner.wait_settings() and forwards whatever the backend emits, mirroring the pattern used by wait_payment_event. No polling.
wait_settings is added to MintPayment with a default implementation that emits the current settings as a single item (stream::once(get_settings())). Backends that support live updates (e.g. a chained payment processor) can override it. Backends that don't (cdk-cln, cdk-bdk, cdk-fake-wallet) get the default: one item then stream stays open until client disconnects or server shuts down.
get_settings() on the PaymentProcessorClient reads the first message from the stream — backward compatible with all existing callers. wait_settings() subscribes to the full stream for callers that want live updates.
The server uses tx.closed() to detect client disconnect without polling, so tasks don't leak when a client calls get_settings() and drops the stream after the first message.
Suggested CHANGELOG Updates
CHANGED
ADDED
Checklist
just quick-checkbefore committingcrates/cdk-ffi)