Destination-aware max-sendable#41
Merged
Merged
Conversation
Sets up the destination-aware max-sendable work from mdkd PR #22. The new ldk-node rev exposes Node::find_route so the estimator can walk a real route and subtract its fees instead of falling back to a static buffer.
The destination-aware path coming next needs a knob for "how much above the cheapest route's fees should we reserve so retries can take a costlier path". Park it on an internal config struct now so the napi layer and the estimator share defaults. sum_outbound_balance and subtract_fee_buffer are split out of compute_estimate ahead of their second caller. The route-based strategy in the next commit reuses sum_outbound_balance to size the find_route amount and keeps subtract_fee_buffer as the no-destination arm of the strategy match. Splitting now keeps that diff to pure additions.
Reshape compute_estimate to accept Option<&PaymentInstructions>
and a find_route closure so the next commit can expose a
destination param on getMaxSendable. JS behaviour is unchanged:
the napi accessor still passes None, hitting the same buffer
path as before.
The destination dispatch lives in compute_estimate via a small
EstimationStrategy enum, with pick_strategy peeling apart the
ConfigurableAmount methods iterator (the 0.6 fork's
PossiblyResolvedPaymentMethod). FixedAmount destinations return
Err(FixedAmount { amount_msat }) so the caller does not have to
re-extract the amount; on-chain-only returns NoLightningMethod;
find_route bubbles up as RoutingFailure(String).
estimate_from_route prices a route at
route.total_fees * route_retry_fee_multiplier_bps / 10_000.
total_fees is computed from a find_route call sized to the full
balance, so the reported budget is slightly conservative
(actual fees on the smaller sent amount will be lower). The
multiplier reserves headroom for LDK to retry along a costlier
path if the chosen route fails at send time.
BOLT12 offers, LNURL-pay, and HRN destinations fall back to the
buffer until the upstream invoice/HRN resolution work lands.
BOLT11 round-trips through bech32 because bitcoin-payment-
instructions pulls upstream rust-lightning while ldk-node pulls
the moneydevkit fork: wire-compatible, distinct Rust types. A
re-parse failure falls back to Buffer rather than panicking.
Add an optional `destination` arg to `MdkNode#getMaxSendable` so JS
callers can ask "how much can I send to this specific payee right
now" and get a fee budget grounded in a real `find_route` call
rather than the 1% buffer fallback.
When supplied, the destination is parsed through the same
HTTPHrnResolver + current-thread runtime path used by `pay`, then
threaded into compute_estimate. The four MaxSendableError variants
map onto the napi surface as:
- NoUsableChannel -> Ok(None)
Preserves the existing null contract so UIs do not have to
re-handle the "node still booting" case.
- FixedAmount -> InvalidArg
Carries the dictated amount in the message so callers can
surface it without re-parsing.
- NoLightningMethod -> InvalidArg
- RoutingFailure -> GenericFailure
- parse failure -> InvalidArg
The regenerated index.d.ts/index.js carry the new optional arg,
the routeRetryFeeMultiplierBps field added two commits back, and
a pile of unrelated whitespace churn from napi-rs's formatter
disagreeing with whatever generated the previously committed
files. Folding the formatter noise into this commit keeps the
diff against the next regen empty.
![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: bc19fbe3a7
ℹ️ 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".
Comment on lines
+156
to
+157
| RouteParameters::from_payment_params_and_value(payment_params, balance_msat); | ||
| let route = find_route(route_params).map_err(MaxSendableError::RoutingFailure)?; |
There was a problem hiding this comment.
compute_estimate asks find_route for balance_msat as the payment value and only subtracts routing fees afterward, so the route search is performed for an amount that already consumes the entire outbound liquidity. When any non-zero routing fee is required (the common case), there are channels where paying balance_msat is impossible but paying balance_msat - fees is valid; this path currently returns RoutingFailure instead of a max-sendable estimate for those destinations.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
An astute observation but I am okay with a single iteration here
| @@ -58,15 +121,87 @@ impl From<&ChannelDetails> for ChannelSnapshot { | |||
| /// where the buffer eats everything yields `Ok(amount_msat: 0)` — the | |||
| /// UI distinguishes "0 sats sendable" from "no channel yet". | |||
| pub(crate) fn compute_estimate( | |||
Contributor
There was a problem hiding this comment.
I believe this needs to be deleted as now compute_estimate() exists?
Contributor
Author
There was a problem hiding this comment.
Yeah this got messed up doesn't even compile. Need to get the pre-commit checks working on my machine for this repo
Comment on lines
+342
to
+345
| let res = compute_estimate(&[], &lsp, BPS, FLOOR); | ||
| let res = compute_estimate(&[], &lsp, &MaxSendableConfig::default()); | ||
| let res = compute_estimate(&[], &lsp(), &MaxSendableConfig::default()); | ||
| let res = buffer_estimate(&[], &lsp(), &MaxSendableConfig::default()); |
Contributor
There was a problem hiding this comment.
res overwritten multiple times?
| /// its own amount (fixed-amount BOLT11/BOLT12), or carries no Lightning | ||
| /// method. Throws `GenericFailure` when routing fails outright. | ||
| /// | ||
| /// Read-only; safe to call whether or not the node has been started. |
Contributor
There was a problem hiding this comment.
I believe this is not true anymore? you need a running node?
src/max_sendable.rs was left in a state that never compiled due to a broken merge. Fixed. The getMaxSendable doc claimed the call is safe on a stopped node. That is only true with no destination. When a destination is supplied the call hits Node::find_route, which returns NotRunning on a stopped node, so the blanket sentence is wrong. Removed. One unrelated clippy lint (len() > 0 in the offer-paths test) was masked while max_sendable.rs would not compile. Fixed so the crate builds clean under -D warnings. The codex bot's suggestion to route against `balance - fees` instead of the full outbound balance is left alone; the single-iteration approximation was explicitly accepted in the PR discussion.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Ports mdkd#22 into
lightning-js.
MdkNode#getMaxSendablenow accepts an optionaldestinationand, when supplied, computes the fee budget from a realNode::find_routeagainst that payee instead of the static 1% buffer.JS surface
nullstill means "no usable LSP channel" (transient case).and routing failures throw napi errors instead of returning
null.routeRetryFeeMultiplierBpsonMaxSendableConfig(default 11_000 = 1.1x) scales the reported fee budget so retries
along a costlier path still fit.
Scope
invoice/HRN resolution moves into the estimator.
InvalidArg(the payee dictated theamount, nothing to estimate).