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

Pay plugin #2215

Merged
merged 26 commits into from Jan 17, 2019

Conversation

Projects
None yet
2 participants
@rustyrussell
Copy link
Contributor

rustyrussell commented Jan 3, 2019

Rebased, split into pr #2234 and this one which does the actual plugin work.

It has shadow routes, route hint support, and expensive-channel-avoiding enhancements, but lacks handling bad onions.

@rustyrussell rustyrussell requested review from cdecker and ZmnSCPxj as code owners Jan 3, 2019

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Jan 8, 2019

Needs a rebase on top of master now that #2194 is merged.

@rustyrussell rustyrussell force-pushed the rustyrussell:pay-plugin branch 3 times, most recently from 3dd73e9 to a68f54b Jan 8, 2019

@rustyrussell rustyrussell referenced this pull request Jan 15, 2019

Merged

Pay final cleanups #2269

@rustyrussell rustyrussell force-pushed the rustyrussell:pay-plugin branch from a68f54b to 4a7a480 Jan 15, 2019

@rustyrussell rustyrussell changed the title WIP: Pay plugin Pay plugin Jan 15, 2019

rustyrussell added some commits Jan 15, 2019

plugin/libplugin: API for C plugins.
Doesn't do logging yet.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins: minimal 'pay' plugin.
I wrote this sync first, then rewrote async, then developed libplugin.
But committing all that just wastes reviewer time, so I present it as
if it was always asnc and using the library helper.

Currently the command it registers is 'pay2', but when it's complete
we'll remove the internal 'pay' and rename it. This does a single
'getroute/sendpay' call.  No retries, no options.

Shockingly, this by itself is almost sufficient to pass our current test
suite with `pay`->`pay2`.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
contrib/pylightning: temporarily convert to use plugin/pay for tests.
That this simply pay plugin passes the tests is a poor reflection on our
test cases, really.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: retry on failure in a loop.
We use the 'exclude' option to getroute for successive attempts.  This
is more robust than having gossipd disable for some limited time.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
pytest: test that pay command retries.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: exclude local channels with insufficient current capacity.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: implement maxfeepercent, maxdelay and exemptfee.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: get our own id during init phase.
We'll want this for routeboost filtering.
plugins/pay: get max-locktime-blocks at init for setting default.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: use final_cltc from bolt11 invoice.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
pytest: test that we correctly use routeboost information from bolt11…
… invoice.

This is implemented in the next patch.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: implement routeboost hints, naive version.
We sanitize the routes: firstly, we assume appending so eliminate the
first hop if the route points straight to us.  Secondly, eliminate empty
hints.  Thirdly, trim overlong hints.

Then we just use the first route hint.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
pytest: test more-than-one-hop route hints.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: retry when routehint fails.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: add shadow CLTV calculation.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: add paystatus command to get gory details of payments.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: store history of payment attempts (non-persisent).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
pay: remove inbuilt command in favor of plugin.
This doesn't actually remove some of the now-unnecessary infrastructure
though.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
getroute: add direction to route returned.
We also ignore it in sendpay.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: eliminate worst channel if we go over fee / delay thresh…
…old.

But keep the error in this case, so we don't always report "no route".

Reported-by: @niftynei
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
pytest: fix spurious valgrind output.
Had a couple of tests randomly fail because a valgrind error file was
not empty.  It contained:

   lightning_channeld: Writing out status 65520: Broken pipe

This shouldn't happen, but the simplest workaround is not to print
that (useless) error.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: paystatus should explicitly describe why it is making ea…
…ch attempt.

So add a new 'strategy' field.  This makes it clearer what is going
on, currently one of:

* "Initial attempt"
* "Excluded channel <scid>"
* "Removed route hint"
* "Excluded expensive channel <scid>"
* "Excluded delaying channel <scid>"

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@cdecker cdecker force-pushed the rustyrussell:pay-plugin branch from 4a7a480 to 5b29b27 Jan 15, 2019

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Jan 15, 2019

Rebased on top of master, reviewing now.

@cdecker
Copy link
Member

cdecker left a comment

Excellent PR, I just need some help understanding some of the choices to put my mind at ease 😉

