message-box-client: payment-bearing paths acknowledge (delete) the relay message before the payment is durably stored

[mitch-burcham]

Summary

In @bsv/message-box-client, several payment-bearing message paths remove the message
from the MessageBox server before the embedded payment is durably internalized into
the wallet. Because the BRC-42 derivation data exists only inside that message, a crash
or a failed internalize between the two steps leaves the output on-chain and
unspendable — there is no re-fetch and no chain rescan that recovers it.

PeerPayClient.acceptPayment already does this in the safe order
(internalize → acknowledge), so within the same package the paths disagree about
whether the server is a durability backstop.

Verified by reading the published build of v2.4.2
(dist/src/MessageBoxClient.js, dist/src/PeerPayClient.js, dist/src/types.d.ts),
cross-checked against v2.4.1. Two independent reviewers reached the same conclusion on
the primary finding.

---

1. MessageBoxClient.acknowledgeNotification — acknowledge before internalize

dist/src/MessageBoxClient.js, acknowledgeNotification:

async acknowledgeNotification(message) {
    await this.acknowledgeMessage({ messageIds: [message.messageId] });   // ← deletes first
    ...
    const recipientOutputs = paymentData.outputs.filter(o => o.protocol === "wallet payment");
    if (recipientOutputs.length < 1) { return false; }                    // ← already deleted
    if ((await this.walletClient.internalizeAction({ ... })).accepted) {  // ← stores second
        return true;
    } else { return false; }                                             // ← already deleted
} catch (paymentError) {
    error("[MB CLIENT ERROR] Failed to internalize recipient payment:", paymentError);
    return false;                                                        // ← already deleted
}

The doc comment states the ordering as intended behaviour, so this reads as deliberate
rather than accidental:

* 1. Calls `acknowledgeMessage()` to remove the message from the server's queue.
* 2. Checks the message body for embedded payment data.
* 3. If a recipient payment exists, attempts to internalize it into the wallet.

Because the delete happens first, every subsequent failure mode becomes a silent
loss rather than a retryable error:

Path	Outcome
Crash / OOM between the two calls	payment lost
internalizeAction throws	caught, logged, return false — payment lost
internalizeAction returns accepted: false	return false — payment lost
outputs.filter(...) matches nothing	return false — payment lost

In all four the caller receives false, which is indistinguishable from "this message
carried no payment". A crash surfaces nothing at all.

Minor, same function: this internalizeAction call omits this.originator, unlike
internalizeRecipientPayment and the PeerPay settlement path. On an originator-sensitive
wallet that is an additional way to fail after the message is already gone.

Suggested fix: internalize first and acknowledge only on success, matching
acceptPayment. That converts silent loss into at-least-once delivery, which the
internalize path already tolerates — a replayed internalize collides on outpoint
uniqueness rather than double-crediting.

---

2. listMessages / listMessagesLite strip the payment envelope while suppressing internalize failure

internalizeRecipientPayment logs and swallows both the throw and the not-accepted case:

if ((await this.walletClient.internalizeAction(/* ... */)).accepted)
    log("[MB CLIENT] Successfully internalized recipient payment");
else
    warn("[MB CLIENT] Recipient payment internalization was not accepted");
} catch (paymentError) {
    error("[MB CLIENT ERROR] Failed to internalize recipient payment:", paymentError);
}

and the returned body is then replaced with only the inner content, discarding payment:

p.message.body = p.messageContent ?? p.parsedBody;

So a caller doing the ordinary poll-then-acknowledge lifecycle gets a message object
from which the payment metadata has been removed, with no indication that
internalization failed, and then acknowledges it. listMessages itself does not
acknowledge, so this is not a loss on its own — the loss lands on the subsequent
acknowledge. It becomes deterministic with acceptPayments: false, where
internalization is skipped but the envelope is stripped anyway; listMessagesLite
always strips.

Suggested fix: surface the internalization outcome to the caller, and preserve the
payment envelope on the returned object when it was not successfully internalized.

---

3. PeerPayClient.rejectPayment ignores acceptPayment's failure and acknowledges before the refund

acceptPayment resolves a string on failure rather than throwing:

} catch (error$2) {
    error(`[PP CLIENT] Error accepting payment: ${String(error$2)}`);
    return "Unable to receive payment!";
}

rejectPayment discards that return value and proceeds to refund:

await this.acceptPayment(payment);
await this.sendPayment({ recipient: payment.sender, amount: payment.token.amount - 1e3 });

Two consequences:

• Acceptance failed: the received output was never internalized, but the refund is
  sent anyway — from unrelated wallet funds — and the original message is then
  acknowledged. The recipient has paid the sender out of pocket and the original output
  is unrecorded.
• Acceptance succeeded: acceptPayment acknowledges immediately, so a crash or a
  sendPayment throw before the refund leaves the sender un-refunded with the queue
  record needed to resume already deleted.

Rejection is a multi-step monetary operation that acknowledges in the middle, so it is
not resumable.

---

4. acceptPayment treats a non-accepted wallet result as success

The success branch is taken without checking accepted, so a wallet returning
{ accepted: false } rather than throwing produces a "Payment internalized successfully"
log, a deleted relay message, and a success-shaped return value.

---

5. basket insertion outputs are filtered out despite being contract-valid

Both internalizeRecipientPayment and acknowledgeNotification filter with:

paymentData.outputs.filter((output) => output.protocol === "wallet payment")

but the package's own Payment type in dist/src/types.d.ts declares both protocols
valid, with dedicated fields for the second:

protocol: 'wallet payment' | 'basket insertion';
insertionRemittance?: {
  basket: BasketStringUnder300Bytes;
  customInstructions?: string;
  tags?: OutputTagStringUnder300Bytes[];
};

A server following the declared interface can therefore send an output that the client
silently discards. With a mixed output set the call internalizes the wallet-payment
subset, returns true, and abandons the rest. Note also that undefined === 'wallet payment' is false, so an output with no protocol field is dropped as well.

---

Impact

Findings 1 and 3 are the ones with unbounded value exposure. The amounts in the
notification path are typically delivery fees, so the per-message value is small, but
the loss is silent, permanent, and unbounded in aggregate.

Happy to open a PR for finding 1 if that is useful — it is a small reordering plus error
propagation, and the ordering already exists in acceptPayment to copy.

[ty-everett]

Initial ordering and wallet-acceptance remediation is merged in #534 (SDK 2.7.0 and Message Box Client 2.5.1 source candidates). Notification payments are internalized with the configured originator and require accepted === true before acknowledgment. PeerPay accepts only affirmative wallet results; refundable payments now internalize, send the refund, then acknowledge. Failed internalization prevents a refund, and a failed refund leaves the message queued. Regression tests cover failure ordering and missing/false wallet acceptance.

This issue remains open. The current Boolean result and message wire formats remain unchanged. Envelope-versus-outcome handling in listMessages/listMessagesLite, mixed basket-insertion handling, durable refund reconciliation and exactly-once semantics remain future work. In particular, an uncertain refund send must be reconciled before retry; this change does not claim exactly-once delivery or refunds.

Merge: 813d8b9c09ba0e4df261d398da2b16bf9f9aa264 into main by admin merge. CI run 35019911817 passed, with all 53 active PR checks successful, zero new Sonar findings, no open CodeQL alerts and no unresolved review threads.