Skip to content
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

setchannel: new command #5103

Merged
merged 13 commits into from
Mar 22, 2022

Conversation

rustyrussell
Copy link
Contributor

No description provided.

Copy link
Collaborator

@m-schmoock m-schmoock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. We should also add htlc_minimum_msat with this PR.

channeld/channeld.c Outdated Show resolved Hide resolved
contrib/pyln-client/pyln/client/lightning.py Outdated Show resolved Hide resolved
doc/lightning-setchannel.7.md Outdated Show resolved Hide resolved
doc/lightning-setchannel.7.md Show resolved Hide resolved
tests/test_pay.py Show resolved Hide resolved
tests/test_pay.py Outdated Show resolved Hide resolved
tests/test_pay.py Outdated Show resolved Hide resolved
tests/test_pay.py Outdated Show resolved Hide resolved
tests/test_pay.py Outdated Show resolved Hide resolved
tests/test_pay.py Outdated Show resolved Hide resolved
@rustyrussell rustyrussell force-pushed the guilt/setchannel branch 2 times, most recently from fd37912 to ac8fbb6 Compare March 16, 2022 19:31
@rustyrussell
Copy link
Contributor Author

OK, large rework of the tests, which fixes them up, and fixes for dual-funding.

We currently don't allow setting it, but it's been requested.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We used to calculate it ourselves.  Unfortunately this needs to
be done in several places, since new_channel() isn't used to fully
create a channel in the case of dual funding :(

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When we let them set it, this matters!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is htlc_maximum_msat in BOLT 7 speak, but this name matches our existing
fields and is clearer in this context.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
1. Add the htlc_max param.
2. Allow parameters to be unset, meaning "don't change".

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Based on setchannelfee, but expanded to allow setting max htlc amount (and others
in future?).

The main differences:
1. It doesn't change values which are not specified (that would be hard to
   add fields to!)
2. It says exactly what all values are in any potentially changed channels.

Changelog-Added: JSON-RPC: new `setchannel` command generalizes `setchannelfee`.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Deprecated: JSON-RPC: `setchannelfee` (use `setchannel`).
We use this in gossmap to represent htlc max/min approximations.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We need to add some, since our internal representations of
htlc_maximum_msat round up, and we need to disable mpp which succeeds
in getting a payment through by splitting.

We also allow dev_routes to suppress invoice routehints altogether.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We still use the channel hint here (as it's the only option), we just
warn about lack of capacity.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Suggested by @m-schmook, I realized that if we append it later I'll
never get it right: I expect parameters min and max, not max and min!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Protocol: you can now alter the `htlc_minimum_msat` and `htlc_maximum_msat` your node advertizes.
…pacity.

And check for the obvious setting min > max.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Comment on lines 2120 to 2122
# If l1 were to send 4,999,999 millisatoshi to l3 via l2, it needs to
# pay l2 the fee it specified in the l2->l3 `channel_update`, calculated as
# per [HTLC Fees](#htlc_fees): base + amt * pm / 10**6
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text still can be updated, as the 'Note: ' below mentions we are not testing for 4,999,999msat

@m-schmoock
Copy link
Collaborator

ACK 7db4743

Short term leak, but leak-detect is right: it's dumb code!

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

```
plugin-pay: MEMLEAK: 0x2505318
plugin-pay:   label=common/utils.c:134:char[]
plugin-pay:   backtrace:
plugin-pay:     ccan/ccan/tal/tal.c:442 (tal_alloc_)
plugin-pay:     ccan/ccan/tal/tal.c:471 (tal_alloc_arr_)
plugin-pay:     common/utils.c:134 (tal_hexstr)
plugin-pay:     common/node_id.c:48 (node_id_to_hexstr)
plugin-pay:     common/node_id.c:50 (fmt_node_id_)
plugin-pay:     common/type_to_string.c:32 (type_to_string_)
plugin-pay:     plugins/libplugin-pay.c:2970 (shadow_route_extend)
plugin-pay:     plugins/libplugin-pay.c:3113 (shadow_route_listchannels)
plugin-pay:     plugins/libplugin.c:563 (handle_rpc_reply)
plugin-pay:     plugins/libplugin.c:731 (rpc_read_response_one)
plugin-pay:     plugins/libplugin.c:751 (rpc_conn_read_response)
plugin-pay:     ccan/ccan/io/io.c:59 (next_plan)
plugin-pay:     ccan/ccan/io/io.c:407 (do_plan)
plugin-pay:     ccan/ccan/io/io.c:417 (io_ready)
plugin-pay:     ccan/ccan/io/poll.c:453 (io_loop)
plugin-pay:     plugins/libplugin.c:1565 (plugin_main)
plugin-pay:     plugins/pay.c:2569 (main)
plugin-pay:   parents:
plugin-pay:     plugins/libplugin.c:155:struct out_req
```
@rustyrussell
Copy link
Contributor Author

Ack 148a0d2

Fixing an unrelated leak report which cropped up...

@rustyrussell rustyrussell merged commit dbc77bc into ElementsProject:master Mar 22, 2022
@m-schmoock
Copy link
Collaborator

@rustyrussell
I'm working on a follow up PR that will make htlc_minimum_msat and htlc_minimum_msat defaults configurable for newly created channels. It will also contain a 'turn off able' switch for the IP discovery feature.

Whats the timeline for the next release? I think we should bring that in before...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants