Channel fund hook #2650

rustyrussell · 2019-05-20T05:12:51Z

Somehow I dropped this: I had a PR which I decided to split into pieces, and never resubmitted. I was reviewing the milestones for 0.7.1 when I noticed.

Closes: #2483

We generated code which didn't compile (we never had one before though). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Instead of lightningd telling us when it's ready, we ask it. This also provides an opportunity to have a plugin hook at this point. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

New fields don't have to be spelled out twice. The raw version are called _only, so we don't miss a call accidentally. We can rename them when we finally deprecated old fields. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

ZmnSCPxj

Minor nit in function arguments.

ZmnSCPxj · 2019-05-20T06:29:10Z

lightningd/json.c

-			  const char *msatfieldname)
+void json_add_amount_msat_compat(struct json_stream *result,
+				 struct amount_msat msat,
+				 const char *rawfieldname,


Other json_add_* functions have field names before the value being printed.
Mildly suggest, to follow other functions.

I'd agree, but it's a lot of churn for a function we're going to phase out, so I think it's a bit gratuitous for now.

m-schmoock

I like really it. As ususal, well structured, testcase, doc..
I didn't finish my implementation it as I noticed you already worked on this last month (#2576 (comment))....

Here my remarks:

If possible add the remote routing fees to the payload
msat/sat fieldname/parameter name confusions everywhere, maybe I misunderstood the _compat function?

m-schmoock · 2019-05-20T07:03:10Z

lightningd/opening_control.c

+	struct amount_msat max_htlc_value_in_flight_msat;
+	struct amount_sat channel_reserve_satoshis;
+	struct amount_msat htlc_minimum_msat;
+	u32 feerate_per_kw;


Do we already know remote routing fees (base, ppm) here? If yes, we should add it to payload so plugin knows...

No, we don't ever get told those directly; we learn them like everyone else, when/if it advertizes the channel.

m-schmoock · 2019-05-20T07:03:30Z

doc/PLUGINS.md

+	"max_htlc_value_in_flight_msat": "18446744073709551615msat",
+	"channel_reserve_satoshis": "1000000msat",
+	"htlc_minimum_msat": "0msat",
+	"feerate_per_kw": 7500


missing comma end of line :-)

Yes, and there's a trailing one on the last line, which isn't valid JSON either. Fixed!

m-schmoock · 2019-05-20T07:05:15Z

lightningd/test/run-invoice-select-inchan.c

+void json_add_amount_sat_compat(struct json_stream *result UNNEEDED,
+				struct amount_sat sat UNNEEDED,
+				const char *rawfieldname UNNEEDED,
+				const char *msatfieldname)


Since you are already touching this line, rename msatfieldname to satfieldname

No, these are always msat fields, since we unified on that as standard.

m-schmoock · 2019-05-20T07:09:02Z

wallet/test/run-wallet.c

+void json_add_amount_sat_compat(struct json_stream *result UNNEEDED,
+				struct amount_sat sat UNNEEDED,
+				const char *rawfieldname UNNEEDED,
+				const char *msatfieldname)


same here, rename to satfieldname

m-schmoock · 2019-05-20T07:11:08Z

wallet/walletrpc.c

-		json_add_amount_sat(response, utxos[i]->amount,
-				    "value", "amount_msat");
+		json_add_amount_sat_compat(response, utxos[i]->amount,
+					   "value", "amount_msat");


Maybe I'm off here, but isn't that supposte to be called amount_sat?

No... we always print amounts as msat now, to avoid confusion.

m-schmoock · 2019-05-20T07:14:25Z

lightningd/json.c

+void json_add_amount_sat_compat(struct json_stream *result,
+				struct amount_sat sat,
+				const char *rawfieldname,
+				const char *msatfieldname)


Should this be satfieldname?

m-schmoock · 2019-05-20T07:14:49Z

lightningd/json.c

+}
+
+void json_add_amount_sat_only(struct json_stream *result,
+			 const char *msatfieldname,


m-schmoock · 2019-05-20T07:15:31Z

wallet/walletrpc.c

+						   "our_amount_msat");
+			json_add_amount_sat_compat(response, c->funding,
+						   "channel_total_sat",
+						   "amount_msat");


... same here in line 512 and 515, since this msat/sat is mixed up everywhere, maybe its just me??

cdecker · 2019-05-20T10:08:38Z

tests/plugins/reject_odd_funding_amounts.py

+
+@plugin.hook('openchannel')
+def on_openchannel(openchannel, plugin):
+    print("{} VARS".format(len(openchannel.keys())))


You could just include from pprint import pprint which recursively prettyprints whatever you pass to it.

Doesn't work as well due to undefined ordering, however, and the fact that it prints "{" and "}":

b"2019-05-20T22:47:55.079Z plugin-reject_odd_funding_amounts.py {'channel_flags': 1," b"2019-05-20T22:47:55.079Z plugin-reject_odd_funding_amounts.py 'channel_reserve_satoshis': '1000000msat'," b"2019-05-20T22:47:55.079Z plugin-reject_odd_funding_amounts.py 'dust_limit_satoshis': '546000msat'," b"2019-05-20T22:47:55.079Z plugin-reject_odd_funding_amounts.py 'feerate_per_kw': 7500,"

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

niftynei · 2019-05-21T00:30:06Z

ACK 2abc7e7

rustyrussell added 3 commits May 20, 2019 14:34

tools/generate-wire.py: handle optional variable-length fields.

b9bd050

We generated code which didn't compile (we never had one before though). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

openingd: check with lightningd when we receive an offer.

d80034b

Instead of lightningd telling us when it's ready, we ask it. This also provides an opportunity to have a plugin hook at this point. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell added openingd plugin labels May 20, 2019

rustyrussell added this to the 0.7.1 milestone May 20, 2019

rustyrussell requested a review from m-schmoock May 20, 2019 05:12

rustyrussell requested a review from cdecker as a code owner May 20, 2019 05:12

ZmnSCPxj approved these changes May 20, 2019

View reviewed changes

m-schmoock requested changes May 20, 2019

View reviewed changes

cdecker reviewed May 20, 2019

View reviewed changes

niftynei approved these changes May 20, 2019

View reviewed changes

rustyrussell added 2 commits May 21, 2019 08:15

openingd: add openchannel hook.

e1d26b6

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

patch reorder-compat-printing.patch

2abc7e7

rustyrussell force-pushed the guilt/channel-fund-hook branch from 54b583d to 2abc7e7 Compare May 20, 2019 22:56

niftynei merged commit e5b5f1d into ElementsProject:master May 21, 2019

Channel fund hook #2650

Channel fund hook #2650

Uh oh!

Conversation

rustyrussell commented May 20, 2019

Uh oh!

ZmnSCPxj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m-schmoock left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niftynei commented May 21, 2019

Uh oh!

Uh oh!

m-schmoock left a comment •

edited

Loading