pay optimizations round 1
#4404
Merged
Conversation
This file contains 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
|
It seems the test I added is flaky, I'll have a look before undrafting it again. |
cdecker
force-pushed
the
no55
branch
2 times, most recently
from
3822c35 to
ae8b311
Compare
rustyrussell
reviewed
Mar 3, 2021
plugins/pay.c
Outdated
Comment on lines
2007
to
2009
| is_b12 = strlen(b11str) > 3 && | ||
| (strstarts(b11str, "lni") || strstarts(b11str, "lno") || | ||
| strstarts(b11str, "lnr")); |
There was a problem hiding this comment.
Hmm, I think it's clearer to use two fail ptrs (esp since this test doesn't work correctly for upper case, and I worry about it breaking if we allow lightning:// prefixes etc)?
Then prefer to the bolt11 failstr if both fail. Later we can apply some kind of heuristic like this if necessary (maybe create a helper like looks_like_bolt12_invoice())?
There was a problem hiding this comment.
Untested:
diff --git a/common/bolt12.c b/common/bolt12.c
index 9cff0ab88..174c66430 100644
--- a/common/bolt12.c
+++ b/common/bolt12.c
@@ -289,6 +289,11 @@ struct tlv_invoice *invoice_decode_nosig(const tal_t *ctx,
return invoice;
}
+bool smells_like_bolt12_invoice(const char *str)
+{
+ return strstarts(str, "lni1") || strstarts(str, "LNI1");
+}
+
static void add_days(struct tm *tm, u32 number)
{
tm->tm_mday += number;
diff --git a/common/bolt12.h b/common/bolt12.h
index 797c5b64b..e9edd9da2 100644
--- a/common/bolt12.h
+++ b/common/bolt12.h
@@ -99,6 +99,9 @@ struct tlv_invoice *invoice_decode_nosig(const tal_t *ctx,
const struct chainparams *must_be_chain,
char **fail);
+/* For better error msgs when we get a bolt11 or bolt12. */
+bool smells_like_bolt12_invoice(const char *str);
+
/* Check a bolt12-style signature. */
bool bolt12_check_signature(const struct tlv_field *fields,
const char *messagename,
As pointed out by @cfromknecht [1] there was no formal standardization of the featurebit, and lnd would try a keysend whenever TLV was supported by the recipient. This mimics that behavior by checking only that TLV is enabled. Changelog-Fixes: keysend: We now attempt to send with keysend even if the node hasn't explicitly opted in by setting a featurebit. [1] ElementsProject#4299 (comment)
Changelog-Fixed: pay: `pay` was reporting in-flight parts as failed
The main responsibility of this new function is to mark a payment process as terminated and set a reasonable error message, that will be displayed to the caller. We also skip the remaining modifiers since they might end up clobbering the message.
cdecker
force-pushed
the
no55
branch
3 times, most recently
from
9f6aef4 to
b8a7d30
Compare
We would happily spin on attempts that are doomed to fail because we don't know the entrypoint. Next up: remove routehints whose entrypoints are known but unreachable.
Changelog-Added: pay: `pay` will now remove routehints that are unusable due to the entrypoint being unknown or unreachable.
We were not aborting if we had routehints, even though all routehints may have been filtered out. Changelog-Fixed: pay: `pay` will now abort early if the destination is not reachable directly nor via routehints.
We were automatically falling back to bolt12 decoding, clobbering the fail message. Ultimately resulting in confusing error messages (expected prefix lni but got lnbtrc). Now we first determine which decoding we're trying to do, and then only decode accordingly. Changelog-Fixed: pay: Report the correct decoding error if bolt11 parsing fails.
The functions just look at the hrp, so they're not guaranteed to guess right, but for this case it's sufficient.
|
After a quick detour decoding both and then deciding on which failure was set, I noticed that we'd be masquerading b12 failures with a generic b11 "wrong prefix" error. So I went and added the |
|
Ack 858d473 |
This was referenced Nov 27, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This started out as a quick fix for #4299, but while debugging I
stumbled over a couple of easy optimizations:
destination, instead requiring only TLV. This is because apparently
keysend is not formally proposed to the spec, and the featurebit is
not reserved. We now will report a more meaningful error message if
the recipient returns an
INVALID_ONION_PAYLOADerror.there is no point in trying them if we can't even get to the
entrypoint. This considers two different scenarios: entrypoint is
unknown and entrypoint is unreachable.
payment_abortto set a top-level error message in thepayresult, which should make it easier for modifiers to be helpful and
give a human-readable error message.
I have some more improvements planned, but this seemed like a good
initial candidate set.