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

Issue #2483 part1: min_capacity_sat config for rejecting tiny channels #2506

Merged
merged 6 commits into from Apr 9, 2019

Conversation

Projects
None yet
3 participants
@m-schmoock
Copy link
Collaborator

commented Mar 26, 2019

SUMMARY
In regards to the IRC meeting logs, as the first part of Issue #2483 I see the need to introduce a config value and switch to automatically reject channel opening requests for tiny channels. The second part of adding a channel accept plugin hook will be in a separate PR that I am already working on. This PR will:

  • Remove magic value min_effective_htlc_capacity = AMOUNT_MSAT(1000000)
  • Introduce min_capacity_sat config value and --min-capacity-sat config switch.
  • Add testcase
  • Add doc
  • Fix memory leaks discussed in PR #2494.

@m-schmoock m-schmoock requested a review from cdecker as a code owner Mar 26, 2019

@m-schmoock m-schmoock force-pushed the m-schmoock:issue-2483-config-value branch 4 times, most recently from f11221f to a2f6e34 Mar 27, 2019

@m-schmoock m-schmoock force-pushed the m-schmoock:issue-2483-config-value branch from a2f6e34 to 5bb6756 Mar 27, 2019

@cdecker
Copy link
Member

left a comment

Doesn't the default value of 1000sat trigger this failure anyway?

if (amount_sat_greater(state->remoteconf.dust_limit,
state->localconf.channel_reserve)) {
negotiation_failed(state, false,
"Our channel reserve %s"
" would be below their dust %s",
type_to_string(tmpctx, struct amount_sat,
&state->localconf.channel_reserve),
type_to_string(tmpctx, struct amount_sat,
&state->remoteconf.dust_limit));
return NULL;
}
if (amount_sat_greater(state->localconf.dust_limit,
state->remoteconf.channel_reserve)) {
negotiation_failed(state, false,
"Our dust limit %s"
" would be above their reserve %s",
type_to_string(tmpctx, struct amount_sat,
&state->localconf.dust_limit),
type_to_string(tmpctx, struct amount_sat,
&state->remoteconf.channel_reserve));
return NULL;
}

Show resolved Hide resolved openingd/openingd.c Outdated
Show resolved Hide resolved common/amount.h

@m-schmoock m-schmoock force-pushed the m-schmoock:issue-2483-config-value branch from 5bb6756 to 2c4bfa0 Mar 28, 2019

@m-schmoock m-schmoock closed this Mar 28, 2019

@m-schmoock m-schmoock reopened this Mar 28, 2019

@m-schmoock

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 28, 2019

@cdecker I updated the PR, I didnt understand this question of you:

Doesn't the default value of 1000sat trigger this failure anyway?

Can you explain in more detailed? Update: I think I understand your question now, and it somehow is relate to this confusion. As you can see in my added testcase, using 999 (or minimal value - 1) or 1000<=value<=5976 sats trigger two different exceptions:

  1. msat <= 999 "channel capacity with funding %s, reserves %s/%s, max_htlc_value_in_flight_msat is %s, channel capacity is %s, which is below %s" (

    /* If the resulting channel doesn't meet our minimum "effective capacity"
    * set by lightningd, don't bother opening it. */
    if (amount_msat_greater_sat(state->min_effective_htlc_capacity,
    capacity)) {
    negotiation_failed(state, am_funder,
    "channel capacity with funding %s,"
    " reserves %s/%s,"
    " max_htlc_value_in_flight_msat is %s,"
    " channel capacity is %s, which is below %s",
    type_to_string(tmpctx, struct amount_sat,
    &state->funding),
    type_to_string(tmpctx, struct amount_sat,
    &remoteconf->channel_reserve),
    type_to_string(tmpctx, struct amount_sat,
    &state->localconf.channel_reserve),
    type_to_string(tmpctx, struct amount_msat,
    &remoteconf->max_htlc_value_in_flight),
    type_to_string(tmpctx, struct amount_sat,
    &capacity),
    type_to_string(tmpctx, struct amount_msat,
    &state->min_effective_htlc_capacity));
    return false;
    }

  2. 1000<=value<=5976 sats "Could not meet their fees and reserve"

    tx = initial_channel_tx(state, &wscript, state->channel,
    &state->first_per_commitment_point[REMOTE],
    REMOTE);
    if (!tx) {
    /* This should not happen: we should never create channels we
    * can't afford the fees for after reserve. */
    negotiation_failed(state, true,
    "Could not meet their fees and reserve");
    goto fail;
    }

TLDR: your exception (lightning/openingd/openingd.c L963 - L984 in 426b22f) is not triggered for 1000sat channels as my testcase shows.

@m-schmoock m-schmoock force-pushed the m-schmoock:issue-2483-config-value branch from 2c4bfa0 to 06fc034 Mar 28, 2019

Show resolved Hide resolved lightningd/opening_control.c Outdated
Show resolved Hide resolved tests/test_connection.py Outdated

@m-schmoock m-schmoock force-pushed the m-schmoock:issue-2483-config-value branch 9 times, most recently from 91919c0 to 90e2928 Mar 29, 2019

@m-schmoock

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 3, 2019

@niftynei

the leading _ is unusual for c-lightning code style. better to just say err_reason

Okay, I was used to use this style on output pointers. I corrected to 'err_reason' on the latest force push 5bf9be1

For the wording of the other exceptions, I am not sure if we should change them (make them more simple) in this PR since they are not my code.

If you still want to have your new wording in this PR, please just tell me quickly and I do another update.

@m-schmoock

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 3, 2019

@niftynei Or should I forward a different wording to err_reason that that is internally used and logged?

@m-schmoock m-schmoock force-pushed the m-schmoock:issue-2483-config-value branch 2 times, most recently from 7ed204d to b8f11dc Apr 4, 2019

@cdecker
Copy link
Member

left a comment

Just two minor comments (mainly the u32 vs u64 rationale for satoshis), looks good to me

Show resolved Hide resolved openingd/openingd.c
Show resolved Hide resolved common/amount.c Outdated
Show resolved Hide resolved lightningd/lightningd.h Outdated

@m-schmoock m-schmoock force-pushed the m-schmoock:issue-2483-config-value branch from b8f11dc to 41c50a5 Apr 5, 2019

@m-schmoock m-schmoock force-pushed the m-schmoock:issue-2483-config-value branch from 41c50a5 to 991d6f9 Apr 5, 2019

m-schmoock added some commits Apr 3, 2019

fix: openingd memory leaks when failing
- when opening was faling for reasons (i.e. channel too small) the fail
handling routing did not clean up *funding and *wscript.
feat: add min_capacity_sat config value and switch
- add config value min_capacity_sat that will replaces the magic value
  min_effective_htlc_capacity = AMOUNT_MSAT(1000000)
- add config switch min_capacity_sat

@m-schmoock m-schmoock force-pushed the m-schmoock:issue-2483-config-value branch from 991d6f9 to 9a8ec2e Apr 8, 2019

@cdecker

cdecker approved these changes Apr 9, 2019

Copy link
Member

left a comment

ACK 9a8ec2e

@cdecker cdecker merged commit 0fc9368 into ElementsProject:master Apr 9, 2019

2 checks passed

ackbot PR ack'd by cdecker
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@m-schmoock m-schmoock deleted the m-schmoock:issue-2483-config-value branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.