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

Misc prep for pay plugin #2234

Merged
merged 22 commits into from Jan 15, 2019

Conversation

Projects
None yet
3 participants
@rustyrussell
Copy link
Contributor

rustyrussell commented Jan 8, 2019

Much of the groundwork for making pay a full plugin is independent work, so I've gathered these all as a group first, for easier reviewing.

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

@rustyrussell rustyrussell force-pushed the rustyrussell:misc-prep-for-pay-plugin branch from 49c7e00 to 2a7c066 Jan 8, 2019

@rustyrussell rustyrussell referenced this pull request Jan 8, 2019

Merged

Pay plugin #2215

@rustyrussell rustyrussell force-pushed the rustyrussell:misc-prep-for-pay-plugin branch from 2a7c066 to 1976704 Jan 9, 2019

@niftynei
Copy link
Collaborator

niftynei left a comment

Left a comment on a few things that looked wonky, otherwise 👍 Commence pay plugin excite!

size_t n = tal_count(*(p)); \
tal_resize((p), n+1); \
(*(p))[n] = (s); \
} while(0)

This comment has been minimized.

@niftynei

niftynei Jan 9, 2019

Collaborator

what's the motivation for putting this in a do/while block at all?

This comment has been minimized.

@rustyrussell

rustyrussell Jan 10, 2019

Author Contributor

It's a standard C trick for when you want a multi-statement macro. This turns it into a single statement, otherwise you can get strange results with if (something) MACRO; not doing what you expect.

This comment has been minimized.

@cdecker

cdecker Jan 10, 2019

Member

Strange, wouldn't surrounding with {} have the same effect, or are statement blocks not allowed in C?

This comment has been minimized.

@rustyrussell

rustyrussell Jan 10, 2019

Author Contributor

Not quite, since the user will add a ';' turning it into two statements. Now, this normally doesn't matter, except for the dangling else rule which says it binds to the last 'if'. So, this suddenly breaks:

if (something) MACRO; else ...

This comment has been minimized.

@cdecker

cdecker Jan 11, 2019

Member

Learnt something new today, thanks :-)


if (!param(cmd, buffer, params,
p_req("id", param_pubkey, &destination),
p_req("msatoshi", param_u64, &msatoshi),
p_req("riskfactor", param_double, &riskfactor),
p_opt_def("cltv", param_number, &cltv, 9),
p_opt_def("fromid", param_pubkey, &source, ld->id),
p_opt("seed", param_tok, &seedtok),
p_opt_def("fuzzpercent", param_percent, &fuzz, 75.0),

This comment has been minimized.

@niftynei

niftynei Jan 9, 2019

Collaborator

lol. nice find.

This comment has been minimized.

@cdecker

cdecker Jan 10, 2019

Member

Whoa, that'd explain some user reports...


/* To choose between variations, we need to know how much we're
* sending (eliminates too-small channels, and also effects the fees
* we'll pay), how to trade off more locktime vs. more fees, and how
* much cltv we need a the final node to give exact values for each
* intermediate hop, as well as how much random fuzz to inject to
* avoid being too predictable. */
if (!fromwire_gossip_getroute_request(msg,
if (!fromwire_gossip_getroute_request(msg, msg,

This comment has been minimized.

@niftynei

niftynei Jan 9, 2019

Collaborator

extra msg here ?

This comment has been minimized.

@rustyrussell

rustyrussell Jan 10, 2019

Author Contributor

It now needs an allocation context to allocate the new fields from...

"""
Show route to {id} for {msatoshi}, using {riskfactor} and optional
{cltv} (default 9). If specified search from {fromid} otherwise use
this node as source. Randomize the route with up to {fuzzpercent}
(0.0 -> 100.0, default 5.0) using {seed} as an arbitrary-size string
seed.
seed. {exclude} is an optional array of scids[xDirection] to exclude.

This comment has been minimized.

@niftynei

niftynei Jan 9, 2019

Collaborator

nit: xDirection -> /Direction ?

This comment has been minimized.

@rustyrussell

rustyrussell Jan 10, 2019

Author Contributor

Ack, good catch. I changed it after @cdecker decided / was The Right Thing here.

&excluded_dir[i])) {
return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
"%.*s is not a valid"
" id+direction",

This comment has been minimized.

@niftynei

niftynei Jan 9, 2019

Collaborator

nit: add correct format example to error message. e.g. not a valid id+direction. Expected in format scid/dir i.e. 3928x19x0/1.

assert only_one(channels1)['destination'] == l2.info['id']
assert channels1 == channels2

channels2 = l2.rpc.listchannels()['channels']

This comment has been minimized.

@niftynei

niftynei Jan 9, 2019

Collaborator

channels2 is unused after this line.

This comment has been minimized.

@rustyrussell

rustyrussell Jan 11, 2019

Author Contributor

Ack... weird make check didn't spot it.

@@ -343,6 +344,8 @@ static struct command_result *json_getroute(struct command *cmd,
p_opt_def("fromid", param_pubkey, &source, ld->id),
p_opt_def("fuzzpercent", param_percent, &fuzz, 5.0),
p_opt("exclude", param_array, &excludetok),
p_opt_def("maxhops", param_number, &max_hops,

This comment has been minimized.

@niftynei

niftynei Jan 9, 2019

Collaborator

changelog entry?

This comment has been minimized.

@rustyrussell

rustyrussell Jan 10, 2019

Author Contributor

I chose against it since getroute is completely undocumented at the moment... which is also bad

This comment has been minimized.

@niftynei

niftynei Jan 11, 2019

Collaborator

ah ok!

@rustyrussell rustyrussell added this to the v0.6.4 milestone Jan 10, 2019

@@ -14,7 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- JSON API: use `\n\n` to terminate responses, for simplified parsing (pylightning now relies on this)
- JSON API: `fundchannel` now includes an `announce` option, when false it will keep channel private. Defaults to true.
- JSON API: `listpeers`'s `channels` now includes a `private` flag to indicate if channel is announced or not.
- JSON API: `invoice` route hints may now include private channels if you have no public ones.
- JSON API: `invoice` route hints may now include private channels if you have no public ones, unless new option `exposeprivatechannels` is false.

This comment has been minimized.

@cdecker

cdecker Jan 10, 2019

Member

Shall we call this privatehints? Still a mouthful, but maybe causes less confusion.

This comment has been minimized.

@rustyrussell

rustyrussell Jan 10, 2019

Author Contributor

But the hint isn't private. It's awkward, an exposeprivates has unwanted connotations :)

json_add_short_channel_id(response,
"short_channel_id",
channel->scid);
json_add_num(response, "channel_direction",

This comment has been minimized.

@cdecker

cdecker Jan 10, 2019

Member

We've always called this just direction and given the context it's redundant.

This comment has been minimized.

@rustyrussell

rustyrussell Jan 10, 2019

Author Contributor

I don't think we've exposed in via JSON before, but I can s/channel_/ everywere.

@@ -1871,17 +1871,20 @@ static struct io_plan *getroute_req(struct io_conn *conn, struct daemon *daemon,
u8 *out;
struct route_hop *hops;
double fuzz;
struct short_channel_id *excluded;
bool *excluded_dir;

This comment has been minimized.

@cdecker

cdecker Jan 10, 2019

Member

Not really a boolean, but I get why this is used.

This comment has been minimized.

@rustyrussell

rustyrussell Jan 10, 2019

Author Contributor

Yeah, used int elsewhere, should do so here.

struct node *n;
u64 *saved_capacity;

assert(tal_count(excluded) == tal_count(excluded_dir));

This comment has been minimized.

@cdecker

cdecker Jan 10, 2019

Member

We could amend short_channeld_id to include a direction, just put set it to -1 if unspecified? I find it awkward to pass two synchronized arrays around.

This comment has been minimized.

@rustyrussell

rustyrussell Jan 10, 2019

Author Contributor

I think that would pollute all the places which don't care. I'll try a new short_channel_id_dir type and see what it looks like?

@@ -292,6 +292,27 @@ static void json_getroute_reply(struct subd *gossip UNUSED, const u8 *reply, con
was_pending(command_success(cmd, response));
}

static bool json_to_short_channel_id_with_dir(const char *buffer,

This comment has been minimized.

@cdecker

cdecker Jan 10, 2019

Member

Things like this would also be much easier if we had a short_channel_id include an optional direction.

@@ -486,6 +488,11 @@ find_route(const tal_t *ctx, struct routing_state *rstate,
return NULL;
}

if (max_hops > ROUTING_MAX_HOPS) {
status_info("find_route: max_hops huge amount %zu", max_hops);

This comment has been minimized.

@cdecker

cdecker Jan 10, 2019

Member

Could probably use a hint as to what the maximum is 😉

This comment has been minimized.

@rustyrussell

rustyrussell Jan 10, 2019

Author Contributor

Ack.

@@ -781,7 +781,7 @@ def get_node(self, disconnect=None, options=None, may_fail=False,
'valgrind',
'-q',
'--trace-children=yes',
'--trace-children-skip=*plugins*,*python*,*bitcoin-cli*',
'--trace-children-skip=*python*,*bitcoin-cli*',

This comment has been minimized.

@cdecker

cdecker Jan 10, 2019

Member

I'm surprised this works, it used to always complain about things in the python interpreter since those plugins are started through a script and not with the interpreter directly. Happy to see that it works ^^

@rustyrussell

This comment has been minimized.

Copy link
Contributor Author

rustyrussell commented Jan 14, 2019

OK, three new commits added on top with changes, and rebased on master (trivial CHANGELOG conflicts).

@rustyrussell rustyrussell force-pushed the rustyrussell:misc-prep-for-pay-plugin branch 2 times, most recently from 577f0ff to 7bce532 Jan 14, 2019

rustyrussell added some commits Jan 15, 2019

utils: make tal_arr_expand safer.
Christian and I both unwittingly used it in form:

	*tal_arr_expand(&x) = tal(x, ...)

Since '=' isn't a sequence point, the compiler can (and does!) cache
the value of x, handing it to tal *after* tal_arr_expand() moves it
due to tal_resize().

The new version is somewhat less convenient to use, but doesn't have
this problem, since the assignment is always evaluated after the
resize.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
devtools/bolt11-cli: print min_final_cltv_expiry.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
lightningd: fix leak when next peer is unknown.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
json: move bitcoin/lightning specific helpers into common/json_helpers.
We don't need them in common/json, since lightning-cli doesn't need these,
but plugins want them.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
getroute: remove seed arg, document fromid, make default fuzzpercent …
…match docs.

seed isn't very useful at this level: I've left it in routing.c
because it might be useful for detailed testing.  Pretty sure it's unused,
so I simply removed it.

The fuzzpercent is documented to default at 5%, but actually was 75%.
Fix that too.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
routeboost: expose private channel in invoice iff we have no public o…
…nes.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
routeboost: don't use channels to dead-end nodes.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
invoice: option to expose/not-expose private channels.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
bolt11: fix encoding of routes of length > 1.
We don't do this yet, so it went unnoticed.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
invoice: add DEVELOPER-only optional dev-routes param.
This lets us test explicit routes which are more complex than the ones
we currently generate.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
test/run-bench-find_route: fix so it runs properly.
We didn't populate the channels properly so it always failed.

Additionally, somewhere along the line we kept using the single scid
so we only created one channel.

Also, the next patch will start comparing the pubkeys, so make valid
ones: use an array so we don't affect the benchmark too much.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
waitsendpay: indicate which channel direction the error was.
You can figure this yourself by knowing the route, but it's better to report
it directly here.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
listpeers: show channel direction for each outgoing channel.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell added some commits Jan 15, 2019

gossipd: allow an array of excluded channels for getroute_request.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
getroute: allow array of channels to exclude.
The pay plugin will use this, rather than the current "suppress for 90 second" hacks.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
getroute: allow caller to specify maximum hops.
This is required for routeboost.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
pytest: don't skip valgrinding plugins.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
json_getroute: don't return generic error.
We use the PAY error code here, but it's appropriate (otherwise the
pay command simply has to substitute it, which seems silly).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
listchannels: allow source arg to list channels by their source node.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Feedback from @niftynei.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
short_channel_id_dir: new primitive for one direction of short_channe…
…l_id

Currently only used by gossipd for channel elimination.

Also print them in canonical form (/[01]), so tests need to be
changed.

Suggested-by: @cdecker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
JSON: remove redundant word "channel" from direction fields.
Suggested-by: @cdecker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:misc-prep-for-pay-plugin branch from 7bce532 to 5f77ff7 Jan 15, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor Author

rustyrussell commented Jan 15, 2019

OK, rebased again because git put all the CHANGELOG entries in the old section!

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Jan 15, 2019

ACK 5f77ff7

@cdecker cdecker merged commit a00c357 into ElementsProject:master Jan 15, 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