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

Added possibility to configure max_concurrent_htlcs value #2858

Conversation

@renepickhardt
Copy link
Collaborator

commented Jul 28, 2019

Eclair has a default value of 30 and I thought why not going with their value and while doing so make it configureable.

@cdecker suggested to call the option max_concurrent_htlcs instead of the name in the BOLTs max_concurrent_htlcs

Here is the relevant line in eclair:

https://github.com/ACINQ/eclair/blob/9afb26e09c69dd5d6a14732baf5dcdf2b7a9142b/eclair-core/src/main/resources/reference.conf#L62

@ZmnSCPxj
Copy link
Collaborator

left a comment

Needs documentation:

diff of command names vs manpage names:
--- /dev/fd/63	2019-07-28 07:26:31.096200068 +0000
+++ /dev/fd/62	2019-07-28 07:26:31.096200068 +0000
@@ -35,7 +35,6 @@
 log-level=
 log-prefix=
 mainnet
-max-concurrent-htlcs=
 max-locktime-blocks=
 min-capacity-sat=
 network=
doc/Makefile:80: recipe for target 'check-manpages' failed

I mildly prefer 483, as we expect (hope?) unilateral closure to be rare.

@renepickhardt

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 29, 2019

I added an entry to doc/lightningd however I have proplems (like last time I tried to add a manpage) to run a2x even after installing asciidoc

a2x --format=manpage doc/lightningd-config.5.txt
a2x: ERROR: "xmllint" --nonet --noout --valid "/Users/rpickhardt/hacken/tmp/lightning/doc/lightningd-config.5.xml" returned non-zero exit status 4

web search was of no help I guess I am missing some dependencies.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

This doesn't solve your problem though, as this doesn't restrict how many HTLCs we'll send to a peer, just how many we'll receive.

I would suggest a follow-up patch which caps the sending number to max(what-peer-set, what-our-max-is)?

@renepickhardt

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2019

While the first paragraph of your review makes sense to me I am confused by your suggestion for a follow up patch.

Shouldn't it be min(what-peer-set, what-our-max-is) instead of max? Or did you mean what-peer-seNt (in the sense of how many did we received so far)?

In my understanding concurrent htlcs was meant to be offered plus received. So the max would not help here as we would have to check against the current sum in both cases or is there a mistake in my thinking?

@renepickhardt

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2019

can't I just modify this line:

> channel->config[recipient].max_accepted_htlcs) {

to test channel->config[each_side].max_accepted_htlcs

with LOCAL and REMOTE as values for each_side and take the min of both values or did I miss something in the code base?

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

can't I just modify this line:

> channel->config[recipient].max_accepted_htlcs) {

to test channel->config[each_side].max_accepted_htlcs

with LOCAL and REMOTE as values for each_side and take the min of both values or did I miss something in the code base?

Yes, exactly, that would have the same effect.

@rustyrussell rustyrussell added this to the 0.7.2 milestone Aug 6, 2019

@renepickhardt renepickhardt force-pushed the renepickhardt:max_concurrent_htlc_configurable branch from f775593 to e564b0c Aug 7, 2019

@renepickhardt

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 7, 2019

I added the additional patch as @rustyrussell requested and rebased to current master. Think everything should be working now. fyi @cdecker

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

I added an entry to doc/lightningd however I have proplems (like last time I tried to add a manpage) to run a2x even after installing asciidoc

a2x --format=manpage doc/lightningd-config.5.txt
a2x: ERROR: "xmllint" --nonet --noout --valid "/Users/rpickhardt/hacken/tmp/lightning/doc/lightningd-config.5.xml" returned non-zero exit status 4

web search was of no help I guess I am missing some dependencies.

I got this too recently. But Works Now, so I'm adding a simple fixup.

@rustyrussell
Copy link
Contributor

left a comment

I've added some fixups for your review @renepickhardt because we're 2 days from rc1 and I'm trying to minimize round-trips across timezones!

If you're happy, either you or I can autosquash them for commit.

@@ -205,6 +205,10 @@ Lightning channel and HTLC options
they're outside this range, we abort their opening attempt. Note
that *commit-fee-max* can (should!) be greater than 100.

*max-concurrent-htlcs*='INTEGER'::
Number of HTLCs one channel can handle concurrently. Should be between
1 and 483 (default 30).

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Aug 8, 2019

Contributor

in each direction. The max possible total will be twice this number. Will add.

@@ -928,6 +939,9 @@ static void register_opts(struct lightningd *ld)
opt_register_arg("--fee-per-satoshi", opt_set_u32, opt_show_u32,
&ld->config.fee_per_satoshi,
"Microsatoshi fee for every satoshi in HTLC");
opt_register_arg("--max-concurrent-htlcs", opt_set_u32, opt_show_u32,
&ld->config.max_concurrent_htlcs,
"Number of HTLCs one channel can handle concurrently. Should be between 1 and 483 (default 30)");

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Aug 8, 2019

Contributor

This doesn't quite work:

--max-concurrent-htlcs <arg>      Number of HTLCs one channel can handle
                                  concurrently. Should be between 1 and
                                  483 (default 30) (default: 0)

because you included a show function, it's used to show the default automagically. However, you only set it in the mainnet config, so it's zero (a very bad number!) for other configs.

30 is a reasonable upper bound to avoid bloating atttacks.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

check-source got upset because it thought your comment was part of the BOLT quote. Split.

@renepickhardt

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 8, 2019

I can autosquash that patch to be one commit in about 1 hour and rebase again if necessary so that it is ready to be merged.

I did not know you test for bolt quotes. That is beautiful!

@renepickhardt
Copy link
Collaborator Author

left a comment

this all together looks good to me. will autosquash and rebase once more

@renepickhardt renepickhardt force-pushed the renepickhardt:max_concurrent_htlc_configurable branch from c4b4f9b to d959421 Aug 8, 2019

@practicalswift

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2019

Nit: Typo in PR title and commit message. Should be "possibility" :-)

@renepickhardt

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 8, 2019

am I supposed to change the commit message now?

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2019

Yes. :)

@renepickhardt renepickhardt force-pushed the renepickhardt:max_concurrent_htlc_configurable branch from d959421 to 697c86c Aug 8, 2019

@renepickhardt

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 8, 2019

@ZmnSCPxj @practicalswift I can't believe I actually did that (and while so I also fixed configer to configure)

@renepickhardt renepickhardt changed the title Added possability to configure max_concurrent_htlcs value Added possibility to configure max_concurrent_htlcs value Aug 8, 2019

@rustyrussell rustyrussell force-pushed the renepickhardt:max_concurrent_htlc_configurable branch 2 times, most recently from 47b73c8 to b657581 Aug 9, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Ack b657581

Rebased to squash fixup and fixed your commit msg

Added possibility to configure max_concurrent_htlcs value for our cha…
…nnels. Eclaire has a default of 30 and I thought why not going with their value and while doing so make it configureable.

@rustyrussell rustyrussell force-pushed the renepickhardt:max_concurrent_htlc_configurable branch from b657581 to e524093 Aug 9, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

... and again to fix merge conflict with master...

Ack e524093

@rustyrussell rustyrussell merged commit dfac1d1 into ElementsProject:master Aug 9, 2019

2 checks passed

ackbot PR ack'd by rustyrussell
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
Projects
None yet
4 participants
You can’t perform that action at this time.