const char *errmsg,
const char *data)
{
printf_json(STDOUT_FILENO,

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

Don't we have some really nice tools for creating JSON messages using the struct json_stream? Why reinvent the wheel here?

This comment has been minimized.

@rustyrussell

rustyrussell Jan 16, 2019

Author Contributor

Because extracting that is a TODO :) This patch series was already really big...

*contents = json_get_member(membuf_elems(&rpc->mb), toks,
"result");
if (!*contents)
plugin_err("JSON reply with no 'result'? '%.*s'",

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

Should be "neither error nor result".

const char *ret;

printf_json(rpc->fd,
"{ 'method': '%s', 'id': 0, 'params': { %s } }",

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

Hardcoded ID seems strange, even though it should be safe since this is sync.

This comment has been minimized.

@rustyrussell

rustyrussell Jan 16, 2019

Author Contributor

Yes, this is kind of special.

@@ -329,7 +329,7 @@ def pay(self, bolt11, msatoshi=None, description=None, riskfactor=None):
"description": description,
"riskfactor": riskfactor
}
return self.call("pay", payload)

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

This commit really needs to be removed in the final series.

This comment has been minimized.

@rustyrussell

rustyrussell Jan 16, 2019

Author Contributor

It's removed at the end?

struct timeabs stoptime;

/* Channels which have failed us. */
const char **excludes;

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

Might want to keep global exclusions at some point as well, otherwise multiple successive payments will repeat the errors of their predecessors. Insanity is doing the exact same thing and expecting different outcomes 😉

This comment has been minimized.

@rustyrussell

rustyrussell Jan 16, 2019

Author Contributor

If we assume temporary failures (perm ones are handed to gossipd still) are caused by insufficient capacity, we can make some assumptions, but it's hard: how long do we assume they're valid. Our current plugin uses 20 seconds!

continue;

v = pseudorand(UINT64_MAX);
if (!best || v > sample) {

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

This is comparing the value v with the value of sample which is uninitialized on the first run.

This comment has been minimized.

@rustyrussell

rustyrussell Jan 16, 2019

Author Contributor

No, best is NULL first time around, and '||' does short-circuit eval...

bitcoind.generate_block(2)
assert not l2.daemon.is_in_log('hit deadline')
bitcoind.generate_block(1)
# FIXME: shadow route can add extra blocks! 50% chance of adding 6...

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

This seems like it's 10x the 6 blocks you mention in the comment, why is that?

This comment has been minimized.

@rustyrussell

rustyrussell Jan 16, 2019

Author Contributor

50% chance of adding 6, means 0.5^10 of adding 60. This gets fixed later when we have paystatus which tells us exactly what it did...

if (!best) {
tal_append_fmt(&pc->ps->shadow,
"No suitable channels found to %s. ",
pc->shadow);

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

Isn't pc->shadow uninitialized here, i.e., set 3 lines below in the happy case?

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

Unless it is being set to the empty string on line 620, but then the message is not all that sensible.

This comment has been minimized.

@rustyrussell

rustyrussell Jan 16, 2019

Author Contributor

Confusion here:

  1. pc->shadow is the string of the current shadow tip. It starts as pc->dest.
  2. pc->ps->shadow is the chatty string about what we're doing. It starts as ""

I'll rename one of them...

This comment has been minimized.

@rustyrussell

rustyrussell Jan 16, 2019

Author Contributor

pc->shadow -> pc->shadow_dest.


/* Try excluding most fee-charging channel (unless it's in
* routeboost). */
charger = find_worst_channel(buf, t, "msatoshi", pc->msatoshi);

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

This is applied to the route that failed our constraints, right? The msatoshi values in the route however are absolute, not deltas, so if I'm not mistaken this will always return the first channel since that's the one that has the maximum cumulative value, or am I missing something here?

This comment has been minimized.

@rustyrussell

rustyrussell Jan 16, 2019

Author Contributor

I think the internal logic of find_worst_channel gets this right:

	if (worst == NULL || val - prev > worstval) {
		worst = route;
		worstval = val - prev;
	}

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

Oh, I missed the comparison with prev that's ok then.

= tal_fmt(pc, "Route wanted delay of %u blocks",
delay);

delayer = find_worst_channel(buf, t, "delay", pc->final_cltv);

This comment has been minimized.

@cdecker

cdecker Jan 16, 2019

Member

And I think the same is true here, we are eliminating not the most expensive hop, but the last one since delays accumulate.

See comment above.

rustyrussell added some commits Jan 16, 2019

libplugin: mention error field in error message.
Reported-by: @cdecker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: clarify field names
Reported-by: @cdecker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: simplify listpeers_done code a little.
Avoid the unnecessary extra var, and don't use "capacity" since
that usually refers to static capacity.

Reported-by: @cdecker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins/pay: add comment on why we don't use an empty string
plugins/pay.c:879:7: error: zero-length gnu_printf format string [-Werror=format-zero-length]

Reported-by: @cdecker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell referenced this pull request Jan 17, 2019

Merged

Json tok speedup #2272

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Jan 17, 2019

ACK f0ac0da

@cdecker cdecker merged commit 7c0863f into ElementsProject:master Jan 17, 2019

2 checks passed

ackbot PR ack'd by cdecker
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment