We no longer need a 'sendpay_result' structure, we can pass
appropriate parameter directly now they're simple calls.
Every waitsendpay command ends in tell_waiters_failed or
tell_waiters_success, which call sendpay_success or sendpay_fail on
all matching waiters. These all return 'struct command_result *'.
In cases where the result is immediately known, we call
sendpay_success/sendpay_fail directly for the command.
This also adds a helpful 'failcodename' field to the JSON output.
[ This was four separate cleanup patches, but that contained much
redundancy and was even worse to review ]
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
With only one caller, we don't need a callback pointer any more; we can simply
call the function.
This required some code shuffling, and I changed the callback function
arguments to be in a more natural order, now they're not used as
callbacks.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now we don't have a second caller for these routines, we can move
them back into pay.c and make the functions static.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As a general rule, lightningd shouldn't parse user packets. We move the
parsing into gossipd, and have it respond only to permanent failures.
Note that we should *not* unconditionally remove a channel on
WIRE_INVALID_ONION_HMAC, as this can be triggered (and we do!) by
feeding sendpay a route with an incorrect pubkey.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We had a bug 0ba547ee10 caused by
short_channel_id overflow. If we'd caught this, we'd have terminated
the peer instead of crashing, so add appropriate checks.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I upgraded my node with --disable-compat, and a heap of channels closed like:
CHANNELD_NORMAL:We disagree on short_channel_ids: I have 557653x0x1351, you say 557653x2373x1",
This is because the scids are strings in the databases, and it failed to parse
them properly.
Now we'll not start if that happens.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Internally libplugin turns ' into ", which causes these messages to produce
bad JSON.
The real fix is to remove the '->" convenience substitution and port the
JSON creation APIs into common/ from lightningd/
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Spurious errors were occuring around checking the provided
current commitment point from the peer on reconnect when
option_data_loss_protect is enabled. The problem was that
we were using an inaccurate measure to screen for which
commitment point to compare the peer's provided one to.
This fixes the problem with screening, plus makes our
data_loss test a teensy bit more robust.
Valgrind seems to be slowing the pay-plugin down enough for the 10
seconds timeout to get triggered on a semi-regular basis.
Reported-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Christian points out that we can iterate by ->size rather than calling
json_next() to find the end (which traverses the entire object!).
Now ->size is reliable (since previous patch), this is OK.
Reported-by: @cdecker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
jsmn would accept invalid JSON objects. This is bad because it would
set ->size incorrectly: we expect to have at least size * 2 tokens (in
pairs). We want to rely on ->size, but this would create an exploitable
buffer overflow!
Fortunately, this is fixed upstream, so we add a test and upgrade to v1.0.0.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Wasn't using valid JSON, but worked anyway. This is actually OK
because we don't rely on tok->size, but we want to, so another fix
coming.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The external/jsmn/README.md only says:
int size; // Number of child (nested) tokens
But it only counts *direct* children, or *direct* members for an object.
This test verifies this (the bug proved to be elsewhere: see next patch!).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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>
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>
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>
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>
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>
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>
We therefore keep a reference to the DB and will wrap and unwrap when
a hook returns.
Notice that this might cause behavior changes when moving logic into a
hook callback, since the continuation runs in a different transaction
than the event that triggered the hook in the first place. Should not
matter too much, since we don't use DB rollbacks at the moment, but
it's something to keep in mind.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This ties all the things together, using the serializer to transform
the payload into a valid `jsonrpc_request`, sending it to the plugin,
and then using the deserializer on the way back before calling the
hook callback with the appropriate information.
Notice that the serializer and deserializer is skipped if we don't
have a plugin that registered for this hook.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
plugin_request_new did nothing special aside from registering the
request ID with the dispatch code. This duty has now been moved to
plugin_request_send instead, which is also exposed so we can use that
code in plugin_hook.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is the first use of the `hooks` autodata field, and it required a
dummy element in order for the section not to be dropped, it'll be
removed once we have actual hooks.
There is very little that is plugin specific in the jsonrpc_request so
this just extracts the common parts so we can reuse them outside of
the plugin compilation unit as well.
None of the existing callbacks was making use of it and we will be
exposing the method callback interface to outside compilation unit
where the struct definition is not visible. So just remove it.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
I might have gone a bit overboard with the type-checking, but
typesafe_cb_cast is quite nice to use, so why not. The macro to
register a new hook encapsulates the entire flow from param
serialization, to dispatch, parsing and callback dispatch in one
bundle. I was tempted to have the callback outside of the
registration, but it's unlikely that we'll have two calls to the same
hook with different callbacks.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Rusty Russell
lisa neigut
Christian Decker