plugin:added invoice creation event

[rbndg]

Hello everyone
This PR adds a new invoice_creation event which is triggered when a new invoice is created.
(This is my first PR to C-Lightning 🤓)

Changelog-Added: plugin: new invoice_creation event

[cdecker]

Welcome @rbndg, good job on your first contribution 👍

I went through and added a couple of comments, but I think most would also apply to the invoice_payment notification from which you took inspiration. I can go through and clean those up at a later time, especially the case of the serializer function seems weird and could do with a bit of type-checking.

I think the only really required change would be to amend the doc, otherwise LGTM ^^

[rbndg]

@cdecker made some changes you recommended.
I'm not sure why some test are failing 🤔

[cdecker]

I allowed myself to rebase on top of master to get rid of the merge commits. I'll check what the CI errors are and report back ^^

[cdecker]

Ok, let me walk you through how you can debug these kinds of issues.

Looking at the CI tests you can see the following segfault in the logs:

lightningd: FATAL SIGNAL 11 (version v0.8.2rc1-6-g460dc9d)
0x55818dd5b1c8 send_backtrace
	common/daemon.c:39
0x55818dd5b26e crashdump
	common/daemon.c:52
0x7f2d663b4f1f ???
	???:0
0x55818dd2190f gossipd_incoming_channels_reply
	lightningd/invoice.c:723
0x55818dd50c4a sd_msg_reply
	lightningd/subd.c:288
0x55818dd511ef sd_msg_read
	lightningd/subd.c:429
0x55818ddb3192 next_plan
	ccan/ccan/io/io.c:59
0x55818ddb3d0f do_plan
	ccan/ccan/io/io.c:407
0x55818ddb3d4d io_ready
	ccan/ccan/io/io.c:417
0x55818ddb5f13 io_loop
	ccan/ccan/io/poll.c:445
0x55818dd23d50 io_loop_with_timers
	lightningd/io_loop_with_timers.c:24
0x55818dd29967 main
	lightningd/lightningd.c:984
0x7f2d66397b96 ???
	???:0
0x55818dd0fc09 ???
	???:0
0xffffffffffffffff ???
	???:0

Notice the last stack frame before we jump into the crashdump handler? It's
pointing at the following line in the code:

df014c6#diff-2c1d183f1046f5050b71988e22959046R723

This accesses some struct fields, and calls notify_invoice_creation, but
is otherwise pretty uninteresting. So my first guess is that one of the
struct pointer might be NULL or uninitialized. Time to take valgrind for
a spin:

VALGRIND=1 pytest -vvv tests -k test_invoice

This just runs one of the failing tests (test_invoice) and wraps all nodes
with valgrind so we get memory checks. Indeed the test still fails, but now
we can see a different error in the logs:

Valgrind error file: valgrind-errors.7267
==7267== Invalid read of size 8
==7267==    at 0x419339: gossipd_incoming_channels_reply (invoice.c:724)
==7267==    by 0x45160B: sd_msg_reply (subd.c:288)
==7267==    by 0x450E81: sd_msg_read (subd.c:429)
==7267==    by 0x4BDBA5: next_plan (io.c:59)
==7267==    by 0x4BE8E8: do_plan (io.c:407)
==7267==    by 0x4BE753: io_ready (io.c:417)
==7267==    by 0x4C086A: io_loop (poll.c:445)
==7267==    by 0x41BBDE: io_loop_with_timers (io_loop_with_timers.c:24)
==7267==    by 0x421169: main (lightningd.c:984)
==7267==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==7267==

See that last line talking about address 0x0? That's a pretty good
indication that we're trying to derefence a NULL-pointer, but which one?
We're derefencing a couple of pointers here:

info->cmd->ld, *info->b11->msat, info->payment_preimage, info->label
     ^    ^    ^     ^    ^           ^                       ^

I marked the dereferencing operations in that snippet. We could step through
them one by one, but we are already dereferencing quite a few of them in the
code above without any issues. So we can exclude quite a few. One thing we do
is to use pointers whenever a value might not be present, and so I'd jump at
the *info->b11->msat, knowing that b11->msat might be a NULL-pointer (we
can create invoices which do not specify an amount). So adding a quick check
just for safety:

	assert(info->b11->msat != NULL);

Running again, this indeed fails:

lightningd: lightningd/invoice.c:724: void gossipd_incoming_channels_reply(struct subd *, const u8 *, const int *, struct invoice_info *): Assertion `info->b11->msat != NULL' failed.

So we now know that this is at least one of the issues we're facing. Now we
need to change the interface of notify_invoice_creation to accept the amount
as a pointer instead (since it can now be NULL) and thread that change
through the call-chain until we get to the serializer which actually uses it.

I added a fixup commit that we're going to squash into the series after review
:-) I hope I could shed some light on how to tackle these opaque errors, let
me know if you need more information or other help 🙂

[rbndg]

Thanks for the detailed answer @cdecker really helpful.

I followed the logs for the last remaining build fail. The problem seems to be that when the channel is closed and 100 blocks are generated, it doesn't consider that the channel is irrevocably closed, hence the peer is not forgotten?
or maybe it hasn't been 100 blocks so it doesn't consider it as irrevocably closed.
https://travis-ci.org/github/ElementsProject/lightning/jobs/680661448#L4191

DEBUG 0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518-onchaind-chan#1: FUNDING_TRANSACTION/FUNDING_OUTPUT->MUTUAL_CLOSE depth 52

[cdecker]

Very good analysis @rbndg, that seems to indeed be the issue here. Sadly we do have a couple of flaky tests that fail in completely unrelated tests and muddy the waters. So I think this particular failure is unrelated with your changes.

I restarted the failing run and it should terminate correctly. I keep a list of failed tests and their logs, and every once in a while I spend a day unflaking them 🤓

[rbndg]

Great! I hope to improve and make more useful contributions eventually!
Thanks for your help.

[cdecker]

Rebased on top of master, merging as soon as CI passes 👍

[cdecker]

ACK e4f0721