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

config: add commit time parameter #5187

Merged
merged 2 commits into from Apr 9, 2021

Conversation

joostjager
Copy link
Collaborator

Channel state update batching is an important mechanism to increase the throughput of a Lightning node. With high degrees of batching many synchronous and expensive disk writes can be avoided. Batching comes at the cost of increased latency. For high-frequency (streaming) applications this may be an acceptable trade-off.

This PR simply makes the batch timer configurable to allow node operators to tune their node to a particular traffic pattern.

As a follow-up it could be worth to consider a dynamic batch interval based on system load ("commit on idle").

@joostjager joostjager force-pushed the config-commit-time branch 2 times, most recently from 0658893 to 9ca8c9c Compare April 8, 2021 12:41
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM, nice addition Joost 👍

@@ -289,6 +293,8 @@ type Config struct {
MaxChanSize int64 `long:"maxchansize" description:"The largest channel size (in satoshis) that we should accept. Incoming channels larger than this will be rejected"`
CoopCloseTargetConfs uint32 `long:"coop-close-target-confs" description:"The target number of blocks that a cooperative channel close transaction should confirm in. This is used to estimate the fee to use as the lower bound during fee negotiation for the channel closure."`

CommitTime time.Duration `long:"commit-time" description:"The maximum time that is allowed to pass between receiving a channel state update and signing the next commitment. Setting this to a longer duration allows for more efficient channel operations at the cost of latency."`

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: instead of "more efficient channel operations" you could write "larger channel throughput" as that may be more descriptive for this parameter.

Copy link
Collaborator Author

@joostjager joostjager Apr 8, 2021

Choose a reason for hiding this comment

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

I considered that, but as it is currently, increasing this value doesn't yet increase throughput dramatically. There are just too many non-batched txes going on. I felt that 'larger channel throughput' would overpromise a bit, even though throughput should indeed be slightly larger.

Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

ACK on the changes, only request is to make the config name more specific :)

sample-lnd.conf Outdated
; The maximum time that is allowed to pass between receiving a channel state
; update and signing the next commitment. Setting this to a longer duration
; allows for more efficient channel operations at the cost of latency.
; commit-time=50ms
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit: would you be opposed to something more like channel-commit-interval?

two reasons in my head:

  • increase specificity and indicate that this relates to channels (we already have db.batch-commit-interval) and leave room for other types of commit paramters. could even be more specific, e.g. channel-sign-commit-interval
  • reuse the interval terminology already present in db.batch-commit-interval and in the much of the code base

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I picked commit-time because it matches the option name in c-lightning. Not a particular good reason. Renamed to what you're suggesting.

@joostjager joostjager force-pushed the config-commit-time branch 2 times, most recently from bd4d074 to ae7863d Compare April 9, 2021 07:02
@joostjager
Copy link
Collaborator Author

Should I also add BatchSize as a parameter? Otherwise we can increase the commit interval, but still a commitment will be signed after 10 updates (the default).

@joostjager
Copy link
Collaborator Author

Added channel-commit-batch-size parameter

Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM 🔥 great addition to also expose the batch size

@cfromknecht cfromknecht merged commit 213b264 into lightningnetwork:master Apr 9, 2021
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

3 participants