Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to send_probe to eclair due to lack of payment secret #2525

Closed
TonyGiorgio opened this issue Aug 25, 2023 · 5 comments · Fixed by #2534
Closed

Unable to send_probe to eclair due to lack of payment secret #2525

TonyGiorgio opened this issue Aug 25, 2023 · 5 comments · Fixed by #2534
Assignees

Comments

@TonyGiorgio
Copy link
Contributor

Because of this issue, eclair requires payment secret when sending probes: ACINQ/eclair#1810 (comment)

Right now I'm getting:

INFO  [lightning::ln::onion_utils:579] Onion Error[from 03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f: invalid_onion_payload(0x4016)] Node indicated that the decrypted onion per-hop payload was not understood by it or is incomplete

And I think it's because when sending probes, it uses sponaneous_empty which does not send payment secret.

match self.pay_route_internal(&route, payment_hash, RecipientOnionFields::spontaneous_empty(),
None, payment_id, None, onion_session_privs, node_signer, best_block_height, &send_payment_along_path
) {
Ok(()) => Ok((payment_hash, payment_id)),
Err(e) => {
self.remove_outbound_if_all_failed(payment_id, &e);
Err(e)
}
}
}

pub fn spontaneous_empty() -> Self {
Self { payment_secret: None, payment_metadata: None, custom_tlvs: Vec::new() }
}

Would always sending payment secret be the right approach? Don't all nodes support it at this point? Or would allowing send_probe to pass through RecipientOnionFields be a work around and I can hardcode their node pubkey for it.

@TheBlueMatt
Copy link
Collaborator

Hmm, yea, interesting. The error clearly indicates that we got to the recipient, which means for probing purposes we should treat it as a success, but for payment purposes this error implies we should never try to route through this node again so we should blame the inbound channel (which is what we do). The simple fix is, indeed, just add a random payment secret to get them to respond with unknown_payment again, though we could also move to a different NetworkUpdate variant that indicates we got there, but the node did something dumb.

I'm also curious why the node isn't accepting this as a keysend - does eclair not support receiving keysends/spontaneous payments?

@TheBlueMatt TheBlueMatt added this to the 0.0.117 milestone Aug 25, 2023
@TonyGiorgio
Copy link
Contributor Author

I'm also curious why the node isn't accepting this as a keysend - does eclair not support receiving keysends/spontaneous payments?

I think they do, this was opened last week: ACINQ/eclair#2724

@TheBlueMatt
Copy link
Collaborator

CC @t-bast any chance you can help illuminate here - tony is getting the above error (PERM|invalid_onion_payload) when trying to probe to an eclair node, but it should presumably be treating it as a keysend and simply failing with unknown_payment.

@t-bast
Copy link

t-bast commented Aug 28, 2023

Hey! Sure, that one is simple enough.

By default, keysend is not activated on eclair nodes (and not activated on ours). That means we'll reject the payment here with an incorrect_or_unknown_payment_details.

But in your case, the keysend preimage field doesn't even seem to be filled, so we don't identify that as a keysend payment at all. Since we have activated payment_secret as mandatory, and we received a payment that is not using keysend and doesn't contain a payment_secret field, we reject it as being an invalid onion.

@tnull tnull self-assigned this Aug 28, 2023
@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Aug 28, 2023

Oh duh lol, y'all identify keysend by the preimage, not the lack of payment secret. Okay, makes sense, we should probably just fill in a random payment secret. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants