xpay: attempt amount could have different meanings
What the developer wrote
Authored by Lagrang3
xpay: attempt amount could have different meanings
Change the name of attempt->deliver to attempt->amount,
do not assume that amount is the "desired delivery", since it might be
the "desired send" depending on the payment context.
Use helpers to get the fees and deliver amount from an attempt.
Changelog-None
Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
What changed, and why it matters
This is a code-cleanup change in Core Lightning's experimental xpay plugin. The developer renamed a field and added helper functions so the code no longer confuses 'amount to deliver to the recipient' with 'amount we send into the network including fees'. The commit itself does not claim to fix a security bug, but it removes several places where the wrong meaning of 'amount' was being used, which could previously have led to incorrect fee calculations or payment accounting.
Treat as a hardening/refactoring patch with latent bug-fix potential. Review whether the pre-patch behavior could cause over-payment, under-delivery, or incorrect retry decisions in xpay. No immediate emergency action is indicated, but operators using xpay should update to a version containing this commit and monitor for related payment-failure or amount-mismatch issues.
Security signals we found
Semantic confusion between send amount and deliver amount in payment logic
Fee calculation previously subtracted deliver amount from first-hop amount, which is correct only if deliver amount equals final-hop amount
total_being_sent renamed to total_being_delivered and now uses final-hop amount_out, fixing aggregation of in-flight delivered value
Blinded-path and final-hop payloads now explicitly use computed deliver amount rather than raw attempt amount
Evidence from the diff
In plugins/xpay/xpay.c, the struct attempt field ‘delivers’ is renamed to ‘amount’ and documented as context-dependent (deliver amount vs send amount). New helpers attempt_deliver(), attempt_fee(), total_being_delivered(), and total_fees_being_sent() are introduced. Call sites that previously used attempt->delivers where the delivered amount was semantically required are switched to attempt_deliver(attempt). Call sites that need the send/inject amount now use attempt->amount. This corrects accounting for delivered amounts, fees, and remaining routing needs.
Changed components
plugins/xpay/xpay.cstruct attemptpayment attempt accountingroute/fee computationonion payload constructioninjectpaymentonion failure handlingInspect captured patch +42 / −24
diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c
index 42c42c83..12105272 100644
--- a/plugins/xpay/xpay.c
+++ b/plugins/xpay/xpay.c
@@ -178,7 +178,10 @@ struct attempt {
u64 partid;
struct payment *payment;
- struct amount_msat delivers;
+
+ /* "amount" is either the intended deliver amount or the send amount,
+ * depending on the payment context. */
+ struct amount_msat amount;
struct timemono start_time;
/* Path we tried, so we can unreserve, and tell askrene the results */
@@ -367,17 +370,25 @@ send_payment_req(struct command *aux_cmd,
static struct amount_msat initial_sent(const struct attempt *attempt)
{
if (tal_count(attempt->hops) == 0)
- return attempt->delivers;
+ return attempt->amount;
return attempt->hops[0].amount_out;
}
static struct amount_msat inject_amount(const struct attempt *attempt)
{
if (tal_count(attempt->hops) == 0)
- return attempt->delivers;
+ return attempt->amount;
return attempt->hops[0].amount_in;
}
+static struct amount_msat attempt_deliver(const struct attempt *attempt)
+{
+ const size_t len = tal_count(attempt->hops);
+ if (len == 0)
+ return attempt->amount;
+ return attempt->hops[len - 1].amount_out;
+}
+
static u32 initial_cltv_delta(const struct attempt *attempt)
{
if (tal_count(attempt->hops) == 0)
@@ -581,7 +592,7 @@ static struct amount_msat total_delivered(const struct payment *payment)
list_for_each(&payment->past_attempts, attempt, list) {
if (!attempt->preimage)
continue;
- if (!amount_msat_accumulate(&sum, attempt->delivers))
+ if (!amount_msat_accumulate(&sum, attempt_deliver(attempt)))
abort();
}
return sum;
@@ -603,7 +614,7 @@ static void outgoing_notify_start(const struct attempt *attempt)
struct json_stream *js = plugin_notification_start(NULL, "pay_part_start");
json_add_attempt_fields(js, attempt);
json_add_amount_msat(js, "total_payment_msat", attempt->payment->amount);
- json_add_amount_msat(js, "attempt_msat", attempt->delivers);
+ json_add_amount_msat(js, "attempt_msat", attempt->amount);
json_array_start(js, "hops");
for (size_t i = 0; i < tal_count(attempt->hops); i++) {
const struct hop *hop = &attempt->hops[i];
@@ -1051,7 +1062,7 @@ static void update_knowledge_from_error(struct command *aux_cmd,
add_result_summary(attempt, LOG_DBG,
"Payment of %s reached destination,"
" but timed out before the rest arrived.",
- fmt_amount_msat(tmpctx, attempt->delivers));
+ fmt_amount_msat(tmpctx, attempt_deliver(attempt)));
return;
}
} else {
@@ -1191,7 +1202,7 @@ static struct command_result *injectpaymentonion_failed(struct command *aux_cmd,
struct attempt *attempt)
{
struct payment *payment = attempt->payment;
- struct amount_msat delivers = attempt->delivers;
+ struct amount_msat amount = attempt->amount;
payment->num_failures++;
@@ -1212,37 +1223,43 @@ static struct command_result *injectpaymentonion_failed(struct command *aux_cmd,
/* If we're not waiting for getroutes, kick one off */
if (amount_msat_is_zero(payment->amount_being_routed))
- return getroutes_for(aux_cmd, payment, delivers);
+ return getroutes_for(aux_cmd, payment, amount);
/* Wait for getroutes to finish */
return command_still_pending(aux_cmd);
}
-static struct amount_msat total_being_sent(const struct payment *payment)
+static struct amount_msat total_being_delivered(const struct payment *payment)
{
struct attempt *attempt;
struct amount_msat sum = AMOUNT_MSAT(0);
list_for_each(&payment->current_attempts, attempt, list) {
- if (!amount_msat_accumulate(&sum, attempt->delivers))
+ if (!amount_msat_accumulate(&sum, attempt_deliver(attempt)))
abort();
}
return sum;
}
+static struct amount_msat attempt_fee(const struct attempt *attempt)
+{
+ const size_t len = tal_count(attempt->hops);
+ if (len == 0)
+ return AMOUNT_MSAT(0);
+ struct amount_msat fee;
+ if (!amount_msat_sub(&fee, attempt->hops[0].amount_in,
+ attempt->hops[len - 1].amount_out))
+ abort();
+ return fee;
+}
+
static struct amount_msat total_fees_being_sent(const struct payment *payment)
{
struct attempt *attempt;
struct amount_msat sum = AMOUNT_MSAT(0);
list_for_each(&payment->current_attempts, attempt, list) {
- struct amount_msat fee;
- if (tal_count(attempt->hops) == 0)
- continue;
- if (!amount_msat_sub(&fee,
- attempt->hops[0].amount_in,
- attempt->delivers))
- abort();
+ struct amount_msat fee = attempt_fee(attempt);
if (!amount_msat_accumulate(&sum, fee))
abort();
}
@@ -1285,6 +1302,7 @@ static void append_blinded_payloads(struct sphinx_path *sp,
{
const struct blinded_path *path = attempt->payment->paths[path_num];
u32 final_cltv = effective_bheight;
+ struct amount_msat deliver = attempt_deliver(attempt);
for (size_t i = 0; i < tal_count(path->path); i++) {
bool first = (i == 0);
@@ -1304,7 +1322,7 @@ static void append_blinded_payloads(struct sphinx_path *sp,
* - MUST NOT include any other tlv field.
*/
payload = onion_blinded_hop(NULL,
- final ? &attempt->delivers : NULL,
+ final ? &deliver : NULL,
final ? &attempt->payment->full_amount : NULL,
final ? &final_cltv : NULL,
path->path[i]->encrypted_recipient_data,
@@ -1380,7 +1398,7 @@ static const u8 *create_onion(const tal_t *ctx,
* that's done in append_blinded_payloads. */
if (!blinded_path) {
u8 *final = onion_final_hop(NULL,
- attempt->delivers,
+ attempt_deliver(attempt),
attempt->payment->final_cltv + effective_bheight,
attempt->payment->full_amount,
attempt->payment->payment_secret,
@@ -1437,7 +1455,7 @@ static struct command_result *do_inject(struct command *aux_cmd,
json_add_string(req->js, "invstring", attempt->payment->invstring);
else
json_add_pubkey(req->js, "destination", &attempt->payment->destination);
- json_add_amount_msat(req->js, "destination_msat", attempt->delivers);
+ json_add_amount_msat(req->js, "destination_msat", attempt_deliver(attempt));
if (attempt->payment->localinvreqid)
json_add_sha256(req->js, "localinvreqid", attempt->payment->localinvreqid);
if (attempt->payment->label)
@@ -1471,13 +1489,13 @@ static struct command_result *reserve_done_err(struct command *aux_cmd,
/* Does not set shared_secrets */
static struct attempt *new_attempt(struct payment *payment,
- struct amount_msat delivers,
+ struct amount_msat amount,
const struct hop *hops TAKES)
{
struct attempt *attempt = tal(payment, struct attempt);
attempt->payment = payment;
- attempt->delivers = delivers;
+ attempt->amount = amount;
attempt->preimage = NULL;
attempt->partid = ++payment->total_num_attempts;
attempt->hops = tal_dup_talarr(attempt, struct hop, hops);
@@ -1573,7 +1591,7 @@ static struct command_result *getroutes_done(struct command *aux_cmd,
/* Do we have more that needs routing? If so, re-ask */
if (!amount_msat_sub(&needs_routing,
payment->amount,
- total_being_sent(payment)))
+ total_being_delivered(payment)))
abort();
was_routing = payment->amount_being_routed;
@@ -1726,7 +1744,7 @@ static struct command_result *waitblockheight_done(struct command *aux_cmd,
if (!amount_msat_sub(&needs_routing,
payment->amount,
- total_being_sent(payment)))
+ total_being_delivered(payment)))
abort();
return getroutes_for(aux_cmd, payment, needs_routing);
}
Why this scored 32/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.